DRPolicy: Add CEL and remove webhook #1175

rakeshgm · 2024-01-16T09:11:31Z

Remove DRPolicy Webhook and add CEL immutability validation for DRPolicy

nirs · 2024-01-16T11:05:52Z

config/crd/bases/ramendr.openshift.io_drpolicies.yaml

@@ -150,6 +150,9 @@ spec:
            required:
            - drClusters
            type: object
+            x-kubernetes-validations:
+            - message: Value is immutable
+              rule: self == oldSelf


Looks like we add the same information twice - once in the kubebuilder comment, and once here. Can we have this only once? What happens if the rule and message are different in both places?

Also this looks really magical - can you link to the docs describing this special syntax?

we only add the comment in the API file and operator-sdk generates/updates this YAML when we run make. This is the general practice as far as I know.
check the link here: CEL Immutability

nirs · 2024-01-16T11:08:02Z

main.go

-		setupLog.Error(err, "unable to create webhook", "webhook", "DRPolicy")
-		os.Exit(1)
-	}
-
 	// setup webhook for drpc


Can we do the same for drpc?

yes, that is the plan. I am planning to send different PRs for each CR, it's just that removal of webhook code is more than adding CEL so, I plan to take one CR at a time so that we don't break ramen. :)

raghavendra-talur · 2024-03-01T18:08:09Z

config/crd/patches/webhook_in_drpolicies.yaml

@@ -12,5 +12,3 @@ spec:
          namespace: system
          name: webhook-service
          path: /convert
-      conversionReviewVersions:


Do we even need this file now? Can this be removed?

I think we need to keep it as it is part of the scaffolding.

Is this and config/crd/patches/cainjection_in_drpolicies.yaml needed though, I am not seeing the scaffolding requirement here?

so, this file was created when the ramen operator was scaffolded initially. I was just trying to revert the changes that we did when adding webhooks and this line was the only change. I tested now by deleting the file. It does not matter. we can safely delete the file I believe.

raghavendra-talur · 2024-03-01T18:24:20Z

config/crd/patches/webhook_in_drpolicies.yaml

@@ -12,5 +12,3 @@ spec:
          namespace: system
          name: webhook-service
          path: /convert
-      conversionReviewVersions:


I think we need to keep it as it is part of the scaffolding.

api/v1alpha1/drpolicy_types.go

ShyamsundarR · 2024-03-05T15:55:19Z

api/v1alpha1/drpolicy_types.go

 // DRPolicySpec defines the desired state of DRPolicy
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.schedulingInterval) || has(self.schedulingInterval)", message="schedulingInterval is required once set"


I think the pattern we need to use is Immutability upon object creation

For example with the current scheme a user would be allowed to set an empty value to begin with and then be allowed to modify it to a valid value, the immutability starts here and prevents further modification.

What we want is immutable post creation, so if the policy started with an empty value it should remain so.

updated the code with Immutability upon object creation pattern for schedulingInterval and drclusters only.

ShyamsundarR · 2024-03-12T23:38:33Z

Tested the DRPolicy PR out and here is what I see...

Initial DRPolicy created on a kind cluster extended with the CRD from the PR:

$ kubectl get drpolicy -A -o yaml
apiVersion: v1
items:
- apiVersion: ramendr.openshift.io/v1alpha1
  kind: DRPolicy
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"ramendr.openshift.io/v1alpha1","kind":"DRPolicy","metadata":{"annotations":{},"name":"drpolicy-sample"},"spec":{"drClusters":["east","west"],"schedulingInterval":"5m"}}
    creationTimestamp: "2024-03-12T22:19:00Z"
    generation: 1
    name: drpolicy-sample
    resourceVersion: "950"
    uid: e59bd15b-5178-476f-bbe7-de8af997831d
  spec:
    drClusters:
    - east
    - west
    schedulingInterval: 5m

Look at last-applied-configuration, does not have replicationClassSelector

Now changed the DRPolicy YAML to add a replicationClassSelector and applied it, and it was successful, and here is the get after that:

$ kubectl apply -f ./drp.yaml 
drpolicy.ramendr.openshift.io/drpolicy-sample configured

$ kubectl get drpolicy -A -o yaml
apiVersion: v1
items:
- apiVersion: ramendr.openshift.io/v1alpha1
  kind: DRPolicy
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"ramendr.openshift.io/v1alpha1","kind":"DRPolicy","metadata":{"annotations":{},"name":"drpolicy-sample"},"spec":{"drClusters":["east","west"],"replicationClassSelector":{"matchLabels":{"class":"ramen"}},"schedulingInterval":"5m"}}
    creationTimestamp: "2024-03-12T22:19:00Z"
    generation: 2
    name: drpolicy-sample
    resourceVersion: "967"
    uid: e59bd15b-5178-476f-bbe7-de8af997831d
  spec:
    drClusters:
    - east
    - west
    replicationClassSelector:
      matchLabels:
        class: ramen
    schedulingInterval: 5m
