Repository navigation
OSAC-3339: validate field_definition paths against proto schema and template parameters - #996
alosadagrande wants to merge 2 commits into
Conversation
|
@alosadagrande: This pull request references OSAC-3339 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 bug 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. |
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (10)
WalkthroughCatalog item validation now checks spec-relative field-definition paths and template parameter references. Bare-metal, cluster, and compute servers resolve templates during Create and Update, with expanded tests and updated path documentation. ChangesCatalog item validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CatalogItemServer
participant CatalogItemValidation
participant TemplatesDAO
Client->>CatalogItemServer: Create or Update catalog item
CatalogItemServer->>CatalogItemValidation: validate field definitions
CatalogItemValidation->>TemplatesDAO: resolve referenced template
TemplatesDAO-->>CatalogItemValidation: template definition
CatalogItemValidation-->>CatalogItemServer: InvalidArgument or success
CatalogItemServer-->>Client: validation response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/servers/private_baremetal_instance_catalog_items_server.go (1)
170-182: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftValidate the post-merge catalog item before checking template parameters.
For masked updates like
mask: ["field_definitions"],request.GetObject().GetTemplate()is the raw value, whiles.generic.Updatelater merges the persistedtemplateinto the draft object first. A partial update that only modifiesfield_definitionsand leaves template parameter definitions intact therefore passes this pre-merge check even when adding new invalidtemplate_parameters.*field definitions, and can fail later with schema validation or incorrect apply behavior. Use the draft object after the generic merge forvalidateFieldDefinitionPaths, not the unmerged request object.🤖 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 `@internal/servers/private_baremetal_instance_catalog_items_server.go` around lines 170 - 182, The Update method currently validates template parameter paths on the unmerged request object. Ensure the generic merge runs before validateFieldDefinitionPathsAndTemplateParams, then validate the resulting draft object while preserving field-definition validation and existing error propagation.internal/servers/private_compute_instance_catalog_items_server.go (1)
164-187: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate on the updated catalog item, not the request object.
UpdatecallsvalidateFieldDefinitionPathsAndTemplateParams(ctx, object)befores.generic.Updateapplies the field mask, so a field-mask update that only changesfield_definitionsusesobject.GetTemplateId()from the request object. Move this validation to the merged object, or load/use the current object’s template fields before validatingtemplate_parameters.*paths.🤖 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 `@internal/servers/private_compute_instance_catalog_items_server.go` around lines 164 - 187, Update the Update method so validateFieldDefinitionPathsAndTemplateParams runs against the catalog item after s.generic.Update applies the field mask, using the merged object’s template fields. Preserve field-definition validation and warning handling, and ensure template_parameters.* path validation uses the updated catalog item rather than the partial request object.internal/servers/private_cluster_catalog_items_server.go (1)
142-154: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftValidate against the persisted template before applying a partial update.
Updatevalidatesrequest.GetObject().GetTemplate()before callings.generic.Update, but the public update path may merge only changedfield_definitionsonto the existing catalog item. Iftemplateis not resented in that update, the request object can have an empty template and validation skipstemplate_parameters.*checks; use the existing private catalog item’s template for this validation.🤖 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 `@internal/servers/private_cluster_catalog_items_server.go` around lines 142 - 154, Update PrivateClusterCatalogItemsServer.Update to load the existing private catalog item before validating partial updates, and use its persisted template when request.GetObject().GetTemplate() is empty or omitted. Ensure validateFieldDefinitionPathsAndTemplateParams validates template_parameters.* against that persisted template before s.generic.Update applies changes, while preserving request-provided template 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 `@internal/servers/catalog_item_validation.go`:
- Around line 140-183: Update validateFieldDefinitionTemplateParams and the
corresponding validation logic around hasTemplateParamPaths to recognize the
bare "template_parameters" path as a template-parameter path, matching
validateFieldDefinitionPaths’ first-segment behavior rather than requiring a
trailing dot. Ensure the malformed path is validated and rejected, and add a
regression test in catalog_item_validation_test.go covering the exact path
"template_parameters".
- Around line 94-127: Update validatePathAgainstDescriptor to reject dot-path
traversal through non-map repeated message fields unless the path explicitly
includes an element index. Preserve nested traversal for singular messages and
map values, and adjust path parsing/validation so map key segments containing
dots are treated as a single escaped or otherwise explicitly denoted key rather
than split into nested field segments.
In `@internal/servers/private_baremetal_instance_catalog_items_server.go`:
- Around line 207-223: Extract the repeated DAO lookup and error-mapping logic
from resolveTemplate into a shared generic helper, such as
resolveTemplateFromDAO, accepting the DAO, template ID, and adapter function.
Update resolveTemplate in the private bare-metal, cluster, and compute servers
to delegate to this helper while preserving each concrete response type’s
existing adapter.
In `@internal/servers/private_cluster_catalog_items_server.go`:
- Around line 167-183: Extract the duplicated template lookup, error mapping,
and adapter construction from resolveTemplate into a shared generic helper
reusable by the private cluster, bare-metal, and compute catalog item servers.
Update each server’s resolveTemplate method to delegate to that helper while
preserving the existing DAO, templateLookupError, and adapter behavior.
In `@internal/servers/private_compute_instance_catalog_items_server.go`:
- Around line 201-216: Extract the repeated template lookup, error mapping, and
adapter construction from resolveTemplate into a shared generic helper reusable
by the private compute instance, bare-metal, and cluster servers. Update each
server’s resolveTemplate to call the helper while preserving its existing DAO,
adapter, and templateLookupError behavior.
---
Outside diff comments:
In `@internal/servers/private_baremetal_instance_catalog_items_server.go`:
- Around line 170-182: The Update method currently validates template parameter
paths on the unmerged request object. Ensure the generic merge runs before
validateFieldDefinitionPathsAndTemplateParams, then validate the resulting draft
object while preserving field-definition validation and existing error
propagation.
In `@internal/servers/private_cluster_catalog_items_server.go`:
- Around line 142-154: Update PrivateClusterCatalogItemsServer.Update to load
the existing private catalog item before validating partial updates, and use its
persisted template when request.GetObject().GetTemplate() is empty or omitted.
Ensure validateFieldDefinitionPathsAndTemplateParams validates
template_parameters.* against that persisted template before s.generic.Update
applies changes, while preserving request-provided template behavior.
In `@internal/servers/private_compute_instance_catalog_items_server.go`:
- Around line 164-187: Update the Update method so
validateFieldDefinitionPathsAndTemplateParams runs against the catalog item
after s.generic.Update applies the field mask, using the merged object’s
template fields. Preserve field-definition validation and warning handling, and
ensure template_parameters.* path validation uses the updated catalog item
rather than the partial request object.
🪄 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: Pro Plus
Run ID: 5b5d94b3-0a19-44e5-86cd-fde6612d5f1a
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/field_definition_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/field_definition_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/field_definition_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/field_definition_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
internal/servers/catalog_item_validation.gointernal/servers/catalog_item_validation_test.gointernal/servers/private_baremetal_instance_catalog_items_server.gointernal/servers/private_baremetal_instance_catalog_items_server_test.gointernal/servers/private_cluster_catalog_items_server.gointernal/servers/private_cluster_catalog_items_server_test.gointernal/servers/private_compute_instance_catalog_items_server.gointernal/servers/private_compute_instance_catalog_items_server_test.goproto/private/osac/private/v1/field_definition_type.protoproto/public/osac/public/v1/field_definition_type.proto
…e parameters Extract shared validation logic (validateCatalogItemFieldDefinitionPaths, validateTemplateParams, templateLookupError) into catalog_item_validation.go to eliminate duplication across the three catalog item servers. Fix proto comments to reflect actual path format (relative to spec, not prefixed with "spec."). Add server-level tests verifying Create/Update reject invalid spec paths and unknown template parameters for all three catalog item types. OSAC-3339 Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
A field_definition with path "template_parameters" (no dot suffix) bypassed both spec path validation and template parameter validation. Reject it early in validateFieldDefinitionPaths with a clear error message. Signed-off-by: Alberto Losada Grande <alosadagrande@gmail.com> Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Alberto Losada Grande <alosadag@redhat.com>
b81df53 to
6b6f18e
Compare
|
/retest |
|
Re-triggered failed runs:
|
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Clean shared validation logic with excellent error messages. One important issue with Update validation timing.
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 1 |
| 💡 Suggestion | 1 |
| if err = validateFieldDefinitions(object.GetFieldDefinitions()); err != nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 Template param validation on Update uses pre-merge state
validateFieldDefinitionPathsAndTemplateParams runs on request.GetObject() before s.generic.Update performs the field mask merge. If a client sends a partial update with only field_definitions in the mask and omits template from the request body, item.GetTemplate() returns "". The validation then rejects with "no template is set" even though the persisted object has a valid template.
This applies to all three catalog item servers (cluster, compute instance, bare metal).
Fix: when templateRef is empty and the request has template_parameters.* paths, fetch the existing catalog item's template from the database before rejecting.
| // validateFieldDefinitionPaths checks that each field definition path corresponds to a valid field | ||
| // in the given spec message descriptor. Paths starting with "template_parameters" are skipped | ||
| // (validated separately by validateFieldDefinitionTemplateParams). | ||
| func validateFieldDefinitionPaths( | ||
| fieldDefinitions []*privatev1.FieldDefinition, | ||
| specDescriptor protoreflect.MessageDescriptor, | ||
| ) error { | ||
| for _, fd := range fieldDefinitions { | ||
| path := fd.GetPath() | ||
| if path == "" { | ||
| continue | ||
| } | ||
| segments := strings.Split(path, ".") | ||
| if segments[0] == "template_parameters" { | ||
| if len(segments) == 1 { | ||
| return grpcstatus.Errorf(grpccodes.InvalidArgument, | ||
| "invalid field_definition path 'template_parameters': "+ | ||
| "must specify a parameter name (e.g., 'template_parameters.param_name')") | ||
| } | ||
| continue |
There was a problem hiding this comment.
💡 Path validation stops at first error while template param validation collects all
validateFieldDefinitionPaths returns on the first invalid path, while validateFieldDefinitionTemplateParams collects all invalid parameters and reports them together. Consider collecting all invalid paths too so the caller can fix everything in one round trip. Minor UX inconsistency.
There was a problem hiding this comment.
Also: "template_parameters" is hardcoded in 5 places across this file (lines 77, 80, 120, 189, 231+). Consider extracting a const like templateParametersPrefix = "template_parameters." - there is no existing constant in the codebase, but this PR adds enough new uses to justify one.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alosadagrande The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
tzvatot
left a comment
tzvatot
left a comment
There was a problem hiding this comment.
Test Review
Good unit test coverage for the validation functions (12 path tests + 3 template param tests). The 12 server-level integration tests verify wiring. Three missing test paths.
| Category | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟡 Important | 3 |
| 💡 Suggestion | 1 |
🟡 No test for "no template is set" error path
validateTemplateParams rejects template_parameters.* paths when templateRef == "". This path is untested at both unit and server level. A catalog item with no template but with template_parameters.foo in field definitions should be rejected.
🟡 No test for template-not-found via DAO lookup
templateLookupError converts dao.ErrNotFound to InvalidArgument("template not found"). All server tests create the template first - none test referencing a nonexistent template ID with template parameter paths.
🟡 No happy-path server test with valid template parameters
All 12 new server tests are rejection tests. No positive test verifies that a Create with valid template_parameters.* paths against a template declaring those parameters actually succeeds through the full server wiring (template DAO lookup, adapter conversion).
💡 DescribeTable opportunity
The 4 tests per server follow identical structure across all three servers. The codebase uses DescribeTable in catalog_item_validation_test.go, but the different proto types make collapsing tricky. Acceptable duplication.
This branch was previously deployed
5 participants
Summary
field_definitionpaths against the spec message descriptor (ClusterSpec,ComputeInstanceSpec,BareMetalInstanceSpec) at catalog item Create/Update time. Invalid paths are rejected with anInvalidArgumenterror listing the valid fields.template_parameters.*paths against the referenced template's declared parameters. If a field definition references an unknown parameter, the error lists valid parameter names.catalog_item_validation.goas shared functions used by all three catalog item servers, using atemplateResolverfunction type for dependency injection of template lookups.FieldDefinition.pathexamples in both public and private proto files — paths are relative to the spec message (e.g.network.pod_cidr), not prefixed withspec..CreateandUpdatereject invalid spec paths and unknown template parameters.Jira
OSAC-3339
Test plan
gofmt -s -w .— no formatting changesbuf lint+buf generate— proto lint passes, generated code up to datego build ./...— compiles cleanlyginkgo run -r internal— 83 suites pass (including 12 new validation tests)🤖 Generated with Claude Code
Summary by CodeRabbit
Validation Improvements
Documentation
spec.prefix.