From dcfe3a06cbca833925c1ba2ea7c4848c0e2fbb7e Mon Sep 17 00:00:00 2001 From: Diego Braga Date: Mon, 13 Jul 2026 21:59:12 +0200 Subject: [PATCH] fix(archive): resolve CompositionDefinition via definition-ref labels + unique-kind fallback searchCompositionDefinition resolved the owning CompositionDefinition by listing ALL CompositionDefinitions and, with more than one present, requiring an EXACT match between the CD's status.apiVersion version suffix and the instance's krateo.io/composition-version label (plus status.kind == instance kind). During a chart-version bump the CD's status version moves ahead (e.g. v1-5-12) while the existing instance's label still says v1-5-11: no match -> 'too many definitions [N] found' -> reconcile permanently wedged, since only a successful reconcile would migrate the label (chicken-and-egg). Composition instances already carry authoritative owner-reference labels (krateo.io/composition-definition-name/-namespace, stamped via SetCompositionDefinitionLabels) that the matcher ignored. Replace the match with an ordered fallback chain, preserving current behavior where it works: 1. definition-ref labels: pick the listed CD matching the instance's composition-definition-name + -namespace labels (authoritative, survives version bumps); stale labels fall through. 2. exact (chart version == composition-version label) AND (status.kind == kind) match, as before. 3. last resort: if EXACTLY ONE CD serves the instance's kind, use it and Warn that version-label skew was tolerated -> unwedges version-bump migrations. Zero or ambiguous still errors. The tot==1 fast path (single CD -> Items[0]) is unchanged. Unit tests (fake dynamic client, table-driven) cover: ref-label match winning under version skew + same-kind ambiguity, exact-match regression, stale-ref fallthrough, unique-kind skew tolerance, ambiguous same-kind error, and the single-CD fast path. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/tools/archive/getter.go | 71 +++++++- internal/tools/archive/getter_search_test.go | 174 +++++++++++++++++++ 2 files changed, 236 insertions(+), 9 deletions(-) create mode 100644 internal/tools/archive/getter_search_test.go 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) + } + }) + } +}