kind: List
metadata:
  resourceVersion: ""

IOW, if we start wit a YAML that has no replicationClassSelector, and then add it later then it is allowed, which we should not allow.

Another test I did was to create a policy with empty list of DRClusters (as below) and it passed. This is a new test case though as older code also allowed this to pass through:

$ kubectl get drpolicy -A -o yaml
apiVersion: v1
items:
- apiVersion: ramendr.openshift.io/v1alpha1
  kind: DRPolicy
  metadata:
    annotations:
      kubectl.kubernetes.io/last-applied-configuration: |
        {"apiVersion":"ramendr.openshift.io/v1alpha1","kind":"DRPolicy","metadata":{"annotations":{},"name":"drpolicy-sample"},"spec":{"drClusters":[],"replicationClassSelector":{"matchLabels":{"class":"ramen"}},"schedulingInterval":"5m"}}
    creationTimestamp: "2024-03-12T22:22:18Z"
    generation: 1
    name: drpolicy-sample
    resourceVersion: "1208"
    uid: 6755a4cb-cdfc-42f2-bb2e-e6a86fee267f
  spec:
    drClusters: []
    replicationClassSelector:
      matchLabels:
        class: ramen
    schedulingInterval: 5m
kind: List
metadata:
  resourceVersion: ""

Overall I think the CEL rules should be amended as below, such that we do not allow the above modifications:

diff --git a/api/v1alpha1/drpolicy_types.go b/api/v1alpha1/drpolicy_types.go
index c5fecea..ff7e80b 100644
--- a/api/v1alpha1/drpolicy_types.go
+++ b/api/v1alpha1/drpolicy_types.go
@@ -8,11 +8,7 @@ import (
 )
 
 // DRPolicySpec defines the desired state of DRPolicy
