From 08cef320b99555772b9019f6949eb74b5f0b3a74 Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Wed, 21 Jan 2026 15:38:28 -0800 Subject: [PATCH 1/3] initial commit --- .../controllers/reconciliation/controller.go | 11 +++++- .../reconciliation/controller_test.go | 37 +++++++++++++++++++ internal/controllers/symphony/controller.go | 1 - internal/controllers/watch/pruning.go | 1 - 4 files changed, 47 insertions(+), 3 deletions(-) diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index ffabbb45..02572361 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -488,8 +488,17 @@ func patchResourceError(err error) flowcontrol.StatusPatchFn { } func summarizeError(err error) string { + if err == nil { + return "" + } + + // Capture all dry-run failures - these are important to surface + if strings.Contains(err.Error(), "dry-run applying update") { + return err.Error() + } + statusErr := &errors.StatusError{} - if err == nil || !goerrors.As(err, &statusErr) { + if !goerrors.As(err, &statusErr) { return "" } status := statusErr.Status() diff --git a/internal/controllers/reconciliation/controller_test.go b/internal/controllers/reconciliation/controller_test.go index edb3d0fb..55798339 100644 --- a/internal/controllers/reconciliation/controller_test.go +++ b/internal/controllers/reconciliation/controller_test.go @@ -1,6 +1,7 @@ package reconciliation import ( + "fmt" "testing" "time" @@ -119,6 +120,42 @@ func TestShouldFailOpen(t *testing.T) { assert.True(t, c.shouldFailOpen(&resource.Resource{FailOpen: ptr.To(true)})) } +func TestSummarizeError(t *testing.T) { + tests := []struct { + name string + err error + expected string + }{ + { + name: "nil error returns empty string", + err: nil, + expected: "", + }, + { + name: "dry-run validation error is captured", + err: fmt.Errorf("dry-run applying update: Deployment.apps \"kube-apiserver\" is invalid: spec.template.spec.initContainers[0].image: Required value"), + expected: "dry-run applying update: Deployment.apps \"kube-apiserver\" is invalid: spec.template.spec.initContainers[0].image: Required value", + }, + { + name: "dry-run webhook error is captured", + err: fmt.Errorf("dry-run applying update: Internal error occurred: failed calling webhook \"validate.kyverno.svc-fail\": failed to call webhook"), + expected: "dry-run applying update: Internal error occurred: failed calling webhook \"validate.kyverno.svc-fail\": failed to call webhook", + }, + { + name: "non-status error returns empty string", + err: fmt.Errorf("some random error"), + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := summarizeError(tt.err) + assert.Equal(t, tt.expected, result) + }) + } +} + func TestRequeueDoesNotPanic(t *testing.T) { type testState struct { snapshot *resource.Snapshot diff --git a/internal/controllers/symphony/controller.go b/internal/controllers/symphony/controller.go index 0de6ba06..b054ef92 100644 --- a/internal/controllers/symphony/controller.go +++ b/internal/controllers/symphony/controller.go @@ -143,7 +143,6 @@ func (c *symphonyController) reconcileReverse(ctx context.Context, symph *apiv1. labelInSync := symph.DeletionTimestamp == nil || (comp.Labels != nil && comp.Labels[deletionLabelKey] == "true") alreadyDeleted := comp.DeletionTimestamp != nil if shouldExist || (alreadyDeleted && labelInSync) { - logger.Info("composition should exist or is already being deleted properly", "compositionName", comp.Name, "compositionNamespace", comp.Namespace) continue } diff --git a/internal/controllers/watch/pruning.go b/internal/controllers/watch/pruning.go index 401522cd..e5341dc4 100644 --- a/internal/controllers/watch/pruning.go +++ b/internal/controllers/watch/pruning.go @@ -40,7 +40,6 @@ func (c *pruningController) Reconcile(ctx context.Context, req ctrl.Request) (ct for i, ir := range comp.Status.InputRevisions { if hasBindingKey(comp, synth, ir.Key) { - logger.Info("input revision still has binding, keeping", "key", ir.Key, "revision", ir.Revision) continue } logger.Info("pruning input revision - no longer has binding", "key", ir.Key, "revision", ir.Revision, "index", i) From 06a1229c1fcda07c807a53a3d225452d6313f0ac Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Wed, 21 Jan 2026 16:24:23 -0800 Subject: [PATCH 2/3] Adding controller fixes --- .../controllers/reconciliation/controller.go | 59 ++++++++----------- .../reconciliation/controller_test.go | 33 ++++++++--- 2 files changed, 51 insertions(+), 41 deletions(-) diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index 02572361..19536cb5 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -298,27 +298,27 @@ func (c *Controller) reconcileSnapshot(ctx context.Context, comp *apiv1.Composit // caused by multiple managers owning overlapping fields. When managers are renamed to "eno", the // subsequent SSA Apply will treat eno as the sole owner and automatically merge the managedFields // entries into a single consolidated entry for eno. - if current != nil && len(c.migratingFieldManagers) > 0 { - wasModified, err := resource.NormalizeConflictingManagers(ctx, current, c.migratingFieldManagers, c.migratingFields) - if err != nil { - return false, fmt.Errorf("normalize conflicting manager failed: %w", err) - } - if wasModified { - logger.Info("Normalized conflicting managers to eno") - err = c.upstreamClient.Update(ctx, current, client.FieldOwner("eno")) - if err != nil { - return false, fmt.Errorf("normalizing managedFields failed: %w", err) - } - // refetch the current before apply dry-run - current, err = c.getCurrent(ctx, res.Resource) - if err != nil { - logger.Error(err, "failed to get current resource after eno ownership migration") - return false, fmt.Errorf("re-fetching after normalizing manager failed: %w", err) - } - - logger.Info("Successfully normalized field managers to eno") - } - } + // if current != nil && len(c.migratingFieldManagers) > 0 { + // wasModified, err := resource.NormalizeConflictingManagers(ctx, current, c.migratingFieldManagers, c.migratingFields) + // if err != nil { + // return false, fmt.Errorf("normalize conflicting manager failed: %w", err) + // } + // if wasModified { + // logger.Info("Normalized conflicting managers to eno") + // err = c.upstreamClient.Update(ctx, current, client.FieldOwner("eno")) + // if err != nil { + // return false, fmt.Errorf("normalizing managedFields failed: %w", err) + // } + // // refetch the current before apply dry-run + // current, err = c.getCurrent(ctx, res.Resource) + // if err != nil { + // logger.Error(err, "failed to get current resource after eno ownership migration") + // return false, fmt.Errorf("re-fetching after normalizing manager failed: %w", err) + // } + + // logger.Info("Successfully normalized field managers to eno") + // } + // } dryRun, err := c.update(ctx, comp, prev, res, current, true) if err != nil { logger.Error(err, "dry-run update failed.") @@ -340,7 +340,7 @@ func (c *Controller) reconcileSnapshot(ctx context.Context, comp *apiv1.Composit dryRunPrev := snap.Unstructured() err = c.upstreamClient.Patch(ctx, dryRunPrev, client.Apply, client.ForceOwnership, client.FieldOwner("eno"), client.DryRunAll) if err != nil { - logger.Error(err, "faile dto get managedFields values") + logger.Error(err, "failed to get managedFields values") return false, fmt.Errorf("getting managed fields values for previous version: %w", err) } @@ -488,17 +488,8 @@ func patchResourceError(err error) flowcontrol.StatusPatchFn { } func summarizeError(err error) string { - if err == nil { - return "" - } - - // Capture all dry-run failures - these are important to surface - if strings.Contains(err.Error(), "dry-run applying update") { - return err.Error() - } - statusErr := &errors.StatusError{} - if !goerrors.As(err, &statusErr) { + if err == nil || !goerrors.As(err, &statusErr) { return "" } status := statusErr.Status() @@ -515,7 +506,9 @@ func summarizeError(err error) string { metav1.StatusReasonMethodNotAllowed, metav1.StatusReasonGone, metav1.StatusReasonForbidden, - metav1.StatusReasonUnauthorized: + metav1.StatusReasonUnauthorized, + metav1.StatusReasonInvalid, + metav1.StatusReasonInternalError: return status.Message default: diff --git a/internal/controllers/reconciliation/controller_test.go b/internal/controllers/reconciliation/controller_test.go index 55798339..b8653211 100644 --- a/internal/controllers/reconciliation/controller_test.go +++ b/internal/controllers/reconciliation/controller_test.go @@ -13,7 +13,9 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -132,18 +134,33 @@ func TestSummarizeError(t *testing.T) { expected: "", }, { - name: "dry-run validation error is captured", - err: fmt.Errorf("dry-run applying update: Deployment.apps \"kube-apiserver\" is invalid: spec.template.spec.initContainers[0].image: Required value"), - expected: "dry-run applying update: Deployment.apps \"kube-apiserver\" is invalid: spec.template.spec.initContainers[0].image: Required value", + name: "non-status error returns empty string", + err: fmt.Errorf("some random error"), + expected: "", }, { - name: "dry-run webhook error is captured", - err: fmt.Errorf("dry-run applying update: Internal error occurred: failed calling webhook \"validate.kyverno.svc-fail\": failed to call webhook"), - expected: "dry-run applying update: Internal error occurred: failed calling webhook \"validate.kyverno.svc-fail\": failed to call webhook", + name: "StatusReasonInvalid is captured", + err: errors.NewInvalid(schema.GroupKind{Group: "apps", Kind: "Deployment"}, "kube-apiserver", nil), + expected: "Deployment.apps \"kube-apiserver\" is invalid", }, { - name: "non-status error returns empty string", - err: fmt.Errorf("some random error"), + name: "StatusReasonInternalError is captured", + err: errors.NewInternalError(fmt.Errorf("failed calling webhook")), + expected: "Internal error occurred: failed calling webhook", + }, + { + name: "StatusReasonBadRequest is captured", + err: errors.NewBadRequest("bad request"), + expected: "bad request", + }, + { + name: "StatusReasonForbidden is captured", + err: errors.NewForbidden(schema.GroupResource{Group: "apps", Resource: "deployments"}, "test", fmt.Errorf("not allowed")), + expected: "deployments.apps \"test\" is forbidden: not allowed", + }, + { + name: "StatusReasonNotFound is not captured", + err: errors.NewNotFound(schema.GroupResource{Group: "apps", Resource: "deployments"}, "test"), expected: "", }, } From 5f8f279db25ab1209c237248787f12f33862fffd Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Wed, 21 Jan 2026 16:24:45 -0800 Subject: [PATCH 3/3] Fix --- .../controllers/reconciliation/controller.go | 42 +++++++++---------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index 19536cb5..7959c4ae 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -298,27 +298,27 @@ func (c *Controller) reconcileSnapshot(ctx context.Context, comp *apiv1.Composit // caused by multiple managers owning overlapping fields. When managers are renamed to "eno", the // subsequent SSA Apply will treat eno as the sole owner and automatically merge the managedFields // entries into a single consolidated entry for eno. - // if current != nil && len(c.migratingFieldManagers) > 0 { - // wasModified, err := resource.NormalizeConflictingManagers(ctx, current, c.migratingFieldManagers, c.migratingFields) - // if err != nil { - // return false, fmt.Errorf("normalize conflicting manager failed: %w", err) - // } - // if wasModified { - // logger.Info("Normalized conflicting managers to eno") - // err = c.upstreamClient.Update(ctx, current, client.FieldOwner("eno")) - // if err != nil { - // return false, fmt.Errorf("normalizing managedFields failed: %w", err) - // } - // // refetch the current before apply dry-run - // current, err = c.getCurrent(ctx, res.Resource) - // if err != nil { - // logger.Error(err, "failed to get current resource after eno ownership migration") - // return false, fmt.Errorf("re-fetching after normalizing manager failed: %w", err) - // } - - // logger.Info("Successfully normalized field managers to eno") - // } - // } + if current != nil && len(c.migratingFieldManagers) > 0 { + wasModified, err := resource.NormalizeConflictingManagers(ctx, current, c.migratingFieldManagers, c.migratingFields) + if err != nil { + return false, fmt.Errorf("normalize conflicting manager failed: %w", err) + } + if wasModified { + logger.Info("Normalized conflicting managers to eno") + err = c.upstreamClient.Update(ctx, current, client.FieldOwner("eno")) + if err != nil { + return false, fmt.Errorf("normalizing managedFields failed: %w", err) + } + // refetch the current before apply dry-run + current, err = c.getCurrent(ctx, res.Resource) + if err != nil { + logger.Error(err, "failed to get current resource after eno ownership migration") + return false, fmt.Errorf("re-fetching after normalizing manager failed: %w", err) + } + + logger.Info("Successfully normalized field managers to eno") + } + } dryRun, err := c.update(ctx, comp, prev, res, current, true) if err != nil { logger.Error(err, "dry-run update failed.")