fix: address CodeRabbit review comments on proxy controller

Clear stale proxy env vars on annotation removal: previously, workloads
that lost the inject-proxy annotation kept proxy env vars owned by the
capi-operator-proxy SSA field manager indefinitely. Now when the annotation
is absent the controller applies an empty env list to all containers in the
pod spec, which removes any previously-owned HTTP_PROXY/HTTPS_PROXY/NO_PROXY
entries. Objects that never had the annotation are unaffected (no ownership
to clear).

Continue reconciling remaining workloads after per-object patch errors:
the three separate applyTo* functions (which also caused a duplication lint
failure) are now merged into a single applyToAllWorkloads+collectWorkloads
pair. Errors are accumulated per-object and joined rather than short-
circuiting the rest of the reconcile loop.

Add assertion message to SetupWithManager in suite_test.go so a BeforeSuite
failure names the failing step clearly.

Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: Todd Short <tshort@redhat.com>

Author: tmshort

pkg/controllers/installer/proxy_controller.go

@@ -18,6 +18,7 @@ package installer
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"strings"
 
@@ -104,6 +105,18 @@ func SetupProxyController(mgr ctrl.Manager, trackingCache managedcache.TrackingC
 	return nil
 }
 
+// workloadItem collects the fields needed to reconcile proxy env vars on a
+// single Deployment, DaemonSet, or StatefulSet without storing a reference to
+// the concrete typed object.
+type workloadItem struct {
+	apiVersion     string
+	kind           string
+	name           string
+	namespace      string
+	podAnnotations map[string]string
+	containers     []corev1.Container
+}
+
 // Reconcile reads the cluster-wide proxy configuration and applies the proxy
 // environment variables to all managed workloads that carry the inject-proxy
 // annotation.
@@ -116,131 +129,158 @@ func (c *ProxyController) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.R
 		return ctrl.Result{}, fmt.Errorf("fetching cluster-wide proxy: %w", err)
 	}
 
-	var errs []error
-
-	for _, applyFn := range []func(context.Context, []corev1.EnvVar) error{
-		c.applyToDeployments,
-		c.applyToDaemonSets,
-		c.applyToStatefulSets,
-	} {
-		if err := applyFn(ctx, proxyVars); err != nil {
-			errs = append(errs, err)
-		}
-	}
-
-	if len(errs) > 0 {
-		combined := errs[0]
-		for _, e := range errs[1:] {
-			combined = fmt.Errorf("%w; %w", combined, e)
-		}
-
-		return ctrl.Result{}, combined
+	if err := c.applyToAllWorkloads(ctx, proxyVars); err != nil {
+		return ctrl.Result{}, err
 	}
 
 	log.Info("Proxy environment variables reconciled")
 
 	return ctrl.Result{}, nil
 }
 
