diff --git a/api/aicr/v1/server.yaml b/api/aicr/v1/server.yaml index d0d823be5..cdf1609d2 100644 --- a/api/aicr/v1/server.yaml +++ b/api/aicr/v1/server.yaml @@ -1653,7 +1653,11 @@ components: properties: name: type: string - description: Component name + description: >- + Component name. Must be unique within componentRefs (across + both enabled and disabled refs) when non-empty; a duplicate + non-empty name is rejected with 400. Refs with an empty + name are exempt from the uniqueness check. type: type: string enum: [Helm, Kustomize] diff --git a/docs/integrator/go-library.md b/docs/integrator/go-library.md index 6efd4ae82..e63047c4c 100644 --- a/docs/integrator/go-library.md +++ b/docs/integrator/go-library.md @@ -248,7 +248,9 @@ identifiers. When you already hold a `pkg/recipe.AllowLists`, use `ResolveRecipe` takes the stable `RecipeRequest` shape and returns the facade `RecipeResult` — a deliberately small struct exposing the -`Name`, `Version`, and `Components` of the resolved recipe. When you +`Name`, `Version`, and `Components` of the resolved recipe. `Components` +lists enabled (deployable) components only; disabled refs remain visible +via `Resolved().ComponentRefs`. When you already hold an `*aicr.Criteria` value — for example, a REST handler that parsed criteria from an incoming HTTP request and wrapped them with `aicr.WrapCriteria` — use `ResolveRecipeFromCriteria`. It returns the diff --git a/docs/integrator/public-api.md b/docs/integrator/public-api.md index 21e86e105..648c1f946 100644 --- a/docs/integrator/public-api.md +++ b/docs/integrator/public-api.md @@ -64,7 +64,7 @@ aliases — the table below documents which. | `aicr.Phase`, `aicr.PhaseDeployment` / `PhasePerformance` / `PhaseConformance` | string consts | **Facade-owned**. Values match `pkg/validator/v1` constants verbatim for byte-identical wire round-trip. | | `aicr.ReportSummary` | `pkg/validator/ctrf.Summary` | **Facade-owned struct** with the CTRF count fields. | | `aicr.ValidateOption` | `pkg/validator.Option` | **Facade-owned** functional-option type that captures into an internal struct and translates at call time. | -| `aicr.RecipeResult` | `pkg/recipe.RecipeResult` | **Facade-owned struct** exposing `Name`, `Version`, `TranslatedAt`, and `Components`. Call `Resolved()` for the full upstream `*pkg/recipe.RecipeResult` (constraints, deployment order, validation config, metadata). The previous `aicr.Recipe` alias was removed in #1115; `ResolveRecipeFromCriteria` and `ResolveRecipeFromSnapshot` now return `*RecipeResult`. | +| `aicr.RecipeResult` | `pkg/recipe.RecipeResult` | **Facade-owned struct** exposing `Name`, `Version`, `TranslatedAt`, and `Components` (enabled/deployable components only — disabled refs remain visible via `Resolved().ComponentRefs`). Call `Resolved()` for the full upstream `*pkg/recipe.RecipeResult` (constraints, deployment order, validation config, metadata). The previous `aicr.Recipe` alias was removed in #1115; `ResolveRecipeFromCriteria` and `ResolveRecipeFromSnapshot` now return `*RecipeResult`. | | `aicr.AllowLists` | `pkg/recipe.AllowLists` | **Facade-owned struct** with `[]string` fields (Accelerators / Services / Intents / OSTypes). Use `aicr.WrapAllowLists` to lift a `*pkg/recipe.AllowLists`. | | `aicr.Criteria` | `pkg/recipe.Criteria` | **Facade-owned struct** whose enum-typed fields (Service / Accelerator / Intent / OS / Platform) project to plain strings; Nodes stays an `int` per the facade's string/int contract. Use `aicr.WrapCriteria` to lift a `*pkg/recipe.Criteria`. | | `aicr.CriteriaRegistry` | `pkg/recipe.CriteriaRegistry` | Documented transparent alias. Kept as an alias intentionally because the registry is behavior-rich (`ParseService`, `SetStrict`, `Values`, ...) and carries mutable per-`DataProvider` state — wrapping would either break the per-Client identity coupling (copy) or add no isolation win over the alias (pointer). | diff --git a/docs/user/api-reference.md b/docs/user/api-reference.md index e3a7189c9..1d058d9ca 100644 --- a/docs/user/api-reference.md +++ b/docs/user/api-reference.md @@ -486,6 +486,10 @@ These are the recipe **components** in [`recipes/registry.yaml`](https://github. > carrying surrounding whitespace are rejected — deployers consume them > verbatim. > Incoherent refs are rejected with HTTP 400 naming the component. +> Component ref names must also be unique within a recipe (enabled or +> disabled refs) when non-empty; a duplicate non-empty name is rejected +> with HTTP 400 naming the conflicting positions. Refs with an empty +> name are exempt from the uniqueness check. ```shell # Basic: pipe recipe to bundle diff --git a/pkg/client/v1/aicr.go b/pkg/client/v1/aicr.go index db0775560..e224e2e33 100644 --- a/pkg/client/v1/aicr.go +++ b/pkg/client/v1/aicr.go @@ -857,6 +857,9 @@ func facadeResultFromInternal(r *recipe.RecipeResult, name string) *RecipeResult internal: r, } for _, c := range r.ComponentRefs { + if !c.IsEnabled() { + continue + } // Project the chart the component actually deploys: a source-only // Helm ref falls back to the component name (the deployers' // EffectiveChart rule), so SDK consumers never see an empty chart @@ -1055,6 +1058,16 @@ func (c *Client) BundleComponents(ctx context.Context, r *RecipeResult) ([]Compo return nil, preflightErr } + // Agree with DefaultBundler.Make (bundler.go), which fails closed with + // ErrCodeInvalidRequest whenever there is nothing to deploy — whether + // that's every component disabled or a zero-componentRef recipe. An + // SDK caller that loops over the result and deploys would otherwise + // silently no-op on either input. + if len(r.Components) == 0 { + return nil, errors.New(errors.ErrCodeInvalidRequest, + "recipe has no enabled components") + } + bundles := make([]ComponentBundle, 0, len(r.Components)) for i := range r.Components { // Bail on every iteration so a long recipe doesn't hold diff --git a/pkg/client/v1/aicr_internal_test.go b/pkg/client/v1/aicr_internal_test.go index c05c24371..e0f6274c1 100644 --- a/pkg/client/v1/aicr_internal_test.go +++ b/pkg/client/v1/aicr_internal_test.go @@ -367,6 +367,60 @@ func TestBundleComponents_HelmComponentLoadsManifestFiles(t *testing.T) { } } +// TestBundleComponents_RejectsAllDisabled locks in that an all-disabled +// recipe is rejected the same way DefaultBundler.Make rejects it +// (bundler.go), rather than silently returning an empty, successful +// bundle list. See #1917. +func TestBundleComponents_RejectsAllDisabled(t *testing.T) { + t.Parallel() + + client := newClientForBundleTest(t) + r := newRecipeResultForBundleTest(client, + []recipe.ComponentRef{ + { + Name: "disabled-a", + Type: recipe.ComponentTypeHelm, + Overrides: map[string]any{"enabled": false}, + }, + }, + nil, // facade Components is empty because the only ref is disabled + ) + _, err := client.BundleComponents(context.Background(), r) + if err == nil { + t.Fatal("expected error for all-disabled recipe, got nil") + } + var se *aicrerrors.StructuredError + if !stderrors.As(err, &se) { + t.Fatalf("expected *aicrerrors.StructuredError, got %T: %v", err, err) + } + if se.Code != aicrerrors.ErrCodeInvalidRequest { + t.Errorf("expected ErrCodeInvalidRequest, got %s", se.Code) + } +} + +// TestBundleComponents_RejectsZeroComponentRefs locks in that a recipe +// with no componentRefs at all is rejected the same way as an all- +// disabled recipe — the zero-ref divergence from DefaultBundler.Make +// that the dropped r.internal.ComponentRefs conjunct used to miss. +// See #1917. +func TestBundleComponents_RejectsZeroComponentRefs(t *testing.T) { + t.Parallel() + + client := newClientForBundleTest(t) + r := newRecipeResultForBundleTest(client, nil, nil) + _, err := client.BundleComponents(context.Background(), r) + if err == nil { + t.Fatal("expected error for zero-componentRef recipe, got nil") + } + var se *aicrerrors.StructuredError + if !stderrors.As(err, &se) { + t.Fatalf("expected *aicrerrors.StructuredError, got %T: %v", err, err) + } + if se.Code != aicrerrors.ErrCodeInvalidRequest { + t.Errorf("expected ErrCodeInvalidRequest, got %s", se.Code) + } +} + // TestRecipeResultFromInternal_PlumbsHelmFields locks in that the // translation from pkg/recipe.ComponentRef into the facade's // ComponentRef carries Source, Chart, and Namespace through. Without @@ -1368,3 +1422,31 @@ func TestFacadeResultFromInternal_ChartProjection(t *testing.T) { } } } + +// TestFacadeResultFromInternal_OmitsDisabledComponents pins the facade +// contract: only deployable (enabled) components appear in the SDK +// facade result. See #1874. +func TestFacadeResultFromInternal_OmitsDisabledComponents(t *testing.T) { + t.Parallel() + + internal := &recipe.RecipeResult{ + ComponentRefs: []recipe.ComponentRef{ + {Name: "enabled-a", Type: recipe.ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + { + Name: "disabled-b", + Type: recipe.ComponentTypeHelm, + Source: "https://charts.example.com", + Version: "1.0.0", + Overrides: map[string]any{"enabled": false}, + }, + }, + } + + out := facadeResultFromInternal(internal, "test") + if len(out.Components) != 1 { + t.Fatalf("components = %d, want 1", len(out.Components)) + } + if out.Components[0].Name != "enabled-a" { + t.Errorf("got component %q, want enabled-a", out.Components[0].Name) + } +} diff --git a/pkg/client/v1/types.go b/pkg/client/v1/types.go index 80581188d..9273b86b7 100644 --- a/pkg/client/v1/types.go +++ b/pkg/client/v1/types.go @@ -268,7 +268,9 @@ type RecipeResult struct { // internal RecipeResult currently carries no build timestamp. TranslatedAt time.Time - // Components lists the deployable components in the recipe. + // Components lists the deployable components in the recipe — enabled + // component refs only. Disabled refs are omitted; call Resolved() for + // the full underlying ComponentRefs (enabled and disabled). Components []ComponentRef // internal holds the upstream pkg/recipe.RecipeResult so diff --git a/pkg/recipe/componentref_coherence_test.go b/pkg/recipe/componentref_coherence_test.go index df377bdeb..cb5c34106 100644 --- a/pkg/recipe/componentref_coherence_test.go +++ b/pkg/recipe/componentref_coherence_test.go @@ -304,3 +304,103 @@ func TestPrepareAndValidate_RejectsReservedDeployerName(t *testing.T) { }) } } + +// TestPrepareAndValidate_RejectsDuplicateNames verifies the common +// validation boundary fails closed on a recipe with two component refs +// sharing the same non-empty Name. Component names key value +// materialization, override routing, validation, filtering, and deployment +// ordering throughout the bundler/validations/deployer paths, all of which +// assume a unique name -> ref mapping. Disabled refs participate too, so a +// disabled duplicate can't silently mask an enabled ref of the same name +// (or vice versa). See #1874. +func TestPrepareAndValidate_RejectsDuplicateNames(t *testing.T) { + tests := []struct { + name string + refs []ComponentRef + wantErr bool + }{ + { + name: "adjacent duplicate", + refs: []ComponentRef{ + {Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "gpu-operator", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + }, + wantErr: true, + }, + { + name: "non-adjacent duplicate", + refs: []ComponentRef{ + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + }, + wantErr: true, + }, + { + name: "disabled duplicate still rejected", + refs: []ComponentRef{ + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "a", Overrides: map[string]any{"enabled": false}}, + }, + wantErr: true, + }, + { + // Both refs are enabled and nameless — the actual shape that + // reaches the deployers if the name=="" exemption were ever + // removed. Disabled nameless refs would pass trivially for an + // unrelated reason (backfillComponentTypes/ValidateCoherence + // skip disabled refs entirely), so this must use enabled ones + // to pin the exemption it's named for. + name: "empty names not flagged", + refs: []ComponentRef{ + {Name: "", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + }, + wantErr: false, + }, + { + name: "unique names ok", + refs: []ComponentRef{ + {Name: "a", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + {Name: "b", Type: ComponentTypeHelm, Source: "https://charts.example.com", Version: "1.0.0"}, + }, + wantErr: false, + }, + { + // Reserved-key rejection runs before the duplicate check, so + // two refs both named ReservedDeployerKey must surface the + // "reserved" message, never the "duplicate" one. Deliberate + // precedence; pinned here so an inversion is caught. + name: "reserved-key precedence over duplicate check", + refs: []ComponentRef{ + {Name: ReservedDeployerKey, Overrides: map[string]any{"enabled": false}}, + {Name: ReservedDeployerKey, Overrides: map[string]any{"enabled": false}}, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + r := &RecipeResult{ComponentRefs: tt.refs} + err := r.PrepareAndValidate() + if (err != nil) != tt.wantErr { + t.Fatalf("PrepareAndValidate() error = %v, wantErr %v", err, tt.wantErr) + } + if !tt.wantErr { + return + } + if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("expected ErrCodeInvalidRequest, got: %v", err) + } + if tt.name == "reserved-key precedence over duplicate check" { + if !strings.Contains(err.Error(), "reserved") { + t.Errorf("error should mention the reservation, got: %v", err) + } + return + } + if !strings.Contains(err.Error(), "duplicate") { + t.Errorf("error should mention the duplicate, got: %v", err) + } + }) + } +} diff --git a/pkg/recipe/loader_provider_test.go b/pkg/recipe/loader_provider_test.go index 59eea96a1..b0a2bd4da 100644 --- a/pkg/recipe/loader_provider_test.go +++ b/pkg/recipe/loader_provider_test.go @@ -15,9 +15,13 @@ package recipe import ( + stderrors "errors" "os" "path/filepath" + "strings" "testing" + + "github.com/NVIDIA/aicr/pkg/errors" ) // newTestLayeredProvider builds a LayeredDataProvider over a temp dir holding a @@ -128,4 +132,40 @@ func TestLoadFromFileWithProvider(t *testing.T) { t.Errorf("DataProvider() = %v, want nil for nil-dp path", rec.DataProvider()) } }) + + t.Run("hand-authored duplicate componentRef names are rejected", func(t *testing.T) { + // Exercises the most-used boundary — a hand-authored, already- + // hydrated RecipeResult file loaded via LoadFromFileWithProvider, + // which is the shape `aicr bundle -r`/`aicr validate -r` and + // POST /v1/bundle load. See #1874. + layered := newTestLayeredProvider(t) + dir := t.TempDir() + path := filepath.Join(dir, "recipe.yaml") + content := "kind: RecipeResult\n" + + "apiVersion: aicr.run/v1alpha2\n" + + "criteria:\n service: eks\n" + + "componentRefs:\n" + + " - name: gpu-operator\n" + + " type: Helm\n" + + " source: https://charts.example.com\n" + + " version: \"1.0.0\"\n" + + " - name: gpu-operator\n" + + " type: Helm\n" + + " source: https://charts.example.com\n" + + " version: \"1.0.0\"\n" + if err := os.WriteFile(path, []byte(content), 0o600); err != nil { + t.Fatalf("write recipe: %v", err) + } + + _, err := LoadFromFileWithProvider(t.Context(), path, "", "vtest", layered) + if err == nil { + t.Fatal("expected an error for duplicate componentRef names, got nil") + } + if !stderrors.Is(err, errors.New(errors.ErrCodeInvalidRequest, "")) { + t.Errorf("expected ErrCodeInvalidRequest, got: %v", err) + } + if !strings.Contains(err.Error(), "duplicate") { + t.Errorf("error should mention the duplicate, got: %v", err) + } + }) } diff --git a/pkg/recipe/metadata.go b/pkg/recipe/metadata.go index 6c136f5b6..068acb431 100644 --- a/pkg/recipe/metadata.go +++ b/pkg/recipe/metadata.go @@ -534,13 +534,13 @@ func (r *RecipeResult) backfillComponentTypes() error { // PrepareAndValidate normalizes a RecipeResult's component refs and rejects // incoherent ones, in the required order: reject refs named with the reserved -// deployer override key (all refs, enabled or disabled), back-fill missing +// deployer override key, reject duplicate non-empty ref names, back-fill missing // types on enabled refs from the registry, canonicalize the type casing, then // validate // coherence (which itself only inspects enabled refs). Boundaries // that produce a RecipeResult WITHOUT running ApplyRegistryDefaults — file load // (LoadFromFileWithProvider) and external adoption (client adoptRecipe) — call -// this single method so the three steps cannot drift or be partially applied +// this single method so the four steps cannot drift or be partially applied // (e.g. validating before canonicalizing would reject legitimate lowercase // types; skipping the back-fill would reject type-less registry refs). The // resolve path (finalizeRecipeResult) instead back-fills via ApplyRegistryDefaults @@ -563,6 +563,11 @@ func (r *RecipeResult) PrepareAndValidate() error { fmt.Sprintf("recipe component %q uses the reserved deployer override key as its name; %q is reserved for --set deployer: Argo deployer options", r.ComponentRefs[i].Name, ReservedDeployerKey)) } } + // Reject duplicate component-ref names (enabled or disabled) BEFORE + // registry back-fill. See validateRefNames for the rationale. + if err := validateRefNames(r.ComponentRefs); err != nil { + return err + } if err := r.backfillComponentTypes(); err != nil { return err } @@ -570,6 +575,35 @@ func (r *RecipeResult) PrepareAndValidate() error { return r.ValidateCoherence() } +// validateRefNames rejects duplicate non-empty component-ref names (enabled +// or disabled). Component names key value materialization, override +// routing, validation, filtering, and deployment ordering throughout the +// bundler/validations/deployer paths, all of which assume a unique +// name -> ref mapping. AICR-generated recipes never produce duplicates, but +// a hand-authored or externally-adopted recipe can, and a disabled +// duplicate must not be allowed to silently mask an enabled ref of the same +// name (or vice versa) — so ALL refs participate in the check, not just +// enabled ones. Empty names are exempt: they are never used as a lookup +// key. Shared between PrepareAndValidate (the file-load, adopt, and +// bundle/validate -r / POST /v1/bundle boundary) and finalizeRecipeResult +// (the criteria-resolve boundary), so both fail closed on the same +// recipes. See #1874. +func validateRefNames(refs []ComponentRef) error { + seen := make(map[string]int, len(refs)) + for i := range refs { + name := refs[i].Name + if name == "" { + continue + } + if first, ok := seen[name]; ok { + return errors.New(errors.ErrCodeInvalidRequest, + fmt.Sprintf("recipe has duplicate component ref name %q (refs %d and %d)", name, first, i)) + } + seen[name] = i + } + return nil +} + // ValidateCoherence rejects enabled ComponentRefs whose deployment-shape fields // are internally inconsistent (see coherenceProblem), aggregating every // offender into one ErrCodeInvalidRequest so the author sees all problems at diff --git a/pkg/recipe/metadata_store.go b/pkg/recipe/metadata_store.go index 303d4b361..43f9dfc69 100644 --- a/pkg/recipe/metadata_store.go +++ b/pkg/recipe/metadata_store.go @@ -968,6 +968,18 @@ func finalizeRecipeResult(provider DataProvider, criteria *Criteria, mergedSpec } result.Metadata.AppliedOverlays = appliedOverlays + // Reject duplicate component-ref names before they reach ValidateCoherence + // or any caller of BuildRecipeResult/ResolveRecipe. RecipeMetadataSpec.Merge + // silently last-wins-collapses same-name refs via componentMap, so this + // only fires when an external --data base carries duplicates AND no + // matching overlay merge collapses them. Mirrors the same check + // PrepareAndValidate runs at the file-load/adopt/bundle boundary, so the + // criteria-resolve boundary (aicr recipe -o, aicr query, + // Client.ResolveRecipe) fails closed on the same recipes too. See #1874. + if err := validateRefNames(mergedSpec.ComponentRefs); err != nil { + return nil, err + } + // Reject refs whose deployment-shape fields are incoherent (e.g. a Helm ref // that also carries a Kustomize tag/path), after defaults populate Type. // The same check also runs at the load and adopt boundaries (see