Repository: GoogleCloudPlatform/cloud-sql-proxy-operator
Pull request #211 (2023-02-17 23:11:14)

feat: Validate AuthProxyWorkload updates to prevent changes to the workload selector.
We do not allow a user to change the selector of an AuthProxyWorkload, as it will require the operator to do
too much bookkeeping. We can keep our code clean and simple if the operator assumes that the
workload selected for a proxy configuration never changes. 

Related to #36 


Patch:
diff --git a/internal/api/v1alpha1/authproxyworkload_webhook.go b/internal/api/v1alpha1/authproxyworkload_webhook.go
index c05cafd..5f96d31 100644
--- a/internal/api/v1alpha1/authproxyworkload_webhook.go
+++ b/internal/api/v1alpha1/authproxyworkload_webhook.go
@@ -16,8 +16,10 @@ package v1alpha1
 
 import (
 	"fmt"
+	"reflect"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
@@ -53,12 +55,36 @@ var _ webhook.Validator = &AuthProxyWorkload{}
 
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
 func (r *AuthProxyWorkload) ValidateCreate() error {
-	return r.validate()
+	allErrs := r.validate()
+	if len(allErrs) > 0 {
+		return apierrors.NewInvalid(
+			schema.GroupKind{
+				Group: GroupVersion.Group,
+				Kind:  "AuthProxyWorkload"},
+			r.Name, allErrs)
+	}
+	return nil
+
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
-func (r *AuthProxyWorkload) ValidateUpdate(_ runtime.Object) error {
-	return r.validate()
+func (r *AuthProxyWorkload) ValidateUpdate(old runtime.Object) error {
+	o, ok := old.(*AuthProxyWorkload)
+	if !ok {
+		return fmt.Errorf("bad request, expected old to be an AuthProxyWorkload")
+	}
+
+	allErrs := r.validate()
+	allErrs = append(allErrs, r.validateUpdateFrom(o)...)
+	if len(allErrs) > 0 {
+		return apierrors.NewInvalid(
+			schema.GroupKind{
+				Group: GroupVersion.Group,
+				Kind:  "AuthProxyWorkload"},
+			r.Name, allErrs)
+	}
+	return nil
+
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
@@ -66,20 +92,48 @@ func (r *AuthProxyWorkload) ValidateDelete() error {
 	return nil
 }
 
-func (r *AuthProxyWorkload) validate() error {
+func (r *AuthProxyWorkload) validate() field.ErrorList {
 	var allErrs field.ErrorList
 
 	allErrs = append(allErrs, validation.ValidateLabelName(r.Name, field.NewPath("metadata", "name"))...)
 	allErrs = append(allErrs, validateWorkload(&r.Spec.Workload, field.NewPath("spec", "workload"))...)
 
-	if len(allErrs) > 0 {
-		return apierrors.NewInvalid(
-			schema.GroupKind{
-				Group: GroupVersion.Group,
-				Kind:  "AuthProxyWorkload"},
-			r.Name, allErrs)
+	return allErrs
+
+}
+
+func (r *AuthProxyWorkload) validateUpdateFrom(op *AuthProxyWorkload) field.ErrorList {
+	var allErrs field.ErrorList
+
+	if r.Spec.Workload.Kind != op.Spec.Workload.Kind {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "workload", "kind"), r.Spec.Workload.Kind,
+			"kind cannot be changed on update"))
 	}
-	return nil
+	if r.Spec.Workload.Name != op.Spec.Workload.Name {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "workload", "name"), r.Spec.Workload.Name,
+			"kind cannot be changed on update"))
+	}
+	if selectorNotEqual(r.Spec.Workload.Selector, op.Spec.Workload.Selector) {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "workload", "selector"), r.Spec.Workload.Selector,
+			"selector cannot be changed on update"))
+	}
+
+	return allErrs
+}
+
+func selectorNotEqual(s *metav1.LabelSelector, os *metav1.LabelSelector) bool {
+	if s == nil && os == nil {
+		return false
+	}
+
+	if s != nil && os != nil {
+		return !reflect.DeepEqual(s, os)
+	}
+
+	return true
 }
 
 var supportedKinds = []string{"CronJob", "Job", "StatefulSet", "Deployment", "DaemonSet", "ReplicaSet", "Pod"}


