diff --git a/internal/controller/manifest/setup.go b/internal/controller/manifest/setup.go index 7606edb449..7e66925558 100644 --- a/internal/controller/manifest/setup.go +++ b/internal/controller/manifest/setup.go @@ -2,6 +2,7 @@ package manifest import ( "fmt" + "reflect" apicorev1 "k8s.io/api/core/v1" "k8s.io/client-go/util/workqueue" @@ -9,6 +10,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" ctrlruntime "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -41,7 +43,15 @@ func SetupWithManager(mgr manager.Manager, managedLabelRemovalService ManagedByLabelRemoval, ) error { if err := ctrl.NewControllerManagedBy(mgr). - For(&v1beta2.Manifest{}). + For(&v1beta2.Manifest{}, builder.WithPredicates(predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + generationChanged := e.ObjectNew.GetGeneration() != e.ObjectOld.GetGeneration() + deletionRequested := e.ObjectOld.GetDeletionTimestamp().IsZero() && + !e.ObjectNew.GetDeletionTimestamp().IsZero() + labelChanged := !reflect.DeepEqual(e.ObjectOld.GetLabels(), e.ObjectNew.GetLabels()) + return generationChanged || deletionRequested || labelChanged + }, + })). Named(controllerName). Watches(&apicorev1.Secret{}, handler.Funcs{}, builder.WithPredicates(predicate.Or(predicate.GenerationChangedPredicate{}, diff --git a/internal/manifest/skrresources/ssa.go b/internal/manifest/skrresources/ssa.go index 6b9ca8be5c..76c3db8eef 100644 --- a/internal/manifest/skrresources/ssa.go +++ b/internal/manifest/skrresources/ssa.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "sort" "time" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" @@ -77,6 +78,7 @@ func (c *ConcurrentDefaultSSA) Run(ctx context.Context, resources []client.Objec if c.allTLSExpired(errs) { return errors.Join(util.ErrClientTLSCertExpired, ErrModuleResourcesSSAFailed) } + sort.Slice(errs, func(i, j int) bool { return errs[i].Error() < errs[j].Error() }) errs = append(errs, ErrModuleResourcesSSAFailed) return errors.Join(errs...) } diff --git a/internal/manifest/skrresources/ssa_test.go b/internal/manifest/skrresources/ssa_test.go index b8a18a4e33..3bedd71603 100644 --- a/internal/manifest/skrresources/ssa_test.go +++ b/internal/manifest/skrresources/ssa_test.go @@ -1,10 +1,13 @@ package skrresources_test import ( + "context" + "errors" "testing" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + machineryruntime "k8s.io/apimachinery/pkg/runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -63,3 +66,48 @@ func TestConcurrentSSA(t *testing.T) { ) } } + +// TestConcurrentSSA_ErrorStringIsDeterministic verifies that when multiple resources +// fail concurrently the combined error string is identical across runs. Without sorting, +// goroutine scheduling produces non-deterministic errors.Join output which defeats +// HasStatusDiff-based change detection and bypasses exponential backoff. +func TestConcurrentSSA_ErrorStringIsDeterministic(t *testing.T) { + t.Parallel() + + resources := []client.Object{ + &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "v1", "kind": "ConfigMap", + "metadata": map[string]any{"name": "z-resource", "namespace": "test"}, + }}, + &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "v1", "kind": "ConfigMap", + "metadata": map[string]any{"name": "a-resource", "namespace": "test"}, + }}, + } + + mc := &alwaysErrClient{Client: fake.NewClientBuilder().Build(), err: errors.New("injected failure")} + collector := skrresources.NewManifestLogCollector(nil, fieldowners.DeclarativeApplier) + ssa := skrresources.ConcurrentSSA(mc, fieldowners.LifecycleManager, collector) + + var first string + for range 20 { + err := ssa.Run(t.Context(), resources) + require.Error(t, err) + if first == "" { + first = err.Error() + } + require.Equal(t, first, err.Error(), "error string must be deterministic across runs") + } +} + +type alwaysErrClient struct { + client.Client + + err error +} + +func (a *alwaysErrClient) Apply( + _ context.Context, _ machineryruntime.ApplyConfiguration, _ ...client.ApplyOption, +) error { + return a.err +} diff --git a/unit-test-coverage-lifecycle-manager.yaml b/unit-test-coverage-lifecycle-manager.yaml index 685d259862..4fa3701ac2 100644 --- a/unit-test-coverage-lifecycle-manager.yaml +++ b/unit-test-coverage-lifecycle-manager.yaml @@ -16,7 +16,7 @@ packages: internal/manifest/labelsremoval: 100 internal/manifest/manifestclient: 5 internal/manifest/modulecr: 56.9 - internal/manifest/skrresources: 30.1 + internal/manifest/skrresources: 38.1 internal/manifest/statecheck: 74.2 internal/manifest/status: 73.8 internal/maintenancewindows: 91.2