Repository: GoogleCloudPlatform/cloud-sql-proxy-operator
Pull request #202 (2023-02-08 22:10:31)

feat: add new field RolloutStrategy control automatic rollout
Adds a new field `RolloutStrategy` to the AuthProxyWorkload that will allow users to control how
the operator will roll out changes when an AuthProxyWorkload is updated.

`RolloutStrategy` has two possible values: 

- `Workload` in which the operator will automatically follow the behavior of the Strategy set on the workload.
- `None` in which the operator will not attempt to roll out changes

hessjcg: Regarding E2E tests: In my judgement, an E2E test case doesn't add a lot of additional quality assurance above the unit test, and it introduces another source of flakey test errors. I'm inclined to skip the e2e test. 

For me, the unit test that shows that the Reconcile() will not modify a deployment when RolloutStrategy == "None" is enough. 

Patch:
diff --git a/config/crd/bases/cloudsql.cloud.google.com_authproxyworkloads.yaml b/config/crd/bases/cloudsql.cloud.google.com_authproxyworkloads.yaml
index 0f1720d..e373e39 100644
--- a/config/crd/bases/cloudsql.cloud.google.com_authproxyworkloads.yaml
+++ b/config/crd/bases/cloudsql.cloud.google.com_authproxyworkloads.yaml
@@ -862,6 +862,13 @@ spec:
                           description: 'Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
                           type: object
                       type: object
+                    rolloutStrategy:
+                      default: Workload
+                      description: 'RolloutStrategy indicates the strategy to use when rolling out changes to the workloads affected by the results. When this is set to `Workload`, changes to this resource will be automatically applied to a running Deployment, StatefulSet, DaemonSet, or ReplicaSet in accordance with the Strategy set on that workload. When this is set to `None`, the operator will take no action to roll out changes to affected workloads. `Workload` will be used by default if no value is set. See: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy'
+                      enum:
+                        - Workload
+                        - None
+                      type: string
                     sqlAdminAPIEndpoint:
                       description: SQLAdminAPIEndpoint is a debugging parameter that when specified will change the Google Cloud api endpoint used by the proxy.
                       type: string
diff --git a/installer/cloud-sql-proxy-operator.yaml b/installer/cloud-sql-proxy-operator.yaml
index c58e32e..81b2d02 100644
--- a/installer/cloud-sql-proxy-operator.yaml
+++ b/installer/cloud-sql-proxy-operator.yaml
@@ -880,6 +880,13 @@ spec:
                           description: 'Requests describes the minimum amount of compute resources required. If Requests is omitted for a container, it defaults to Limits if that is explicitly specified, otherwise to an implementation-defined value. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
                           type: object
                       type: object
+                    rolloutStrategy:
+                      default: Workload
+                      description: 'RolloutStrategy indicates the strategy to use when rolling out changes to the workloads affected by the results. When this is set to `Workload`, changes to this resource will be automatically applied to a running Deployment, StatefulSet, DaemonSet, or ReplicaSet in accordance with the Strategy set on that workload. When this is set to `None`, the operator will take no action to roll out changes to affected workloads. `Workload` will be used by default if no value is set. See: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy'
+                      enum:
+                        - Workload
+                        - None
+                      type: string
                     sqlAdminAPIEndpoint:
                       description: SQLAdminAPIEndpoint is a debugging parameter that when specified will change the Google Cloud api endpoint used by the proxy.
                       type: string
diff --git a/internal/api/v1alpha1/authproxyworkload_types.go b/internal/api/v1alpha1/authproxyworkload_types.go
index 04c7348..659ca74 100644
--- a/internal/api/v1alpha1/authproxyworkload_types.go
+++ b/internal/api/v1alpha1/authproxyworkload_types.go
@@ -62,6 +62,18 @@ const (
 	// ReasonUpToDate relates to condition WorkloadUpToDate, this reason is set
 	// when there are no workloads related to this AuthProxyWorkload resource.
 	ReasonUpToDate = "UpToDate"
+
+	// WorkloadStrategy is the RolloutStrategy value that indicates that
+	// when the AuthProxyWorkload is updated or deleted, the changes should be
+	// applied to affected workloads (Deployments, StatefulSets, etc.) following
+	// the Strategy defined by that workload.
+	// See: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy
+	WorkloadStrategy = "Workload"
+
+	// NoneStrategy is the RolloutStrategy value that indicates that the.
+	// when the AuthProxyWorkload is updated or deleted, no action should be taken
+	// by the operator to update the affected workloads.
+	NoneStrategy = "None"
 )
 
 // AuthProxyWorkloadSpec defines the desired state of AuthProxyWorkload