Test patch:
diff --git a/internal/api/v1alpha1/authproxyworkload_test.go b/internal/api/v1alpha1/authproxyworkload_test.go
index 91faef8..d6e2a64 100644
--- a/internal/api/v1alpha1/authproxyworkload_test.go
+++ b/internal/api/v1alpha1/authproxyworkload_test.go
@@ -145,6 +145,82 @@ func TestAuthProxyWorkload_ValidateUpdate(t *testing.T) {
 			},
 			wantValid: true,
 		},
+		{
+			desc: "Invalid, WorkloadSelectorSpec.Kind changed",
+			spec: cloudsqlapi.AuthProxyWorkloadSpec{
+				Workload: cloudsqlapi.WorkloadSelectorSpec{
+					Kind: "Deployment",
+					Name: "webapp",
+				},
+				Instances: []cloudsqlapi.InstanceSpec{{
+					ConnectionString: "proj:region:db2",
+					PortEnvName:      "DB_PORT",
+				}},
+			},
+			oldSpec: cloudsqlapi.AuthProxyWorkloadSpec{
+				Workload: cloudsqlapi.WorkloadSelectorSpec{
+					Kind: "StatefulSet",
+					Name: "webapp",
+				},
+				Instances: []cloudsqlapi.InstanceSpec{{
+					ConnectionString: "proj:region:db1",
+					PortEnvName:      "DB_PORT",
+				}},
+			},
+			wantValid: false,
+		},
+		{
+			desc: "Invalid, WorkloadSelectorSpec.Name changed",
+			spec: cloudsqlapi.AuthProxyWorkloadSpec{
+				Workload: cloudsqlapi.WorkloadSelectorSpec{
+					Kind: "Deployment",
+					Name: "things",
+				},
+				Instances: []cloudsqlapi.InstanceSpec{{
+					ConnectionString: "proj:region:db2",
+					PortEnvName:      "DB_PORT",
+				}},
+			},
+			oldSpec: cloudsqlapi.AuthProxyWorkloadSpec{
+				Workload: cloudsqlapi.WorkloadSelectorSpec{
+					Kind: "Deployment",
+					Name: "webapp",
+				},
+				Instances: []cloudsqlapi.InstanceSpec{{
+					ConnectionString: "proj:region:db1",
+					PortEnvName:      "DB_PORT",
+				}},
+			},
+			wantValid: false,
+		},
+		{
+			desc: "Invalid, WorkloadSelectorSpec.Selector changed",
+			spec: cloudsqlapi.AuthProxyWorkloadSpec{
+				Workload: cloudsqlapi.WorkloadSelectorSpec{
+					Kind: "Deployment",
+					Selector: &v1.LabelSelector{
+						MatchLabels: map[string]string{"app": "sample"},
+					},
+				},
+				Instances: []cloudsqlapi.InstanceSpec{{
+					ConnectionString: "proj:region:db2",
+					PortEnvName:      "DB_PORT",
+				}},
+			},
+			oldSpec: cloudsqlapi.AuthProxyWorkloadSpec{
+				Workload: cloudsqlapi.WorkloadSelectorSpec{
+					Kind: "Deployment",
+					Selector: &v1.LabelSelector{
+						MatchLabels: map[string]string{"app": "other"},
+					},
+				},
+				Instances: []cloudsqlapi.InstanceSpec{{
+					ConnectionString: "proj:region:db1",
+					PortEnvName:      "DB_PORT",
+				}},
+			},
+			wantValid: false,
+		},
 	}
 
 	for _, tc := range data {
@@ -155,7 +231,8 @@ func TestAuthProxyWorkload_ValidateUpdate(t *testing.T) {
 			}
 			oldP := cloudsqlapi.AuthProxyWorkload{
 				ObjectMeta: v1.ObjectMeta{Name: "sample"},
-				Spec:       tc.oldSpec}
+				Spec:       tc.oldSpec,
+			}
 
 			err := p.ValidateUpdate(&oldP)
 			gotValid := err == nil
@@ -164,7 +241,7 @@ func TestAuthProxyWorkload_ValidateUpdate(t *testing.T) {
 			case tc.wantValid && !gotValid:
 				t.Errorf("wants create valid, got error %v", err)
 			case !tc.wantValid && gotValid:
-				t.Errorf("wants an error on create, got no error")
+				t.Errorf("wants an error on update, got no error")
 			default:
 				t.Logf("update passed %s", tc.desc)
 				// test passes, do nothing.