Repository: GoogleCloudPlatform/cloud-sql-proxy-operator
Pull request #212 (2023-02-21 18:28:38)

feat: Updating the RolloutStrategy field is not allowed.
RolloutStrategy controls when and how changes to the AuthProxyWorkload are rolled out to 
running workloads. Changing this value after creation would cause an inconsistent rollout state. 
 
Related to #36


Patch:
diff --git a/internal/api/v1alpha1/authproxyworkload_webhook.go b/internal/api/v1alpha1/authproxyworkload_webhook.go
index 5f96d31..d531030 100644
--- a/internal/api/v1alpha1/authproxyworkload_webhook.go
+++ b/internal/api/v1alpha1/authproxyworkload_webhook.go
@@ -121,9 +121,35 @@ func (r *AuthProxyWorkload) validateUpdateFrom(op *AuthProxyWorkload) field.Erro
 			"selector cannot be changed on update"))
 	}
 
+	allErrs = append(allErrs, validateRolloutStrategyChange(r.Spec.AuthProxyContainer, op.Spec.AuthProxyContainer)...)
+
 	return allErrs
 }
 
+// validateRolloutStrategyChange ensures that the rollout strategy does not
+// change on update, taking default values into account.
+func validateRolloutStrategyChange(c *AuthProxyContainerSpec, oc *AuthProxyContainerSpec) []*field.Error {
+	var allErrs field.ErrorList
+	var (
+		s  = WorkloadStrategy
+		os = WorkloadStrategy
+	)
+	if c != nil && c.RolloutStrategy != "" {
+		s = c.RolloutStrategy
+	}
+	if oc != nil && oc.RolloutStrategy != "" {
+		os = oc.RolloutStrategy
+	}
+	if s != os {
+		allErrs = append(allErrs, field.Invalid(
+			field.NewPath("spec", "authProxyContainer", "rolloutStrategy"), s,
+			fmt.Sprintf("rolloutStrategy cannot be changed on update from %s", os)))
+	}
+
+	return allErrs
+
+}
+
 func selectorNotEqual(s *metav1.LabelSelector, os *metav1.LabelSelector) bool {
 	if s == nil && os == nil {
 		return false


Test patch:
diff --git a/internal/api/v1alpha1/authproxyworkload_test.go b/internal/api/v1alpha1/authproxyworkload_test.go
index d6e2a64..88f1ce1 100644
--- a/internal/api/v1alpha1/authproxyworkload_test.go
+++ b/internal/api/v1alpha1/authproxyworkload_test.go
@@ -92,6 +92,7 @@ func TestAuthProxyWorkload_ValidateCreate(t *testing.T) {
 				ObjectMeta: v1.ObjectMeta{Name: "sample"},
 				Spec:       tc.spec,
 			}
+			p.Default()
 			err := p.ValidateCreate()
 			gotValid := err == nil
 			switch {
@@ -219,7 +220,6 @@ func TestAuthProxyWorkload_ValidateUpdate(t *testing.T) {
 					PortEnvName:      "DB_PORT",
 				}},
 			},
-			wantValid: false,
 		},
 	}
 
@@ -233,13 +233,119 @@ func TestAuthProxyWorkload_ValidateUpdate(t *testing.T) {
 				ObjectMeta: v1.ObjectMeta{Name: "sample"},
 				Spec:       tc.oldSpec,
 			}
+			p.Default()
+			oldP.Default()
 
 			err := p.ValidateUpdate(&oldP)
 			gotValid := err == nil
 
 			switch {
 			case tc.wantValid && !gotValid:
-				t.Errorf("wants create valid, got error %v", err)
+				t.Errorf("wants update valid, got error %v", err)
+			case !tc.wantValid && gotValid:
+				t.Errorf("wants an error on update, got no error")
+			default:
+				t.Logf("update passed %s", tc.desc)
+				// test passes, do nothing.
+			}
+		})
+	}
+}
+
+func TestAuthProxyWorkload_ValidateUpdate_AuthProxyContainerSpec(t *testing.T) {
+	data := []struct {
+		desc      string
+		spec      *cloudsqlapi.AuthProxyContainerSpec
+		oldSpec   *cloudsqlapi.AuthProxyContainerSpec
+		wantValid bool
+	}{
+		{
+			desc: "Invalid when AuthProxyContainerSpec.RolloutStrategy changes from explict to different default value",
+			spec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "None",
+			},
+			oldSpec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "Workload",
+			},
+		},
+		{
+			desc: "Valid when AuthProxyContainerSpec.RolloutStrategy goes from default to same explicit value",
+			spec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "Workload",
+			},
+			wantValid: true,
+		},
+		{
+			desc: "Invalid when AuthProxyContainerSpec.RolloutStrategy changes from default to different explicit value",
+			spec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "None",
+			},
+			wantValid: false,
+		},
+		{
+			desc: "Invalid when AuthProxyContainerSpec.RolloutStrategy changes to different explicit value",
+			spec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "None",
+			},
+			oldSpec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "Workload",
+			},
+			wantValid: false,
+		},
+		{
+			desc: "Invalid when AuthProxyContainerSpec.RolloutStrategy changes from explict to different default value",
+			spec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "None",
+			},
+			oldSpec: &cloudsqlapi.AuthProxyContainerSpec{
+				RolloutStrategy: "Workload",
+			},
+			wantValid: false,
+		},
+	}
+	for _, tc := range data {
+		t.Run(tc.desc, func(t *testing.T) {
+			p := cloudsqlapi.AuthProxyWorkload{
+				ObjectMeta: v1.ObjectMeta{Name: "sample"},
+				Spec: cloudsqlapi.AuthProxyWorkloadSpec{
+					Workload: cloudsqlapi.WorkloadSelectorSpec{
+						Kind: "Deployment",
+						Selector: &v1.LabelSelector{
+							MatchLabels: map[string]string{"app": "sample"},
+						},
+					},
+					AuthProxyContainer: tc.spec,
+					Instances: []cloudsqlapi.InstanceSpec{{
+						ConnectionString: "proj:region:db2",
+						PortEnvName:      "DB_PORT",
+					}},
+				},
+			}
+			oldP := cloudsqlapi.AuthProxyWorkload{
+				ObjectMeta: v1.ObjectMeta{Name: "sample"},
+				Spec: cloudsqlapi.AuthProxyWorkloadSpec{
+					Workload: cloudsqlapi.WorkloadSelectorSpec{
+						Kind: "Deployment",
+						Selector: &v1.LabelSelector{
+							MatchLabels: map[string]string{"app": "sample"},
+						},
+					},
+					AuthProxyContainer: tc.oldSpec,
+					Instances: []cloudsqlapi.InstanceSpec{{
+						ConnectionString: "proj:region:db2",
+						PortEnvName:      "DB_PORT",
+					}},
+				},
+			}
+			p.Default()
+			oldP.Default()
+
+			err := p.ValidateUpdate(&oldP)
+			gotValid := err == nil
+
+			switch {
+			case tc.wantValid && !gotValid:
+				t.Errorf("wants update valid, got error %v", err)
 			case !tc.wantValid && gotValid:
 				t.Errorf("wants an error on update, got no error")
 			default:
@@ -248,6 +354,7 @@ func TestAuthProxyWorkload_ValidateUpdate(t *testing.T) {
 			}
 		})
 	}
+
 }
 
 func printFieldErrors(t *testing.T, err error) {