@@ -144,6 +156,19 @@ type AuthProxyContainerSpec struct {
 	// will use the latest known compatible proxy image.
 	//+kubebuilder:validation:Optional
 	Image string `json:"image,omitempty"`
+
+	// RolloutStrategy indicates the strategy to use when rolling out changes to
+	// the workloads affected by the results. When this is set to
+	// `Workload`, changes to this resource will be automatically applied
+	// to a running Deployment, StatefulSet, DaemonSet, or ReplicaSet in
+	// accordance with the Strategy set on that workload. When this is set to
+	// `None`, the operator will take no action to roll out changes to affected
+	// workloads. `Workload` will be used by default if no value is set.
+	// See: https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy
+	//+kubebuilder:validation:Optional
+	//+kubebuilder:validation:Enum=Workload;None
+	//+kubebuilder:default=Workload
+	RolloutStrategy string `json:"rolloutStrategy,omitempty"`
 }
 
 // InstanceSpec describes the configuration for how the proxy should expose
diff --git a/internal/api/v1alpha1/authproxyworkload_webhook.go b/internal/api/v1alpha1/authproxyworkload_webhook.go
index 65f0913..d088090 100644
--- a/internal/api/v1alpha1/authproxyworkload_webhook.go
+++ b/internal/api/v1alpha1/authproxyworkload_webhook.go
@@ -39,7 +39,10 @@ var _ webhook.Defaulter = &AuthProxyWorkload{}
 // Default implements webhook.Defaulter so a webhook will be registered for the type
 func (r *AuthProxyWorkload) Default() {
 	authproxyworkloadlog.Info("default", "name", r.Name)
-	// TODO(user): fill in your defaulting logic.
+	if r.Spec.AuthProxyContainer != nil &&
+		r.Spec.AuthProxyContainer.RolloutStrategy == "" {
+		r.Spec.AuthProxyContainer.RolloutStrategy = WorkloadStrategy
+	}
 }
 
 // TODO(user): change verbs to "verbs=create;update;delete" if you want to enable deletion validation.
diff --git a/internal/controller/authproxyworkload_controller.go b/internal/controller/authproxyworkload_controller.go
index e808bfc..d5961e8 100644
--- a/internal/controller/authproxyworkload_controller.go
+++ b/internal/controller/authproxyworkload_controller.go
@@ -276,6 +276,10 @@ func (r *AuthProxyWorkloadReconciler) needsAnnotationUpdate(wl workload.Workload
 		return false
 	}
 
+	if isRolloutStrategyNone(resource) {
+		return false
+	}
+
 	k, v := workload.PodAnnotation(resource)
 	// Check if the correct annotation exists
 	an := wl.PodTemplateAnnotations()
@@ -295,6 +299,11 @@ func (r *AuthProxyWorkloadReconciler) updateAnnotation(wl workload.Workload, res
 		return
 	}
 
+	// The user has set "None" as the rollout strategy. Ignore it.
+	if isRolloutStrategyNone(resource) {
+		return
+	}
+
 	k, v := workload.PodAnnotation(resource)
 
 	// add the annotation if needed...
@@ -307,6 +316,12 @@ func (r *AuthProxyWorkloadReconciler) updateAnnotation(wl workload.Workload, res
 	mpt.SetPodTemplateAnnotations(an)
 }
 
