feat(dataplane): move DataPlane image validation to CRD CEL rule (#999)

* feat(dataplane): move DataPlane image validation to CRD CEL rule * tests: fix tests
Kong · Jan 14, 2025 · b8a34da · b8a34da
1 parent 8e683c6
commit b8a34da
Show file tree

Hide file tree

Showing 9 changed files with 87 additions and 40 deletions.
diff --git a/api/v1beta1/controlplane_types.go b/api/v1beta1/controlplane_types.go
@@ -35,6 +35,7 @@ func init() {
 // +kubebuilder:resource:shortName=kocp,categories=kong;all
 // +kubebuilder:printcolumn:name="Ready",description="The Resource is ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status`
 // +kubebuilder:printcolumn:name="Provisioned",description="The Resource is provisioned",type=string,JSONPath=`.status.conditions[?(@.type=='Provisioned')].status`
+// +kubebuilder:validation:XValidation:message="ControlPlane requires an image to be set on controller container",rule="has(self.spec.deployment.podTemplateSpec) && has(self.spec.deployment.podTemplateSpec.spec.containers) && self.spec.deployment.podTemplateSpec.spec.containers.exists(c, c.name == 'controller' && has(c.image))"
 
 // ControlPlane is the Schema for the controlplanes API
 // +apireference:kgo:include
@@ -56,8 +57,6 @@ type ControlPlaneList struct {
 	Items           []ControlPlane `json:"items"`
 }
 
-// +kubebuilder:validation:XValidation:message="ControlPlane requires an image to be set on controller container",rule="has(self.deployment.podTemplateSpec) && has(self.deployment.podTemplateSpec.spec.containers) && self.deployment.podTemplateSpec.spec.containers.exists(c, c.name == 'controller' && has(c.image))"
-
 // ControlPlaneSpec defines the desired state of ControlPlane
 // +apireference:kgo:include
 type ControlPlaneSpec struct {

diff --git a/api/v1beta1/dataplane_types.go b/api/v1beta1/dataplane_types.go
@@ -36,6 +36,7 @@ func init() {
 // +kubebuilder:resource:shortName=kodp,categories=kong;all
 // +kubebuilder:validation:XValidation:message="Extension not allowed for DataPlane",rule="has(self.spec.extensions) ? self.spec.extensions.all(e, e.group == 'gateway-operator.konghq.com' && e.kind == 'KonnectExtension') : true"
 // +kubebuilder:printcolumn:name="Ready",description="The Resource is ready",type=string,JSONPath=`.status.conditions[?(@.type=='Ready')].status`
+// +kubebuilder:validation:XValidation:message="DataPlane requires an image to be set on proxy container",rule="has(self.spec.deployment.podTemplateSpec) && has(self.spec.deployment.podTemplateSpec.spec.containers) && self.spec.deployment.podTemplateSpec.spec.containers.exists(c, c.name == 'proxy' && has(c.image))"
 
 // DataPlane is the Schema for the dataplanes API
 // +apireference:kgo:include

diff --git a/config/crd/bases/gateway-operator.konghq.com_controlplanes.yaml b/config/crd/bases/gateway-operator.konghq.com_controlplanes.yaml
@@ -8256,11 +8256,6 @@ spec:
                   If omitted, Ingress resources will not be supported by the ControlPlane.
                 type: string
             type: object
-            x-kubernetes-validations:
-            - message: ControlPlane requires an image to be set on controller container
-              rule: has(self.deployment.podTemplateSpec) && has(self.deployment.podTemplateSpec.spec.containers)
-                && self.deployment.podTemplateSpec.spec.containers.exists(c, c.name
-                == 'controller' && has(c.image))
           status:
             description: ControlPlaneStatus defines the observed state of ControlPlane
             properties:
@@ -8333,6 +8328,11 @@ spec:
                 x-kubernetes-list-type: map
             type: object
         type: object
+        x-kubernetes-validations:
+        - message: ControlPlane requires an image to be set on controller container
+          rule: has(self.spec.deployment.podTemplateSpec) && has(self.spec.deployment.podTemplateSpec.spec.containers)
+            && self.spec.deployment.podTemplateSpec.spec.containers.exists(c, c.name
+            == 'controller' && has(c.image))
     served: true
     storage: true
     subresources:

diff --git a/config/crd/bases/gateway-operator.konghq.com_dataplanes.yaml b/config/crd/bases/gateway-operator.konghq.com_dataplanes.yaml
@@ -9450,6 +9450,10 @@ spec:
         - message: Extension not allowed for DataPlane
           rule: 'has(self.spec.extensions) ? self.spec.extensions.all(e, e.group ==
             ''gateway-operator.konghq.com'' && e.kind == ''KonnectExtension'') : true'
+        - message: DataPlane requires an image to be set on proxy container
+          rule: has(self.spec.deployment.podTemplateSpec) && has(self.spec.deployment.podTemplateSpec.spec.containers)
+            && self.spec.deployment.podTemplateSpec.spec.containers.exists(c, c.name
+            == 'proxy' && has(c.image))
     served: true
     storage: true
     subresources:

diff --git a/config/crd/dataplane/gateway-operator.konghq.com_dataplanes.yaml b/config/crd/dataplane/gateway-operator.konghq.com_dataplanes.yaml
@@ -9450,6 +9450,10 @@ spec:
         - message: Extension not allowed for DataPlane
           rule: 'has(self.spec.extensions) ? self.spec.extensions.all(e, e.group ==
             ''gateway-operator.konghq.com'' && e.kind == ''KonnectExtension'') : true'
+        - message: DataPlane requires an image to be set on proxy container
+          rule: has(self.spec.deployment.podTemplateSpec) && has(self.spec.deployment.podTemplateSpec.spec.containers)
+            && self.spec.deployment.podTemplateSpec.spec.containers.exists(c, c.name
+            == 'proxy' && has(c.image))
     served: true
     storage: true
     subresources:

diff --git a/internal/validation/dataplane/validation.go b/internal/validation/dataplane/validation.go
@@ -112,10 +112,6 @@ func (v *Validator) ValidateDataPlaneDeploymentOptions(namespace string, opts *o
 		return fmt.Errorf("couldn't find proxy container in DataPlane spec")
 	}
 
-	if container.Image == "" {
-		return errors.New("DataPlane requires an image")
-	}
-
 	// validate db mode.
 	dbMode, _, err := k8sutils.GetEnvValueFromContainer(context.Background(), container, namespace, consts.EnvVarKongDatabase, v.c)
 	if err != nil {

diff --git a/modules/admission/server_test.go b/modules/admission/server_test.go
@@ -357,34 +357,6 @@ func TestHandleDataPlaneValidation(t *testing.T) {
 			hasError: true,
 			errMsg:   "database backend xxx of DataPlane not supported currently",
 		},
-		{
-			name: "validate_error:missing_container_image",
-			dataplane: &operatorv1beta1.DataPlane{
-				ObjectMeta: metav1.ObjectMeta{
-					Name:      "test-db-off-in-secret",
-					Namespace: "default",
-				},
-				Spec: operatorv1beta1.DataPlaneSpec{
-					DataPlaneOptions: operatorv1beta1.DataPlaneOptions{
-						Deployment: operatorv1beta1.DataPlaneDeploymentOptions{
-							DeploymentOptions: operatorv1beta1.DeploymentOptions{
-								PodTemplateSpec: &corev1.PodTemplateSpec{
-									Spec: corev1.PodSpec{
-										Containers: []corev1.Container{
-											{
-												Name: consts.DataPlaneProxyContainerName,
-											},
-										},
-									},
-								},
-							},
-						},
-					},
-				},
-			},
-			hasError: true,
-			errMsg:   "DataPlane requires an image",
-		},
 	}
 
 	for _, tc := range testCases {

diff --git a/test/crdsvalidation/dataplane_test.go b/test/crdsvalidation/dataplane_test.go
@@ -0,0 +1,65 @@
+package crdsvalidation
+
+import (
+	"testing"
+
+	"github.com/samber/lo"
+	"golang.org/x/net/context"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	operatorv1beta1 "github.com/kong/gateway-operator/api/v1beta1"
+	"github.com/kong/gateway-operator/modules/manager/scheme"
+	"github.com/kong/gateway-operator/test/envtest"
+
+	kcfgcrdsvalidation "github.com/kong/kubernetes-configuration/test/crdsvalidation"
+)
+
+func TestDataPlane(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	cfg, ns := envtest.Setup(t, ctx, scheme.Get())
+
+	t.Run("spec", func(t *testing.T) {
+		kcfgcrdsvalidation.TestCasesGroup[*operatorv1beta1.DataPlane]{
+			{
+				Name: "not providing image fails",
+				TestObject: &operatorv1beta1.DataPlane{
+					ObjectMeta: metav1.ObjectMeta{
+						GenerateName: "dp-",
+						Namespace:    ns.Name,
+					},
+					Spec: operatorv1beta1.DataPlaneSpec{},
+				},
+				ExpectedErrorMessage: lo.ToPtr("DataPlane requires an image to be set on proxy container"),
+			},
+			{
+				Name: "providing image succeeds",
+				TestObject: &operatorv1beta1.DataPlane{
+					ObjectMeta: metav1.ObjectMeta{
+						GenerateName: "dp-",
+						Namespace:    ns.Name,
+					},
+					Spec: operatorv1beta1.DataPlaneSpec{
+						DataPlaneOptions: operatorv1beta1.DataPlaneOptions{
+							Deployment: operatorv1beta1.DataPlaneDeploymentOptions{
+								DeploymentOptions: operatorv1beta1.DeploymentOptions{
+									PodTemplateSpec: &corev1.PodTemplateSpec{
+										Spec: corev1.PodSpec{
+											Containers: []corev1.Container{
+												{
+													Name:  "proxy",
+													Image: "kong:3.9",
+												},
+											},
+										},
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		}.RunWithConfig(t, cfg, scheme.Get())
+	})
+}
diff --git a/test/integration/test_validating.go b/test/integration/test_validating.go
@@ -49,6 +49,7 @@ func testDataPlaneReconcileValidation(t *testing.T, namespace *corev1.Namespace)
 	testCases := []struct {
 		name             string
 		dataplane        *operatorv1beta1.DataPlane
+		creationErr      bool
 		validatingOK     bool
 		conditionMessage string
 	}{
@@ -60,7 +61,7 @@ func testDataPlaneReconcileValidation(t *testing.T, namespace *corev1.Namespace)
 					Name:      uuid.NewString(),
 				},
 			},
-			validatingOK:     false,
+			creationErr:      true,
 			conditionMessage: "DataPlane requires an image",
 		},
 
@@ -180,6 +181,11 @@ func testDataPlaneReconcileValidation(t *testing.T, namespace *corev1.Namespace)
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
 			dataplane, err := dataplaneClient.Create(GetCtx(), tc.dataplane, metav1.CreateOptions{})
+			if tc.creationErr {
+				require.Error(t, err, "should return error when create dataplane for case %s", tc.name)
+				return
+			}
+
 			require.NoErrorf(t, err, "should not return error when create dataplane for case %s", tc.name)
 
 			if tc.validatingOK {