-// +kubebuilder:validation:XValidation:rule="!has(oldSelf.replicationClassSelector) || has(self.replicationClassSelector)", message="replicationClassSelector is required once set"
-// +kubebuilder:validation:XValidation:rule="!has(oldSelf.volumeSnapshotClassSelector) || has(self.volumeSnapshotClassSelector)", message="volumeSnapshotClassSelector is required once set"
 type DRPolicySpec struct {
-	// Important: Run "make" to regenerate code after modifying this file
-
 	// scheduling Interval for replicating Persistent Volume
 	// data to a peer cluster. Interval is typically in the
 	// form <num><m,h,d>. Here <num> is a number, 'm' means
@@ -25,19 +21,20 @@ type DRPolicySpec struct {
 	// Label selector to identify all the VolumeReplicationClasses.
 	// This selector is assumed to be the same for all subscriptions that
 	// need DR protection. It will be passed in to the VRG when it is created
-	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="replicationClassSelector is immutable"
-	ReplicationClassSelector metav1.LabelSelector `json:"replicationClassSelector,omitempty"`
+	ReplicationClassSelector metav1.LabelSelector `json:"replicationClassSelector"`
 
 	// Label selector to identify all the VolumeSnapshotClasses.
 	// This selector is assumed to be the same for all subscriptions that
 	// need DR protection. It will be passed in to the VRG when it is created
-	// +kubebuilder:validation:Optional
+	// +kubebuilder:validation:Required
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="volumeSnapshotClassSelector is immutable"
-	VolumeSnapshotClassSelector metav1.LabelSelector `json:"volumeSnapshotClassSelector,omitempty"`
+	VolumeSnapshotClassSelector metav1.LabelSelector `json:"volumeSnapshotClassSelector"`
 
 	// List of DRCluster resources that are governed by this policy
 	// +kubebuilder:validation:Required
+	// +kubebuilder:validation:XValidation:rule="size(self) == 2", message="drClusters requires a list of 2 clusters"
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="drClusters is immutable"
 	DRClusters []string `json:"drClusters"`
 }

For the tests, unfortunately as it uses a go struct for the DRPolicy, it is able to write out initial configuration with empty values, and so when we use YAML this is working differently.

I am not sure yet how to test with YAML and envtest, so that is unfortunate. We could test it in a kind cluster though and ensure that CEL rules are adhered to. I just used hack/setup-kind-cluster.sh to get a kind cluster, extended the CRD for DRPolicy and then tested with various YAMLs as above.

rakeshgm · 2024-03-13T11:07:17Z

Thanks for the detail input. I have pushed the changes.

Signed-off-by: rakeshgm <[email protected]>

excluding lll on long comments helps in kubebuilder validation rules to not be linted Signed-off-by: rakeshgm <[email protected]>

Signed-off-by: rakeshgm <[email protected]>

rakeshgm marked this pull request as draft January 16, 2024 09:11

rakeshgm force-pushed the rm-webhooks branch from 4ce676d to 2883fab Compare January 16, 2024 09:28

nirs reviewed Jan 16, 2024

View reviewed changes

rakeshgm force-pushed the rm-webhooks branch from 2883fab to a10cf6c Compare January 16, 2024 16:04

rakeshgm force-pushed the rm-webhooks branch 2 times, most recently from e2011af to 77d6fd6 Compare January 23, 2024 16:54

rakeshgm force-pushed the rm-webhooks branch 8 times, most recently from d9f2d72 to 698ca2c Compare February 12, 2024 17:50

rakeshgm marked this pull request as ready for review February 12, 2024 17:55

rakeshgm requested review from ShyamsundarR and raghavendra-talur February 13, 2024 09:48

rakeshgm force-pushed the rm-webhooks branch from 698ca2c to 742bbb0 Compare March 1, 2024 06:33

raghavendra-talur requested changes Mar 1, 2024

View reviewed changes

raghavendra-talur approved these changes Mar 1, 2024

View reviewed changes

ShyamsundarR reviewed Mar 4, 2024

View reviewed changes

api/v1alpha1/drpolicy_types.go Show resolved Hide resolved

rakeshgm force-pushed the rm-webhooks branch from 742bbb0 to b997696 Compare March 5, 2024 07:02

ShyamsundarR reviewed Mar 5, 2024

View reviewed changes

rakeshgm force-pushed the rm-webhooks branch from b997696 to a37b408 Compare March 5, 2024 16:43

rakeshgm changed the title ~~Remove DRPolicy Webhook~~ DRPolicy: Add CEL and remove webhook Mar 7, 2024

rakeshgm force-pushed the rm-webhooks branch 3 times, most recently from 88de718 to f6f82d2 Compare March 11, 2024 14:37

rakeshgm force-pushed the rm-webhooks branch from f6f82d2 to bfa83b6 Compare March 13, 2024 10:42

rakeshgm force-pushed the rm-webhooks branch from bfa83b6 to d3e3f3d Compare March 13, 2024 10:44

rakeshgm force-pushed the rm-webhooks branch from d3e3f3d to 9e7691c Compare March 13, 2024 11:09

rakeshgm added 3 commits March 13, 2024 18:13

remove DRPolicy webhook

7b830f6

Signed-off-by: rakeshgm <[email protected]>

linters/lll: exclude lll rule on long comments

755d275

excluding lll on long comments helps in kubebuilder validation rules to not be linted Signed-off-by: rakeshgm <[email protected]>

cel: DRPolicy CEL tests

1b556eb

Signed-off-by: rakeshgm <[email protected]>

rakeshgm force-pushed the rm-webhooks branch from 9e7691c to 1b556eb Compare March 13, 2024 12:43

ShyamsundarR approved these changes Mar 14, 2024

View reviewed changes

ShyamsundarR merged commit 16b07cf into RamenDR:main Mar 14, 2024
14 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRPolicy: Add CEL and remove webhook #1175

DRPolicy: Add CEL and remove webhook #1175

rakeshgm commented Jan 16, 2024

nirs Jan 16, 2024

rakeshgm Jan 16, 2024 •

edited

Loading

nirs Jan 16, 2024

rakeshgm Jan 16, 2024

raghavendra-talur Mar 1, 2024

raghavendra-talur Mar 1, 2024

ShyamsundarR Mar 5, 2024

rakeshgm Mar 13, 2024

raghavendra-talur Mar 1, 2024

ShyamsundarR Mar 5, 2024

rakeshgm Mar 7, 2024 •

edited

Loading

ShyamsundarR commented Mar 12, 2024

rakeshgm commented Mar 13, 2024 •

edited

Loading

		// DRPolicySpec defines the desired state of DRPolicy
		// +kubebuilder:validation:XValidation:rule="!has(oldSelf.schedulingInterval) \|\| has(self.schedulingInterval)", message="schedulingInterval is required once set"

DRPolicy: Add CEL and remove webhook #1175

DRPolicy: Add CEL and remove webhook #1175

Conversation

rakeshgm commented Jan 16, 2024

Choose a reason for hiding this comment

rakeshgm Jan 16, 2024 • 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

rakeshgm Mar 7, 2024 • edited Loading

Choose a reason for hiding this comment

ShyamsundarR commented Mar 12, 2024

rakeshgm commented Mar 13, 2024 • edited Loading

rakeshgm Jan 16, 2024 •

edited

Loading

rakeshgm Mar 7, 2024 •

edited

Loading

rakeshgm commented Mar 13, 2024 •

edited

Loading