OSAC-1416: enforce field_definitions as complete contract on catalog item usage - #783
Conversation
|
@alosadagrande: This pull request references OSAC-1416 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Caution Review failedAn error occurred during the review process. Please try again later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b85b011 to
d632f47
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/CATALOG_ITEMS.md`:
- Around line 138-146: The compute-instance example in the catalog docs uses
unsupported field paths, so update the example to match the valid
compute-instance schema used elsewhere in this PR. Replace the
`field_definitions` entries that reference `ssh_public_key` and `host_type` with
supported paths such as `ssh_key`, `cores`, `memory_gib`, and
`network_attachments`, keeping the example consistent with the catalog item
model.
- Around line 46-49: Update the catalog-item documentation to remove the
“silently discarded” wording for unsupported field_definition.path values and
replace it with the current server behavior. In CATALOG_ITEMS.md, revise the
paragraph under the custom parameters section so it no longer suggests unknown
paths are ignored; make the description accurately reflect that invalid paths
are not accepted and can cause catalog-item definitions to fail.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 83733a9f-3559-4bff-a80b-cf39d95dec48
📒 Files selected for processing (5)
docs/CATALOG_ITEMS.mdinternal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/private_clusters_server_test.gointernal/servers/private_compute_instances_server_test.go
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
edb120c to
c9107e5
Compare
This comment was marked as resolved.
This comment was marked as resolved.
ygalblum
left a comment
ygalblum
left a comment
There was a problem hiding this comment.
Most comments revolve the same question. Does this requirement impose additional work on the CatalogItem creator? Will they know to add these fields that they don't wish to control at all?
Thanks for the review @ygalblum . Let me address your main concern. The implementation follows the catalog-items EP and OSAC-1416 (filed by @tzvatot ). The EP explicitly The fields list defines the complete contract between the admin and the user for a given catalog item. Fields not listed in fields are neither editable nor pre-defined; the admin is responsible for ensuring that every field required by the underlying template is covered by a field definition." EP, API Behavior section So yes, the CatalogItem creator must list every field that the user is expected to provide, even fields they don't want to restrict — that's the EP design. If a field isn't listed, it's not available to the user. One important thing to note: the previous behavior was silently overwriting user-provided values for non-editable fields and silently accepting unlisted fields. This PR replaces that silent behavior with explicit error messages that tell the user exactly what went wrong:
This is actually better UX for both the admin and the user: instead of silently doing the wrong thing, the server now gives actionable feedback. Regarding validating invalid paths at CatalogItem creation time (your comment on catalog_item_validation.go:127): that would be a good improvement. The EP already calls this out as a known gap in the Risks section, but it's out of scope for this ticket. You're right that ssh_key is a poor example in the docs — I'll fix that along with the other doc issues (cores/memory_gib → instance_type, ssh_key vs ssh_public_key inconsistency) in a follow-up commit. @tzvatot can you confirm this matches your intent for OSAC-1416? |
When creating a resource via catalog item, the server now rejects any user-provided spec field that is not covered by a field_definition entry. The catalog_item and template fields are always allowed as system fields. A field_definition that points to a parent path covers all children (e.g., "network" covers "network.pod_cidr"). Ref: OSAC-1416 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
When a field_definition is marked as non-editable and the user provides a value for it, the server now returns InvalidArgument instead of silently overwriting the value with the default. Ref: OSAC-1416 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
Update CATALOG_ITEMS.md to document that unlisted spec fields and user-provided values for non-editable fields are now rejected with InvalidArgument instead of being silently ignored or overwritten. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
Existing tests in private_clusters_server_test.go and private_compute_instances_server_test.go expected non-editable fields to be silently overwritten. Update them to expect InvalidArgument. Also add network_attachments to field_definitions in compute instance tests so the unlisted field check does not trigger. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
…og items doc Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
Add template_parameters to the allowedPaths map in applyFieldDefinitions so BareMetalInstance specs that combine catalog items with template parameters are not rejected. Update is_windows and BareMetalInstance non-editable override tests to match the new enforcement behavior. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
f5f1244 to
2c695a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/CATALOG_ITEMS.md`:
- Around line 263-269: Update the “How CLI flags interact” and “Server-side
processing” documentation to state that editable fields with no catalog default
require a user-provided value, otherwise the request is rejected with
InvalidArgument. Ensure the ComputeInstanceCatalogItem behavior for editable
fields such as instance_type, run_strategy, and network_attachments is
accurately reflected.
- Line 162: In the ComputeInstanceCatalogItem documentation note, replace the
phrase “in order to use it” with “to use it,” preserving the rest of the
sentence and example unchanged.
- Around line 142-147: Update the image.source_type and image.source_ref entries
in the catalog example to explicitly include editable: false, matching the
existing ClusterCatalogItem example and preserving their current non-editable
behavior.
🪄 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: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 392365be-7096-49eb-a491-a0423dcccd6a
📒 Files selected for processing (1)
docs/CATALOG_ITEMS.md
| - If a field is **not listed** in `field_definitions`, the server **rejects** the request with | ||
| `InvalidArgument`. | ||
| - If a field is **non-editable** in the catalog item and the user provides a value, the server | ||
| **rejects** the request with `InvalidArgument`. | ||
| - If a field is **editable**, the CLI flag value is accepted and validated against | ||
| `validation_schema` if one is defined. | ||
| - If an editable field is **not provided** by the user, the catalog item's default is applied. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Document that editable fields without a default require user input.
The doc states editable fields without a user value get the default applied (lines 269, 284), but the code returns InvalidArgument when an editable field has no default and the user provides no value ("field '%s' is required but no value was provided and no default is defined"). The ComputeInstanceCatalogItem example at lines 152–160 has instance_type, run_strategy, and network_attachments as editable with no default — users must provide these or the request fails. This behavior is undocumented.
📝 Suggested additions
Add a bullet to the "How CLI flags interact" section (after line 269):
- If an editable field is **not provided** by the user, the catalog item's default is applied.
+- If an editable field has **no default** and the user does not provide a value, the server
+ **rejects** the request with `InvalidArgument` — the field is required.And update the "Server-side processing" section (line 284):
- **Editable fields without a user value**: the default is applied
+ (or rejected with `InvalidArgument` if no default is defined)Also applies to: 281-284
🤖 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/CATALOG_ITEMS.md` around lines 263 - 269, Update the “How CLI flags
interact” and “Server-side processing” documentation to state that editable
fields with no catalog default require a user-provided value, otherwise the
request is rejected with InvalidArgument. Ensure the ComputeInstanceCatalogItem
behavior for editable fields such as instance_type, run_strategy, and
network_attachments is accurately reflected.
|
/retest |
|
No failed workflow runs found for this PR at commit |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alosadagrande, ygalblum The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/lgmt |
|
/lgtm |
|
@alosadagrande: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Review Summary
Solid implementation. field_definitions is now enforced as a strict allowlist for catalog item-based resource creation: unlisted spec fields are rejected, and user-provided values for non-editable fields return InvalidArgument instead of being silently overridden. The validation helpers (collectLeafPaths, isPathCovered) are well-designed, error messages are deterministic, and test coverage is thorough.
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 1 |
| 💡 Suggestion | 1 |
See inline comments for details.
Design note (non-blocking): ygalblum's concern about the cliff behavior (empty field_definitions allows everything, one entry makes it a strict allowlist) is valid UX feedback worth discussing at the EP level, but not in scope for this PR.
| spec := &privatev1.ComputeInstanceSpec{ | ||
| IsWindows: &falseVal, | ||
| } | ||
| It("applies non-editable default for bool field is_windows on compute instance spec", func() { |
There was a problem hiding this comment.
🟡 Missing test: non-editable boolean field with explicit false value
The old test "forces is_windows value" set IsWindows: &falseVal and verified override. This replacement only tests default application when no user value is provided. There's no corresponding test that sets is_windows: false and expects InvalidArgument rejection.
This matters because protojson serialization of false depends on field optionality:
- Optional
*bool:falseserializes to"is_windows": false- caught byuserHasValue && userVal != nil - Non-optional
bool:falseis zero value, protojson omits it - silently passes as "no value"
If the proto field later changes from optional to non-optional, the rejection would silently stop working for false values.
Suggestion: Add a companion test:
It("rejects user value false for non-editable bool field is_windows", func() {
falseVal := false
spec := &privatev1.ComputeInstanceSpec{
IsWindows: &falseVal,
}
defaultVal, err := structpb.NewValue(true)
Expect(err).ToNot(HaveOccurred())
fieldDefs := []*privatev1.FieldDefinition{{
Path: "is_windows",
Editable: false,
Default: defaultVal,
}}
err = applyFieldDefinitions(spec, fieldDefs)
Expect(err).To(HaveOccurred())
Expect(status.Code(err)).To(Equal(codes.InvalidArgument))
Expect(err.Error()).To(ContainSubstring("not editable"))
})| } | ||
| } | ||
|
|
||
| func collectLeafPaths(m map[string]any, prefix string) []string { |
There was a problem hiding this comment.
💡 collectLeafPaths silently passes empty nested objects
When a user sends a nested message with no fields set (e.g., "network": {}), this function recurses into the empty map and returns no paths. The empty nested object passes the unlisted field check without a corresponding field_definition.
Low practical impact since protojson omits empty sub-messages for non-optional fields, but for optional message fields that can serialize as {}, this is a gap in the "complete contract" enforcement.
Consider either treating empty nested maps as leaf paths (emit the parent path when the nested map has zero keys), or documenting this as accepted behavior.
PR osac-project#866 removed the Cores field from ComputeInstanceSpec but didn't update the test added by PR osac-project#783 in catalog_item_validation_test.go. Replace with RunStrategy to test the same unlisted-field rejection. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Dan Manor <dmanor@redhat.com>
This branch was previously deployed
4 participants
Summary
field_definitionswhen creating a resource via catalog item (InvalidArgumentwith field names)InvalidArgument)applyFieldDefinitionsdocstring to reflect the new behaviorJira
https://issues.redhat.com/browse/OSAC-1416
Test plan
applyFieldDefinitionstests covering unlisted fields, non-editable rejection, happy path, system fields, nested paths, ComputeInstanceSpecgofmt,buf generate,go buildpassginkgo run -r internalpasses (1 pre-existing failure on migration hash unrelated to this change)🤖 Generated with Claude Code
Summary by CodeRabbit
field_definitions, including nested paths.InvalidArgument; catalog defaults are applied only when the value is omitted.CATALOG_ITEMS.mdto reflect the new rejection and non-editable field behaviors.