-func (c *ProxyController) applyToDeployments(ctx context.Context, proxyVars []corev1.EnvVar) error {
-	list := &appsv1.DeploymentList{}
-	if err := c.trackingCache.List(ctx, list); err != nil {
-		return fmt.Errorf("listing managed deployments: %w", err)
-	}
-
-	for i := range list.Items {
-		deploy := &list.Items[i]
-		annotation, ok := deploy.Spec.Template.Annotations[ProxyInjectAnnotation]
+// containerNamesFromAnnotation parses the comma-separated inject-proxy annotation value.
+func containerNamesFromAnnotation(annotation string) []string {
+	var names []string
 
-		if !ok {
-			continue
+	for _, name := range strings.Split(annotation, ",") {
+		if n := strings.TrimSpace(name); n != "" {
+			names = append(names, n)
 		}
+	}
 
-		if err := c.applyProxyVars(ctx, "apps/v1", "Deployment", deploy.Name, deploy.Namespace, annotation, proxyVars); err != nil {
-			return err
-		}
+	return names
+}
+
+// containerNamesFromSpec returns the names of all containers in a pod spec.
+func containerNamesFromSpec(containers []corev1.Container) []string {
+	names := make([]string, len(containers))
+	for i, c := range containers {
+		names[i] = c.Name
 	}
 
-	return nil
+	return names
 }
 
-func (c *ProxyController) applyToDaemonSets(ctx context.Context, proxyVars []corev1.EnvVar) error {
-	list := &appsv1.DaemonSetList{}
-	if err := c.trackingCache.List(ctx, list); err != nil {
-		return fmt.Errorf("listing managed daemonsets: %w", err)
+// applyToAllWorkloads collects all managed Deployments, DaemonSets, and
+// StatefulSets from the tracking cache and reconciles their proxy env vars.
+// It continues past per-object errors and returns all failures combined.
+func (c *ProxyController) applyToAllWorkloads(ctx context.Context, proxyVars []corev1.EnvVar) error {
+	items, err := c.collectWorkloads(ctx)
+	if err != nil {
+		return err
 	}
 
-	for i := range list.Items {
-		ds := &list.Items[i]
+	var errs []error
+
+	for _, item := range items {
+		var containers []string
 
-		annotation, ok := ds.Spec.Template.Annotations[ProxyInjectAnnotation]
+		effectiveVars := proxyVars
 
-		if !ok {
-			continue
+		annotation, hasAnnotation := item.podAnnotations[ProxyInjectAnnotation]
+		if hasAnnotation {
+			containers = containerNamesFromAnnotation(annotation)
+		} else {
+			// No annotation: clear any stale proxy env vars we previously owned
+			// so that opting out removes the env vars instead of leaving them.
+			containers = containerNamesFromSpec(item.containers)
+			effectiveVars = nil
 		}
 
-		if err := c.applyProxyVars(ctx, "apps/v1", "DaemonSet", ds.Name, ds.Namespace, annotation, proxyVars); err != nil {
-			return err
+		if err := c.applyProxyVars(ctx, item.apiVersion, item.kind, item.name, item.namespace, containers, effectiveVars); err != nil {
+			errs = append(errs, err)
 		}
 	}
 
-	return nil
+	return errors.Join(errs...)
 }
 
-func (c *ProxyController) applyToStatefulSets(ctx context.Context, proxyVars []corev1.EnvVar) error {
-	list := &appsv1.StatefulSetList{}
-	if err := c.trackingCache.List(ctx, list); err != nil {
-		return fmt.Errorf("listing managed statefulsets: %w", err)
+// collectWorkloads lists all managed Deployments, DaemonSets, and StatefulSets
+// from the tracking cache and returns them as workloadItems.
+func (c *ProxyController) collectWorkloads(ctx context.Context) ([]workloadItem, error) {
+	var items []workloadItem
+
+	deployList := &appsv1.DeploymentList{}
+	if err := c.trackingCache.List(ctx, deployList); err != nil {
+		return nil, fmt.Errorf("listing managed deployments: %w", err)
 	}
 
-	for i := range list.Items {
-		ss := &list.Items[i]
+	for i := range deployList.Items {
+		d := &deployList.Items[i]
+		items = append(items, workloadItem{
+			apiVersion:     "apps/v1",
+			kind:           "Deployment",
+			name:           d.Name,
+			namespace:      d.Namespace,
+			podAnnotations: d.Spec.Template.Annotations,
+			containers:     d.Spec.Template.Spec.Containers,
+		})
+	}
 
-		annotation, ok := ss.Spec.Template.Annotations[ProxyInjectAnnotation]
+	dsList := &appsv1.DaemonSetList{}
+	if err := c.trackingCache.List(ctx, dsList); err != nil {
+		return nil, fmt.Errorf("listing managed daemonsets: %w", err)
+	}
 
-		if !ok {
-			continue
-		}
+	for i := range dsList.Items {
+		ds := &dsList.Items[i]
+		items = append(items, workloadItem{
+			apiVersion:     "apps/v1",
+			kind:           "DaemonSet",
+			name:           ds.Name,
+			namespace:      ds.Namespace,
+			podAnnotations: ds.Spec.Template.Annotations,
+			containers:     ds.Spec.Template.Spec.Containers,
+		})
+	}
 
-		if err := c.applyProxyVars(ctx, "apps/v1", "StatefulSet", ss.Name, ss.Namespace, annotation, proxyVars); err != nil {
-			return err
-		}
+	ssList := &appsv1.StatefulSetList{}
+	if err := c.trackingCache.List(ctx, ssList); err != nil {
+		return nil, fmt.Errorf("listing managed statefulsets: %w", err)
 	}
 
-	return nil
+	for i := range ssList.Items {
+		ss := &ssList.Items[i]
+		items = append(items, workloadItem{
+			apiVersion:     "apps/v1",
+			kind:           "StatefulSet",
+			name:           ss.Name,
+			namespace:      ss.Namespace,
+			podAnnotations: ss.Spec.Template.Annotations,
+			containers:     ss.Spec.Template.Spec.Containers,
+		})
+	}
+
+	return items, nil
 }
 
-// applyProxyVars SSA-applies the proxy env vars to each container named in the
-// comma-separated annotation value. The proxy controller owns only the three
-// proxy env var entries; all other fields remain owned by boxcutter.
+// applyProxyVars SSA-applies proxy env vars to each named container on the
+// workload object. When proxyVars is empty/nil (proxy removed or no annotation),
+// it applies an empty env list, which removes any env var entries previously
+// owned by the proxy controller's SSA field manager. Only the three proxy env
+// var entries are owned; all other fields remain owned by boxcutter.
 func (c *ProxyController) applyProxyVars(
 	ctx context.Context,
-	apiVersion, kind, name, namespace, annotation string,
+	apiVersion, kind, name, namespace string,
+	containerNames []string,
 	proxyVars []corev1.EnvVar,
 ) error {
 	log := ctrl.LoggerFrom(ctx).WithName(proxyControllerName)
 
-	containerNames := strings.Split(annotation, ",")
-	containerPatches := make([]map[string]interface{}, 0, len(containerNames))
+	if len(containerNames) == 0 {
+		return nil
+	}
 
+	containerPatches := make([]map[string]interface{}, 0, len(containerNames))
 	envEntries := proxyEnvEntries(proxyVars)
 
 	for _, containerName := range containerNames {
-		containerName = strings.TrimSpace(containerName)
-		if containerName == "" {
-			continue
-		}
-
 		containerPatches = append(containerPatches, map[string]interface{}{
 			"name": containerName,
 			"env":  envEntries,
 		})
 	}
 
-	if len(containerPatches) == 0 {
-		return nil
-	}
-
 	patch := &unstructured.Unstructured{
 		Object: map[string]interface{}{
 			"apiVersion": apiVersion,

pkg/controllers/installer/suite_test.go

@@ -107,7 +107,7 @@ var _ = BeforeSuite(func() {
 	)
 
 	_, err = SetupWithManager(mgr, allProviderProfiles, triggerSource)
-	Expect(err).To(Succeed())
+	Expect(err).To(Succeed(), "SetupWithManager should register the installer controller")
 	Expect(test.AddNamespaceFinalizerCleanup(mgr)).To(Succeed())
 
 	// Start manager in background.