diff --git a/api/v1/composition.go b/api/v1/composition.go index fedbe0a5..f01a87cf 100644 --- a/api/v1/composition.go +++ b/api/v1/composition.go @@ -18,6 +18,7 @@ const ( WaitingOnDependenciesReason string = "WaitingOnDependencies" WaitingOnDependencyNotFoundReason string = "DependencyNotFound" WaitingOnDependencyNotReadyReason string = "DependencyNotReady" + WaitingOnDependentsDeletedReason string = "DependentNotDeleted" ) // +kubebuilder:object:root=true diff --git a/e2e/inter_composition_ordering_test.go b/e2e/inter_composition_ordering_test.go new file mode 100644 index 00000000..1af9d7c5 --- /dev/null +++ b/e2e/inter_composition_ordering_test.go @@ -0,0 +1,469 @@ +package e2e + +import ( + "context" + "testing" + "time" + + apiv1 "github.com/Azure/eno/api/v1" + fw "github.com/Azure/eno/e2e/framework" + flow "github.com/Azure/go-workflow" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" +) + +const ( + // testHoldFinalizer is added to C's ConfigMap by the test to delay + // composition C's cleanup, giving time to verify A and B are blocked. + testHoldFinalizer = "e2e-test/hold" +) + +// TestInterCompositionOrderingLifecycleTest exercises the full lifecycle of compositions +// with inter-composition ordering: initial creation, adding new variations with dependencies, +// removing variations (simulating feature disable with delayed resource cleanup), and finally +// namespace deletion to verify the symphony-deleting flow with DependsOn compositions. +// +// Dependency graph (after phase 2): +// +// synth-a (root, no deps) +// ├── synth-b (depends on synth-a) +// │ └── synthc-ol (depends on synth-b AND synthc-ul) +// └── synthc-ul (depends on synth-a) +// └── synthc-ol (depends on synth-b AND synthc-ul) +func TestInterCompositionOrderingLifecycleTest(t *testing.T) { + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Minute) + defer cancel() + + cli := fw.NewClient(t) + + // Resource namespace + testNS := fw.UniqueName("testing-ns1") + + // Synthesizer names + synthNameA := fw.UniqueName("lc-synth-a") + synthNameB := fw.UniqueName("lc-synth-b") + synthNameCOL := fw.UniqueName("lc-synthc-ol") + synthNameCUL := fw.UniqueName("lc-synthc-ul") + symphonyName := fw.UniqueName("lc-symphony") + + // ConfigMap names + cmNameA := fw.UniqueName("lc-cm-a") + cmNameB := fw.UniqueName("lc-cm-b") + cmNameCOL := fw.UniqueName("lc-cm-col") + cmNameCUL := fw.UniqueName("lc-cm-cul") + + // ConfigMaps produced by each synthesizer (all in testNS) + cmA := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{Name: cmNameA, Namespace: testNS}, + Data: map[string]string{"source": "A"}, + } + cmB := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{Name: cmNameB, Namespace: testNS}, + Data: map[string]string{"source": "B"}, + } + cmCOL := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{ + Name: cmNameCOL, + Namespace: testNS, + Annotations: map[string]string{ + "eno.azure.io/deletion-strategy": "foreground", + }, + }, + Data: map[string]string{"source": "COL"}, + } + cmCUL := &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{APIVersion: "v1", Kind: "ConfigMap"}, + ObjectMeta: metav1.ObjectMeta{Name: cmNameCUL, Namespace: testNS}, + Data: map[string]string{"source": "CUL"}, + } + + // Synthesizers + synthA := fw.NewMinimalSynthesizer(synthNameA, fw.WithCommand(fw.ToCommand(cmA))) + synthB := fw.NewMinimalSynthesizer(synthNameB, fw.WithCommand(fw.ToCommand(cmB))) + synthCOL := fw.NewMinimalSynthesizer(synthNameCOL, fw.WithCommand(fw.ToCommand(cmCOL))) + synthCUL := fw.NewMinimalSynthesizer(synthNameCUL, fw.WithCommand(fw.ToCommand(cmCUL))) + + // Namespace object + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: testNS}, + } + + // Phase 1 symphony: only synth-a and synth-b + symphony := &apiv1.Symphony{ + ObjectMeta: metav1.ObjectMeta{Name: symphonyName, Namespace: testNS}, + Spec: apiv1.SymphonySpec{ + Variations: []apiv1.Variation{ + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameA}, + }, + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameB}, + DependsOn: []apiv1.VariationDependency{ + {Synthesizer: synthNameA}, + }, + }, + }, + }, + } + + symphonyKey := types.NamespacedName{Name: symphonyName, Namespace: testNS} + + // --- Safety net cleanup --- + t.Cleanup(func() { + cleanupCtx, cleanupCancel := context.WithTimeout(context.Background(), 90*time.Second) + defer cleanupCancel() + + // Remove any test finalizers from ConfigMaps + for _, cmName := range []string{cmNameCOL} { + cm := &corev1.ConfigMap{} + cmKey := types.NamespacedName{Name: cmName, Namespace: testNS} + if err := cli.Get(cleanupCtx, cmKey, cm); err == nil { + if controllerutil.RemoveFinalizer(cm, testHoldFinalizer) { + _ = cli.Update(cleanupCtx, cm) + } + } + } + for _, obj := range []client.Object{synthA, synthB, synthCOL, synthCUL} { + fw.Cleanup(t, cleanupCtx, cli, obj) + } + // Delete namespace last (cascades everything inside) + _ = cli.Delete(cleanupCtx, ns) + }) + + // ========== WORKFLOW STEPS ========== + + createNS := fw.CreateStep(t, "createNamespace", cli, ns) + createSynthA := fw.CreateStep(t, "createSynthA", cli, synthA) + createSynthB := fw.CreateStep(t, "createSynthB", cli, synthB) + createSynthCOL := fw.CreateStep(t, "createSynthCOL", cli, synthCOL) + createSynthCUL := fw.CreateStep(t, "createSynthCUL", cli, synthCUL) + createSymphony := fw.CreateStep(t, "createSymphony", cli, symphony) + + // --- Phase 1: Wait for symphony ready with synth-a and synth-b --- + waitPhase1Ready := flow.Func("waitPhase1Ready", func(ctx context.Context) error { + fw.WaitForSymphonyReady(t, ctx, cli, symphonyKey, 5*time.Minute) + t.Log("phase 1: symphony is ready (synth-a, synth-b)") + return nil + }) + + // --- Phase 1: Verify creation ordering A → B --- + verifyPhase1Order := flow.Func("verifyPhase1Order", func(ctx context.Context) error { + compBySynth := getCompsBySynth(t, ctx, cli, testNS, symphonyName) + + require.Contains(t, compBySynth, synthNameA) + require.Contains(t, compBySynth, synthNameB) + + synA := compBySynth[synthNameA].Status.CurrentSynthesis + synB := compBySynth[synthNameB].Status.CurrentSynthesis + require.NotNil(t, synA) + require.NotNil(t, synB) + require.NotNil(t, synA.Ready, "A should be ready") + require.NotNil(t, synB.Ready, "B should be ready") + + t.Logf("phase 1 ready timestamps: A=%s, B=%s", synA.Ready.Time, synB.Ready.Time) + require.False(t, synA.Ready.Time.After(synB.Ready.Time), + "A (ready=%s) should be ready before B (ready=%s)", synA.Ready.Time, synB.Ready.Time) + + t.Log("phase 1 creation order verified: A → B") + return nil + }) + + // --- Phase 2: Add synthc-ol and synthc-ul to the symphony --- + updateSymphonyPhase2 := flow.Func("updateSymphonyPhase2", func(ctx context.Context) error { + sym := &apiv1.Symphony{} + require.NoError(t, cli.Get(ctx, symphonyKey, sym)) + + sym.Spec.Variations = []apiv1.Variation{ + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameA}, + }, + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameB}, + DependsOn: []apiv1.VariationDependency{ + {Synthesizer: synthNameA}, + }, + }, + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameCUL}, + DependsOn: []apiv1.VariationDependency{ + {Synthesizer: synthNameA}, + }, + }, + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameCOL}, + DependsOn: []apiv1.VariationDependency{ + {Synthesizer: synthNameB}, + {Synthesizer: synthNameCUL}, + }, + }, + } + + require.NoError(t, cli.Update(ctx, sym)) + t.Log("phase 2: updated symphony to add synthc-ol and synthc-ul") + return nil + }) + + waitPhase2Ready := flow.Func("waitPhase2Ready", func(ctx context.Context) error { + // Wait for all 4 compositions to be ready + err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) { + compBySynth := getCompsBySynth(t, ctx, cli, testNS, symphonyName) + for _, name := range []string{synthNameA, synthNameB, synthNameCOL, synthNameCUL} { + comp, ok := compBySynth[name] + if !ok { + return false, nil + } + if comp.Status.CurrentSynthesis == nil || comp.Status.CurrentSynthesis.Ready == nil { + return false, nil + } + } + return true, nil + }) + require.NoError(t, err, "timed out waiting for phase 2 symphony to become ready") + t.Log("phase 2: all 4 compositions are ready") + return nil + }) + + // --- Phase 2: Verify ordering --- + // synthc-ul depends on synth-a, synthc-ol depends on synth-b AND synthc-ul + // So: A ready before CUL, B ready before COL, CUL ready before COL + verifyPhase2Order := flow.Func("verifyPhase2Order", func(ctx context.Context) error { + compBySynth := getCompsBySynth(t, ctx, cli, testNS, symphonyName) + + synA := compBySynth[synthNameA].Status.CurrentSynthesis + synB := compBySynth[synthNameB].Status.CurrentSynthesis + synCOL := compBySynth[synthNameCOL].Status.CurrentSynthesis + synCUL := compBySynth[synthNameCUL].Status.CurrentSynthesis + + t.Logf("phase 2 ready timestamps: A=%s, B=%s, CUL=%s, COL=%s", + synA.Ready.Time, synB.Ready.Time, synCUL.Ready.Time, synCOL.Ready.Time) + + require.False(t, synA.Ready.Time.After(synCUL.Ready.Time), + "A should be ready before CUL") + require.False(t, synB.Ready.Time.After(synCOL.Ready.Time), + "B should be ready before COL") + require.False(t, synCUL.Ready.Time.After(synCOL.Ready.Time), + "CUL should be ready before COL") + + t.Log("phase 2 ordering verified: A → {B, CUL} → COL") + return nil + }) + + // --- Phase 3: Remove synthc-ol and synthc-ul (simulate feature disable) --- + // First add a test finalizer to synthc-ol's ConfigMap to simulate slow CRD cleanup + addCOLFinalizer := flow.Func("addCOLFinalizer", func(ctx context.Context) error { + cm := &corev1.ConfigMap{} + cmKey := types.NamespacedName{Name: cmNameCOL, Namespace: testNS} + require.NoError(t, cli.Get(ctx, cmKey, cm)) + if controllerutil.AddFinalizer(cm, testHoldFinalizer) { + require.NoError(t, cli.Update(ctx, cm)) + t.Logf("phase 3: added test finalizer to ConfigMap %s", cmNameCOL) + } + return nil + }) + + // Capture composition keys for COL and CUL before removing them + var compKeyCOL, compKeyCUL types.NamespacedName + + capturePhase3Keys := flow.Func("capturePhase3Keys", func(ctx context.Context) error { + compBySynth := getCompsBySynth(t, ctx, cli, testNS, symphonyName) + compKeyCOL = client.ObjectKeyFromObject(compBySynth[synthNameCOL]) + compKeyCUL = client.ObjectKeyFromObject(compBySynth[synthNameCUL]) + t.Logf("phase 3: captured keys COL=%s, CUL=%s", compKeyCOL, compKeyCUL) + return nil + }) + + // Update symphony to remove synthc-ol and synthc-ul + updateSymphonyPhase3 := flow.Func("updateSymphonyPhase3", func(ctx context.Context) error { + sym := &apiv1.Symphony{} + require.NoError(t, cli.Get(ctx, symphonyKey, sym)) + + sym.Spec.Variations = []apiv1.Variation{ + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameA}, + }, + { + Synthesizer: apiv1.SynthesizerRef{Name: synthNameB}, + DependsOn: []apiv1.VariationDependency{ + {Synthesizer: synthNameA}, + }, + }, + } + + require.NoError(t, cli.Update(ctx, sym)) + t.Log("phase 3: removed synthc-ol and synthc-ul from symphony") + return nil + }) + + // Verify deletion ordering for removed variations: + // synthc-ol (leaf, depends on synthc-ul) should delete first + // synthc-ul should wait until synthc-ol is gone + // The finalizer on COL's ConfigMap stalls its cleanup + verifyPhase3DeletionOrder := flow.Func("verifyPhase3DeletionOrder", func(ctx context.Context) error { + // Wait 3 seconds — COL's ConfigMap finalizer stalls cleanup + t.Log("phase 3: waiting 3s for controller to process deletions...") + time.Sleep(3 * time.Second) + + // Both COL and CUL should still exist (COL is stuck, CUL is blocked by COL) + compCOL := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, compKeyCOL, compCOL), + "COL should still exist (its ConfigMap finalizer is holding it)") + t.Log("phase 3: after 3s COL still exists (ConfigMap finalizer holding)") + + compCUL := &apiv1.Composition{} + require.NoError(t, cli.Get(ctx, compKeyCUL, compCUL), + "CUL should still exist (COL depends on CUL, COL is stuck)") + t.Log("phase 3: after 3s CUL still exists (blocked by COL)") + + // Now remove the test finalizer to unblock the cascade + cm := &corev1.ConfigMap{} + cmKey := types.NamespacedName{Name: cmNameCOL, Namespace: testNS} + require.NoError(t, cli.Get(ctx, cmKey, cm)) + if controllerutil.RemoveFinalizer(cm, testHoldFinalizer) { + require.NoError(t, cli.Update(ctx, cm)) + t.Logf("phase 3: removed test finalizer from ConfigMap %s", cmNameCOL) + } + + // Poll until both COL and CUL are deleted, verifying ordering invariant + existsCOL, existsCUL := true, true + var deletionOrder []string + + err := wait.PollUntilContextTimeout(ctx, 500*time.Millisecond, 3*time.Minute, true, func(ctx context.Context) (bool, error) { + nowCOL, nowCUL := true, true + + if existsCOL { + comp := &apiv1.Composition{} + if err := cli.Get(ctx, compKeyCOL, comp); apierrors.IsNotFound(err) { + nowCOL = false + } + } else { + nowCOL = false + } + + if existsCUL { + comp := &apiv1.Composition{} + if err := cli.Get(ctx, compKeyCUL, comp); apierrors.IsNotFound(err) { + nowCUL = false + } + } else { + nowCUL = false + } + + // INVARIANT: CUL must not be deleted while COL still exists + require.False(t, !nowCUL && nowCOL, + "ORDERING VIOLATION: CUL was deleted while COL still exists") + + if existsCOL && !nowCOL { + t.Log("phase 3: COL deleted") + deletionOrder = append(deletionOrder, "COL") + } + if existsCUL && !nowCUL { + t.Log("phase 3: CUL deleted") + deletionOrder = append(deletionOrder, "CUL") + } + + existsCOL, existsCUL = nowCOL, nowCUL + return !existsCOL && !existsCUL, nil + }) + require.NoError(t, err, "timed out waiting for COL and CUL to be deleted") + t.Logf("phase 3 deletion order: %v", deletionOrder) + t.Log("phase 3 ordering verified: COL → CUL") + return nil + }) + + // --- Phase 4: Delete the namespace to test symphony-deleting flow with DependsOn --- + deleteNamespace := flow.Func("deleteNamespace", func(ctx context.Context) error { + t.Logf("phase 4: deleting namespace %s", testNS) + return cli.Delete(ctx, ns) + }) + + verifyPhase4Cleanup := flow.Func("verifyPhase4Cleanup", func(ctx context.Context) error { + // Wait for all compositions to be gone (namespace deletion cascades) + err := wait.PollUntilContextTimeout(ctx, 2*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) { + compList := &apiv1.CompositionList{} + if err := cli.List(ctx, compList, client.InNamespace(testNS)); err != nil { + // Namespace may be gone already + return true, nil + } + remaining := 0 + for i := range compList.Items { + for _, ref := range compList.Items[i].OwnerReferences { + if ref.Name == symphonyName { + remaining++ + t.Logf("phase 4: composition %s (synth=%s) still exists", + compList.Items[i].Name, compList.Items[i].Spec.Synthesizer.Name) + } + } + } + if remaining > 0 { + t.Logf("phase 4: %d compositions remaining", remaining) + return false, nil + } + return true, nil + }) + require.NoError(t, err, "timed out waiting for compositions to be cleaned up after namespace deletion") + t.Log("phase 4: all compositions cleaned up after namespace deletion") + return nil + }) + + cleanupSynthesizers := fw.CleanupStep(t, "cleanupSynthesizers", cli, synthA, synthB, synthCOL, synthCUL) + + // ========== WORKFLOW DAG ========== + w := new(flow.Workflow) + w.Add( + // Setup: create namespace and all synthesizers in parallel, then symphony + flow.Step(createSymphony).DependsOn(createNS, createSynthA, createSynthB, createSynthCOL, createSynthCUL), + + // Phase 1: verify initial creation ordering (A → B) + flow.Step(waitPhase1Ready).DependsOn(createSymphony), + flow.Step(verifyPhase1Order).DependsOn(waitPhase1Ready), + + // Phase 2: add synthc-ol and synthc-ul, verify ordering + flow.Step(updateSymphonyPhase2).DependsOn(verifyPhase1Order), + flow.Step(waitPhase2Ready).DependsOn(updateSymphonyPhase2), + flow.Step(verifyPhase2Order).DependsOn(waitPhase2Ready), + + // Phase 3: remove synthc-ol and synthc-ul with delayed cleanup + flow.Step(capturePhase3Keys).DependsOn(verifyPhase2Order), + flow.Step(addCOLFinalizer).DependsOn(capturePhase3Keys), + flow.Step(updateSymphonyPhase3).DependsOn(addCOLFinalizer), + flow.Step(verifyPhase3DeletionOrder).DependsOn(updateSymphonyPhase3), + + // Phase 4: delete namespace, verify cleanup without stuck compositions + flow.Step(deleteNamespace).DependsOn(verifyPhase3DeletionOrder), + flow.Step(verifyPhase4Cleanup).DependsOn(deleteNamespace), + + // Final cleanup of cluster-scoped synthesizers + flow.Step(cleanupSynthesizers).DependsOn(verifyPhase4Cleanup), + ) + + require.NoError(t, w.Do(ctx)) +} + +// getCompsBySynth lists compositions in the given namespace owned by the named symphony +// and returns them keyed by synthesizer name. +func getCompsBySynth(t *testing.T, ctx context.Context, cli client.Client, ns, symphonyName string) map[string]*apiv1.Composition { + t.Helper() + compList := &apiv1.CompositionList{} + require.NoError(t, cli.List(ctx, compList, client.InNamespace(ns))) + + result := map[string]*apiv1.Composition{} + for i := range compList.Items { + c := &compList.Items[i] + for _, ref := range c.OwnerReferences { + if ref.Name == symphonyName { + result[c.Spec.Synthesizer.Name] = c + } + } + } + return result +} diff --git a/internal/controllers/composition/controller.go b/internal/controllers/composition/controller.go index 2d39d865..6f496dbf 100644 --- a/internal/controllers/composition/controller.go +++ b/internal/controllers/composition/controller.go @@ -3,6 +3,7 @@ package composition import ( "context" "fmt" + "path" "time" krmv1 "github.com/Azure/eno/pkg/krm/functions/api/v1" @@ -16,7 +17,9 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" @@ -29,6 +32,7 @@ const ( enoCompositionForceDeleteAnnotation = "eno.azure.io/forceDeleteWhenSymphonyGone" AKSComponentLabel = "aks.azure.com/component-type" // TODO(ruinanliu): Temp workaround remove after 14802391 is released addOnLabelValue = "addon" // TODO(ruinanliu): Temp workaround remove after 14802391 is released + EnoCleanupFinalizer = "eno.azure.io/cleanup" ) type compositionController struct { @@ -41,13 +45,43 @@ func NewController(mgr ctrl.Manager, podTimeout time.Duration) error { client: mgr.GetClient(), podTimeout: podTimeout, } + depPredicate := predicate.TypedFuncs[*apiv1.Composition]{ + CreateFunc: func(e event.TypedCreateEvent[*apiv1.Composition]) bool { return false }, + DeleteFunc: func(e event.TypedDeleteEvent[*apiv1.Composition]) bool { return true }, + UpdateFunc: func(e event.TypedUpdateEvent[*apiv1.Composition]) bool { + // We only notify dependents if their finalizer is removed, meaning actually deleted + prevHasFinalizer := controllerutil.ContainsFinalizer(e.ObjectOld, EnoCleanupFinalizer) + curHasFinalizer := controllerutil.ContainsFinalizer(e.ObjectNew, EnoCleanupFinalizer) + return prevHasFinalizer && !curHasFinalizer + }, + GenericFunc: func(e event.TypedGenericEvent[*apiv1.Composition]) bool { return false }, + } return ctrl.NewControllerManagedBy(mgr). For(&apiv1.Composition{}). WatchesRawSource(source.Kind(mgr.GetCache(), &apiv1.Synthesizer{}, c.newSynthEventHandler())). + WatchesRawSource(source.Kind(mgr.GetCache(), &apiv1.Composition{}, c.newDependencyEventHandler(), depPredicate)). WithLogConstructor(manager.NewLogConstructor(mgr, "compositionController")). Complete(c) } +func (c *compositionController) newDependencyEventHandler() handler.TypedEventHandler[*apiv1.Composition, reconcile.Request] { + fn := func(ctx context.Context, comp *apiv1.Composition) (reqs []reconcile.Request) { + // When a composition's finalizer is removed (effectively deleted), + // notify the compositions it depends on so they can re-check + // hasActiveDependents and unblock their own deletion. + for _, dep := range comp.Spec.DependsOn { + reqs = append(reqs, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: dep.Name, + Namespace: dep.Namespace, + }, + }) + } + return reqs + } + return handler.TypedEnqueueRequestsFromMapFunc(fn) +} + func (c *compositionController) newSynthEventHandler() handler.TypedEventHandler[*apiv1.Synthesizer, reconcile.Request] { fn := func(ctx context.Context, synth *apiv1.Synthesizer) (reqs []reconcile.Request) { logger := logr.FromContextOrDiscard(ctx) @@ -87,7 +121,7 @@ func (c *compositionController) Reconcile(ctx context.Context, req ctrl.Request) return c.reconcileDeletedComposition(ctx, comp) } - if controllerutil.AddFinalizer(comp, "eno.azure.io/cleanup") { + if controllerutil.AddFinalizer(comp, EnoCleanupFinalizer) { err = c.client.Update(ctx, comp) if err != nil { logger.Error(err, "failed to update composition") @@ -144,6 +178,43 @@ func (c *compositionController) Reconcile(ctx context.Context, req ctrl.Request) func (c *compositionController) reconcileDeletedComposition(ctx context.Context, comp *apiv1.Composition) (ctrl.Result, error) { logger := logr.FromContextOrDiscard(ctx) + + // In case in a partial cleanup, we need to check if we want to force remove finalizer before checking hasActiveDependents + if c.shouldForceRemoveFinalizer(ctx, comp) { + logger.Info("force removing finalizer: owning symphony is gone", + "compositionName", comp.Name, "compositionNamespace", comp.Namespace) + if controllerutil.RemoveFinalizer(comp, EnoCleanupFinalizer) { + if err := c.client.Update(ctx, comp); err != nil { + logger.Error(err, "Failed to remove finalizer") + return ctrl.Result{}, err + } + } + logger.Info("removed finalizer from composition") + return ctrl.Result{}, nil + } + + // check whether the composition has any active dependants + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + if err != nil { + logger.Error(err, "failed to get the compositions' dependents") + return ctrl.Result{}, err + } + + if blocked { + logger.Info("waiting for dependents to be deleted", "dependentCount", len(blockedBy)) + return c.updateDependencyStatus(ctx, comp, apiv1.WaitingOnDependentsReason, blockedBy) + } + + // Once the given compositions's dependents are deleted then we can clear the dependency status + // Note that once we clear the hasActiveDependents step we will proceed to deletion. This clear step + // is just for the correctness step during this breif window + if comp.Status.DependencyStatus != nil { + if _, err = c.clearDependencyStatus(ctx, comp); err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } + syn := comp.Status.CurrentSynthesis if syn != nil { // Deletion increments the composition's generation, but the reconstitution cache is only invalidated @@ -166,19 +237,12 @@ func (c *compositionController) reconcileDeletedComposition(ctx context.Context, } if syn.Reconciled == nil { - // If this is an addon composition whose owning Symphony is already gone, - // force-remove the finalizer so the composition doesn't get stuck forever. - if c.shouldForceRemoveFinalizer(ctx, comp) { - logger.Info("force removing finalizer for composition because owning symphony is gone and composition is being marked force delete", - "compositionName", comp.Name, "compositionNamespace", comp.Namespace) - } else { - logger.Info("refusing to remove composition finalizer because it is still being reconciled") - return ctrl.Result{}, nil - } + logger.Info("refusing to remove composition finalizer because it is still being reconciled") + return ctrl.Result{}, nil } } - if controllerutil.RemoveFinalizer(comp, "eno.azure.io/cleanup") { + if controllerutil.RemoveFinalizer(comp, EnoCleanupFinalizer) { err := c.client.Update(ctx, comp) if err != nil { logger.Error(err, "failed to remove finalizer") @@ -286,6 +350,10 @@ func buildSimplifiedStatus(synth *apiv1.Synthesizer, comp *apiv1.Composition) *a current := comp.Status.Simplified if comp.DeletionTimestamp != nil { + if comp.Status.DependencyStatus != nil && comp.Status.DependencyStatus.Blocked { + status.Status = comp.Status.DependencyStatus.Reason + return status + } status.Status = "Deleting" return status } @@ -351,3 +419,60 @@ func buildSimplifiedStatus(synth *apiv1.Synthesizer, comp *apiv1.Composition) *a status.Status = "Unknown" return status } + +func (c *compositionController) hasActiveDependents(ctx context.Context, comp *apiv1.Composition) (bool, []apiv1.BlockedByRef, error) { + logger := logr.FromContextOrDiscard(ctx) + logger.Info("Checking dependents status", "CompositionName", comp.GetName(), "Compositionnamespace", comp.GetNamespace()) + + key := path.Join(comp.GetNamespace(), comp.GetName()) + var dependents apiv1.CompositionList + err := c.client.List(ctx, &dependents, client.MatchingFields{manager.IdxCompositionsByDependency: key}) + if err != nil { + logger.Error(err, "failed to list active dependents for composition") + return false, nil, fmt.Errorf("listing dependents: %w", err) + } + + var blockedBy []apiv1.BlockedByRef + for _, dep := range dependents.Items { + // Skip dependents that does not have a finalizer + if dep.DeletionTimestamp != nil && !controllerutil.ContainsFinalizer(&dep, EnoCleanupFinalizer) { + continue + } + + blockedBy = append(blockedBy, apiv1.BlockedByRef{ + Name: dep.Name, + Namespace: dep.Namespace, + Reason: apiv1.WaitingOnDependentsDeletedReason, + }) + } + + logger.Info("composition dependents check complete", "key", key, "namespace", comp.GetNamespace(), "name", comp.GetName(), "blockedBy", blockedBy) + return len(blockedBy) > 0, blockedBy, nil +} + +func (c *compositionController) updateDependencyStatus(ctx context.Context, comp *apiv1.Composition, reason string, blockedBy []apiv1.BlockedByRef) (ctrl.Result, error) { + newStatus := &apiv1.DependencyStatus{ + Blocked: true, + Reason: reason, + BlockedBy: blockedBy, + } + if equality.Semantic.DeepEqual(comp.Status.DependencyStatus, newStatus) { + return ctrl.Result{}, nil // The dependency is already up to date, no need to do anything + } + + copy := comp.DeepCopy() + copy.Status.DependencyStatus = newStatus + if err := c.client.Status().Patch(ctx, copy, client.MergeFrom(comp)); err != nil { + return ctrl.Result{}, fmt.Errorf("updating dependency status: %w", err) + } + return ctrl.Result{}, nil +} + +func (c *compositionController) clearDependencyStatus(ctx context.Context, comp *apiv1.Composition) (ctrl.Result, error) { + copy := comp.DeepCopy() + copy.Status.DependencyStatus = nil + if err := c.client.Status().Patch(ctx, copy, client.MergeFrom(comp)); err != nil { + return ctrl.Result{}, fmt.Errorf("clearing dependency failed: %w", err) + } + return ctrl.Result{}, nil +} diff --git a/internal/controllers/composition/controller_test.go b/internal/controllers/composition/controller_test.go index a4ee576b..5fee7c2c 100644 --- a/internal/controllers/composition/controller_test.go +++ b/internal/controllers/composition/controller_test.go @@ -448,3 +448,176 @@ func TestShouldForceRemoveFinalizer(t *testing.T) { }) } } + +func TestHasActiveDependents(t *testing.T) { + const namespace = "default" + + t.Run("no dependents", func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Name = "parent" + comp.Namespace = namespace + + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, comp) + c := &compositionController{client: cli} + + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + require.NoError(t, err) + assert.False(t, blocked) + assert.Empty(t, blockedBy) + }) + + t.Run("one active dependent", func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Name = "parent" + comp.Namespace = namespace + + dep := &apiv1.Composition{} + dep.Name = "child" + dep.Namespace = namespace + dep.Finalizers = []string{EnoCleanupFinalizer} + dep.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "parent", Namespace: namespace}} + + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, comp, dep) + c := &compositionController{client: cli} + + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + require.NoError(t, err) + assert.True(t, blocked) + require.Len(t, blockedBy, 1) + assert.Equal(t, "child", blockedBy[0].Name) + assert.Equal(t, namespace, blockedBy[0].Namespace) + assert.Equal(t, apiv1.WaitingOnDependentsDeletedReason, blockedBy[0].Reason) + }) + + t.Run("dependent being deleted with finalizer still blocks", func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Name = "parent" + comp.Namespace = namespace + + dep := &apiv1.Composition{} + dep.Name = "child" + dep.Namespace = namespace + dep.Finalizers = []string{EnoCleanupFinalizer} + dep.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "parent", Namespace: namespace}} + + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, comp, dep) + // Simulate deletion by deleting the dependent (fake client sets DeletionTimestamp when finalizers exist) + require.NoError(t, cli.Delete(ctx, dep)) + c := &compositionController{client: cli} + + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + require.NoError(t, err) + assert.True(t, blocked) + require.Len(t, blockedBy, 1) + assert.Equal(t, "child", blockedBy[0].Name) + }) + + t.Run("dependent being deleted without finalizer does not block", func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Name = "parent" + comp.Namespace = namespace + + dep := &apiv1.Composition{} + dep.Name = "child" + dep.Namespace = namespace + // No finalizer - dependent will be fully deleted + dep.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "parent", Namespace: namespace}} + + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, comp, dep) + // Delete the dependent - without finalizers it gets removed immediately in fake client + require.NoError(t, cli.Delete(ctx, dep)) + c := &compositionController{client: cli} + + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + require.NoError(t, err) + assert.False(t, blocked) + assert.Empty(t, blockedBy) + }) + + t.Run("multiple dependents mixed states", func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Name = "parent" + comp.Namespace = namespace + + activeDep := &apiv1.Composition{} + activeDep.Name = "active-child" + activeDep.Namespace = namespace + activeDep.Finalizers = []string{EnoCleanupFinalizer} + activeDep.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "parent", Namespace: namespace}} + + anotherActiveDep := &apiv1.Composition{} + anotherActiveDep.Name = "another-active-child" + anotherActiveDep.Namespace = namespace + anotherActiveDep.Finalizers = []string{EnoCleanupFinalizer} + anotherActiveDep.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "parent", Namespace: namespace}} + + // This dependent has no finalizer - will be fully deleted + deletableDep := &apiv1.Composition{} + deletableDep.Name = "deletable-child" + deletableDep.Namespace = namespace + deletableDep.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "parent", Namespace: namespace}} + + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, comp, activeDep, anotherActiveDep, deletableDep) + // Delete the one without a finalizer + require.NoError(t, cli.Delete(ctx, deletableDep)) + c := &compositionController{client: cli} + + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + require.NoError(t, err) + assert.True(t, blocked) + assert.Len(t, blockedBy, 2) + + names := []string{blockedBy[0].Name, blockedBy[1].Name} + assert.Contains(t, names, "active-child") + assert.Contains(t, names, "another-active-child") + }) + + t.Run("unrelated composition does not count as dependent", func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Name = "parent" + comp.Namespace = namespace + + unrelated := &apiv1.Composition{} + unrelated.Name = "unrelated" + unrelated.Namespace = namespace + unrelated.Finalizers = []string{EnoCleanupFinalizer} + // Depends on a different composition + unrelated.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "other-parent", Namespace: namespace}} + + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, comp, unrelated) + c := &compositionController{client: cli} + + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + require.NoError(t, err) + assert.False(t, blocked) + assert.Empty(t, blockedBy) + }) + + t.Run("dependent without finalizer and not deleted is active", func(t *testing.T) { + comp := &apiv1.Composition{} + comp.Name = "parent" + comp.Namespace = namespace + + dep := &apiv1.Composition{} + dep.Name = "child-no-finalizer" + dep.Namespace = namespace + // No finalizer, not deleted - still counts as active + dep.Spec.DependsOn = []apiv1.CompositionDependency{{Name: "parent", Namespace: namespace}} + + ctx := testutil.NewContext(t) + cli := testutil.NewClient(t, comp, dep) + c := &compositionController{client: cli} + + blocked, blockedBy, err := c.hasActiveDependents(ctx, comp) + require.NoError(t, err) + assert.True(t, blocked) + require.Len(t, blockedBy, 1) + assert.Equal(t, "child-no-finalizer", blockedBy[0].Name) + }) +} diff --git a/internal/controllers/reconciliation/controller.go b/internal/controllers/reconciliation/controller.go index 7959c4ae..40e627fb 100644 --- a/internal/controllers/reconciliation/controller.go +++ b/internal/controllers/reconciliation/controller.go @@ -149,7 +149,7 @@ func (c *Controller) Reconcile(ctx context.Context, req resource.Request) (ctrl. logger.Info("reconcileResource") snap, current, ready, modified, err := c.reconcileResource(ctx, comp, prev, resource) - failingOpen := c.shouldFailOpen(resource) + failingOpen := c.shouldFailOpen(snap, resource) if failingOpen { logger.Info("FailOpen - suppressing errors") err = nil @@ -165,15 +165,16 @@ func (c *Controller) Reconcile(ctx context.Context, req resource.Request) (ctrl. return ctrl.Result{Requeue: true}, nil } - deleted := current == nil || - (current.GetDeletionTimestamp() != nil && !snap.ForegroundDeletion) || - (snap.Deleted() && (snap.Orphan || snap.Disable || failingOpen)) // orphaning should be reflected on the status. + deleted := markResourceAsDeleted(current, snap, failingOpen) c.writeBuffer.PatchStatusAsync(ctx, &resource.ManifestRef, patchResourceState(deleted, ready)) return c.requeue(logger, snap, ready) } -func (c *Controller) shouldFailOpen(resource *resource.Resource) bool { +func (c *Controller) shouldFailOpen(snap *resource.Snapshot, resource *resource.Resource) bool { + if snap != nil && snap.Deleted() && snap.ForegroundDeletion { + return false + } return (resource.FailOpen == nil && c.failOpen) || (resource.FailOpen != nil && *resource.FailOpen) } @@ -533,3 +534,21 @@ func isErrNoKindMatch(err error) bool { } return strings.Contains(err.Error(), "no matches for kind") } + +func markResourceAsDeleted(current *unstructured.Unstructured, snap *resource.Snapshot, failingOpen bool) bool { + // Resource does not exist at all + if current == nil { + return true + } + + // Resource has a deletion timestamp and is using background deletion + if current.GetDeletionTimestamp() != nil && !snap.ForegroundDeletion { + return true + } + + // Orphaned or disabled resources should be reflected as deleted + if snap.Deleted() && (snap.Orphan || snap.Disable || failingOpen) { + return true + } + return false +} diff --git a/internal/controllers/reconciliation/controller_test.go b/internal/controllers/reconciliation/controller_test.go index b8653211..552def1f 100644 --- a/internal/controllers/reconciliation/controller_test.go +++ b/internal/controllers/reconciliation/controller_test.go @@ -15,6 +15,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -112,14 +113,56 @@ func TestBuildNonStrategicPatch_NilPrevious(t *testing.T) { func TestShouldFailOpen(t *testing.T) { c := &Controller{failOpen: true} - assert.True(t, c.shouldFailOpen(&resource.Resource{FailOpen: nil})) - assert.False(t, c.shouldFailOpen(&resource.Resource{FailOpen: ptr.To(false)})) - assert.True(t, c.shouldFailOpen(&resource.Resource{FailOpen: ptr.To(true)})) + assert.True(t, c.shouldFailOpen(nil, &resource.Resource{FailOpen: nil})) + assert.False(t, c.shouldFailOpen(nil, &resource.Resource{FailOpen: ptr.To(false)})) + assert.True(t, c.shouldFailOpen(nil, &resource.Resource{FailOpen: ptr.To(true)})) c.failOpen = false - assert.False(t, c.shouldFailOpen(&resource.Resource{FailOpen: nil})) - assert.False(t, c.shouldFailOpen(&resource.Resource{FailOpen: ptr.To(false)})) - assert.True(t, c.shouldFailOpen(&resource.Resource{FailOpen: ptr.To(true)})) + assert.False(t, c.shouldFailOpen(nil, &resource.Resource{FailOpen: nil})) + assert.False(t, c.shouldFailOpen(nil, &resource.Resource{FailOpen: ptr.To(false)})) + assert.True(t, c.shouldFailOpen(nil, &resource.Resource{FailOpen: ptr.To(true)})) + + // Foreground deletion: snap.Deleted() && snap.ForegroundDeletion always returns false + deletedForeground := &resource.Snapshot{ + Resource: &resource.Resource{}, + Disable: true, // makes Deleted() return true + ForegroundDeletion: true, + } + c.failOpen = true + assert.False(t, c.shouldFailOpen(deletedForeground, &resource.Resource{FailOpen: nil})) + assert.False(t, c.shouldFailOpen(deletedForeground, &resource.Resource{FailOpen: ptr.To(true)})) + assert.False(t, c.shouldFailOpen(deletedForeground, &resource.Resource{FailOpen: ptr.To(false)})) + + c.failOpen = false + assert.False(t, c.shouldFailOpen(deletedForeground, &resource.Resource{FailOpen: nil})) + assert.False(t, c.shouldFailOpen(deletedForeground, &resource.Resource{FailOpen: ptr.To(true)})) + assert.False(t, c.shouldFailOpen(deletedForeground, &resource.Resource{FailOpen: ptr.To(false)})) + + // Deleted but NOT foreground deletion: normal fail-open logic applies + deletedBackground := &resource.Snapshot{ + Resource: &resource.Resource{}, + Disable: true, // makes Deleted() return true + ForegroundDeletion: false, + } + c.failOpen = true + assert.True(t, c.shouldFailOpen(deletedBackground, &resource.Resource{FailOpen: nil})) + assert.True(t, c.shouldFailOpen(deletedBackground, &resource.Resource{FailOpen: ptr.To(true)})) + assert.False(t, c.shouldFailOpen(deletedBackground, &resource.Resource{FailOpen: ptr.To(false)})) + + c.failOpen = false + assert.False(t, c.shouldFailOpen(deletedBackground, &resource.Resource{FailOpen: nil})) + assert.True(t, c.shouldFailOpen(deletedBackground, &resource.Resource{FailOpen: ptr.To(true)})) + assert.False(t, c.shouldFailOpen(deletedBackground, &resource.Resource{FailOpen: ptr.To(false)})) + + // Not deleted with foreground deletion set: normal fail-open logic applies + notDeletedForeground := &resource.Snapshot{ + Resource: &resource.Resource{}, + ForegroundDeletion: true, + } + c.failOpen = true + assert.True(t, c.shouldFailOpen(notDeletedForeground, &resource.Resource{FailOpen: nil})) + assert.True(t, c.shouldFailOpen(notDeletedForeground, &resource.Resource{FailOpen: ptr.To(true)})) + assert.False(t, c.shouldFailOpen(notDeletedForeground, &resource.Resource{FailOpen: ptr.To(false)})) } func TestSummarizeError(t *testing.T) { @@ -249,3 +292,93 @@ func TestRequeueDoesNotPanic(t *testing.T) { }). Evaluate(t) } + +func TestMarkResourceAsDeleted(t *testing.T) { + now := metav1.Now() + + newCurrentWithDeletionTimestamp := func() *unstructured.Unstructured { + u := &unstructured.Unstructured{Object: map[string]any{}} + u.SetDeletionTimestamp(&now) + return u + } + + tests := []struct { + name string + current *unstructured.Unstructured + snap *resource.Snapshot + failingOpen bool + expected bool + }{ + { + name: "current is nil - resource does not exist", + current: nil, + snap: &resource.Snapshot{Resource: &resource.Resource{}}, + expected: true, + }, + { + name: "current has deletion timestamp with background deletion", + current: newCurrentWithDeletionTimestamp(), + snap: &resource.Snapshot{Resource: &resource.Resource{}, ForegroundDeletion: false}, + expected: true, + }, + { + name: "current has deletion timestamp with foreground deletion", + current: newCurrentWithDeletionTimestamp(), + snap: &resource.Snapshot{Resource: &resource.Resource{}, ForegroundDeletion: true}, + expected: false, + }, + { + name: "resource is disabled - treated as deleted", + current: &unstructured.Unstructured{Object: map[string]any{}}, + snap: &resource.Snapshot{Resource: &resource.Resource{}, Disable: true}, + expected: true, + }, + { + name: "resource is disabled and orphaned - treated as deleted", + current: &unstructured.Unstructured{Object: map[string]any{}}, + snap: &resource.Snapshot{Resource: &resource.Resource{}, Disable: true, Orphan: true}, + expected: true, + }, + { + name: "resource exists and is not deleting", + current: &unstructured.Unstructured{Object: map[string]any{}}, + snap: &resource.Snapshot{Resource: &resource.Resource{}}, + expected: false, + }, + { + name: "failingOpen but resource not deleted", + current: &unstructured.Unstructured{Object: map[string]any{}}, + snap: &resource.Snapshot{Resource: &resource.Resource{}}, + failingOpen: true, + expected: false, + }, + { + name: "disabled resource with failingOpen and no foreground deletion", + current: &unstructured.Unstructured{Object: map[string]any{}}, + snap: &resource.Snapshot{Resource: &resource.Resource{}, Disable: true}, + failingOpen: true, + expected: true, + }, + { + name: "disabled resource with failingOpen and foreground deletion - disable takes precedence", + current: &unstructured.Unstructured{Object: map[string]any{}}, + snap: &resource.Snapshot{Resource: &resource.Resource{}, Disable: true, ForegroundDeletion: true}, + failingOpen: true, + expected: true, + }, + { + name: "failingOpen with foreground deletion - should not mark as deleted", + current: newCurrentWithDeletionTimestamp(), + snap: &resource.Snapshot{Resource: &resource.Resource{}, ForegroundDeletion: true}, + failingOpen: true, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := markResourceAsDeleted(tt.current, tt.snap, tt.failingOpen) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/internal/controllers/reconciliation/ordering_test.go b/internal/controllers/reconciliation/ordering_test.go index 4ccacec5..c29509c3 100644 --- a/internal/controllers/reconciliation/ordering_test.go +++ b/internal/controllers/reconciliation/ordering_test.go @@ -754,9 +754,8 @@ func TestDeletionOrderLiveness(t *testing.T) { "name": "fail-open", "namespace": "default", "annotations": map[string]any{ - "eno.azure.io/deletion-group": "678", - "eno.azure.io/fail-open": "true", - "eno.azure.io/deletion-strategy": "Foreground", + "eno.azure.io/deletion-group": "678", + "eno.azure.io/fail-open": "true", }, }, }, diff --git a/internal/manager/indices.go b/internal/manager/indices.go index f10fbdf2..dbf45b27 100644 --- a/internal/manager/indices.go +++ b/internal/manager/indices.go @@ -13,6 +13,7 @@ const ( IdxCompositionsBySymphony = ".compositionsBySymphony" IdxCompositionsByBinding = ".compositionsByBinding" IdxSynthesizersByRef = ".synthesizersByRef" + IdxCompositionsByDependency = ".spec.dependsOn" ) func indexController() client.IndexerFunc { @@ -54,3 +55,17 @@ func indexSynthRefs() client.IndexerFunc { return keys } } + +func indexCompositionsByDependency() client.IndexerFunc { + return func(o client.Object) []string { + comp, ok := o.(*apiv1.Composition) + if !ok { + return nil + } + var keys []string + for _, dep := range comp.Spec.DependsOn { + keys = append(keys, path.Join(dep.Namespace, dep.Name)) + } + return keys + } +} diff --git a/internal/manager/indices_test.go b/internal/manager/indices_test.go new file mode 100644 index 00000000..17cceb14 --- /dev/null +++ b/internal/manager/indices_test.go @@ -0,0 +1,51 @@ +package manager + +import ( + "testing" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestIndexCompositionsByDependency(t *testing.T) { + indexer := indexCompositionsByDependency() + + t.Run("non-composition object returns nil", func(t *testing.T) { + pod := &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Name: "test"}} + assert.Nil(t, indexer(pod)) + }) + + t.Run("composition with no dependencies", func(t *testing.T) { + comp := &apiv1.Composition{ + Spec: apiv1.CompositionSpec{}, + } + assert.Nil(t, indexer(comp)) + }) + + t.Run("composition with one dependency", func(t *testing.T) { + comp := &apiv1.Composition{ + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Namespace: "ns1", Name: "dep1"}, + }, + }, + } + keys := indexer(comp) + assert.Equal(t, []string{"ns1/dep1"}, keys) + }) + + t.Run("composition with multiple dependencies", func(t *testing.T) { + comp := &apiv1.Composition{ + Spec: apiv1.CompositionSpec{ + DependsOn: []apiv1.CompositionDependency{ + {Namespace: "ns1", Name: "dep1"}, + {Namespace: "ns2", Name: "dep2"}, + }, + }, + } + keys := indexer(comp) + assert.Equal(t, []string{"ns1/dep1", "ns2/dep2"}, keys) + }) +} diff --git a/internal/manager/manager.go b/internal/manager/manager.go index 0e1437a1..371e8afa 100644 --- a/internal/manager/manager.go +++ b/internal/manager/manager.go @@ -175,6 +175,10 @@ func newMgr(logger logr.Logger, opts *Options, isController, isReconciler bool) if err != nil { return nil, err } + err = mgr.GetFieldIndexer().IndexField(context.Background(), &apiv1.Composition{}, IdxCompositionsByDependency, indexCompositionsByDependency()) + if err != nil { + return nil, err + } } mgr.AddHealthzCheck("ping", healthz.Ping) diff --git a/internal/testutil/testutil.go b/internal/testutil/testutil.go index ac544351..ffaf570c 100644 --- a/internal/testutil/testutil.go +++ b/internal/testutil/testutil.go @@ -6,6 +6,7 @@ import ( "errors" "fmt" "os" + "path" "path/filepath" goruntime "runtime" "strconv" @@ -50,7 +51,15 @@ func NewClientWithInterceptors(t testing.TB, ict *interceptor.Funcs, objs ...cli builder := fake.NewClientBuilder(). WithScheme(scheme). WithObjects(objs...). - WithStatusSubresource(&apiv1.ResourceSlice{}, &apiv1.Composition{}, &apiv1.Symphony{}) + WithStatusSubresource(&apiv1.ResourceSlice{}, &apiv1.Composition{}, &apiv1.Symphony{}). + WithIndex(&apiv1.Composition{}, manager.IdxCompositionsByDependency, func(o client.Object) []string { + comp := o.(*apiv1.Composition) + var keys []string + for _, dep := range comp.Spec.DependsOn { + keys = append(keys, path.Join(dep.Namespace, dep.Name)) + } + return keys + }) if ict != nil { builder.WithInterceptorFuncs(*ict)