replication: add webhook for volumereplicationclass #257

Madhu-1 · 2022-10-20T09:30:02Z

Add webhook for VolumeReplicationClass to validate update CR.

Signed-off-by: Madhu Rajanna [email protected]

nixpanic · 2022-10-20T11:04:25Z

.github/workflows/codespell.yml

+          # kubebuilder latest release has a label with name kuberentes
+          # TODO: remove this in next kubebuilder update as this is fixed
+          # in kubebuilder master
+          ignore_words_list: kuberentes


please add a link to the kubebuilder issue in this comment, that makes it easier to verify what next release contains the fix

there was no issue created for it, it was fixed in a PR kubernetes-sigs/kubebuilder#2964 here

nixpanic · 2022-10-20T11:07:35Z

config/certmanager/certificate.yaml

+kind: Issuer
+metadata:
+  labels:
+    app.kuberentes.io/name: issuer


please add a comment here that it is spelled correctly, and point to the codespell config

this is auto generated file adding commit might get overriden when its rebuild

nixpanic · 2022-10-20T11:09:12Z

config/webhook/service.yaml

@@ -0,0 +1,20 @@
+
+apiVersion: v1


starting with an empty line? Maybe include the yaml-start-of-document --- instead (and in other yaml files)

again this is auto generated file

nixpanic · 2022-10-20T11:17:40Z

PROJECT

@@ -57,4 +57,7 @@ resources:
  kind: VolumeReplicationClass


did you use a kubebuilder command to generate the initial files? Please include the command(s) in the commit message so that others can reproduce it, or follow the steps to add other webhooks.

nixpanic · 2022-10-20T11:19:14Z

apis/replication.storage/v1alpha1/volumereplicationclass_webhook.go

+)
+
+// log is for logging in this package.
+var volumereplicationclasslog = logf.Log.WithName("volumereplicationclass-resource")


vrcLog would do too. But it isn't used much, so the long name might not be too inconvenient.

nixpanic · 2022-10-20T11:21:10Z

apis/replication.storage/v1alpha1/volumereplicationclass_webhook.go

+// log is for logging in this package.
+var volumereplicationclasslog = logf.Log.WithName("volumereplicationclass-resource")
+
+func (r *VolumeReplicationClass) SetupWebhookWithManager(mgr ctrl.Manager) error {


Why r for VolumeReplicationClass? I guess it's used elsewhere and can not easily be modified without changing other files. r is not a self-explaining name when using it inside functions.

this is also auto generated not manually added, i think it need changes in many places

nixpanic · 2022-10-20T11:23:12Z

apis/replication.storage/v1alpha1/volumereplicationclass_webhook.go

+}
+
+// ValidateUpdate implements webhook.Validator. Validates the updated
+// VolumeReplicationClass and does not allow changes to existing spec object.


If it does not allow any changes to the .Spec part, why not use reflect.DeepEqual for that? No need to compare .Provisioner and .Parameters separately.

not comparing spec because for now we have only two fields in spec but later we might add new fields which are suppose to be editable also that the reason to compare it explicitly

nixpanic · 2022-10-20T11:25:35Z

apis/replication.storage/v1alpha1/webhook_suite_test.go

+	}).Should(Succeed())
+
+}, 60)
+


Where are the tests? I expected to find a By() or similar. At least one positive and one negative test would be nice.

Again this is an auto-generated file, current we dont have any functional suite tests. We have already 2 suite files in the repo. https://github.com/csi-addons/kubernetes-csi-addons/blob/main/controllers/csiaddons/suite_test.go https://github.com/csi-addons/kubernetes-csi-addons/blob/main/controllers/replication.storage/suite_test.go. i need to see how to have a single suite to run with the mock controller. I will take it up in the next PR to see how to make it possible.

Adding scalefolding for webhooks for volumereplicationclass. below is the command to generate it. ``` $operator-sdk create webhook --group replication.storage --version v1alpha1 --kind VolumeReplicationClass --programmatic-validation ``` Signed-off-by: Madhu Rajanna <[email protected]>

Added go logic to block updating the provisioner name or the parameters in the existing VolumeReplicationClass Signed-off-by: Madhu Rajanna <[email protected]>

This commit contains the yaml changes to enable the webhook in csv Signed-off-by: Madhu Rajanna <[email protected]>

exclude kuberentes from codespell as there is a typo in label name in latest kubebuilder release and it is fixed in latest master and should be available in next release. Signed-off-by: Madhu Rajanna <[email protected]>

Provided an option to make webhook configurable in the manager deployment . The webhook will be enabled by default when operator is deployed as OLM bundle as OLM takes care of ceritifcates and its disabled in standalone deployment for now as we have dependency with certificates and cert manager as it needs to be setup separately. Signed-off-by: Madhu Rajanna <[email protected]>

`$` is not required if we dont have any output from the command in the markdown. Signed-off-by: Madhu Rajanna <[email protected]>

add namePrefix and namespace for webhook and the certificates to match with other resources. Signed-off-by: Madhu Rajanna <[email protected]>

A new all-in-one yaml files will be generated and it will include all the required RBAC, CRD's , Webhooks, Certificates in a single file. Signed-off-by: Madhu Rajanna <[email protected]>

