diff --git a/internal/tools/archive/getter.go b/internal/tools/archive/getter.go index 2badb6e..2801188 100644 --- a/internal/tools/archive/getter.go +++ b/internal/tools/archive/getter.go @@ -315,20 +315,73 @@ func (g *dynamicGetter) searchCompositionDefinition(gvr schema.GroupVersionResou compositionDefinition := &all.Items[0] if tot > 1 { + instanceLabels := mg.GetLabels() found := false - for _, el := range all.Items { - version, kind, err := getChartVersionKind(&el) - if err != nil { - g.logger.Debug("Failed to get chart version and kind", "error", err.Error(), "compositionDefinitionName", el.GetName(), "compositionDefinitionNamespace", el.GetNamespace(), "gvr", gvr.String()) - continue + + // 1. Authoritative: the definition-ref labels stamped on the composition instance. + // These identify the owning CompositionDefinition by name+namespace and survive + // chart-version bumps, unlike the composition-version label which is only migrated + // by a successful reconcile. + refName := instanceLabels[compositionMeta.CompositionDefinitionNameLabel] + refNamespace := instanceLabels[compositionMeta.CompositionDefinitionNamespaceLabel] + if refName != "" && refNamespace != "" { + for i := range all.Items { + el := &all.Items[i] + if el.GetName() == refName && el.GetNamespace() == refNamespace { + compositionDefinition = el + g.logger.Debug("Resolved composition definition via definition-ref labels", "compositionDefinitionName", refName, "compositionDefinitionNamespace", refNamespace, "gvr", gvr.String()) + found = true + break + } + } + if !found { + // Stale labels are possible: fall through to version/kind matching. + g.logger.Debug("Definition-ref labels did not match any composition definition, falling back to version/kind matching", "compositionDefinitionName", refName, "compositionDefinitionNamespace", refNamespace, "gvr", gvr.String()) + } + } + + // 2. Exact match on chart version + kind (previous behavior). + if !found { + for i := range all.Items { + el := &all.Items[i] + version, kind, err := getChartVersionKind(el) + if err != nil { + g.logger.Debug("Failed to get chart version and kind", "error", err.Error(), "compositionDefinitionName", el.GetName(), "compositionDefinitionNamespace", el.GetNamespace(), "gvr", gvr.String()) + continue + } + if version == instanceLabels[compositionMeta.CompositionVersionLabel] && kind == mg.GetKind() { + compositionDefinition = el + g.logger.Debug("Found matching composition definition", "compositionDefinitionName", el.GetName(), "compositionDefinitionNamespace", el.GetNamespace(), "gvr", gvr.String()) + found = true + break + } + } + } + + // 3. Last resort: a single definition serving this kind. During a chart-version bump + // the definition's status version moves ahead of the instance's composition-version + // label (which only a successful reconcile would migrate), so an exact version match + // can never succeed and reconciliation wedges. Tolerate the skew when the owner is + // unambiguous. + if !found { + var sameKind []*unstructured.Unstructured + for i := range all.Items { + el := &all.Items[i] + _, kind, err := getChartVersionKind(el) + if err != nil { + continue + } + if kind == mg.GetKind() { + sameKind = append(sameKind, el) + } } - if version == mg.GetLabels()[compositionMeta.CompositionVersionLabel] && kind == mg.GetKind() { - compositionDefinition = &el - g.logger.Debug("Found matching composition definition", "compositionDefinitionName", el.GetName(), "compositionDefinitionNamespace", el.GetNamespace(), "gvr", gvr.String()) + if len(sameKind) == 1 { + compositionDefinition = sameKind[0] + g.logger.Warn("Resolved composition definition by unique kind, tolerating composition-version label skew", "compositionDefinitionName", compositionDefinition.GetName(), "compositionDefinitionNamespace", compositionDefinition.GetNamespace(), "expectedVersion", instanceLabels[compositionMeta.CompositionVersionLabel], "kind", mg.GetKind(), "gvr", gvr.String()) found = true - break } } + if !found { return nil, fmt.Errorf("too many definitions [%d] found for '%v' in namespace: %s", tot, gvr.String(), mg.GetNamespace()) diff --git a/internal/tools/archive/getter_search_test.go b/internal/tools/archive/getter_search_test.go new file mode 100644 index 0000000..382ddcc --- /dev/null +++ b/internal/tools/archive/getter_search_test.go @@ -0,0 +1,174 @@ +package archive + +import ( + "strings" + "testing" + + compositionMeta "github.com/krateoplatformops/composition-dynamic-controller/pkg/meta" + "github.com/krateoplatformops/unstructured-runtime/pkg/logging" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + dynamicfake "k8s.io/client-go/dynamic/fake" +) + +// searchTestCDGVR is the GVR searchCompositionDefinition lists. +var searchTestCDGVR = schema.GroupVersionResource{ + Group: "core.krateo.io", + Version: "v1alpha1", + Resource: "compositiondefinitions", +} + +// newSearchTestCD builds a CompositionDefinition with the status fields the matcher reads +// (status.apiVersion carries the chart-version suffix, status.kind the served kind). +func newSearchTestCD(name, namespace, statusVersion, statusKind string) *unstructured.Unstructured { + cd := &unstructured.Unstructured{} + cd.SetGroupVersionKind(schema.GroupVersionKind{Group: "core.krateo.io", Version: "v1alpha1", Kind: "CompositionDefinition"}) + cd.SetNamespace(namespace) + cd.SetName(name) + _ = unstructured.SetNestedField(cd.Object, "composition.krateo.io/"+statusVersion, "status", "apiVersion") + _ = unstructured.SetNestedField(cd.Object, statusKind, "status", "kind") + return cd +} + +// newSearchTestComposition builds a composition instance with the given kind and +// composition-version label; extraLabels lets cases add the definition-ref labels. +func newSearchTestComposition(kind, versionLabel string, extraLabels map[string]string) *unstructured.Unstructured { + lbl := map[string]string{ + compositionMeta.CompositionVersionLabel: versionLabel, + } + for k, v := range extraLabels { + lbl[k] = v + } + mg := &unstructured.Unstructured{} + mg.SetAPIVersion("composition.krateo.io/" + versionLabel) + mg.SetKind(kind) + mg.SetNamespace("krateo-system") + mg.SetName("my-instance") + mg.SetLabels(lbl) + return mg +} + +func definitionRefLabels(name, namespace string) map[string]string { + return map[string]string{ + compositionMeta.CompositionDefinitionNameLabel: name, + compositionMeta.CompositionDefinitionNamespaceLabel: namespace, + } +} + +func TestSearchCompositionDefinition(t *testing.T) { + gvr := schema.GroupVersionResource{Group: "composition.krateo.io", Version: "v1-5-11", Resource: "fireworksapps"} + + cases := []struct { + name string + definitions []*unstructured.Unstructured + instance *unstructured.Unstructured + wantName string // expected resolved CompositionDefinition name; "" => expect error + wantErr string // substring the error must contain when wantName == "" + }{ + { + // (a) The instance's composition-version label lags the CD (chart bump in flight) + // AND two CDs serve the same kind, so neither exact-version nor unique-kind can + // resolve: only the definition-ref labels identify the owner. + name: "definition-ref labels win despite version skew and same-kind ambiguity", + definitions: []*unstructured.Unstructured{ + newSearchTestCD("portal", "krateo-system", "v1-5-12", "FireworksApp"), + newSearchTestCD("portal-old", "krateo-system", "v1-5-10", "FireworksApp"), + }, + instance: newSearchTestComposition("FireworksApp", "v1-5-11", + definitionRefLabels("portal", "krateo-system")), + wantName: "portal", + }, + { + // (b) Regression: the pre-existing exact (version, kind) match still resolves + // when no definition-ref labels are present. + name: "exact version+kind match still works without ref labels", + definitions: []*unstructured.Unstructured{ + newSearchTestCD("portal", "krateo-system", "v1-5-11", "FireworksApp"), + newSearchTestCD("other", "krateo-system", "v2-0-0", "OtherApp"), + }, + instance: newSearchTestComposition("FireworksApp", "v1-5-11", nil), + wantName: "portal", + }, + { + // (b bis) Stale definition-ref labels (owner renamed/deleted) must not wedge: + // fall through to the exact version+kind match. + name: "stale ref labels fall through to exact version match", + definitions: []*unstructured.Unstructured{ + newSearchTestCD("portal", "krateo-system", "v1-5-11", "FireworksApp"), + newSearchTestCD("other", "krateo-system", "v2-0-0", "OtherApp"), + }, + instance: newSearchTestComposition("FireworksApp", "v1-5-11", + definitionRefLabels("gone", "krateo-system")), + wantName: "portal", + }, + { + // (c) Version-bump wedge: label says v1-5-11, CD moved to v1-5-12, no ref labels. + // Exactly one CD serves the kind -> unique-kind fallback unwedges the migration. + name: "unique-kind fallback tolerates version skew without ref labels", + definitions: []*unstructured.Unstructured{ + newSearchTestCD("portal", "krateo-system", "v1-5-12", "FireworksApp"), + newSearchTestCD("other", "krateo-system", "v2-0-0", "OtherApp"), + }, + instance: newSearchTestComposition("FireworksApp", "v1-5-11", nil), + wantName: "portal", + }, + { + // (d) Two CDs serve the same kind, version skew, no ref labels: still ambiguous, + // keep the existing error rather than guessing. + name: "ambiguous same-kind definitions with version skew still error", + definitions: []*unstructured.Unstructured{ + newSearchTestCD("portal", "krateo-system", "v1-5-12", "FireworksApp"), + newSearchTestCD("portal-old", "krateo-system", "v1-5-10", "FireworksApp"), + }, + instance: newSearchTestComposition("FireworksApp", "v1-5-11", nil), + wantErr: "too many definitions", + }, + { + // tot == 1 fast path unchanged: the single CD is used even with version skew. + name: "single definition fast path unchanged", + definitions: []*unstructured.Unstructured{ + newSearchTestCD("portal", "krateo-system", "v1-5-12", "FireworksApp"), + }, + instance: newSearchTestComposition("FireworksApp", "v1-5-11", nil), + wantName: "portal", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + scheme := runtime.NewScheme() + gvrToListKind := map[schema.GroupVersionResource]string{searchTestCDGVR: "CompositionDefinitionList"} + + objs := make([]runtime.Object, 0, len(tc.definitions)) + for _, cd := range tc.definitions { + objs = append(objs, cd) + } + dyn := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(scheme, gvrToListKind, objs...) + + g := &dynamicGetter{ + dynamicClient: dyn, + logger: logging.NewNopLogger(), + } + + got, err := g.searchCompositionDefinition(gvr, tc.instance) + + if tc.wantName == "" { + if err == nil { + t.Fatalf("expected error containing %q, got definition %q", tc.wantErr, got.GetName()) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("expected error containing %q, got: %v", tc.wantErr, err) + } + return + } + + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.GetName() != tc.wantName { + t.Errorf("resolved definition = %q, want %q", got.GetName(), tc.wantName) + } + }) + } +}