added webhook validation for DRCluster #567

rakeshgm · 2022-10-19T13:26:02Z

Implemented validation using webhook using cert-manager.
DRCluster CR objects Region and S3ProfileName are immutable.

Signed-off-by: rakeshgm rakeshgm@redhat.com

rakeshgm · 2022-10-19T13:27:49Z

I believe most of the changes are complete. I need to complete one round of testing to check if the webhook is being called. so making the PR as draft for now.

controllers/drcluster_controller_test.go

iamniting

It looks good to me @rakeshgm

Madhu-1

Can you please double-check that some yaml changes are missing, like a certificate.yaml and update in CSV, etc?

Madhu-1 · 2022-10-28T08:36:41Z

api/v1alpha1/drcluster_webhook.go

+	if !reflect.DeepEqual(r.Spec.Region, oldDRCluster.Spec.Region) {
+		return fmt.Errorf("Region value cannot be changed")
+	}
+
+	if !reflect.DeepEqual(r.Spec.S3ProfileName, oldDRCluster.Spec.S3ProfileName) {
+		return fmt.Errorf("S3ProfileName cannot be changed")
+	}


These are just strings, right? Why do we need deepEqual just string comparison is enough.

I will check and change

Madhu-1 · 2022-10-28T08:37:37Z

api/v1alpha1/drcluster_webhook.go

+// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
+func (r *DRCluster) ValidateDelete() error {
+	drclusterlog.Info("validate delete", "name", r.Name)
+
+	return nil
+}


No need for this one, as we dont have a Delete validator.

yes, I could remove it. just kept it like that in case we decide in the future to have delete validation, this anyway is not getting called and returning nil. I am not against removing it. just putting my thought.

Madhu-1 · 2022-10-28T08:39:11Z

api/v1alpha1/drcluster_webhook.go

+		return fmt.Errorf("S3ProfileName cannot be changed")
+	}
+
+	return r.ValidateDRCluster()


When we will hit this case? Isn't it already covered in Create, and the above two checks will avoid setting empty values?

it won't hit as now. but we are planning to add other validations like CIDR format checking in validateDRCluster. so just added it for future-proofing.

Madhu-1 · 2022-10-28T08:40:52Z

api/v1alpha1/drcluster_webhook.go

+	if r.Spec.Region == "" {
+		return fmt.Errorf("Region cannot be empty")
+	}
+
+	if r.Spec.S3ProfileName == "" {
+		return fmt.Errorf("S3ProfileName cannot be empty")
+	}


As empty Region and S3ProfileName are not allowed during creation itself. Can we do this validation at the CR itself

validate tags are not added in drcluster api file. i will add them in the next PR and refactor this code.

rakeshgm · 2022-10-28T11:56:25Z

Can you please double-check that some yaml changes are missing, like a certificate.yaml and update in CSV, etc?

so CSV file is getting generated/updated during the build with bundle dir. we are not adding it to the repo. @ShyamsundarR and I checked this before. so the files needed to get webhook up and running are all added.

nirs · 2022-10-31T21:19:50Z

Implemented validation using webhook. DRCluster CR objects Region and S3ProfileName are immutable.

What is the purpose of the webhook? how will use it? Is it used both on the hub and
on the managed clusters?

nirs · 2022-10-31T21:15:34Z

main.go

+	if err = (&ramendrv1alpha1.DRCluster{}).SetupWebhookWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create webhook", "webhook", "DRCluster")
+		os.Exit(1)
+	}


Blank line here will be more consistent.

code block moved to different a section.

rakeshgm · 2022-11-01T11:25:20Z

Implemented validation using webhook. DRCluster CR objects Region and S3ProfileName are immutable.

What is the purpose of the webhook? how will use it? Is it used both on the hub and on the managed clusters?