+// isRolloutStrategyNone returns true when user has set "None" as the rollout strategy.
+func isRolloutStrategyNone(resource *cloudsqlapi.AuthProxyWorkload) bool {
+	return resource.Spec.AuthProxyContainer != nil &&
+		resource.Spec.AuthProxyContainer.RolloutStrategy == cloudsqlapi.NoneStrategy
+}
+
 // workloadsReconciled  State 3.1: If workloads are all up to date, mark the condition
 // "UpToDate" true and do not requeue.
 func (r *AuthProxyWorkloadReconciler) reconcileResult(ctx context.Context, l logr.Logger, resource, orig *cloudsqlapi.AuthProxyWorkload, reason, message string, upToDate bool) (ctrl.Result, error) {
diff --git a/internal/workload/podspec_updates.go b/internal/workload/podspec_updates.go
index 4607ba4..c815a44 100644
--- a/internal/workload/podspec_updates.go
+++ b/internal/workload/podspec_updates.go
@@ -55,7 +55,7 @@ func PodAnnotation(r *cloudsqlapi.AuthProxyWorkload) (string, string) {
 	k := fmt.Sprintf("%s/%s", cloudsqlapi.AnnotationPrefix, r.Name)
 	v := fmt.Sprintf("%d", r.Generation)
 	// if r was deleted, use a different value
-	if r.GetDeletionTimestamp() != nil {
+	if !r.GetDeletionTimestamp().IsZero() {
 		v = fmt.Sprintf("%d-deleted-%s", r.Generation, r.GetDeletionTimestamp().Format(time.RFC3339))
 	}
 


Test patch:
diff --git a/internal/controller/authproxyworkload_controller_test.go b/internal/controller/authproxyworkload_controller_test.go
index b3c2a31..c1ccac1 100644
--- a/internal/controller/authproxyworkload_controller_test.go
+++ b/internal/controller/authproxyworkload_controller_test.go
@@ -58,7 +58,7 @@ func TestReconcileState11(t *testing.T) {
 		Name:      "test",
 	}, "project:region:db")
 
-	err := runReconcileTestcase(p, []client.Object{p}, true, "", "")
+	_, _, err := runReconcileTestcase(p, []client.Object{p}, true, "", "")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -115,7 +115,7 @@ func TestReconcileState21ByName(t *testing.T) {
 	addFinalizers(p)
 	addPodWorkload(p)
 
-	err := runReconcileTestcase(p, []client.Object{p}, false, metav1.ConditionTrue, v1alpha1.ReasonNoWorkloadsFound)
+	_, _, err := runReconcileTestcase(p, []client.Object{p}, false, metav1.ConditionTrue, v1alpha1.ReasonNoWorkloadsFound)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -129,7 +129,7 @@ func TestReconcileState21BySelector(t *testing.T) {
 	addFinalizers(p)
 	addSelectorWorkload(p, "Pod", "app", "things")
 
-	err := runReconcileTestcase(p, []client.Object{p}, false, metav1.ConditionTrue, v1alpha1.ReasonNoWorkloadsFound)
+	_, _, err := runReconcileTestcase(p, []client.Object{p}, false, metav1.ConditionTrue, v1alpha1.ReasonNoWorkloadsFound)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -165,11 +165,82 @@ func TestReconcileState32(t *testing.T) {
 		}},
 	}
 
-	err := runReconcileTestcase(p, []client.Object{p, pod}, wantRequeue, wantStatus, wantReason)
+	c, ctx, err := runReconcileTestcase(p, []client.Object{p, pod}, wantRequeue, wantStatus, wantReason)
 	if err != nil {
 		t.Fatal(err)
 	}
 
+	// Fetch the deployment and make sure the annotations show the
+	// deleted resource.
+	d := &appsv1.Deployment{}
+	err = c.Get(ctx, types.NamespacedName{
+		Namespace: pod.GetNamespace(),
+		Name:      pod.GetName(),
+	}, d)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if got, want := d.Spec.Template.ObjectMeta.Annotations[reqName], "2"; !strings.HasPrefix(got, "2") {
+		t.Fatalf("got %v, wants annotation value to have prefix %v", got, want)
+	}
+
+}
+
+func TestReconcileState32RolloutStrategyNone(t *testing.T) {
+	const (
+		wantRequeue = false
+		wantStatus  = metav1.ConditionTrue
+		wantReason  = v1alpha1.ReasonFinishedReconcile
+		labelK      = "app"
+		labelV      = "things"
+	)
+
+	p := testhelpers.BuildAuthProxyWorkload(types.NamespacedName{
+		Namespace: "default",
+		Name:      "test",
+	}, "project:region:db")
+	p.Spec.AuthProxyContainer = &v1alpha1.AuthProxyContainerSpec{
+		RolloutStrategy: v1alpha1.NoneStrategy,
+	}
+	p.Generation = 2
+	addFinalizers(p)
+	addSelectorWorkload(p, "Deployment", labelK, labelV)
+
+	// mimic a deployment that was updated by the webhook
+	deployment := &appsv1.Deployment{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "thing",
+			Namespace: "default",
+			Labels:    map[string]string{labelK: labelV},
+		},
+		Spec: appsv1.DeploymentSpec{Template: corev1.PodTemplateSpec{
+			ObjectMeta: metav1.ObjectMeta{Annotations: map[string]string{
+				"annotation": "set",
+			}},
+		}},
+	}
+
+	c, ctx, err := runReconcileTestcase(p, []client.Object{p, deployment}, wantRequeue, wantStatus, wantReason)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Fetch the deployment and make sure the annotations show the
+	// deleted resource.
+	d := &appsv1.Deployment{}
+	err = c.Get(ctx, types.NamespacedName{
+		Namespace: deployment.GetNamespace(),
+		Name:      deployment.GetName(),
+	}, d)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if got, want := len(d.Spec.Template.ObjectMeta.Annotations), 1; got != want {
+		t.Fatalf("got %v annotations, wants %v annotations", got, want)
+	}
+
 }
 
 func TestReconcileState33(t *testing.T) {
@@ -202,7 +273,7 @@ func TestReconcileState33(t *testing.T) {
 		}},
 	}
 
