feat: Validate AuthProxyWorkload updates to prevent changes to the wo…

…rkload selector. (#211) 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
GoogleCloudPlatform · Feb 21, 2023 · 4304283 · 4304283
1 parent 3ede42d
commit 4304283
Show file tree

Hide file tree

Showing 2 changed files with 144 additions and 13 deletions.
diff --git 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.

diff --git 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,33 +55,85 @@ 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
 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"}