Conversation
|
@tzvatot: This pull request references OSAC-704 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. |
|
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:
WalkthroughAdds catalog_item fields to cluster and compute instance protos (private + public). Implements catalog-item-backed creation: DAO wiring in builders, Create dispatch between spec.catalog_item and spec.template with mutual-exclusivity checks, catalog-item lookup and access validation, applying catalog field definitions to proto specs (defaults for non-editable fields; JSON Schema validation and defaults for editable fields), and enforced immutability of spec.catalog_item on updates. Tests cover create-by-id/name, missing/unpublished errors, field-definition behavior, and immutability. go.mod pins github.com/santhosh-tekuri/jsonschema/v6 v6.0.2. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/servers/private_compute_instances_server.go (1)
183-187:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate network references after catalog-item transforms.
validateNetworkReferencesruns beforevalidateAndTransformCatalogItem, but field definitions can still writespec.subnetandspec.security_groups. That lets the persisted network config bypass the existence/READY/VNet checks entirely on the catalog-item path.Also applies to: 198-203
🤖 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_instances_server.go` around lines 183 - 187, The validateNetworkReferences call is happening too early and can miss fields added by validateAndTransformCatalogItem; move the validateNetworkReferences(ctx, request.GetObject()) invocation to after validateAndTransformCatalogItem (and any catalog-item transform code) so that spec.subnet and spec.security_groups set during transformation are validated; apply the same relocation for the other occurrence noted around lines 198-203 so both code paths run transforms first and only then call validateNetworkReferences.
🤖 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 73-76: When a field is non-editable (fd.GetEditable() == false) we
currently call applyDefault(specMap, path, defaultVal) but applyDefault is a
no-op if defaultVal is nil, leaving user values intact; update the non-editable
branch to explicitly enforce a default by checking if defaultVal is nil and
returning a validation error (or clearing/setting the field to a required zero
value) instead of silently calling applyDefault. Concretely, inside the block
that checks fd.GetEditable(), add a check for defaultVal == nil and return an
error mentioning the offending path/field; otherwise call applyDefault(specMap,
path, defaultVal) as before. Ensure the same change is applied to the other
occurrence that calls applyDefault when non-editable.
In `@internal/servers/private_compute_instances_server.go`:
- Around line 198-213: The catalog-item branch in
validateAndTransformCatalogItem injects spec.template but then skips template
validation/defaulting and required-spec checks; update the flow so that after
calling s.validateAndTransformCatalogItem(...) you also call
s.fetchAndValidateTemplate(ctx, request.GetObject()) and then
s.applySpecDefaults(request.GetObject().GetSpec(), template) (same as the
non-catalog path), handling and returning any errors just like the else branch,
before proceeding to generic.Create; reference the existing functions
validateAndTransformCatalogItem, fetchAndValidateTemplate, and applySpecDefaults
and ensure error assignments/returns mirror the current non-catalog branch.
- Around line 561-568: The current error handling in the lookup block leaks
internal DAO/details by including "%v" in the Internal grpc error; instead, keep
the PermissionDenied branch as-is (errors.As against *dao.ErrDenied) but for
other errors log the original error (using the server logger) and replace the
grpcstatus.Errorf(grpccodes.Internal, "failed to lookup catalog item '%s': %v",
key, err) with an opaque message such as grpcstatus.Errorf(grpccodes.Internal,
"failed to lookup catalog item '%s'", key); mirror the approach used in
fetchTemplate: log the full err locally and return only the generic message to
the client. Ensure references: dao.ErrDenied, key variable, and the surrounding
lookup block in private_compute_instances_server.go are updated accordingly.
- Around line 341-343: The immutability checks miss cases where the update mask
contains a parent path like "spec" which should count as updating child fields;
change hasMaskPrefix (used to set updatingCatalogItem, updatingTemplate,
updatingTemplateParams from updateMask) so it returns true when a mask path
equals the target prefix OR when a mask path is a parent of the target (for
example maskPath == "spec" should match "spec.catalog_item"), then use the
updated hasMaskPrefix for determining updatingCatalogItem, updatingTemplate, and
updatingTemplateParams to ensure parent-path updates do not bypass immutability
checks.
In `@proto/public/osac/public/v1/cluster_type.proto`:
- Around line 158-160: The documentation for the mutually-exclusive fields is
inconsistent: update the doc comment for the `template` field so it matches the
`catalog_item` comment and clearly states the create-time contract (i.e., either
`template` or `catalog_item` must be provided). Locate the `template` field
comment in the same proto message that defines `catalog_item` and change its
text to a concise sentence such as “Either `template` or `catalog_item` is
required on create; they are mutually exclusive during migration.” ensuring both
`template` and `catalog_item` docs describe the same mutually-exclusive/required
behavior.
In `@proto/public/osac/public/v1/compute_instance_type.proto`:
- Around line 159-161: Update the documentation for the "template" field to
reflect the new create contract: it is mutually exclusive with "catalog_item"
(template XOR catalog_item) rather than always mandatory; modify the "template"
comment (the field named template referenced earlier in the file) to state that
either template or catalog_item must be provided on create and that they are
mutually exclusive, and clarify server behavior when catalog_item is used
(server fetches catalog item and applies its field definitions).
---
Outside diff comments:
In `@internal/servers/private_compute_instances_server.go`:
- Around line 183-187: The validateNetworkReferences call is happening too early
and can miss fields added by validateAndTransformCatalogItem; move the
validateNetworkReferences(ctx, request.GetObject()) invocation to after
validateAndTransformCatalogItem (and any catalog-item transform code) so that
spec.subnet and spec.security_groups set during transformation are validated;
apply the same relocation for the other occurrence noted around lines 198-203 so
both code paths run transforms first and only then call
validateNetworkReferences.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c8b38633-d62b-4002-9d95-09729fa728be
⛔ Files ignored due to path filters (9)
go.sumis excluded by!**/*.suminternal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
go.modinternal/servers/catalog_item_validation.gointernal/servers/private_clusters_server.gointernal/servers/private_clusters_server_test.gointernal/servers/private_compute_instances_server.gointernal/servers/private_compute_instances_server_test.goproto/private/osac/private/v1/cluster_type.protoproto/private/osac/private/v1/compute_instance_type.protoproto/public/osac/public/v1/cluster_type.protoproto/public/osac/public/v1/compute_instance_type.proto
18bc82d to
7233fe6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/servers/catalog_item_validation.go (1)
73-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce default presence for non-editable fields.
Non-editable fields without a default value are not enforced. Line 74 calls
applyDefault, which no-ops whendefaultValis nil (line 121), leaving user-provided values unchanged. This breaks the non-editable guarantee for misconfigured catalog items.🤖 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/catalog_item_validation.go` around lines 73 - 76, When a field is non-editable (fd.GetEditable() == false) we must require a default instead of silently no-oping; update the branch that currently calls applyDefault(specMap, path, defaultVal) so that if defaultVal is nil you return a validation error (rather than letting applyDefault no-op). Specifically, in the code path referencing fd.GetEditable(), check defaultVal for nil and produce an error indicating the non-editable field at path lacks a default; otherwise call applyDefault as before.
🤖 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.
Duplicate comments:
In `@internal/servers/catalog_item_validation.go`:
- Around line 73-76: When a field is non-editable (fd.GetEditable() == false) we
must require a default instead of silently no-oping; update the branch that
currently calls applyDefault(specMap, path, defaultVal) so that if defaultVal is
nil you return a validation error (rather than letting applyDefault no-op).
Specifically, in the code path referencing fd.GetEditable(), check defaultVal
for nil and produce an error indicating the non-editable field at path lacks a
default; otherwise call applyDefault as before.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d232cbde-71a1-49cb-a5c7-cde3312dcc07
⛔ Files ignored due to path filters (9)
go.sumis excluded by!**/*.suminternal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
go.modinternal/servers/catalog_item_validation.gointernal/servers/private_clusters_server.gointernal/servers/private_clusters_server_test.gointernal/servers/private_compute_instances_server.gointernal/servers/private_compute_instances_server_test.goproto/private/osac/private/v1/cluster_type.protoproto/private/osac/private/v1/compute_instance_type.protoproto/public/osac/public/v1/cluster_type.protoproto/public/osac/public/v1/compute_instance_type.proto
🚧 Files skipped from review as they are similar to previous changes (9)
- proto/public/osac/public/v1/cluster_type.proto
- proto/private/osac/private/v1/compute_instance_type.proto
- proto/public/osac/public/v1/compute_instance_type.proto
- go.mod
- proto/private/osac/private/v1/cluster_type.proto
- internal/servers/private_clusters_server.go
- internal/servers/private_compute_instances_server_test.go
- internal/servers/private_compute_instances_server.go
- internal/servers/private_clusters_server_test.go
c266bd7 to
bf4ea03
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhernand, tzvatot 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 |
|
@tzvatot: The following test 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. |
bf4ea03 to
bacb3ae
Compare
|
New changes are detected. LGTM label has been removed. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
internal/servers/private_compute_instances_server.go (2)
205-220:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun template validation/defaulting after catalog-item transformation.
After
validateAndTransformCatalogItemreturns, this branch falls through togeneric.Createwithout callingfetchAndValidateTemplateorapplySpecDefaults. Catalog-item creates can therefore skip template existence checks, template-parameter validation/defaulting, and required-spec validation.Proposed fix
if catalogItemRef != "" { err = s.validateAndTransformCatalogItem(ctx, request.GetObject()) if err != nil { return } - } else { - template, templateErr := s.fetchAndValidateTemplate(ctx, request.GetObject()) - if templateErr != nil { - err = templateErr - return - } - err = s.applySpecDefaults(request.GetObject().GetSpec(), template) - if err != nil { - return - } } + + template, templateErr := s.fetchAndValidateTemplate(ctx, request.GetObject()) + if templateErr != nil { + err = templateErr + return + } + err = s.applySpecDefaults(request.GetObject().GetSpec(), template) + if err != nil { + return + }🤖 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_instances_server.go` around lines 205 - 220, The catalog-item branch exits after validateAndTransformCatalogItem, skipping template checks and defaulting; after calling validateAndTransformCatalogItem (on request.GetObject()), immediately call fetchAndValidateTemplate(ctx, request.GetObject()) and, if no error, call applySpecDefaults(request.GetObject().GetSpec(), template), handling and returning any errors the same way the template branch does so template existence, parameter validation/defaulting, and required-spec validation run for catalog-item creates before proceeding to generic.Create.
365-369:⚠️ Potential issue | 🟠 Major | ⚡ Quick winParent update masks still bypass the immutability guard.
hasMaskPrefixdoesn't treat a parent path likespecas matchingspec.catalog_item,spec.template, orspec.template_parameters. A client can sendupdate_mask.paths = ["spec"]and change these immutable fields without hitting this check.Proposed fix
func hasMaskPrefix(mask *fieldmaskpb.FieldMask, prefixes ...string) bool { if mask == nil || len(mask.GetPaths()) == 0 { return true } for _, path := range mask.GetPaths() { for _, prefix := range prefixes { - if path == prefix || strings.HasPrefix(path, prefix+".") { + if path == prefix || + strings.HasPrefix(path, prefix+".") || + strings.HasPrefix(prefix, path+".") { return true } } } return false }🤖 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_instances_server.go` around lines 365 - 369, The immutability guard misses cases where the client sends a parent path like "spec" because the current checks only call hasMaskPrefix(updateMask, "spec.template" / "spec.template_parameters" / "spec.catalog_item"); update the logic that sets updatingTemplate, updatingTemplateParams, and updatingCatalogItem to also treat an explicit "spec" update as matching (e.g., OR in a check that updateMask.paths contains "spec" or use a helper hasMaskExact(updateMask, "spec")/hasMaskParent(updateMask, "spec")), so any updateMask that lists "spec" will flip the same updating* booleans and trigger the immutability guard (referencing hasMaskPrefix, updatingTemplate, updatingTemplateParams, updatingCatalogItem, and updateMask).
🤖 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.
Duplicate comments:
In `@internal/servers/private_compute_instances_server.go`:
- Around line 205-220: The catalog-item branch exits after
validateAndTransformCatalogItem, skipping template checks and defaulting; after
calling validateAndTransformCatalogItem (on request.GetObject()), immediately
call fetchAndValidateTemplate(ctx, request.GetObject()) and, if no error, call
applySpecDefaults(request.GetObject().GetSpec(), template), handling and
returning any errors the same way the template branch does so template
existence, parameter validation/defaulting, and required-spec validation run for
catalog-item creates before proceeding to generic.Create.
- Around line 365-369: The immutability guard misses cases where the client
sends a parent path like "spec" because the current checks only call
hasMaskPrefix(updateMask, "spec.template" / "spec.template_parameters" /
"spec.catalog_item"); update the logic that sets updatingTemplate,
updatingTemplateParams, and updatingCatalogItem to also treat an explicit "spec"
update as matching (e.g., OR in a check that updateMask.paths contains "spec" or
use a helper hasMaskExact(updateMask, "spec")/hasMaskParent(updateMask,
"spec")), so any updateMask that lists "spec" will flip the same updating*
booleans and trigger the immutability guard (referencing hasMaskPrefix,
updatingTemplate, updatingTemplateParams, updatingCatalogItem, and updateMask).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: b54b0001-a560-4718-9714-538139243171
⛔ Files ignored due to path filters (9)
go.sumis excluded by!**/*.suminternal/api/osac/private/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/compute_instance_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
go.modinternal/servers/catalog_item_validation.gointernal/servers/private_clusters_server.gointernal/servers/private_clusters_server_test.gointernal/servers/private_compute_instances_server.gointernal/servers/private_compute_instances_server_test.goproto/private/osac/private/v1/cluster_type.protoproto/private/osac/private/v1/compute_instance_type.protoproto/public/osac/public/v1/cluster_type.protoproto/public/osac/public/v1/compute_instance_type.proto
🚧 Files skipped from review as they are similar to previous changes (9)
- proto/private/osac/private/v1/cluster_type.proto
- proto/public/osac/public/v1/compute_instance_type.proto
- proto/public/osac/public/v1/cluster_type.proto
- proto/private/osac/private/v1/compute_instance_type.proto
- go.mod
- internal/servers/private_clusters_server.go
- internal/servers/private_compute_instances_server_test.go
- internal/servers/private_clusters_server_test.go
- internal/servers/catalog_item_validation.go
… create validation https://redhat.atlassian.net/browse/OSAC-704 - Add catalog_item field to ClusterSpec (field 8) and ComputeInstanceSpec (field 14) in both private and public protos, coexisting with template - Add catalog item DAO to PrivateClustersServer for lookup - Dispatch Create between template and catalog_item paths (mutually exclusive) - Add catalog_item_validation.go: field definition processing (non-editable override, editable validation via JSON Schema, defaults), access checks - Add santhosh-tekuri/jsonschema/v6 (https://github.com/santhosh-tekuri/jsonschema) for JSON Schema draft 2020-12 validation WIP: ComputeInstance server changes, unit tests, update immutability pending. Generated with [Claude Code](https://claude.com/claude-code)
https://redhat.atlassian.net/browse/OSAC-704 - Fix critical: use protojson.MarshalOptions{UseProtoNames: true} so field paths match snake_case keys (not camelCase) - Replace proto reflection with typed catalogItem interface - Share single jsonschema.Compiler per request (not per field) - Extract applyDefault helper to remove duplicated marshalling logic - Remove dead code (convertProtoPathToJSON, toSnakeCase, resolveFieldByPath) - Add comment: tenant visibility enforced by GenericDAO tenancy logic Generated with [Claude Code](https://claude.com/claude-code)
https://redhat.atlassian.net/browse/OSAC-704 - Add catalog items DAO to PrivateComputeInstancesServer - Dispatch Create between catalog_item and template paths (mutually exclusive) - Add validateAndTransformCatalogItem + lookupCatalogItem methods Generated with [Claude Code](https://claude.com/claude-code)
https://redhat.atlassian.net/browse/OSAC-704 - Reject updates that change spec.catalog_item (immutable like template) - Applied to both Cluster and ComputeInstance update flows Generated with [Claude Code](https://claude.com/claude-code)
Add 18 tests for catalog item create path in both Cluster and ComputeInstance servers: happy path, name lookup, not found, unpublished, mutual exclusivity with template, field definition enforcement (non-editable override, schema validation, default application), and update immutability. Fix bug in validateAgainstSchema where strings.NewReader was passed to jsonschema/v6 Compiler.AddResource which expects a parsed JSON value. Generated with [Claude Code](https://claude.com/claude-code)
Remove unnecessary catalog item creation from mutual exclusivity tests (the check fires before lookup). Add schema validation success path tests to verify valid values pass through. Generated with [Claude Code](https://claude.com/claude-code)
Generated with [Claude Code](https://claude.com/claude-code)
- Use exact Equal assertions for error messages instead of ContainSubstring, matching existing codebase convention - Convert schema validation pass/fail tests to DescribeTable Generated with [Claude Code](https://claude.com/claude-code)
- Reject non-editable fields with no default value (catalog item misconfiguration) instead of silently passing through user values - Log DAO errors server-side and return opaque message to client instead of leaking internal error details - Update template field docs in public proto: "either template or catalog_item is required" instead of "template is mandatory" Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: Elad Tabak <etabak@redhat.com>
bacb3ae to
160bec1
Compare
Upstream PR osac-project#543 replaced multi-valued Tenants/Creators with singular Tenant/Creator fields. Update catalog item test helpers to match. Generated with [Claude Code](https://claude.com/claude-code)
Summary
catalog_itemfield toClusterSpecandComputeInstanceSpec(private + public protos)spec.catalog_itemis immutable on Update for both resource typesvalidateAgainstSchema:jsonschema/v6 Compiler.AddResourceexpects a parsed JSON value, not anio.Reader— schema validation was brokenTest plan
ginkgo run -r internal/servers)buf lintpassesginkgo run it)Jira: OSAC-704
Generated with Claude Code