replication: add webhook for volumereplicationclass by Madhu-1 · Pull Request #257 · csi-addons/kubernetes-csi-addons

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

Add webhook for VolumeReplicationClass to validate update CR.

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

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

+          # 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

+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

@@ -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

@@ -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

+)
+
+// 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

+// 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

+}
+
+// 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

+	}).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 <madhupr007@gmail.com>

Added go logic to block updating the provisioner name or the parameters in the existing VolumeReplicationClass Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

This commit contains the yaml changes to enable the webhook in csv Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

`$` is not required if we dont have any output from the command in the markdown. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

add namePrefix and namespace for webhook and the certificates to match with other resources. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

added a new section for deployment of cert-manager and the controller with webhook enabled. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

Rakshith-R

LGTM,
Thanks!

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

+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 Rakshith-R, Yuggupta27, nixpanic and yati1998 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 <madhupr007@gmail.com>

webhooks: yaml changes to enable webhook

6d08392

This commit contains the yaml changes to enable the webhook in csv Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

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 <madhupr007@gmail.com>

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

Conversation

Madhu-1 commented Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Madhu-1 commented Nov 10, 2022

Uh oh!

Rakshith-R left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nixpanic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Madhu-1 commented Oct 20, 2022 •

edited

Loading