A more detailed explanation of webhooks is here.
In ramen we are implementing webhooks for validating CRs and as of now and this PR is about validating DRCluster CR.
webhooks are configured as part of ramen deployment. They are used/called when the CR is created/updated and in our case, it will be DRCluster CR. This webhooks is currently used only on the hub cluster.

Please check this issuse for further information as to why we are implementing webhooks

Signed-off-by: rakeshgm <rakeshgm@redhat.com>

rakeshgm marked this pull request as draft October 19, 2022 13:26

rakeshgm commented Oct 19, 2022

View reviewed changes

controllers/drcluster_controller_test.go Outdated Show resolved Hide resolved

rakeshgm force-pushed the validate-using-webhooks branch 8 times, most recently from af33e9d to e0e5c7b Compare October 21, 2022 16:19

rakeshgm force-pushed the validate-using-webhooks branch 5 times, most recently from 80be6dc to 25f5cdc Compare October 27, 2022 16:13

rakeshgm marked this pull request as ready for review October 27, 2022 16:15

rakeshgm requested review from iamniting and ShyamsundarR October 27, 2022 16:15

rakeshgm force-pushed the validate-using-webhooks branch 2 times, most recently from 43f8b44 to 6de1506 Compare October 27, 2022 17:58

iamniting approved these changes Oct 28, 2022

View reviewed changes

rakeshgm force-pushed the validate-using-webhooks branch from 6de1506 to cc01c15 Compare October 28, 2022 05:03

Madhu-1 reviewed Oct 28, 2022

View reviewed changes

rakeshgm force-pushed the validate-using-webhooks branch 2 times, most recently from 7a76639 to e7541fb Compare October 31, 2022 09:23

nirs reviewed Oct 31, 2022

View reviewed changes

rakeshgm force-pushed the validate-using-webhooks branch from e7541fb to 1961604 Compare November 1, 2022 06:52

rakeshgm force-pushed the validate-using-webhooks branch 3 times, most recently from 046ae84 to b74f6fc Compare November 1, 2022 10:48

rakeshgm changed the title ~~added webhook validation for DRcluster~~ added webhook validation for DRCluster Nov 1, 2022

added webhook validation for DRcluster

bea40ac

Signed-off-by: rakeshgm <rakeshgm@redhat.com>

rakeshgm force-pushed the validate-using-webhooks branch from b74f6fc to bea40ac Compare November 2, 2022 05:56

ShyamsundarR approved these changes Nov 4, 2022

View reviewed changes

ShyamsundarR merged commit cafb6ae into RamenDR:main Nov 4, 2022
6 checks passed

rakeshgm mentioned this pull request Nov 4, 2022

Need to install cert-manager in k8s setup where ramen is running #588

Closed

added webhook validation for DRCluster #567

added webhook validation for DRCluster #567

rakeshgm commented Oct 19, 2022 •

edited

rakeshgm commented Oct 19, 2022

iamniting left a comment

Madhu-1 left a comment

Madhu-1 Oct 28, 2022

rakeshgm Oct 28, 2022

Madhu-1 Oct 28, 2022

rakeshgm Oct 28, 2022

Madhu-1 Oct 28, 2022

rakeshgm Oct 28, 2022

Madhu-1 Oct 28, 2022

rakeshgm Oct 28, 2022

rakeshgm commented Oct 28, 2022

nirs commented Oct 31, 2022

nirs Oct 31, 2022

rakeshgm Nov 1, 2022

rakeshgm commented Nov 1, 2022 •

edited

added webhook validation for DRCluster #567

added webhook validation for DRCluster #567

Conversation

rakeshgm commented Oct 19, 2022 • edited

rakeshgm commented Oct 19, 2022

iamniting left a comment

Choose a reason for hiding this comment

Madhu-1 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

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

rakeshgm commented Oct 28, 2022

nirs commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakeshgm commented Nov 1, 2022 • edited

rakeshgm commented Oct 19, 2022 •

edited

rakeshgm commented Nov 1, 2022 •

edited