-	err := runReconcileTestcase(p, []client.Object{p, pod}, wantRequeue, wantStatus, wantReason)
+	_, _, err := runReconcileTestcase(p, []client.Object{p, pod}, wantRequeue, wantStatus, wantReason)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -289,10 +360,10 @@ func TestReconcileDeleteUpdatesWorkload(t *testing.T) {
 
 }
 
-func runReconcileTestcase(p *v1alpha1.AuthProxyWorkload, clientObjects []client.Object, wantRequeue bool, wantStatus metav1.ConditionStatus, wantReason string) error {
+func runReconcileTestcase(p *v1alpha1.AuthProxyWorkload, clientObjects []client.Object, wantRequeue bool, wantStatus metav1.ConditionStatus, wantReason string) (client.WithWatch, context.Context, error) {
 	cb, err := clientBuilder()
 	if err != nil {
-		return err // shouldn't ever happen
+		return nil, nil, err // shouldn't ever happen
 	}
 
 	c := cb.WithObjects(clientObjects...).Build()
@@ -300,10 +371,10 @@ func runReconcileTestcase(p *v1alpha1.AuthProxyWorkload, clientObjects []client.
 	r, req, ctx := reconciler(p, c)
 	res, err := r.Reconcile(ctx, req)
 	if err != nil {
-		return err
+		return nil, nil, err
 	}
 	if res.Requeue != wantRequeue {
-		return fmt.Errorf("got %v, want %v for requeue", res.Requeue, wantRequeue)
+		return nil, nil, fmt.Errorf("got %v, want %v for requeue", res.Requeue, wantRequeue)
 	}
 
 	for _, o := range clientObjects {
@@ -316,17 +387,17 @@ func runReconcileTestcase(p *v1alpha1.AuthProxyWorkload, clientObjects []client.
 	if wantStatus != "" || wantReason != "" {
 		cond := findCondition(p.Status.Conditions, v1alpha1.ConditionUpToDate)
 		if cond == nil {
-			return fmt.Errorf("the UpToDate condition was nil, wants condition to exist")
+			return nil, nil, fmt.Errorf("the UpToDate condition was nil, wants condition to exist")
 		}
 		if wantStatus != "" && cond.Status != wantStatus {
-			return fmt.Errorf("got %v, want %v for UpToDate condition status", cond.Status, wantStatus)
+			return nil, nil, fmt.Errorf("got %v, want %v for UpToDate condition status", cond.Status, wantStatus)
 		}
 		if wantReason != "" && cond.Reason != wantReason {
-			return fmt.Errorf("got %v, want %v for UpToDate condition reason", cond.Reason, wantReason)
+			return nil, nil, fmt.Errorf("got %v, want %v for UpToDate condition reason", cond.Reason, wantReason)
 		}
 	}
 
-	return nil
+	return c, ctx, nil
 }
 
 func clientBuilder() (*fake.ClientBuilder, error) {