Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion internal/controller/manifest/setup.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@ package manifest

import (
"fmt"
"reflect"

apicorev1 "k8s.io/api/core/v1"
"k8s.io/client-go/util/workqueue"
ctrl "sigs.k8s.io/controller-runtime"
"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"
Expand Down Expand Up @@ -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{},
Expand Down
2 changes: 2 additions & 0 deletions internal/manifest/skrresources/ssa.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"sort"
"time"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -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...)
}
Expand Down
48 changes: 48 additions & 0 deletions internal/manifest/skrresources/ssa_test.go
Original file line number Diff line number Diff line change
@@ -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"

Expand Down Expand Up @@ -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
}
2 changes: 1 addition & 1 deletion unit-test-coverage-lifecycle-manager.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading