Repository navigation
MGMT-24213: Add cluster template spec_defaults and server-side default merging - #474
Conversation
…t merging Add ClusterTemplateSpecDefaults to cluster template proto (public + private) with optional fields: pull_secret, ssh_public_key, release_image, network. Apply template defaults to ClusterSpec during cluster creation in the private server, following the same pattern as ComputeInstance spec_defaults: - User-provided values always take precedence - Network defaults merge field-by-field (pod_cidr, service_cidr) - Cloned to prevent shared state between template and spec Validate CIDR format when provided (net.ParseCIDR). Credentials (pull_secret, ssh_public_key) are not required at API time — the Ansible role falls back to a provider default Secret.
|
@tzvatot: This pull request references MGMT-24213 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 task 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 Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
WalkthroughThis PR introduces template-level default cluster specification values. It adds proto message types ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 39 minutes and 36 seconds.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: 1
🧹 Nitpick comments (1)
internal/utils/cluster_spec_defaults.go (1)
26-104: ⚡ Quick winDeduplicate the CIDR validation path.
The helper itself looks fine, but the server still has a second
net.ParseCIDRblock later invalidateAndTransformCluster. Routing that path throughValidateClusterSpecFieldstoo would keep the behavior and error text from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/utils/cluster_spec_defaults.go` around lines 26 - 104, The CIDR validation logic is duplicated: move the validation in validateAndTransformCluster to reuse ValidateClusterSpecFields/validateClusterNetwork so the behavior and error messages stay consistent; update validateAndTransformCluster to call ValidateClusterSpecFields(spec) (or validateClusterNetwork(spec.GetNetwork())) instead of running its own net.ParseCIDR checks, and remove the redundant net.ParseCIDR blocks to avoid drift while preserving existing error text from validateClusterNetwork.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/servers/private_clusters_server.go`:
- Around line 525-530: The update path currently skips network/CIDR validation,
so add the same spec defaults+validation used on create into the
ClustersUpdateRequest update flow: after applying template defaults
(template.GetSpecDefaults()) and merging the request into cluster.GetSpec(),
call utils.ValidateClusterSpecFields(cluster.GetSpec()) and return the error if
non-nil. Update the Update (or ClustersUpdateRequest handling) code path to
mirror the create logic that uses utils.ApplyClusterSpecDefaults and
utils.ValidateClusterSpecFields so invalid spec.network/CIDRs cannot be
persisted.
---
Nitpick comments:
In `@internal/utils/cluster_spec_defaults.go`:
- Around line 26-104: The CIDR validation logic is duplicated: move the
validation in validateAndTransformCluster to reuse
ValidateClusterSpecFields/validateClusterNetwork so the behavior and error
messages stay consistent; update validateAndTransformCluster to call
ValidateClusterSpecFields(spec) (or validateClusterNetwork(spec.GetNetwork()))
instead of running its own net.ParseCIDR checks, and remove the redundant
net.ParseCIDR blocks to avoid drift while preserving existing error text from
validateClusterNetwork.
🪄 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: 67923c1b-2584-45c5-b98e-b6d9902739a2
⛔ Files ignored due to path filters (4)
internal/api/osac/private/v1/cluster_template_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/private/v1/cluster_template_type_protoopaque.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_template_type.pb.gois excluded by!**/*.pb.gointernal/api/osac/public/v1/cluster_template_type_protoopaque.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (5)
internal/servers/private_clusters_server.gointernal/utils/cluster_spec_defaults.gointernal/utils/cluster_spec_defaults_test.goproto/private/osac/private/v1/cluster_template_type.protoproto/public/osac/public/v1/cluster_template_type.proto
| // Apply spec defaults from the template (user values take precedence): | ||
| utils.ApplyClusterSpecDefaults(cluster.GetSpec(), template.GetSpecDefaults()) | ||
|
|
||
| // Validate cluster spec fields (CIDR format, etc.) after defaults have been applied: | ||
| if err = utils.ValidateClusterSpecFields(cluster.GetSpec()); err != nil { | ||
| return err |
There was a problem hiding this comment.
Validate spec.network on update too.
This only protects the create path. If ClustersUpdateRequest can patch spec.network, invalid CIDRs can still be persisted because Update never calls utils.ValidateClusterSpecFields.
🔧 Suggested fix
func (s *PrivateClustersServer) Update(ctx context.Context,
request *privatev1.ClustersUpdateRequest) (response *privatev1.ClustersUpdateResponse, err error) {
err = s.validateNoDuplicateConditions(request.GetObject())
if err != nil {
return
}
err = s.validateTemplateImmutability(ctx, request)
if err != nil {
return
}
err = s.validateNodeSetsUpdate(ctx, request)
if err != nil {
return
}
+ if err = utils.ValidateClusterSpecFields(request.GetObject().GetSpec()); err != nil {
+ return
+ }
err = s.generic.Update(ctx, request, &response)
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Apply spec defaults from the template (user values take precedence): | |
| utils.ApplyClusterSpecDefaults(cluster.GetSpec(), template.GetSpecDefaults()) | |
| // Validate cluster spec fields (CIDR format, etc.) after defaults have been applied: | |
| if err = utils.ValidateClusterSpecFields(cluster.GetSpec()); err != nil { | |
| return err | |
| func (s *PrivateClustersServer) Update(ctx context.Context, | |
| request *privatev1.ClustersUpdateRequest) (response *privatev1.ClustersUpdateResponse, err error) { | |
| err = s.validateNoDuplicateConditions(request.GetObject()) | |
| if err != nil { | |
| return | |
| } | |
| err = s.validateTemplateImmutability(ctx, request) | |
| if err != nil { | |
| return | |
| } | |
| err = s.validateNodeSetsUpdate(ctx, request) | |
| if err != nil { | |
| return | |
| } | |
| if err = utils.ValidateClusterSpecFields(request.GetObject().GetSpec()); err != nil { | |
| return | |
| } | |
| err = s.generic.Update(ctx, request, &response) | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/servers/private_clusters_server.go` around lines 525 - 530, The
update path currently skips network/CIDR validation, so add the same spec
defaults+validation used on create into the ClustersUpdateRequest update flow:
after applying template defaults (template.GetSpecDefaults()) and merging the
request into cluster.GetSpec(), call
utils.ValidateClusterSpecFields(cluster.GetSpec()) and return the error if
non-nil. Update the Update (or ClustersUpdateRequest handling) code path to
mirror the create logic that uses utils.ApplyClusterSpecDefaults and
utils.ValidateClusterSpecFields so invalid spec.network/CIDRs cannot be
persisted.
- Rename shadowed 'net' variable to 'specNet' in mergeClusterNetworkDefaults - Remove pull_secret from public ClusterTemplateSpecDefaults (write-only field should not be exposed in template GET responses)
|
[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 |
3 participants
Summary
Add
spec_defaultsto cluster templates, aligning CaaS with the VMaaS pattern for server-side default resolution.ClusterTemplateSpecDefaultsmessage to cluster template proto (public + private) with optional fields:pull_secret,ssh_public_key,release_image,networkClusterSpecduring cluster creation in the private server (same pattern as ComputeInstance)net.ParseCIDR)pull_secret,ssh_public_key) are not required at API time — the Ansible role falls back to a provider default SecretJira
MGMT-24213
Related
spec_defaults(PR MGMT-23769: Provide up front validation and handling of template prov… #414)Test plan
ApplyClusterSpecDefaults(all defaults, user override, partial network merge, clone safety)ValidateClusterSpecFields(nil spec, valid CIDRs, invalid CIDRs)ginkgo run -r internal)buf lintpassesSummary by CodeRabbit