diff --git a/api/aicr/v1/server.yaml b/api/aicr/v1/server.yaml index cdc691070..db77a0c3f 100644 --- a/api/aicr/v1/server.yaml +++ b/api/aicr/v1/server.yaml @@ -986,6 +986,8 @@ paths: version: v25.3.3 source: https://helm.ngc.nvidia.com/nvidia chart: gpu-operator + deploymentOrder: + - gpu-operator responses: "200": description: Zip archive containing generated bundles @@ -1495,7 +1497,26 @@ components: path: type: string description: Path within the source to the kustomization (Kustomize) - description: List of component references with deployment order + patches: + type: array + items: + type: string + description: >- + Unsupported / not applied. No deployer applies patch files, so + an enabled ref that sets `patches` is rejected with 400 rather + than silently producing an unpatched bundle. Do not set. See + issue #1588. + description: >- + List of component references. Deployment order is not per-ref; it is + the top-level `deploymentOrder` array. + deploymentOrder: + type: array + items: + type: string + description: >- + Topologically sorted component names giving the order to deploy in. + Preserve this on a GET→POST round trip (POST /v1/bundle accepts this + same shape) — dropping it changes deployment sequencing. constraints: type: object description: Deployment constraints (driver versions, etc.) diff --git a/docs/integrator/recipe-development.md b/docs/integrator/recipe-development.md index e814140e8..85fd7ac08 100644 --- a/docs/integrator/recipe-development.md +++ b/docs/integrator/recipe-development.md @@ -222,6 +222,11 @@ componentRefs: A component must have either `helm` OR `kustomize` configuration, not both. +> **`patches` is not supported.** The `componentRefs[].patches` field is not +> applied by any deployer. An enabled ref that sets `patches` is rejected at +> recipe resolution (rather than silently producing an unpatched bundle), so do +> not use it. See [#1588](https://github.com/NVIDIA/aicr/issues/1588). + ## Component Configuration ### Configuration Patterns diff --git a/docs/user/cli-reference.md b/docs/user/cli-reference.md index 20c10b5b3..419933229 100644 --- a/docs/user/cli-reference.md +++ b/docs/user/cli-reference.md @@ -495,6 +495,7 @@ componentRefs: type: Helm version: vXX.Y.Z # illustrative; see Component Catalog for current pins source: https://helm.ngc.nvidia.com/nvidia + chart: gpu-operator deploymentOrder: - gpu-operator constraints: diff --git a/pkg/client/v1/aicr_internal_test.go b/pkg/client/v1/aicr_internal_test.go index d39418628..693d5f279 100644 --- a/pkg/client/v1/aicr_internal_test.go +++ b/pkg/client/v1/aicr_internal_test.go @@ -1128,7 +1128,13 @@ func (b *blockingReadFileProvider) ReadFile(ctx context.Context, path string) ([ default: close(b.readStarted) } - <-b.readUnblock + // Honor the DataProvider contract: return the context error if canceled + // rather than parking forever. + select { + case <-b.readUnblock: + case <-ctx.Done(): + return nil, ctx.Err() + } } return b.underlying.ReadFile(ctx, path) } diff --git a/pkg/recipe/componentref_coherence_test.go b/pkg/recipe/componentref_coherence_test.go index cc67f28d5..dd7eddd7a 100644 --- a/pkg/recipe/componentref_coherence_test.go +++ b/pkg/recipe/componentref_coherence_test.go @@ -82,7 +82,8 @@ func TestComponentRefCoherenceProblem(t *testing.T) { wantBad: true, }, { - // The REST wire format / OpenAPI example uses lowercase; it must be + // Lowercase is accepted as backward-compatible input (hand-authored + // recipes / older clients); the canonical form is Helm. It must be // accepted case-insensitively, not rejected as an unsupported type. name: "lowercase helm is accepted", ref: ComponentRef{Name: "a", Type: ComponentType("helm"), Version: "v1"}, @@ -98,6 +99,18 @@ func TestComponentRefCoherenceProblem(t *testing.T) { ref: ComponentRef{Name: "a", Type: ComponentType("helm"), Version: "v1", Tag: "v2"}, wantBad: true, }, + { + // Patches are carried through resolution but applied by no deployer; + // a ref that declares them is rejected rather than silently dropped. + name: "helm with patches rejected", + ref: ComponentRef{Name: "a", Type: ComponentTypeHelm, Version: "v1", Patches: []string{"p.yaml"}}, + wantBad: true, + }, + { + name: "kustomize with patches rejected", + ref: ComponentRef{Name: "a", Type: ComponentTypeKustomize, Path: "deploy", Patches: []string{"p.yaml"}}, + wantBad: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/pkg/recipe/metadata.go b/pkg/recipe/metadata.go index e01ba17af..2fde4dd95 100644 --- a/pkg/recipe/metadata.go +++ b/pkg/recipe/metadata.go @@ -95,7 +95,11 @@ type ComponentRef struct { // Merge order: base values → ValuesFile → Overrides (highest precedence). Overrides map[string]any `json:"overrides,omitempty" yaml:"overrides,omitempty"` - // Patches is a list of patch files to apply (for Kustomize). + // Patches is a list of patch files (intended for Kustomize). NOT CURRENTLY + // APPLIED by any deployer — an enabled ref that declares patches is rejected + // by ValidateCoherence (disabled refs are skipped) rather than silently + // producing an unpatched bundle. See #1588 (implement application or drop + // the field). Patches []string `json:"patches,omitempty" yaml:"patches,omitempty"` // DependencyRefs is a list of component names this component depends on. @@ -223,27 +227,46 @@ func (ref *ComponentRef) ApplyRegistryDefaults(config *ComponentConfig) { // resolution rather than silently deploying as a different type (or producing // a signed attestation whose metadata does not match what deploys): // -// - the deployers do not trust the declared Type — Helm/Helmfile/ArgoCD drop -// it, and localformat classifies any ref carrying a Tag or Path as -// Kustomize (see pkg/bundler/deployer/localformat classify/write) — so a -// Helm ref must not carry Kustomize fields; -// - a Kustomize ref needs a Path to build from; and -// - a Tag is only meaningful with a Source (git repo / OCI ref). +// - the field-classifying deployers (localformat, and the Helm/Helmfile/ArgoCD +// generators built on it) do not trust the declared Type — localformat +// classifies any ref carrying a Tag or Path as Kustomize (see +// pkg/bundler/deployer/localformat classify/write). The Flux generator +// differs: it switches on the declared Type. So a Helm ref carrying a +// tag/path builds as Kustomize under the field-classifiers but as Helm +// under Flux — the same ref deploys differently by deployer — which is why +// a Helm ref must not carry Kustomize fields. (An explicitly Kustomize ref, +// conversely, is rejected outright by the Helm-only Flux generator — see +// #1588.); +// - a Kustomize ref needs a Path to build from; +// - a Tag is only meaningful with a Source (git repo / OCI ref); and +// - no deployer applies ComponentRef.Patches, so a ref that declares patch +// files is rejected rather than silently producing an unpatched bundle +// (see #1588). // // Keep these in lockstep with the localformat rules. func (ref *ComponentRef) coherenceProblem() string { + // Patches are unsupported for every deployment type: the field is carried + // through resolution but no deployer applies it (localformat's Component has + // no patches field). Fail closed on any type rather than drop it silently. + if len(ref.Patches) > 0 { + return fmt.Sprintf("component %q declares patches, but no deployer applies patch files; "+ + "remove `patches` (removing it does not change the generated bundle). See #1588.", ref.Name) + } + hasTag, hasPath := ref.Tag != "", ref.Path != "" - // Match the type case-insensitively: the resolver emits the canonical - // ComponentType ("Helm"/"Kustomize"), but the REST wire format and - // hand-authored recipes may use lowercase (see the /v1/bundle OpenAPI - // example), and the deployers classify by tag/path rather than by this - // field — so "helm" and "Helm" must be treated the same here. + // Match the type case-insensitively: the resolver and the OpenAPI examples + // use the canonical ComponentType ("Helm"/"Kustomize"), but lowercase is + // accepted as backward-compatible input from hand-authored recipes or older + // clients, and the field-classifying deployers key off tag/path (while Flux + // switches on Type) — so "helm" and "Helm" must be treated the same. switch { case strings.EqualFold(string(ref.Type), string(ComponentTypeHelm)): if hasTag || hasPath { return fmt.Sprintf("component %q is Helm but carries Kustomize field(s) (tag=%q, path=%q); "+ - "the deployers classify any ref with a tag or path as Kustomize, so it would deploy as a "+ - "different type than declared", ref.Name, ref.Tag, ref.Path) + "the field-classifying deployers would treat it as Kustomize while the Type-switching Flux "+ + "deployer would treat it as Helm, so the same ref deploys differently — remove the tag/path, "+ + "or convert it into a coherent Kustomize ref (which needs a path, and a source if it sets a tag)", + ref.Name, ref.Tag, ref.Path) } case strings.EqualFold(string(ref.Type), string(ComponentTypeKustomize)): if !hasPath { @@ -263,9 +286,10 @@ func (ref *ComponentRef) coherenceProblem() string { default: // After ApplyRegistryDefaults every registry-backed ref has a supported // Type; an empty or unknown Type here means an externally-supplied ref - // the registry did not populate. The deployers classify by tag/path, not - // by this field, so an unsupported Type would deploy ambiguously — fail - // closed rather than silently accept it. + // the registry did not populate. The field-classifying deployers key off + // tag/path (ignoring this field) while Flux switches on it, so an + // unsupported Type would deploy ambiguously or be rejected — fail closed + // rather than silently accept it. return fmt.Sprintf("component %q has unsupported type %q; expected %q or %q", ref.Name, ref.Type, ComponentTypeHelm, ComponentTypeKustomize) }