added a new section for deployment of cert-manager and the controller with webhook enabled. Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 · 2022-11-10T17:10:43Z

I have tested both yaml-based and bundle-based deployments both work as expected.

use local built image from CI testing. Signed-off-by: Madhu Rajanna <[email protected]>

Rakshith-R

LGTM,
Thanks!

Rakshith-R · 2022-11-14T11:06:24Z

apis/replication.storage/v1alpha1/volumereplicationclass_webhook.go

+func (v *VolumeReplicationClass) ValidateCreate() error {
+	return nil
+}


The pr description mentions webhooks for create too
but here it is empty.

update the PR description, Create is not required as it should/can be handled as part of CRD schema validation.

ok thanks !

nixpanic

I don't know much about WebHooks, but this looks reasonable to me.

Syncing latest changes from upstream main for kubernetes-csi-addons

Madhu-1 added the DNM Do Not Merge label Oct 20, 2022

mergify bot requested review from nixpanic, Rakshith-R, yati1998 and Yuggupta27 October 20, 2022 09:30

Madhu-1 force-pushed the webhook branch from 8b79868 to 52bc915 Compare October 20, 2022 09:31

nixpanic requested changes Oct 20, 2022

View reviewed changes

Madhu-1 added 8 commits November 10, 2022 18:03

webhooks: add custom webhook logic

99b16f9

Added go logic to block updating the provisioner name or the parameters in the existing VolumeReplicationClass Signed-off-by: Madhu Rajanna <[email protected]>

webhooks: yaml changes to enable webhook

6d08392

This commit contains the yaml changes to enable the webhook in csv Signed-off-by: Madhu Rajanna <[email protected]>

ci: exclude kuberentes from codespell

8c222eb

exclude kuberentes from codespell as there is a typo in label name in latest kubebuilder release and it is fixed in latest master and should be available in next release. Signed-off-by: Madhu Rajanna <[email protected]>

doc: remove $ if there is no output

91e9e14

`$` is not required if we dont have any output from the command in the markdown. Signed-off-by: Madhu Rajanna <[email protected]>

webhooks: add namePrefix and namespace for webhook

d36a9f7

add namePrefix and namespace for webhook and the certificates to match with other resources. Signed-off-by: Madhu Rajanna <[email protected]>

webhooks: generate new all-in-one yaml

770c2e5

A new all-in-one yaml files will be generated and it will include all the required RBAC, CRD's , Webhooks, Certificates in a single file. Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 force-pushed the webhook branch from 078199b to 8c82680 Compare November 10, 2022 17:03

Madhu-1 removed the DNM Do Not Merge label Nov 10, 2022

doc: add documentation for webhook deployment

a48f56e

added a new section for deployment of cert-manager and the controller with webhook enabled. Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 force-pushed the webhook branch from 8c82680 to a48f56e Compare November 10, 2022 17:07

Madhu-1 requested a review from nixpanic November 10, 2022 17:10

Madhu-1 force-pushed the webhook branch 8 times, most recently from 24d9b0e to 7d30db3 Compare November 10, 2022 17:58

ci: build local image for CI

f48412d

use local built image from CI testing. Signed-off-by: Madhu Rajanna <[email protected]>

Madhu-1 force-pushed the webhook branch from 7d30db3 to f48412d Compare November 10, 2022 18:00

Rakshith-R approved these changes Nov 14, 2022

View reviewed changes

Rakshith-R reviewed Nov 14, 2022

View reviewed changes

nixpanic approved these changes Nov 15, 2022

View reviewed changes

mergify bot merged commit e819549 into csi-addons:main Nov 15, 2022

Nikhil-Ladha pushed a commit to Nikhil-Ladha/kubernetes-csi-addons that referenced this pull request Feb 19, 2025

Merge pull request csi-addons#257 from red-hat-storage/sync_us--main

d391c7d

Syncing latest changes from upstream main for kubernetes-csi-addons

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replication: add webhook for volumereplicationclass #257

replication: add webhook for volumereplicationclass #257

Madhu-1 commented Oct 20, 2022 •

edited

Loading

nixpanic Oct 20, 2022

Madhu-1 Oct 20, 2022

nixpanic Oct 20, 2022

Madhu-1 Oct 20, 2022

nixpanic Oct 20, 2022

Madhu-1 Oct 20, 2022

nixpanic Oct 20, 2022

nixpanic Oct 20, 2022

nixpanic Oct 20, 2022

Madhu-1 Oct 20, 2022

nixpanic Oct 20, 2022

Madhu-1 Oct 20, 2022

nixpanic Oct 20, 2022

Madhu-1 Nov 10, 2022

Madhu-1 commented Nov 10, 2022

Rakshith-R left a comment

Rakshith-R Nov 14, 2022

Madhu-1 Nov 14, 2022

Rakshith-R Nov 15, 2022

nixpanic left a comment

replication: add webhook for volumereplicationclass #257

replication: add webhook for volumereplicationclass #257

Conversation

Madhu-1 commented Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Nov 10, 2022

Rakshith-R left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic left a comment

Choose a reason for hiding this comment

Madhu-1 commented Oct 20, 2022 •

edited

Loading