feat(recipe): implement the ADR-015 profile core - #1933
Conversation
yuanchen8911
changed the title
Recipe evidence checkNo leaf overlays affected by this PR. This gate is warning-only and never blocks merge. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds configuration profiles with strict validation, profile-aware resolution, selected-profile metadata, ownership locking, and catalog projection. Introduces strict v2 recipe, query, and bundle endpoints while preserving legacy v1 behavior. Propagates profile selection through CLI, configuration, client APIs, bundling, Helm deployment, mirror discovery, serialization, evidence projection, and TestGrid publication. Updates API schemas and documentation accordingly. Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/server/serve_test.go (1)
80-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test asserts against its own literal, not against
Serve's route table.
routesis a copy of the map inserve.go, solen(routes) != 6and the membership loop can never detect a route thatServeforgot to register — the two can drift freely. Extracting the map into an unexportednewRoutes(h, bh)helper inserve.goand having bothServeand this test call it would make the v2 additions actually covered.♻️ Sketch
+// serve.go +func newRoutes(h *recipeHandler, bh *bundleHandler) map[string]http.HandlerFunc { + return map[string]http.HandlerFunc{ + "/v1/recipe": h.HandleRecipes, + "/v1/query": h.HandleQuery, + "/v1/bundle": bh.HandleBundles, + "/v2/recipe": h.HandleRecipesV2, + "/v2/query": h.HandleQueryV2, + "/v2/bundle": bh.HandleBundlesV2, + } +}- routes := map[string]http.HandlerFunc{ - "/v1/recipe": h.HandleRecipes, - ... - } + routes := newRoutes(h, bh)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/server/serve_test.go` around lines 80 - 103, Extract the route map construction from Serve into an unexported newRoutes(h, bh) helper in serve.go, returning the complete v1 and v2 route table. Update Serve to use this helper, and update the test to obtain routes through newRoutes instead of duplicating the literal map so membership and count assertions validate the production route table.docs/integrator/data-flow.md (1)
260-285: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow
metadata.selectedProfilein the structure diagram.The new text promises selected profile and owned paths, but the diagram’s metadata section does not show them. Add the v1alpha3-only field to avoid presenting an incomplete artifact shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/integrator/data-flow.md` around lines 260 - 285, Add metadata.selectedProfile to the RecipeResult structure diagram’s metadata section, marking it as v1alpha3-only and showing the selected profile value. Keep the existing metadata fields and diagram structure unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/aicr/v1/server.yaml`:
- Around line 2236-2263: Align the v2 request schema with the handler’s strict
decoding by documenting that nested criteria rejects unknown fields. Update the
`Criteria` schema or introduce a v2-specific criteria schema, or explicitly
state this constraint in the `RecipeV2Request` and `QueryV2Request`
descriptions; keep the existing envelope structure and strict behavior
unchanged.
- Around line 855-1275: Add a 413 Payload Too Large response to the POST
/v2/recipe and POST /v2/query OpenAPI definitions, matching the handlers’
MaxRecipePOSTBytes behavior. Document the response with the standard
X-Request-Id header and Error schema, alongside the existing 400/429/500
responses for createRecipeV2 and createQueryV2.
In `@docs/design/015-recipe-configuration-profiles.md`:
- Around line 311-316: Update the `#1761` reference in the fragment schema
section so it remains inline text or uses valid Markdown link syntax; do not
leave it at the start of a line where it is parsed as an ATX heading.
In `@docs/integrator/data-flow.md`:
- Around line 260-262: Update the Markdown fence around the RecipeResult diagram
in the data-flow documentation to use the text language label (`text`) instead
of an unlabeled fence, while preserving the diagram content unchanged.
In `@docs/integrator/recipe-development.md`:
- Around line 503-509: Revise the “Activation gate” section to remove the
requirement that the resolved overlay declare gpu-operator.driver.enabled=false
before driver auto-detection runs. Explain that profiles only prevent injection
from modifying an owned path, and document the existing static configuration
prerequisite separately while preserving the only-false behavior and EKS
warning.
In `@pkg/client/v1/aicr_internal_test.go`:
- Around line 1201-1228: Strengthen TestAdoptRecipe_BoundsProviderIO by ensuring
the deadlineRequiredProvider wrapper records ReadFile/WalkDir activity and
asserting the counter is non-zero after AdoptRecipe succeeds. Use a
profile-bearing recipe fixture with RecipeProfileAPIVersion and
Metadata.SelectedProfile if needed to force PrepareAndValidateWithContext into
ValidateProfileValuesWithContext and exercise the bounded provider I/O path.
In `@pkg/mirror/discover_test.go`:
- Around line 394-422: Extract the repeated profiled recipe setup into a
package-scoped newProfiledRecipe helper, preserving the gpu-operator component
and SelectedProfile values. Also consolidate the duplicated ComponentPath.Parse
logic into a mustParseOverride(t, raw) helper, then replace both fixture copies
and local parsing helpers with these shared functions.
- Around line 699-728: Add the complementary test
TestDiscover_RegistryFailureWithoutOverridesIsNotFatal near
TestDiscover_OverrideAliasRegistryFailureIsFatal, using the malformed registry
and component setup but no WithValueOverrides option. Assert Discover returns no
error, preserving the behavior that registry parsing is skipped when no
overrides are configured.
In `@pkg/recipe/catalog.go`:
- Around line 98-113: The per-entry logic in
MetadataStore.ListCatalogWithProfiles repeatedly scans overlays and resolves
inheritance, causing quadratic catalog listing. Memoize resolved profile
declarations using each entry’s criteria fingerprint, or pre-resolve once per
declaring overlay and reuse the results when assigning profileSummary; preserve
the existing error propagation and output behavior.
In `@pkg/recipe/loader.go`:
- Around line 90-114: Update LoadFromFileWithProvider to read the profile
artifact once and retain its raw bytes, then pass those bytes into
recipe.DecodeRecipeResult so strict profile validation and inputAPIVersion
detection use the same document. Remove the second
serializer.FromFileWithKubeconfigContext decode for RecipeProfileAPIVersion
while preserving provider binding through dp and the existing kind validation.
In `@pkg/recipe/profile_resolution.go`:
- Around line 223-225: Replace the unstable sort.Slice call ordering
mergedSpec.Constraints with a stable sort while preserving the existing
Name-based comparison. Update the sorting logic in the profile resolution flow
so constraints with identical names retain their original order and produce
reproducible artifacts.
- Around line 227-236: Update the component lookup in the profile-resolution
loop around componentIndex and mergedSpec.ComponentRefs to check whether
profileRef.Name exists before using its index. If the component is unknown, fail
closed using the surrounding function’s established error-handling behavior;
only initialize, copy, and merge Overrides after a valid index is confirmed.
In `@pkg/recipe/profile_test.go`:
- Around line 231-252: Update the table-driven tests around
ValidateProfileDeclaration to add a wantOwned map[string][]string field to each
case and compare owned against it without branching on tt.name. Add or adjust an
invalid case so Default is a valid identifier while a per-value name is invalid,
ensuring profile.go’s per-value validation path is exercised.
In `@pkg/recipe/profile.go`:
- Around line 402-410: Move the r == nil guard in PrepareAndValidateWithContext
before calling r.PrepareAndValidate(), returning nil immediately for a nil
receiver. Preserve the existing validation flow for non-nil results, including
the Metadata.SelectedProfile check and ValidateProfileValuesWithContext call.
In `@pkg/serializer/reader_test.go`:
- Around line 1470-1475: Add a table-driven test case alongside the existing
“legacy non-strict JSON remains additive” case in the reader tests, using
non-strict JSON input containing a valid JSON value followed by another
document. Assert that this trailing-document input remains accepted under legacy
non-strict behavior, while keeping the existing strict-mode rejection coverage
unchanged.
In `@pkg/server/bundle_handler.go`:
- Around line 109-125: The decode paths return bare errors and lose structured
error codes. In pkg/server/bundle_handler.go lines 109-125, update the JSON
decoder, io.ReadAll, and recipe.DecodeRecipeResult failures to use
aicrerrors.PropagateOrWrap with the appropriate error code and context; in
pkg/recipe/decode.go lines 35-60, wrap serializer.NewReader failures with
errors.PropagateOrWrap. Preserve existing successful decode behavior and
structured codes.
In `@pkg/server/recipe_handler.go`:
- Around line 128-176: Remove the unreachable second http.MaxBytesError handling
branch after body decoding; io.ReadAll in the recipe POST read path already
handles and returns for that error. Keep the existing read-error handling and
decode/validation flow unchanged for all other errors.
- Around line 629-655: The body-format resolution logic in
legacyRecipeBodyFormat and legacyQueryBodyFormat should be moved into shared
helpers under pkg/recipe, alongside the corresponding parsers. Preserve each
parser’s exact Content-Type rules, including YAML recognition and JSON defaults
for criteria versus JSON detection and YAML defaults for query bodies, then
update the v1 profile guard to call those shared helpers instead of local
implementations.
---
Outside diff comments:
In `@docs/integrator/data-flow.md`:
- Around line 260-285: Add metadata.selectedProfile to the RecipeResult
structure diagram’s metadata section, marking it as v1alpha3-only and showing
the selected profile value. Keep the existing metadata fields and diagram
structure unchanged.
In `@pkg/server/serve_test.go`:
- Around line 80-103: Extract the route map construction from Serve into an
unexported newRoutes(h, bh) helper in serve.go, returning the complete v1 and v2
route table. Update Serve to use this helper, and update the test to obtain
routes through newRoutes instead of duplicating the literal map so membership
and count assertions validate the production route table.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 49c7e94d-51a1-40a2-91a9-bc757c76a29a
📒 Files selected for processing (65)
api/aicr/v1/server.yamldocs/contributor/api-server.mddocs/contributor/recipe.mddocs/design/015-recipe-configuration-profiles.mddocs/integrator/data-flow.mddocs/integrator/go-library.mddocs/integrator/recipe-development.mddocs/user/api-reference.mddocs/user/cli-config.mddocs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/bundler_test.gopkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/validations/checks.gopkg/bundler/validations/checks_test.gopkg/cli/consts.gopkg/cli/mirror.gopkg/cli/query.gopkg/cli/query_test.gopkg/cli/recipe.gopkg/cli/recipe_test.gopkg/client/v1/aicr.gopkg/client/v1/aicr_internal_test.gopkg/client/v1/aicr_test.gopkg/client/v1/bundle.gopkg/client/v1/gpu_driver_state.gopkg/client/v1/stability_test.gopkg/client/v1/types.gopkg/component/overrides.gopkg/component/overrides_test.gopkg/config/accessors.gopkg/config/accessors_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/validate.gopkg/evidence/project/synthesize.gopkg/evidence/project/synthesize_test.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/recipe/builder.gopkg/recipe/catalog.gopkg/recipe/decode.gopkg/recipe/loader.gopkg/recipe/loader_test.gopkg/recipe/metadata.gopkg/recipe/metadata_store.gopkg/recipe/metadata_store_test.gopkg/recipe/profile.gopkg/recipe/profile_resolution.gopkg/recipe/profile_test.gopkg/recipe/query.gopkg/serializer/reader.gopkg/serializer/reader_test.gopkg/server/bundle_handler.gopkg/server/bundle_handler_test.gopkg/server/consts.gopkg/server/doc.gopkg/server/recipe_handler.gopkg/server/recipe_handler_test.gopkg/server/serve.gopkg/server/serve_test.gopkg/server/server.gotools/testgrid-publish/bundle.gotools/testgrid-publish/bundle_test.go
09e5cec to
dd0b710
Compare
|
Addressed the two outside-diff observations in dd0b710:
|
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
pkg/serializer/reader_test.go (1)
1463-1521: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStill missing the legacy non-strict YAML trailing-document case.
A past review flagged this same gap: the legacy/non-strict rows only cover JSON (unknown field + trailing document); non-strict YAML is untested for trailing-document tolerance. A regression that applies YAML trailing-document rejection unconditionally would pass this table undetected.
💚 Proposed additional case
{ name: "legacy non-strict JSON accepts trailing document", format: FormatJSON, input: `{"name":"test","value":1} {"name":"other","value":2}`, }, + { + name: "legacy non-strict YAML tolerates trailing document", + format: FormatYAML, + input: "name: test\nvalue: 1\n---\nname: other\nvalue: 2\n", + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/serializer/reader_test.go` around lines 1463 - 1521, Add a non-strict YAML test case to TestReader_Strict covering multiple YAML documents separated by a document marker, with strict set to false and no expected error. Keep the existing legacy non-strict JSON cases unchanged and verify the test preserves trailing-document tolerance for YAML.pkg/recipe/catalog.go (1)
94-120: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoization keyed on leaf
Criterialikely doesn't fix the quadratic re-resolution.
resolvedByCriteriacaches per-entryCriteria, butListCatalogreturns one entry per overlay (leaves and ancestors) and each overlay'sSpec.Criteriais typically distinct from its siblings to disambiguate it — so cache hits are rare in practice, andFindMatchingOverlays+resolveProfileDeclaration(itself walkingresolveInheritanceChainper matched overlay) still run close to once per catalog entry. This is the same cost profile flagged in a prior review of this function; consider memoizing per declaring overlay/ancestor chain instead (e.g., resolve declarations once per distinct base+ancestor set, then project onto entries) to actually amortize the cost acrossaicr recipe listcalls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/recipe/catalog.go` around lines 94 - 120, Replace the per-entry resolvedByCriteria memoization in ListCatalogWithProfiles with caching keyed by each distinct declaring overlay or base/ancestor chain, so resolveProfileDeclaration and resolveInheritanceChain are performed once per shared declaration set rather than once per leaf Criteria. Reuse the cached effective profile when projecting profileSummary onto catalog entries, while preserving existing filtering and resolution error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/aicr/v1/server.yaml`:
- Around line 2383-2386: Update the advertiser field description in the API
schema to state that it must be absent for now because ValidateProfileContract
rejects any non-empty value, including external; retain the enum and clarify
that support is reserved for the future GKE allocation-policy extension.
- Around line 1262-1293: Update the POST /v2/bundle response definitions to
include a 413 Payload Too Large response, matching the existing Error schema and
response-header conventions used by the route and the corresponding v2
recipe/query POST endpoints.
In `@pkg/mirror/discover.go`:
- Around line 389-399: Add a context-cancellation check at the start of the
ComponentRefs loop in GetComponentRegistryFor, before per-component override
lookup and hydration work. Return the context error immediately when canceled,
while preserving the existing effectiveOverridesForComponent behavior and error
propagation.
In `@pkg/recipe/criteria_test.go`:
- Around line 1124-1159: Extend TestParseCriteriaFromBody with a table-driven
case using malformed JSON and an unrecognized content type, then assert the
returned error matches the unsupported-content-type error contract from the
branch in ParseCriteriaFromBody. Preserve the existing cases and verify both the
error outcome and any expected result values for this new input.
In `@pkg/recipe/profile.go`:
- Around line 165-217: Update the union-totality tracking in the profile value
loop so every referenced component contributes its synthetic enabled path to
valuePaths, including components with no overrides. Use the same component/path
representation as ownedSet, sort it with the existing paths, and keep the
comparison against expected so values with different component sets are
rejected.
In `@pkg/recipe/query_request.go`:
- Around line 35-42: Update QueryRequestBodyFormat to normalize contentType case
before checking for "json", so uppercase and mixed-case JSON media types return
serializer.FormatJSON while existing YAML fallback behavior remains unchanged.
Update the corresponding test expectation to cover the case-insensitive JSON
behavior.
In `@pkg/serializer/reader.go`:
- Around line 484-519: Update ReadFileBytesWithKubeconfigContext to return the
bytes already buffered by NewFileReaderWithContext via reader.input instead of
wrapping it with io.LimitReader and io.ReadAll. Remove the redundant length
validation while preserving the existing serializer creation, close handling,
format, and error behavior.
In `@pkg/server/bundle_handler_test.go`:
- Around line 161-201: Add two HTTP-level subtests alongside the existing v2
bundle cases using the shared post helper: submit profileBundleBody(t) with
set=gpu-operator:driver.enabled=true and dynamic=gpu-operator:driver.enabled
query overrides, and assert each response returns http.StatusBadRequest. These
tests should cover the profile lock enforced by ValidateProfileLock.
In `@pkg/server/recipe_handler.go`:
- Around line 227-231: Update the documented 400 responses for v1 recipe and
query endpoints in server.yaml to include the profile-based rejection from the
v1 handler, clearly indicating that profiled recipes require /v2/recipe. Keep
the existing malformed-criteria, allowlist, and uncovered-dimension causes
unchanged.
- Around line 557-565: Update decodeStrictV2Envelope and bodyHasTopLevelProfile
so serializer construction and deserialization failures are wrapped with
pkg/errors and contextual messages, ensuring client-facing errors retain the
INVALID_REQUEST code prefix. Propagate errors that already carry the correct
pkg/errors code without double-wrapping them, including failures returned by
Deserialize.
---
Duplicate comments:
In `@pkg/recipe/catalog.go`:
- Around line 94-120: Replace the per-entry resolvedByCriteria memoization in
ListCatalogWithProfiles with caching keyed by each distinct declaring overlay or
base/ancestor chain, so resolveProfileDeclaration and resolveInheritanceChain
are performed once per shared declaration set rather than once per leaf
Criteria. Reuse the cached effective profile when projecting profileSummary onto
catalog entries, while preserving existing filtering and resolution error
behavior.
In `@pkg/serializer/reader_test.go`:
- Around line 1463-1521: Add a non-strict YAML test case to TestReader_Strict
covering multiple YAML documents separated by a document marker, with strict set
to false and no expected error. Keep the existing legacy non-strict JSON cases
unchanged and verify the test preserves trailing-document tolerance for YAML.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 22042aa9-be7e-44f0-9dc3-e8004e89ade4
📒 Files selected for processing (69)
api/aicr/v1/server.yamldocs/contributor/api-server.mddocs/contributor/recipe.mddocs/design/015-recipe-configuration-profiles.mddocs/integrator/data-flow.mddocs/integrator/go-library.mddocs/integrator/recipe-development.mddocs/user/api-reference.mddocs/user/cli-config.mddocs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/bundler_test.gopkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/validations/checks.gopkg/bundler/validations/checks_test.gopkg/cli/consts.gopkg/cli/mirror.gopkg/cli/query.gopkg/cli/query_test.gopkg/cli/recipe.gopkg/cli/recipe_test.gopkg/client/v1/aicr.gopkg/client/v1/aicr_internal_test.gopkg/client/v1/aicr_test.gopkg/client/v1/bundle.gopkg/client/v1/gpu_driver_state.gopkg/client/v1/stability_test.gopkg/client/v1/types.gopkg/component/overrides.gopkg/component/overrides_test.gopkg/config/accessors.gopkg/config/accessors_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/validate.gopkg/evidence/project/synthesize.gopkg/evidence/project/synthesize_test.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/recipe/builder.gopkg/recipe/catalog.gopkg/recipe/criteria.gopkg/recipe/criteria_test.gopkg/recipe/decode.gopkg/recipe/loader.gopkg/recipe/loader_test.gopkg/recipe/metadata.gopkg/recipe/metadata_store.gopkg/recipe/metadata_store_test.gopkg/recipe/profile.gopkg/recipe/profile_resolution.gopkg/recipe/profile_test.gopkg/recipe/query.gopkg/recipe/query_request.gopkg/recipe/query_test.gopkg/serializer/reader.gopkg/serializer/reader_test.gopkg/server/bundle_handler.gopkg/server/bundle_handler_test.gopkg/server/consts.gopkg/server/doc.gopkg/server/recipe_handler.gopkg/server/recipe_handler_test.gopkg/server/serve.gopkg/server/serve_test.gopkg/server/server.gotools/testgrid-publish/bundle.gotools/testgrid-publish/bundle_test.go
dd0b710 to
01d8670
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
pkg/recipe/profile.go (1)
165-217: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUnion totality still excludes the synthetic
enabledpath, so values can reference different component sets.
valuePathscollects only override-derived paths (Line 196) whileownedSetgainsenabledfor every referenced component (Line 205). A value that lists acomponentRefwith no overrides and a sibling value that omits that component entirely both produce identicalvaluePathsand pass the totality check, yetOwnedPathslocks a component the sibling never declares.🐛 Include component presence in the totality comparison
if ownedSet[ref.Name] == nil { ownedSet[ref.Name] = make(map[string]struct{}) } ownedSet[ref.Name][profileComponentEnabledPath] = struct{}{} + valuePaths = append(valuePaths, ref.Name+":"+profileComponentEnabledPath) } sort.Strings(valuePaths)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/recipe/profile.go` around lines 165 - 217, Update the union-totality path collection in the loop over value.ComponentRefs so every referenced component contributes its synthetic profileComponentEnabledPath to valuePaths, including refs with no overrides. Keep the existing override-path and ownedSet handling, ensure the component identifier and enabled path use the same representation as expected, and compare values based on complete component presence.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/bundler/bundler.go`:
- Around line 309-312: Guard b.Config before invoking buildDynamicValuesMap in
the surrounding bundling flow, ensuring zero-value or struct-literal
DefaultBundler instances return through the existing validation path instead of
dereferencing a nil configuration. Preserve the current dynamicValues error
propagation for configured bundlers.
In `@pkg/recipe/loader.go`:
- Around line 144-151: Move the unsupported inputAPIVersion validation
immediately after inputAPIVersion is captured, before the overlay branch and any
RecipeMetadata.BuildFromCriteria hydration. Preserve the existing acceptance of
empty, supported, and RecipeProfileAPIVersion values, while returning the same
invalid-request error for unsupported versions.
In `@pkg/serializer/reader_test.go`:
- Around line 200-239: Refactor TestReadFileBytesWithKubeconfigContext into a
table-driven test containing rows for the local file, missing file, invalid
ConfigMap URI, and ConfigMap kubeconfig error cases. Store each row’s
setup/input and expected bytes/format or errors.ErrCode, then run them through
one subtest loop while preserving the existing assertions and per-case temporary
file or kubeconfig setup.
In `@pkg/serializer/reader.go`:
- Around line 123-127: Extract the duplicated option-application loop from
NewReader and NewFileReaderWithContext into a shared applyReaderOptions helper
accepting *Reader and variadic ReaderOption values; invoke this helper from both
constructors while preserving the existing nil-option check and application
order.
---
Duplicate comments:
In `@pkg/recipe/profile.go`:
- Around line 165-217: Update the union-totality path collection in the loop
over value.ComponentRefs so every referenced component contributes its synthetic
profileComponentEnabledPath to valuePaths, including refs with no overrides.
Keep the existing override-path and ownedSet handling, ensure the component
identifier and enabled path use the same representation as expected, and compare
values based on complete component presence.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 79c2e062-f83c-4b06-921e-b09defabef28
📒 Files selected for processing (69)
api/aicr/v1/server.yamldocs/contributor/api-server.mddocs/contributor/recipe.mddocs/design/015-recipe-configuration-profiles.mddocs/integrator/data-flow.mddocs/integrator/go-library.mddocs/integrator/recipe-development.mddocs/user/api-reference.mddocs/user/cli-config.mddocs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/bundler_test.gopkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/validations/checks.gopkg/bundler/validations/checks_test.gopkg/cli/consts.gopkg/cli/mirror.gopkg/cli/query.gopkg/cli/query_test.gopkg/cli/recipe.gopkg/cli/recipe_test.gopkg/client/v1/aicr.gopkg/client/v1/aicr_internal_test.gopkg/client/v1/aicr_test.gopkg/client/v1/bundle.gopkg/client/v1/gpu_driver_state.gopkg/client/v1/stability_test.gopkg/client/v1/types.gopkg/component/overrides.gopkg/component/overrides_test.gopkg/config/accessors.gopkg/config/accessors_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/validate.gopkg/evidence/project/synthesize.gopkg/evidence/project/synthesize_test.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/recipe/builder.gopkg/recipe/catalog.gopkg/recipe/criteria.gopkg/recipe/criteria_test.gopkg/recipe/decode.gopkg/recipe/loader.gopkg/recipe/loader_test.gopkg/recipe/metadata.gopkg/recipe/metadata_store.gopkg/recipe/metadata_store_test.gopkg/recipe/profile.gopkg/recipe/profile_resolution.gopkg/recipe/profile_test.gopkg/recipe/query.gopkg/recipe/query_request.gopkg/recipe/query_test.gopkg/serializer/reader.gopkg/serializer/reader_test.gopkg/server/bundle_handler.gopkg/server/bundle_handler_test.gopkg/server/consts.gopkg/server/doc.gopkg/server/recipe_handler.gopkg/server/recipe_handler_test.gopkg/server/serve.gopkg/server/serve_test.gopkg/server/server.gotools/testgrid-publish/bundle.gotools/testgrid-publish/bundle_test.go
01d8670 to
9bf3f52
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/recipe/metadata_store.go (1)
1029-1101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSound profile resolution logic; consider extracting the shared finalize+validate block.
BuildRecipeResultWithProfile(lines 1084-1100) andBuildRecipeResultWithEvaluatorAndProfile(lines 1251-1268) duplicate the same "apply profile → finalize → stampAPIVersion/Metadata.SelectedProfile→ValidateProfileValuesWithContext" sequence. Since this is the contract-enforcement path (profile artifact tagging + validation), duplicating it risks the two paths drifting apart on a future fix.♻️ Suggested consolidation
func (s *MetadataStore) finalizeWithProfile( ctx context.Context, criteria *Criteria, mergedSpec *RecipeMetadataSpec, appliedOverlays []string, effectiveProfile *ProfileDeclaration, selection string, evaluator ConstraintEvaluatorFunc, ) (*RecipeResult, error) { selected, err := applyEffectiveProfile(mergedSpec, effectiveProfile, selection, evaluator) if err != nil { return nil, err } result, err := finalizeRecipeResult(s.provider, criteria, mergedSpec, appliedOverlays) //nolint:contextcheck if err != nil { return nil, err } if selected == nil { return result, nil } result.APIVersion = RecipeProfileAPIVersion result.Metadata.SelectedProfile = selected if err := result.ValidateProfileValuesWithContext(ctx); err != nil { return nil, err } return result, nil }Both call sites would call this helper, with the evaluator path additionally setting
result.Metadata.ExcludedOverlays/ConstraintWarningson the returned result.Also applies to: 1113-1270
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/recipe/metadata_store.go` around lines 1029 - 1101, Extract the shared apply-profile, finalize, profile metadata tagging, and validation sequence from BuildRecipeResultWithProfile and BuildRecipeResultWithEvaluatorAndProfile into a MetadataStore helper such as finalizeWithProfile. Pass the merged spec, overlays, effective profile, selection, and evaluator through the helper; preserve the evaluator path’s additional ExcludedOverlays and ConstraintWarnings assignments after the helper returns.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/serializer/reader.go`:
- Around line 617-623: Update the ConfigMap retrieval error handling in the
reader flow around k8sClient.CoreV1().ConfigMaps(namespace).Get to classify
ErrCodeNotFound only when apierrors.IsNotFound(err) is true; preserve the
existing not-found message for that case and propagate other errors with their
appropriate status instead of wrapping them as not found.
- Around line 617-618: Rename the shared timeout symbol ConfigMapWriteTimeout to
the neutral ConfigMapTimeout, updating its definition and every read/write
reference consistently, including the context timeout setup in the reader flow.
In `@pkg/server/bundle_handler_test.go`:
- Around line 204-216: The “v2 rejects unknown query parameter” subtest
duplicates request setup instead of using the existing helper. Replace the
manual httptest request and recorder setup with post(t, true,
"?profie=gpuStack%3Ddriver-installed", profileBundleBody(t)), then retain the
existing status assertion against the helper’s response.
In `@pkg/server/bundle_handler.go`:
- Around line 323-342: The duplicated allowlist in
validateV2BundleQueryParameters must stay synchronized with ParseBundleConfig.
Derive validation from the parser’s key definitions where possible; otherwise
add a test that compares both allowlists and fails when they diverge, ensuring
newly supported query parameters are accepted by /v2/bundle.
---
Outside diff comments:
In `@pkg/recipe/metadata_store.go`:
- Around line 1029-1101: Extract the shared apply-profile, finalize, profile
metadata tagging, and validation sequence from BuildRecipeResultWithProfile and
BuildRecipeResultWithEvaluatorAndProfile into a MetadataStore helper such as
finalizeWithProfile. Pass the merged spec, overlays, effective profile,
selection, and evaluator through the helper; preserve the evaluator path’s
additional ExcludedOverlays and ConstraintWarnings assignments after the helper
returns.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f2eea5d4-58cd-4156-b798-7c9fd7597c01
📒 Files selected for processing (69)
api/aicr/v1/server.yamldocs/contributor/api-server.mddocs/contributor/recipe.mddocs/design/015-recipe-configuration-profiles.mddocs/integrator/data-flow.mddocs/integrator/go-library.mddocs/integrator/recipe-development.mddocs/user/api-reference.mddocs/user/cli-config.mddocs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/bundler_test.gopkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/validations/checks.gopkg/bundler/validations/checks_test.gopkg/cli/consts.gopkg/cli/mirror.gopkg/cli/query.gopkg/cli/query_test.gopkg/cli/recipe.gopkg/cli/recipe_test.gopkg/client/v1/aicr.gopkg/client/v1/aicr_internal_test.gopkg/client/v1/aicr_test.gopkg/client/v1/bundle.gopkg/client/v1/gpu_driver_state.gopkg/client/v1/stability_test.gopkg/client/v1/types.gopkg/component/overrides.gopkg/component/overrides_test.gopkg/config/accessors.gopkg/config/accessors_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/validate.gopkg/evidence/project/synthesize.gopkg/evidence/project/synthesize_test.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/recipe/builder.gopkg/recipe/catalog.gopkg/recipe/criteria.gopkg/recipe/criteria_test.gopkg/recipe/decode.gopkg/recipe/loader.gopkg/recipe/loader_test.gopkg/recipe/metadata.gopkg/recipe/metadata_store.gopkg/recipe/metadata_store_test.gopkg/recipe/profile.gopkg/recipe/profile_resolution.gopkg/recipe/profile_test.gopkg/recipe/query.gopkg/recipe/query_request.gopkg/recipe/query_test.gopkg/serializer/reader.gopkg/serializer/reader_test.gopkg/server/bundle_handler.gopkg/server/bundle_handler_test.gopkg/server/consts.gopkg/server/doc.gopkg/server/recipe_handler.gopkg/server/recipe_handler_test.gopkg/server/serve.gopkg/server/serve_test.gopkg/server/server.gotools/testgrid-publish/bundle.gotools/testgrid-publish/bundle_test.go
9bf3f52 to
1f416f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/mirror/discover_test.go`:
- Around line 466-493: Extend TestDiscover_SetEnabledFalseSkipsComponent into a
table-driven test covering both disabled and non-boolean enabled overrides.
Retain the existing false case assertions, and add an invalid value case that
verifies Discover returns ErrCodeInvalidRequest with the contextual error from
prepareMirrorCandidate, without rendering or retaining the component.
In `@pkg/recipe/metadata_store.go`:
- Around line 1084-1100: The profile-processing tail is duplicated across the
build paths and can drift over time. Extract the shared apply-profile, finalize,
metadata-stamping, and validation sequence into a helper such as
MetadataStore.finalizeWithProfile, parameterized by ctx, criteria, mergedSpec,
appliedOverlays, effectiveProfile, selection, and evaluator; preserve each
caller’s distinct trailing excluded-overlay assignment.
In `@pkg/recipe/profile_test.go`:
- Around line 909-971: Add table cases to the profile validation tests covering
both baseline-side rejection gates: a baseline owned path resolving to
PathBlocked, expecting the “recipe path … is blocked” error, and ownedPaths
naming a component absent from the recipe, expecting the “profile-owned
component %q is absent from the recipe” error. Reuse the existing fixtures and
test structure around the current cases to verify both failures remain
fail-closed.
In `@pkg/recipe/profile.go`:
- Around line 414-444: The profile validation loops over map-backed OwnedPaths
nondeterministically and performs hydration without cancellation checks. Update
ValidateProfileValuesWithContext and ValidateProfileLock to collect and sort
owned component keys before iterating, and check ctx.Done() at the start of each
iteration before hydration, returning the context error when cancelled; preserve
existing validation and error behavior otherwise.
- Around line 518-541: Update ValidateProfileLock and
ValidateProfileValuesWithContext to reuse the hydrated component-values map
produced during validation, passing it through instead of calling
GetValuesForComponentWithContext again for each profile-owned component.
Preserve existing validation and error behavior while ensuring each component’s
values are loaded only once per lock check.
In `@pkg/server/recipe_handler.go`:
- Around line 501-516: The duplicate-value validation currently used by
singleQueryValue must also cover every allowlisted query key in
validateV2QueryParameters. Update that v2 validation flow so conflicting
repeated values such as service=eks and service=gke return the existing
invalid-request error, while repeated identical values remain accepted and
unrelated keys retain their current handling.
- Around line 572-577: Update v2EnvelopeFormat to distinguish recognized JSON
and YAML media types instead of defaulting every non-JSON value to YAML; treat
an empty or unsupported Content-Type as invalid and propagate
ErrCodeInvalidRequest through the strict v2 request handling path. Preserve YAML
selection only for explicitly recognized YAML media types.
- Around line 98-101: Extract the shared v2 query-key allowlist used by both
handlers into a package-level set, preserving the existing common keys. Update
the `/v2/query` handler’s validation around validateV2QueryParameters to add
selector-specific support without mutating or duplicating the shared set, while
the other v2 handler continues using only the common allowlist.
In `@tools/testgrid-publish/bundle_test.go`:
- Around line 120-136: Update the “profile artifact rejected until publication
support lands” test case in the parseCriteria test table to assert the expected
rejection message using wantErrContains: “profile-bearing TestGrid publication
is deferred to the profile adoption rollout”, while retaining wantErr: true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: d37b7169-116b-4de5-9b71-381ce32330f4
📒 Files selected for processing (71)
api/aicr/v1/server.yamldocs/contributor/api-server.mddocs/contributor/recipe.mddocs/design/015-recipe-configuration-profiles.mddocs/integrator/data-flow.mddocs/integrator/go-library.mddocs/integrator/recipe-development.mddocs/user/api-reference.mddocs/user/cli-config.mddocs/user/cli-reference.mdpkg/bundler/bundler.gopkg/bundler/bundler_test.gopkg/bundler/deployer/argocdhelm/argocdhelm.gopkg/bundler/deployer/argocdhelm/argocdhelm_test.gopkg/bundler/handler.gopkg/bundler/handler_test.gopkg/bundler/validations/checks.gopkg/bundler/validations/checks_test.gopkg/cli/consts.gopkg/cli/mirror.gopkg/cli/query.gopkg/cli/query_test.gopkg/cli/recipe.gopkg/cli/recipe_test.gopkg/client/v1/aicr.gopkg/client/v1/aicr_internal_test.gopkg/client/v1/aicr_test.gopkg/client/v1/bundle.gopkg/client/v1/gpu_driver_state.gopkg/client/v1/stability_test.gopkg/client/v1/types.gopkg/component/overrides.gopkg/component/overrides_test.gopkg/config/accessors.gopkg/config/accessors_test.gopkg/config/config.gopkg/config/config_test.gopkg/config/validate.gopkg/evidence/project/synthesize.gopkg/evidence/project/synthesize_test.gopkg/mirror/discover.gopkg/mirror/discover_test.gopkg/recipe/builder.gopkg/recipe/catalog.gopkg/recipe/criteria.gopkg/recipe/criteria_test.gopkg/recipe/decode.gopkg/recipe/loader.gopkg/recipe/loader_test.gopkg/recipe/metadata.gopkg/recipe/metadata_store.gopkg/recipe/metadata_store_test.gopkg/recipe/profile.gopkg/recipe/profile_resolution.gopkg/recipe/profile_test.gopkg/recipe/query.gopkg/recipe/query_request.gopkg/recipe/query_test.gopkg/serializer/reader.gopkg/serializer/reader_test.gopkg/server/bundle_handler.gopkg/server/bundle_handler_test.gopkg/server/consts.gopkg/server/doc.gopkg/server/recipe_handler.gopkg/server/recipe_handler_test.gopkg/server/serve.gopkg/server/serve_test.gopkg/server/server.gotools/testgrid-publish/bundle.gotools/testgrid-publish/bundle_test.go
42309c6 to
12062c6
Compare
2ace4ec to
9e65073
Compare
|
@yuanchen8911 this PR now has merge conflicts with |
9e65073 to
ad2c4c5
Compare
yuanchen8911
changed the title
ee22ef8 to
58c5386
Compare
njhensley
left a comment
njhensley
left a comment
There was a problem hiding this comment.
📋 Multi-Persona Review — ADR-015 configuration-profile core
Method: four independent persona reviewers (Correctness/Domain · Security · API-contract · Operability/Go-idioms) reviewed in parallel, then every finding was re-derived from the resolved code by an adversarial senior meta-reviewer. Anchors pinned to head 58c53860.
Tier legend: 🔴 Blocker · 🟠 Major · 🟡 Minor · 🔵 Nitpick
Overall assessment
This is an unusually defensive, fail-closed implementation, and it is dormant — no recipe family adopts profiles yet. Across all four lenses, no blocker or major issue survived independent verification. Verified as correct: the cyclic-reference detector accurately mirrors what serializer.DeepCopyAny recurses into; the scalar allowlist covers the JSON 2⁵³ boundary and float truncation; ctx.Done() is checked in every owned-component loop; the Argo CD Helm render-time guard cannot be bypassed by false/null/empty containers; strict v2 media-type + decode paths hold; 5xx responses withhold the internal cause; and the OpenAPI↔code contract is tightly gated. Determinism-sensitive paths use MarshalYAMLDeterministic. The lazily-created static/ handling (checkStaticPath / removeStaleStaticDir) is clean — Lstat symlink rejection, correct 4xx-vs-5xx coding, fail-loud stale-dir removal.
Two minor gaps and three nitpicks are inline below. Both minors fail closed.
Recommendation: ✅ Approve with comments.
Confirmed non-issue (examined, refuted)
- Post-loop
ctx.Err()inListCatalogWithProfiles(pkg/recipe/catalog.go:138): not redundant — the in-loop check runs at the top of each iteration, so the post-loop check is the only guard covering cancellation during the final iteration's body. Legitimate fail-closed final guard.
Summary
| 🔴 Blocker | 🟠 Major | 🟡 Minor | 🔵 Nitpick |
|---|---|---|---|
| 0 | 0 | 2 | 3 |
F1 (union-totality) and F2 (v1/v2 schema-vs-server drift) are the two worth addressing before the AKS adoption PR builds on this.
| expected = valuePaths | ||
| continue | ||
| } | ||
| if !slices.Equal(valuePaths, expected) { |
There was a problem hiding this comment.
🟡 Minor — Union-totality check excludes the synthetic "enabled" path, so a presence-only componentRef escapes totality and over-locks siblings
The per-value totality check at profile.go:217 compares valuePaths, which is built only from flattened override paths (profile.go:201). A componentRef with empty/absent overrides (e.g. - name: Y with no overrides) is accepted (only an empty name is rejected) and contributes zero entries to valuePaths — yet ownedSet[Y]["enabled"] is stamped unconditionally for every ref (profile.go:210). Because OwnedPaths is declaration-wide, two profile values that reference different component sets via presence-only refs pass slices.Equal(valuePaths, expected), and Y's presence lock then leaks to every selection.
applyEffectiveProfile (profile_resolution.go) subsequently requires every owned component enabled, so selecting a value that never referenced Y is rejected with "component Y is missing or disabled" whenever Y is disabled in the surviving composition — surprising and hard to diagnose. It fails closed (never over-permits), so this is minor, not a blocker.
Suggested fix: fold the synthetic enabled marker into the per-value totality surface (append ref.Name+":enabled" to valuePaths), or validate that the set of referenced component names is identical across all values, so presence-only refs are subject to the same value=value equality check as override-bearing refs.
There was a problem hiding this comment.
Fixed in e0385f0. Confirmed the mechanism first: valuePaths is appended to only inside the for _, path := range paths loop, while the synthetic enabled marker is stamped into ownedSet unconditionally per ref. A presence-only ref therefore contributes nothing to the totality comparison while still landing in the declaration-wide OwnedPaths.
Took the first of your two suggestions — the marker now joins the totality surface (ref.Name + ":" + profileComponentEnabledPath appended alongside the flattened paths), so presence-only refs are compared value-to-value like any other. That is equivalent to your second suggestion for this case and needs no separate component-name set.
Added a TestValidateProfileDeclaration case pairing a value that references nfd presence-only against one that does not. It fails with union totality when the appended marker is removed, so the guard has a demonstrated failing state rather than only passing today.
There was a problem hiding this comment.
Correction: the fix cited above was subsequently reverted, deliberately. Including the synthetic enabled path in totality contradicted ADR-015, which specifies totality is evaluated before synthetic presence paths are added and that the per-component enabled path is part of ownedPaths but exempt from totality (see Deferred Decision 3 and the 2026-07-27 amendment). Two independent reviewers converged on the same code-vs-ADR conflict, and the ADR was taken as authoritative, so the current head restores the exemption and the test now asserts the ADR semantics.
On the over-locking concern itself: a presence-only componentRef still contributes its synthetic enabled path to ownedPaths, so the lock surface is intentional — what the exemption changes is only that a value may reference a component its siblings do not, without every sibling being forced to reference it too. If you think the ADR itself is wrong here, happy to reopen that in the ADR rather than diverge the implementation from it.
There was a problem hiding this comment.
Update: the fix commit referenced above was rewritten by later amends. At the current head a201987 this landed in its final form — and note the direction reversed after cross-checking ADR-015: totality is evaluated over leaf-flattened override paths only, with the synthetic enabled marker exempt (ADR-015 evaluates totality before synthetic presence is added). The test now asserts a presence-only ref is accepted, with the declaration-wide OwnedPaths still carrying the presence lock. The integration suite in pkg/recipe/profile_integration_test.go exercises this end to end.
| not: | ||
| required: [selectedProfile] | ||
|
|
||
| BundleRecipeV2Request: |
There was a problem hiding this comment.
🟡 Minor — /v2/bundle oneOf rejects empty-header legacy artifacts that the Go decoder and /v1/bundle accept
RecipeResult has no omitempty on Kind/APIVersion, so a round-tripped legacy artifact emits kind: "" + apiVersion: "". DecodeRecipeResult (pkg/recipe/decode.go:46-55) only enforces kind when apiVersion == v1alpha3, so the server accepts that empty-header body on both routes. This same PR deliberately widened /v1/bundle's BundleRecipeRequest to admit it (enum ["", aicr.run/v1alpha2] / ["", RecipeResult]) to fix the #1941 drift.
But BundleRecipeV2Request's oneOf (server.yaml:2880) has three branches — LegacyRecipeResponse (→ RecipeResponse, apiVersion enum [v1alpha2,v1alpha3], kind enum [RecipeResult]), VersionlessLegacyRecipeResponse (requires kind, not:required[apiVersion]), and ProfileRecipeResponse (v1alpha3) — and an empty-header body matches zero of them (spec-invalid). So /v2/bundle is stricter than its own server and stricter than /v1/bundle on an identical body.
This is a spec-vs-server contract gap (server more permissive than the documented /v2 union) — codegen clients / gateway request-validators would reject a body aicrd actually processes. No runtime break for clients that send kind: RecipeResult.
Suggested fix: widen VersionlessLegacyRecipeResponse (or add a branch) to admit apiVersion: "" + kind: "", mirroring the /v1/bundle BundleRecipeRequest widening, so both routes' schemas match the round-tripped legacy shape.
There was a problem hiding this comment.
Fixed in e0385f0, and this became the alignment work after #1943 merged.
VersionlessLegacyRecipeResponse required apiVersion absent (not: required: [apiVersion]) and kind in [RecipeResult], so an empty-header body matched zero branches exactly as you described. It is now widened to mirror /v1/bundle's BundleRecipeRequest: apiVersion enum [""] with the field no longer prohibited, and kind enum ["", RecipeResult].
The empty-only enum is what keeps oneOf well-formed — a versioned artifact still fails this branch, so each shape matches exactly one:
| body | matching branch |
|---|---|
apiVersion: "", kind: "" |
Versionless |
apiVersion absent |
Versionless |
aicr.run/v1alpha2 |
Legacy |
aicr.run/v1alpha3 + selectedProfile |
Profile |
The sync test asserted the old constraint (not.required contains apiVersion), so it was pinning the over-strict contract. It now asserts the enums instead, with a comment recording why prohibiting apiVersion outright was wrong.
Note this rebase also merged the two BundleRecipeRequest and RecipeResponse definitions that #1943 and this PR each introduced at different points in the file — git took both silently and only yamllint caught the duplication.
There was a problem hiding this comment.
Update: this is in its final form at head a201987. VersionlessLegacyRecipeResponse admits apiVersion enum [""] and kind enum ["", RecipeResult], so the empty-header round-trip body matches exactly one oneOf branch; the sync test asserts the enums. Validated with a draft-4 schema validator over real artifacts (empty-header accepted, versioned shapes unchanged).
| fmt.Sprintf("refusing to overwrite non-AICR profile lock template %q", outputPath)) | ||
| } | ||
| content := []byte(buf.String()) | ||
| if err := os.WriteFile(outputPath, content, 0600); err != nil { |
There was a problem hiding this comment.
🔵 Nitpick — Profile lock template inspect+write follow symlinks (no O_NOFOLLOW)
inspectProfileLockTemplate (argocdhelm.go:539, os.Open) and the write (argocdhelm.go:508, os.WriteFile) both follow symlinks at the fixed path templates/aicr-profile-lock.yaml. A pre-planted symlink there yields a fail-closed refusal (header mismatch) in the common case, or a 0600 overwrite through the symlink if the target happens to begin with the exact AICR header. Requires attacker write access to the operator's output dir, so low severity — but the new #1942 fix in this same file added checkStaticPath (argocdhelm.go), which uses os.Lstat to reject a symlink at the static/ path, and pkg/corroborate/meta.go / verifier.go / checksum/inventory.go already use O_NOFOLLOW. The lock-template write is now the odd one out within its own file.
Suggested fix: open with os.OpenFile(..., os.O_CREATE|os.O_WRONLY|os.O_NOFOLLOW, 0600) (and O_NOFOLLOW on the inspect open) to match the existing pattern.
There was a problem hiding this comment.
Verified the mechanism: inspectProfileLockTemplate opens with os.Open and the write uses os.WriteFile, so both follow a symlink at that fixed path.
Not changing it, on reachability. templatesDir is created by the generator itself (SafeJoin + MkdirAll at argocdhelm.go:395-399), and checksum.ValidateOutputRoot runs at bundler.go:343 before any deployer, rejecting a symlink anywhere under the output root. So a pre-existing symlink cannot survive to this point on the DefaultBundler path — reaching it needs a local process planting one inside the generation window, and anything with write access to --output can already rewrite the finished bundle and its checksums.txt. Closing it properly means openat-based operations through the deployer's whole write path, which buys nothing in a supported configuration.
This is the same class and disposition as the checkStaticPath TOCTOU on #1950, where the conclusion was to scope the comment rather than model an attacker who already has write access. Happy to reconsider if you would rather see the guard extended for consistency — it is a judgement call, not a factual dispute.
| // explicitly null profile. Both JSON and YAML decode null into a nil *string, | ||
| // but the strict v2 contract permits only a string in name=value form when the | ||
| // field is present. | ||
| func validateV2EnvelopeProfile(data []byte, contentType string, profile *string) error { |
There was a problem hiding this comment.
🔵 Nitpick — v2 envelope body is parsed twice for present-vs-null profile detection
validateV2EnvelopeProfile (recipe_handler.go:613) re-Unmarshals the already-strict-decoded, size-bounded body via bodyHasTopLevelProfile purely to distinguish an omitted profile from an explicit null (a nil *string can't tell them apart). No DoS concern (body is capped by MaxRecipePOSTBytes), and this is close to the standard idiom for present-vs-null detection — flagging only as a minor hot-path inefficiency.
Optional: capture presence during the primary strict decode via a json.RawMessage / yaml.Node field on the envelope to avoid the second pass.
There was a problem hiding this comment.
Confirmed: bodyHasTopLevelProfile re-unmarshals the already-decoded body at recipe_handler.go:171 and :323, in addition to the strict decode that produced the envelope.
Leaving it as is for this PR. The body is size-bounded before either pass, so the cost is a second parse of a small payload on the v2 envelope path only, and the second parse buys a distinction the strict decoder genuinely cannot express: an omitted profile versus an explicit profile: null. Folding that into the primary decode means either a *string with a custom unmarshaler or a json.RawMessage peek, both of which add more surface than the duplicate parse removes.
If you would prefer it collapsed, the cleaner shape is decoding into a struct with Profile *string plus a presence sentinel rather than re-parsing — worth doing as its own change with its own tests rather than inside the profile core.
| } | ||
|
|
||
| // PathsIntersect reports exact, ancestor, or descendant path intersection. | ||
| func PathsIntersect(a, b string) bool { |
There was a problem hiding this comment.
🔵 Nitpick — Exported PathsIntersect has no direct unit test
Exported PathsIntersect (profile.go:787) has no direct test; it's exercised only via isDeferredAllocationPolicyPath, the dynamic-path guard, and OwnsProfilePath. It is therefore not at 0% coverage and does not trip the repo's new-exported-func block rule — but its ancestor/descendant/exact-match semantics are subtle and the allocation-policy deferral gate depends on it being exact.
Suggested fix: add a small table-driven test (a.b vs a.b.c → true; a.b vs a.c → false; gpuResourcesEnabledOverride vs gpuResourcesEnabledOverrideExtra → false, no false-prefix match).
There was a problem hiding this comment.
Added in e0385f0. TestPathsIntersect covers the exported helper directly with 12 cases, each asserted in both argument orders since the function must be symmetric.
The cases that matter for the profile lock are the ancestor/descendant pairs (driver vs driver.enabled, both directions) and the segment-boundary one: driver must not intersect driverRoot, since comparison is per dot-segment rather than per character. A regression there would otherwise surface only indirectly through isDeferredAllocationPolicyPath, the dynamic-path guard, or OwnsProfilePath.
There was a problem hiding this comment.
Update: TestPathsIntersect is at the current head a201987 (the SHA cited above was rewritten by later amends) — 12 cases, both argument orders, including the driver/driverRoot segment-boundary case.
6dafeb8 to
12062c6
Compare
26449e4 to
a201987
Compare
yuanchen8911
changed the title
Signed-off-by: Yuan Chen <yuanchen97@gmail.com>
a201987 to
c0db667
Compare
yuanchen8911
changed the title
2 participants
Summary
Implement the ADR-015 configuration-profile core without adopting it in an existing recipe family. This adds fail-closed profile resolution and ownership locking, strict v2 API and artifact contracts, and documentation for the supported integration boundaries.
Motivation / Context
Recipes need to capture mutually exclusive, qualified installation modes as part of the resolved recipe rather than relying on bundle-time overrides. This is the first roll-in PR: AKS adoption will follow, then GKE adoption.
Fixes: N/A
Related: #1761, #1716
Type of Change
Component(s) Affected
cmd/aicr,pkg/cli)cmd/aicrd,pkg/server)pkg/recipe)pkg/bundler,pkg/component/*)pkg/collector,pkg/snapshotter)pkg/validator)pkg/errors,pkg/k8s)docs/,examples/)Implementation Notes
name=value.aicr.run/v1alpha3RecipeResultartifacts. Unprofiled results retain the legacy API version and behavior.RecipeResultartifacts, before observation or serialization, and enforce the same lock at recipe, bundle, mirror, and Argo CD Helm boundaries./v2/recipe,/v2/query, and/v2/bundleroutes. V2 POST bodies accept onlyapplication/jsonandapplication/x-yaml, and nested criteria reject unknown fields.apiVersions independent./v2/recipecan return eitheraicr.run/v1alpha2oraicr.run/v1alpha3, while/v2/bundleaccepts both plus versionless legacy artifacts./v1/bundlerequest schema with the input the runtime already accepts.BundleRecipeRequestrequires neither header field and admits an emptyapiVersionorkind, matching a legacy artifact decoded and remarshaled throughRecipeResult(neither field carriesomitempty). Header enums move fromRecipeResponseBaseonto each wrapper, soGET /v1/reciperesponses stay strict. Fixes the same contract drift as Align /v1/bundle OpenAPI schema with versionless legacy recipe support #1941 on the v1 route./v2/bundleas a version-partitioned union without anapiVersiondiscriminator. Legacy artifacts stampedaicr.run/v1alpha2or omittingapiVersionreuse the additive known-field schema and cannot carry a profile lock;aicr.run/v1alpha3is closed and requiresmetadata.selectedProfile.metadata.excludedOverlaysby artifact version. Legacy artifacts accept string entries and additive objects with a nonempty name; v1alpha3 requires closed objects with a nonempty name and a recognized optional reason. Profile artifacts also require completeconstraintWarningsentries; the schema defines everyComponentReffield type and closes nested expected-resource objects./v2/bundlefailures from vendored-chart and upstream operations.UNAUTHORIZEDand context or Kubernetes API timeout failures asTIMEOUT. Warn when a preinstalled driver conflicts with a profile-owned driver setting.Testing
The full qualification run passed, including race-enabled unit tests, the 80% aggregate coverage gate (81.0%), lint, 23 end-to-end suites, vulnerability scanning, license checks, and repository-specific validation. Coverage comparisons against
origin/mainfound no affected package decrease greater than 0.5 percentage points.Across the review-fix deltas, the focused race tests above and affected-package Go lint passed with zero issues. Focused YAML lint and diff checks passed, and independent review feedback on the delta was addressed. The final documentation changes passed the MDX safety check, offline link checks, diff check, and an independent review with zero findings. Full qualification, full-module lint, and coverage were not rerun after those deltas.
Independent verification (commit
c0db66781)The CLI, server, and
aicrdwere rebuilt from this revision and compared againstmainat905bec6e1. Every comparison asserts non-empty output and reports byte or file counts. A run that fails identically on both binaries is classified separately and never registers as a pass.Regression — no output difference anywhere. 800 comparisons across the full catalog (all 100 resolvable entries;
monitoring-hpais excluded because its criteria are entirelyanyand nothing resolves it).recipe: 100 comparisons, 8,306,000 bytes of generated YAML, 0 diffsbundle: 500 comparisons (100 entries ×helm,argocd,argocd-helm,flux,helmfile), 22,603 files compared withdiff -r, 0 diffsquery: 100 comparisons, 0 diffsmirror list: 100 comparisons, 0 diffsrecipe list: identical, 14,240 bytesFifty bundle comparisons produced no comparison: the ten AKS entries fail on both binaries at the keyless-toleration gate, which requires
--accelerated-node-toleration. They are counted asbothfail, not as agreement. The AKS bundle path is covered separately by the live-cluster run below, which passes that flag.Profile mechanism. The catalog declares no profile, so the regression sweep above exercises only the unprofiled path. Two things cover the new path instead.
TestProfileResolutionEndToEndand its siblings inpkg/recipe/profile_integration_test.golayer a profiled overlay fromtestdata/profile-overlay/over the embedded catalog and resolve through the ordinaryBuilder, so criteria matching, declaration discovery, value selection, override merging, and ownership locking run together rather than as isolated units. Eleven subtests cover default and explicit selection, the owned-path union, override delivery to the componentRefs, the raw-artifact gate, four fail-closed selections, and the legacy half of the biconditional. Both directions were confirmed to fail when the fixture is perturbed: flipping the declared default fails the selection assertion, and dropping one value's override trips union totality.The fixture is test input, not catalog. It carries no distinguishing constraint, which ADR-015 requires of a shippable declaration, and the file says so — the driver-ownership signal does not exist yet.
Beyond CI, the same mechanism was driven manually through an out-of-tree
--dataoverlay to cover bundle rendering, which the integration tests do not reach.aicr.run/v1alpha3withmetadata.selectedProfilepopulated, and bundle cleanly underhelm(321 files),argocd(290), andargocd-helm(307) — 1,836 files in total.ownedPathsspans both components, including the syntheticenabledpresence path.--setdiverging from a profile-owned path.mainbinary reads the same profiled overlay asaicr.run/v1alpha2with noselectedProfile, confirming the mechanism is inert to anything that does not understand it.Live cluster — AKS H100 (
aicr-test6, 2× ND96isr_H100_v5, Kubernetes 1.35). Run against a pinned kubeconfig with the cluster identity verified before use.aicr validatepassed all three phases against the cluster — deployment, conformance, and performance includingnccl-all-reduce-bw.API surface, live against
aicrd. Fifteen checks, all passing; byte counts are asserted so a 200 with an empty body fails./v1/recipeand/v2/recipereturn identical bodies for unprofiled criteria: 76,439 bytes each./v2acceptsapplication/jsonandapplication/x-yaml, and rejects a missing content type,text/plain, and the undeclaredapplication/yamlalias./v2rejects unknown envelope fields and unknown query parameters;/v1remains lenient as designed. Both reject explicit profile input against an unprofiled composition./v2/queryreturns a 16,168-byte body.Review-fix verification. Each fix in this round was checked against a failing state rather than only a passing one.
enabledmarker into the comparison and rejected a declaration whose values legitimately differ in which components they reference presence-only.PathPresentfor every non-synthetic owned path. Requiring merely "not absent" left a hole: overrides of{"driver": null}observe asPathBlockedinline, andmergeValuesdeletes a nil-valued key, so the hydrated observation isPathAbsentand neither check fired. Reproduced before the fix, and the added test fails when the fix is reverted./v2/bundleschema widening was validated with a draft-4 schema validator over real generated artifacts — the check a request-validating gateway or codegen client performs, and one a live request cannot make, since the server accepted these bodies either way.apiVersion: "",kind: ""oneOfbranchapiVersionabsentaicr.run/v1alpha2aicr.run/v1alpha3profiledEach shape matches exactly one branch, so the widening admits the round-tripped legacy body without relaxing the versioned ones.
The full
./pkg/... ./cmd/...suite passes: 75 packages ok, 0 failures.golangci-lint run ./...reports 0 issues.Risk Assessment
The mechanism spans recipe generation, APIs, clients, serialization, and bundle paths. It remains dormant until a recipe composition declares a profile. Compatibility tests cover unprofiled and legacy behavior, while strict v2 media-type and artifact handling intentionally reject undeclared or contradictory inputs.
Rollout notes: No recipe family adopts profiles in this PR. Follow-up PRs will adopt the mechanism for AKS first and GKE next.
Checklist
make testwith-race)make lint)git commit -S) — GPG signing info