WIP: SPLAT-2615: Added support for dynamic AWS dedicated hosts #2650
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@vr4manta: This pull request references SPLAT-2615 which is a valid jira issue. 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. |
|
Hello @vr4manta! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughAdds AWS dedicated-host dynamic allocation support: introduces 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/L
|
@vr4manta: This pull request references SPLAT-2615 which is a valid jira issue. 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. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
machine/v1beta1/types_awsprovider.go (2)
458-470: Critical:DynamicHostis missing from theHostAffinityenum validation.The
HostAffinitytype at line 459-460 uses+kubebuilder:validation:Enum:=DedicatedHost;AnyAvailablebut does not includeDynamicHost. This means any attempt to setaffinity: DynamicHostwill fail validation at the API server level.🐛 Proposed fix to add DynamicHost to enum validation
// HostAffinity selects how an instance should be placed on AWS Dedicated Hosts. -// +kubebuilder:validation:Enum:=DedicatedHost;AnyAvailable +// +kubebuilder:validation:Enum:=DedicatedHost;AnyAvailable;DynamicHost type HostAffinity string
431-456: Missing validation rule:dynamicHostshould be required when affinity isDynamicHost.The existing XValidation rule at line 432 enforces that
dedicatedHostis required whenaffinity == 'DedicatedHost'. A similar rule should be added for theDynamicHostcase, and mutual exclusivity betweendedicatedHostanddynamicHostshould be enforced.🔧 Proposed fix to add validation rules
// HostPlacement is the type that will be used to configure the placement of AWS instances. // +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DedicatedHost' ? has(self.dedicatedHost) : true",message="dedicatedHost is required when affinity is DedicatedHost, and optional otherwise" +// +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DynamicHost' ? has(self.dynamicHost) : true",message="dynamicHost is required when affinity is DynamicHost" +// +kubebuilder:validation:XValidation:rule="!(has(self.dedicatedHost) && has(self.dynamicHost))",message="dedicatedHost and dynamicHost are mutually exclusive" // +union type HostPlacement struct {
🤖 Fix all issues with AI agents
In `@machine/v1beta1/zz_generated.swagger_doc_generated.go`:
- Line 140: The "affinity" description string in
zz_generated.swagger_doc_generated.go currently lists allowed values as
"AnyAvailable and DedicatedHost" but also documents DynamicHost behavior; update
the description for the "affinity" entry to include DynamicHost in the allowed
values list (e.g., "Allowed values are AnyAvailable, DedicatedHost, and
DynamicHost") so the documentation matches the subsequent paragraphs describing
`dynamicHost`; locate the "affinity" key in the generated swagger doc string and
edit the sentence accordingly.
In `@openapi/openapi.json`:
- Around line 24244-24264: The discriminator mapping and validation are
inconsistent: update the x-kubernetes-unions mapping so the "dynamicHost" field
maps to "DynamicHost" (not "DynamicHostAllocation") to match the affinity enum,
and add "DynamicHost" to the kubebuilder enum validation for the affinity field
(ensure the kubebuilder:validation:Enum on the affinity constant includes
DedicatedHost;DynamicHost;AnyAvailable); modify the entries for affinity,
dynamicHost, and dedicatedHost accordingly.
🧹 Nitpick comments (5)
openapi/openapi.json (5)
4582-4585: MissingmaxLengthconstraint for the name field.The description states the name "must not exceed 256 characters," but the schema lacks a
maxLength: 256constraint to enforce this at the API level. Without this constraint, the API will accept strings exceeding the documented limit.Suggested fix
"name": { "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", - "type": "string" + "type": "string", + "maxLength": 256 }
5875-5886: MissingmaxItemsconstraint.The description specifies "must not contain more than 500 entries," but no
maxItems: 500constraint is defined.Suggested fix
"conditionalUpdateRisks": { "description": "conditionalUpdateRisks contains the list of risks associated with conditionalUpdates. When performing a conditional update, all its associated risks will be compared with the set of accepted risks in the spec.desiredUpdate.acceptRisks field. If all risks for a conditional update are included in the spec.desiredUpdate.acceptRisks set, the conditional update can proceed, otherwise it is blocked. The risk names in the list must be unique. conditionalUpdateRisks must not contain more than 500 entries.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk" }, + "maxItems": 500, "x-kubernetes-list-map-keys": [ "name" ], "x-kubernetes-list-type": "map" },
6094-6102: Missing validation constraints forriskNames.The description specifies entry max length of 256 characters and array max of 500 entries, but neither
maxLengthnormaxItemsconstraints are defined.Suggested fix
"riskNames": { "description": "riskNames represents the set of the names of conditionalUpdateRisks that are relevant to this update for some clusters. The Applies condition of each conditionalUpdateRisks entry declares if that risk applies to this cluster. A conditional update is accepted only if each of its risks either does not apply to the cluster or is considered acceptable by the cluster administrator. The latter means that the risk names are included in value of the spec.desiredUpdate.acceptRisks field. Entries must be unique and must not exceed 256 characters. riskNames must not contain more than 500 entries.", "type": "array", "items": { "type": "string", - "default": "" + "default": "", + "maxLength": 256 }, + "maxItems": 500, "x-kubernetes-list-type": "set" },
6129-6140: MissingmaxItemsconstraint.The description states "must not contain more than one entry," but no
maxItems: 1constraint is defined.Suggested fix
"conditions": { "description": "conditions represents the observations of the conditional update risk's current status. Known types are: * Applies, for whether the risk applies to the current cluster. The condition's types in the list must be unique. conditions must not contain more than one entry.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" }, + "maxItems": 1, "x-kubernetes-list-map-keys": [ "type" ], "x-kubernetes-list-type": "map" },
11639-11650: MissingmaxItemsconstraint.The description specifies "must not contain more than 1000 entries," but no
maxItems: 1000constraint is defined.Suggested fix
"acceptRisks": { "description": "acceptRisks is an optional set of names of conditional update risks that are considered acceptable. A conditional update is performed only if all of its risks are acceptable. This list may contain entries that apply to current, previous or future updates. The entries therefore may not map directly to a risk in .status.conditionalUpdateRisks. acceptRisks must not contain more than 1000 entries. Entries in this list must be unique.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/com.github.openshift.api.config.v1.AcceptRisk" }, + "maxItems": 1000, "x-kubernetes-list-map-keys": [ "name" ], "x-kubernetes-list-type": "map" },
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.gomachine/v1beta1/zz_generated.deepcopy.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🧰 Additional context used
🧬 Code graph analysis (3)
machine/v1beta1/zz_generated.deepcopy.go (2)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
machine/v1beta1/types_awsprovider.go (1)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)
🔇 Additional comments (15)
machine/v1beta1/types_awsprovider.go (2)
486-492: LGTM onDynamicHostAllocationSpecstruct definition.The struct is well-defined with appropriate documentation and follows established patterns for tag specifications in the codebase.
450-455: Verify: Feature gate annotation may be needed for consistency.The
DynamicHostAllocationfield doesn't have a feature gate annotation, while the parentHostfield (line 318-319) is gated behindAWSDedicatedHosts. Since the new field is withinHostPlacement, it may inherit the gating, but verify this is intentional and aligns with the team's conventions.machine/v1beta1/zz_generated.deepcopy.go (2)
560-581: LGTM on generated DeepCopy methods forDynamicHostAllocationSpec.The auto-generated deepcopy logic correctly handles the
Tagsmap with proper nil checking and element-by-element copying, consistent with other map-based fields in the codebase.
963-967: LGTM onHostPlacement.DeepCopyIntoextension.The generated code correctly uses
DeepCopyIntoforDynamicHostAllocationsince it contains a map, unlikeDedicatedHostwhich uses direct assignment for its primitive field.openapi/generated_openapi/zz_generated.openapi.go (5)
811-817: LGTM!Schema registration for
DynamicHostAllocationSpecfollows the existing alphabetical ordering and naming convention.
41233-41260: LGTM!The
DynamicHostAllocationSpecschema is well-structured with thetagsproperty correctly defined as a map of strings. The description clearly states the one-host-per-machine allocation behavior.
42036-42041: LGTM!The updated
affinitydescription comprehensively documents the newDynamicHostoption, clearly explaining the allocation behavior and the requirement for thedynamicHostfield.
42049-42054: LGTM!The
dynamicHostproperty is correctly defined with a reference toDynamicHostAllocationSpec. The description appropriately notes the mutual exclusivity withdedicatedHost.
42062-42074: LGTM!The discriminator mapping correctly associates
dynamicHostwithDynamicHostAllocation(matching the Go field name pattern), and dependencies properly includeDynamicHostAllocationSpec.machine/v1beta1/zz_generated.swagger_doc_generated.go (2)
104-111: LGTM!The swagger documentation for
DynamicHostAllocationSpecis well-structured and accurately reflects the type definition fromtypes_awsprovider.go. The descriptions are clear and follow the established pattern in this file.
142-142: LGTM!The
dynamicHostdocumentation clearly describes its purpose and explicitly notes the mutual exclusivity withdedicatedHost, which helps prevent misconfiguration.openapi/openapi.json (4)
6351-6351: TLS profile documentation updates look good.The updated descriptions now correctly reference the Mozilla Server Side TLS configuration guidelines and provide clearer cipher suite examples for each profile type.
Also applies to: 8537-8537, 8815-8815, 9745-9745, 11323-11323, 11334-11350
11684-11686: LGTM!The description text is accurate and grammatically correct.
28978-28978: LGTM!The capability descriptions are consistently updated to include
GuidedTour.Also applies to: 29627-29627
23763-23776: No action required. The schema intentionally delegates tag validation to the AWS provider rather than enforcing constraints at the schema level. This is consistent with the established pattern throughout the codebase, where tag fields (of typeadditionalPropertieswith string values) have no length or count constraints. AWS tag limits (50 tags max, 128-character key limit, 256-character value limit) will be enforced by the AWS provider at runtime, which is the appropriate place for cloud-provider-specific validation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
machine/v1beta1/types_awsprovider.go (1)
431-456: Missing validation:DynamicHostaffinity should requiredynamicHostfield.The existing
XValidationrule at line 432 only enforces thatDedicatedHostaffinity requires thededicatedHostfield. However, the comment at line 438 states that when affinity isDynamicHost, thedynamicHostfield "must be set." This requirement is not enforced by any validation rule.Additionally, consider enforcing mutual exclusivity between
dedicatedHostanddynamicHostfields.🛠️ Suggested validation update
// HostPlacement is the type that will be used to configure the placement of AWS instances. // +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DedicatedHost' ? has(self.dedicatedHost) : true",message="dedicatedHost is required when affinity is DedicatedHost, and optional otherwise" +// +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DynamicHost' ? has(self.dynamicHost) : true",message="dynamicHost is required when affinity is DynamicHost" +// +kubebuilder:validation:XValidation:rule="!(has(self.dedicatedHost) && has(self.dynamicHost))",message="dedicatedHost and dynamicHost are mutually exclusive" // +union type HostPlacement struct {
♻️ Duplicate comments (2)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
140-140: Update "Allowed values" to include DynamicHost.The description states "Allowed values are AnyAvailable and DedicatedHost" but then proceeds to describe behavior for
DynamicHost. The allowed values list should include all three options for consistency.openapi/openapi.json (1)
24256-24264: Discriminator value inconsistency:dynamicHostmaps to"DynamicHostAllocation"but should be"DynamicHost".The affinity field description (line 24244) indicates the enum value is
DynamicHost, notDynamicHostAllocation. Compare withdedicatedHostwhich correctly maps to"DedicatedHost". The discriminator value should match the affinity enum value.
🧹 Nitpick comments (1)
machine/v1beta1/types_awsprovider.go (1)
485-492: Consider tag representation consistency across the API.The
Tagsfield inDynamicHostAllocationSpecusesmap[string]string, while instance tags inAWSMachineProviderConfig(line 32) use[]TagSpecificationwith separateNameandValuefields. This inconsistency in tag representation across the API could be confusing for users.Additionally, AWS enforces tag constraints: keys must be 1–128 characters and values 0–256 characters, and tag keys cannot start with the reserved prefix
aws:. Consider documenting these constraints or adding validation if applicable to this new dedicated host allocation feature.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.gomachine/v1beta1/zz_generated.deepcopy.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🧰 Additional context used
🧬 Code graph analysis (3)
machine/v1beta1/types_awsprovider.go (1)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)
machine/v1beta1/zz_generated.deepcopy.go (2)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(488-492)machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
🔇 Additional comments (19)
machine/v1beta1/zz_generated.deepcopy.go (2)
560-581: LGTM - Generated deepcopy functions forDynamicHostAllocationSpecfollow correct patterns.The map deep copy logic for
Tagsmatches the established pattern used elsewhere in this file (e.g.,AzureMachineProviderSpec.Tags,GCPDisk.Labels).
963-967: LGTM - Correctly usesDeepCopyIntofor the new field.The code properly calls
DeepCopyIntoinstead of shallow assignment (**out = **in) becauseDynamicHostAllocationSpeccontains aTagsmap that requires deep copying. This is consistent with how other pointer fields containing reference types are handled in this file.openapi/generated_openapi/zz_generated.openapi.go (5)
811-817: LGTM!The new
DynamicHostAllocationSpectype is correctly registered in the OpenAPI definitions map, following the existing alphabetical ordering convention.
41233-41260: LGTM!The
DynamicHostAllocationSpecschema is well-defined:
- Clear description explaining the purpose (dynamic dedicated host allocation with exactly one host per machine)
- The
tagsproperty correctly defines amap[string]stringusing theAdditionalPropertiespattern- The
tagsfield being optional is appropriate for this use case
42035-42041: LGTM!The affinity field description is well-updated to document the new
DynamicHostoption. The behavior is clearly explained: automatic dedicated host allocation with restart affinity to the same host.
42046-42057: LGTM!The
dynamicHostproperty is correctly added to theHostPlacementschema:
- References the new
DynamicHostAllocationSpectype- Description clearly documents mutual exclusivity with
dedicatedHost- Appropriately optional since it's only required when affinity is
DynamicHost
42062-42075: LGTM!The discriminator mapping and dependencies are correctly updated:
dynamicHostfield maps toDynamicHostAllocationdiscriminator value, aligning with the expected affinity enum value- Dependencies array properly includes
DynamicHostAllocationSpecmachine/v1beta1/zz_generated.swagger_doc_generated.go (2)
104-111: LGTM!The new
DynamicHostAllocationSpecswagger documentation correctly describes the type and itstagsfield, aligning with the struct definition intypes_awsprovider.go.
142-142: LGTM!The
dynamicHostdocumentation correctly describes mutual exclusivity withdedicatedHostand the automatic allocation behavior.machine/v1beta1/types_awsprovider.go (1)
458-471: LGTM!The
HostAffinityenum validation is correctly updated to includeDynamicHost, and the newHostAffinityDynamicHostconstant follows the established naming pattern with appropriate documentation.openapi/openapi.json (9)
4575-4587: LGTM - AcceptRisk type definition looks correct.The type structure is appropriate. Note that while the description mentions the name must be non-empty and ≤256 characters, the JSON schema itself doesn't include
minLength/maxLengthconstraints. This is typical for generated OpenAPI specs where validation is enforced via kubebuilder tags at the Go type level.
5875-5886: LGTM - conditionalUpdateRisks field properly structured.The map-type list with
nameas the key ensures uniqueness. The max 500 entries constraint is documented appropriately.
6094-6102: LGTM - riskNames field correctly uses set semantics.The
x-kubernetes-list-type: setensures entry uniqueness as required.
6129-6140: LGTM - conditions field follows standard Kubernetes pattern.Map-type list keyed by
typeis the correct pattern for condition arrays.
6351-6351: LGTM - TLS profile documentation improvements.The description updates provide consistent formatting and clearer examples for TLS configuration.
Also applies to: 8537-8537, 8815-8815, 9745-9745, 11323-11323, 11334-11350
11639-11650: LGTM - acceptRisks field properly integrated into Update type.The map-type list with
namekey ensures administrators can manage accepted risks with uniqueness guarantees.
11685-11686: LGTM - acceptedRisks description clarification.
23763-23776: LGTM - DynamicHostAllocationSpec type definition is well-structured.The
tagsproperty correctly usesadditionalPropertiesfor key-value string pairs, which aligns with AWS resource tagging patterns.
28978-28979: LGTM - Console capability documentation updates.Also applies to: 29627-29627
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
/assign @everettraven |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@machine/v1beta1/types_awsprovider.go`:
- Line 433: Fix the typo in the kubebuilder XValidation message: update the
validation annotation for the fields affinity and dynamicHost (the line
containing +kubebuilder:validation:XValidation) so the message reads
"dynamicHost is only allowed when affinity is DynamicHost, and forbidden
otherwise" (correcting "DyanimcHost" to "DynamicHost") to ensure user-facing
validation errors are spelled correctly.
♻️ Duplicate comments (1)
openapi/openapi.json (1)
24244-24263: Re-verify dynamicHost discriminator value matches the affinity enum.
If the affinity enum value isDynamicHost, the union mapping should use that exact discriminator string; otherwise, validation can drift. Please confirm the enum value in the Go types and re-generate if needed.Run this read-only check to confirm the affinity enum strings and union tags used in the Go types:
#!/bin/bash # Verify HostAffinity enum values and HostPlacement union tags in Go types. rg -n "type HostAffinity|HostAffinityDynamicHost|kubebuilder:validation:Enum" --type=go -C3 rg -n "type HostPlacement|affinity|dynamicHost|dedicatedHost|x-kubernetes-unions" --type=go -C3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.gomachine/v1beta1/zz_generated.deepcopy.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🚧 Files skipped from review as they are similar to previous changes (2)
- openapi/generated_openapi/zz_generated.openapi.go
- machine/v1beta1/zz_generated.swagger_doc_generated.go
🔇 Additional comments (20)
machine/v1beta1/types_awsprovider.go (3)
435-457: LGTM!The
HostPlacementunion is well-structured with proper discriminator and member tags. The documentation clearly describes the behavior for each affinity option.
460-472: LGTM!The
HostAffinityenum extension follows the established naming conventions, and the new constant is properly documented.
486-493: LGTM!The struct is appropriately minimal and extensible. The
Tagsfield usingmap[string]stringaligns with AWS conventions.Consider whether validation for AWS tag limits (key: 128 chars, value: 256 chars) would be beneficial to provide earlier feedback to users, though this could also be handled at the controller level.
machine/v1beta1/zz_generated.deepcopy.go (2)
560-581: LGTM!Auto-generated deepcopy implementation correctly handles the
Tagsmap deep copy, consistent with similar patterns elsewhere in this file.
963-967: LGTM!The
HostPlacement.DeepCopyIntoextension correctly handles the newDynamicHostAllocationfield by allocating a new struct and callingDeepCopyIntoto properly copy the nestedTagsmap.openapi/openapi.json (15)
4575-4587: AcceptRisk schema addition looks consistent.
5875-5886: conditionalUpdateRisks schema looks good.
6094-6102: riskNames schema looks consistent.
6129-6140: conditions list schema looks good.
6351-6353: TLS min version description tweak is fine.
8537-8539: IntermediateTLSProfile description update is fine.
8815-8817: ModernTLSProfile description update is fine.
9745-9747: OldTLSProfile description update is fine.
11323-11325: TLS min version description tweak is fine.
11334-11350: TLS profile descriptions updated consistently.
11639-11650: acceptRisks map schema looks correct.
11685-11686: acceptedRisks description update is fine.
23763-23775: DynamicHostAllocationSpec schema looks good.
28978-28980: Capability name description update is fine.
29627-29628: Capabilities list description update is fine.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
machine/v1beta1/types_awsprovider.go (1)
431-456: Missing validation rule for mutual exclusivity betweendedicatedHostanddynamicHost.The documentation at line 452-453 states that
dynamicHostis mutually exclusive withdedicatedHost, but no XValidation rule enforces this. The current rules allow both fields to be set simultaneously whenaffinityisDynamicHost.Consider adding a validation rule:
📝 Suggested fix
// +kubebuilder:validation:XValidation:rule="has(self.affinity) && self.affinity == 'DedicatedHost' ? has(self.dedicatedHost) : true",message="dedicatedHost is required when affinity is DedicatedHost, and optional otherwise" // +kubebuilder:validation:XValidation:rule="has(self.affinity) && has(self.dynamicHost) ? self.affinity == 'DynamicHost' : true",message="dynamicHost is only allowed when affinity is DynamicHost, and forbidden otherwise" +// +kubebuilder:validation:XValidation:rule="!(has(self.dedicatedHost) && has(self.dynamicHost))",message="dedicatedHost and dynamicHost are mutually exclusive" // +union type HostPlacement struct {
♻️ Duplicate comments (2)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
140-142: "Allowed values" list is missing DynamicHost.Line 140 states "Allowed values are AnyAvailable and DedicatedHost" but then describes behavior for
DynamicHost. Update to "Allowed values are AnyAvailable, DedicatedHost and DynamicHost" for consistency.📝 Suggested fix
- "affinity": "affinity specifies the affinity setting for the instance. Allowed values are AnyAvailable and DedicatedHost. When Affinity is set to DedicatedHost, an instance started onto a specific host always restarts on the same host if stopped. In this scenario, the `dedicatedHost` field must be set. When Affinity is set to DynamicHost, a dedicated host will be allocated and assigned to the instance and the instance will always restart on this host if stopped. In this scenario, the `dynamicHost` field may be set to provide additional settings. When Affinity is set to AnyAvailable, and you stop and restart the instance, it can be restarted on any available host. When Affinity is set to AnyAvailable and the `dedicatedHost` field is defined, it runs on specified Dedicated Host, but may move if stopped.", + "affinity": "affinity specifies the affinity setting for the instance. Allowed values are AnyAvailable, DedicatedHost and DynamicHost. When Affinity is set to DedicatedHost, an instance started onto a specific host always restarts on the same host if stopped. In this scenario, the `dedicatedHost` field must be set. When Affinity is set to DynamicHost, a dedicated host will be allocated and assigned to the instance and the instance will always restart on this host if stopped. In this scenario, the `dynamicHost` field may be set to provide additional settings. When Affinity is set to AnyAvailable, and you stop and restart the instance, it can be restarted on any available host. When Affinity is set to AnyAvailable and the `dedicatedHost` field is defined, it runs on specified Dedicated Host, but may move if stopped.",openapi/openapi.json (1)
24244-24262: Verify affinity enum + discriminator alignment (DynamicHost).
The affinity description in Line 24244 mentions DynamicHost but the “allowed values” list doesn’t include it, and the discriminator mapsdynamicHostto"DynamicHostAllocation". Please confirm the actualHostAffinityTypeenum values and update the discriminator/description if needed so they match exactly.Run this to verify enum values and kubebuilder validation in the Go types:
#!/bin/bash # Inspect HostAffinityType enum values and validation rg -n "type HostAffinityType|HostAffinityDynamicHost|HostAffinityDedicatedHost|HostAffinityAnyAvailable|kubebuilder:validation:Enum" --type=go -C2 machine/v1beta1/types_awsprovider.go # Show HostPlacement union mapping in openapi.json for comparison rg -n "\"affinity\"|\"dedicatedHost\"|\"dynamicHost\"|\"fields-to-discriminateBy\"" -C2 openapi/openapi.json
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (5)
machine/v1beta1/types_awsprovider.gomachine/v1beta1/zz_generated.deepcopy.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.json
🧰 Additional context used
🧬 Code graph analysis (3)
machine/v1beta1/zz_generated.deepcopy.go (2)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(489-493)machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
machine/v1beta1/types_awsprovider.go (1)
DynamicHostAllocationSpec(489-493)
machine/v1beta1/types_awsprovider.go (1)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
DynamicHostAllocationSpec(109-111)
🔇 Additional comments (25)
openapi/generated_openapi/zz_generated.openapi.go (5)
811-817: LGTM!The new
DynamicHostAllocationSpecschema mapping is correctly added in alphabetical order, following the existing pattern.
41233-41260: LGTM!The
DynamicHostAllocationSpecschema is correctly defined with an optionaltagsproperty asmap[string]string. The schema aligns with the PR objectives of allowing administrators to provide tags for dynamically allocated hosts.
42035-42041: LGTM!The updated
affinitydescription comprehensively documents the newDynamicHostoption and its relationship with thedynamicHostfield, maintaining consistency with the existing documentation pattern forDedicatedHost.
42049-42057: LGTM!The
dynamicHostproperty is correctly added with a reference toDynamicHostAllocationSpec. The description properly documents the mutual exclusivity withdedicatedHost, and the enforcement is handled via XValidation rules in the source types.
42062-42075: LGTM!The discriminator mapping correctly associates the
dynamicHostfield with theDynamicHostAllocationdiscriminator value, following the existing pattern used fordedicatedHost. The dependencies are properly updated to include the newDynamicHostAllocationSpecreference.machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
104-111: LGTM - Documentation for DynamicHostAllocationSpec is clear and accurate.The swagger documentation correctly describes the new
DynamicHostAllocationSpectype and itstagsfield.machine/v1beta1/types_awsprovider.go (2)
459-471: LGTM - HostAffinity enum and constants are correctly defined.The
DynamicHostvalue is properly added to the enum validation and the constantHostAffinityDynamicHostis well-documented.
486-493: LGTM - DynamicHostAllocationSpec type definition looks correct.The struct is minimal and follows the pattern established for other AWS types. The
Tagsfield is optional and usesmap[string]stringwhich aligns with AWS tag conventions.Consider whether tag key/value validation (length limits, character restrictions) should be added to align with AWS constraints, or if this is intentionally deferred to AWS API validation.
machine/v1beta1/zz_generated.deepcopy.go (2)
560-581: LGTM - Auto-generated deep copy methods are correct.The
DeepCopyIntocorrectly handles theTagsmap by creating a new map and copying key-value pairs. TheDeepCopymethod follows the standard nil-check pattern.
963-967: LGTM - HostPlacement deep copy correctly handles new DynamicHostAllocation field.The generated code properly allocates a new
DynamicHostAllocationSpecand invokesDeepCopyInto, which is necessary because the struct contains a map that requires deep copying.openapi/openapi.json (15)
4575-4587: LGTM — AcceptRisk schema addition is clear.
5875-5886: Looks good — conditionalUpdateRisks list-map wiring is consistent.
6094-6102: LGTM — riskNames set schema is fine.
6129-6140: LGTM — conditions map is consistent with k8s Condition usage.
6351-6351: Doc-only change; no concerns.
8537-8537: Doc-only change; no concerns.
8815-8815: Doc-only change; no concerns.
9745-9745: Doc-only change; no concerns.
11323-11323: Doc-only change; no concerns.
11334-11350: Doc-only change; no concerns.
11639-11650: LGTM — acceptRisks list-map schema is consistent.
11685-11686: Doc-only change; no concerns.
23763-23775: LGTM — DynamicHostAllocationSpec schema is clear.
28978-28979: Doc-only change; no concerns.
29627-29627: Doc-only change; no concerns.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
After some reviews, gonna make some small changes to not add a new affinity to keep in line with AWS. |
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 (1)
machine/v1beta1/types_awsprovider.go (1)
433-444: HostAffinity enum missingDynamicHostvalue.The validation rule on line 433 checks for
self.affinity == 'DynamicHost'and the doc comment on line 439 describes DynamicHost behavior, but theHostAffinityenum (lines 453-463) only definesAnyAvailableandDedicatedHost. TheDynamicHostvalue is missing from the enum, which will cause validation failures when users try to setaffinity: DynamicHost.🐛 Proposed fix - Add DynamicHost to HostAffinity enum
// HostAffinity selects how an instance should be placed on AWS Dedicated Hosts. -// +kubebuilder:validation:Enum:=DedicatedHost;AnyAvailable +// +kubebuilder:validation:Enum:=DedicatedHost;AnyAvailable;DynamicHost type HostAffinity string const ( // HostAffinityAnyAvailable lets the platform select any available dedicated host. HostAffinityAnyAvailable HostAffinity = "AnyAvailable" // HostAffinityDedicatedHost requires specifying a particular host via dedicatedHost.host.hostID. HostAffinityDedicatedHost HostAffinity = "DedicatedHost" + + // HostAffinityDynamicHost enables dynamic allocation of a dedicated host. + HostAffinityDynamicHost HostAffinity = "DynamicHost" )
🤖 Fix all issues with AI agents
In `@machine/v1beta1/types_awsprovider.go`:
- Around line 475-477: Update the XValidation message for the reverse rule that
checks "has(self.id) ? self.allocationStrategy == 'Provided' : true" so it
correctly describes the reverse direction (i.e., that id is forbidden when
allocationStrategy is not Provided); locate the annotation containing that rule
near the other XValidation tags and change its message to something like "id is
forbidden when allocationStrategy is not Provided" while keeping the rule itself
unchanged.
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 42051-42054: Update the source comment for the affinity field in
machine/v1beta1/types_awsprovider.go to list all three allowed enum values
(AnyAvailable, DedicatedHost, and DynamicHost) so it matches the validation and
OpenAPI generation; locate the comment attached to the Affinity field (e.g., the
affinity struct tag or the Affinity field docstring) and change the sentence
that currently says "Allowed values are AnyAvailable and DedicatedHost" to
include DynamicHost as well, then re-run the OpenAPI generation to refresh
zz_generated.openapi.go.
- Around line 41179-41205: Update the comment for the DedicatedHost struct field
dynamicHostAllocation in machine/v1beta1/types_awsprovider.go to accurately
describe that dynamicHostAllocation is mutually exclusive with specifying id and
that allocationStrategy controls whether a host is provided via id or
dynamically allocated (rather than referencing a nonexistent dynamicHost or
dedicatedHost terms); mention that dynamicHostAllocation always requests exactly
one host and that id must be a host identifier starting with "h-" of length 10
or 19 when allocationStrategy indicates admin-provided host, then regenerate the
OpenAPI (zz_generated.openapi.go) so the generated SchemaProps.Description for
dynamicHostAllocation, allocationStrategy, and id reflect these constraints.
In `@openapi/openapi.json`:
- Around line 23742-23745: Update the description for the dynamicHostAllocation
field in the DedicatedHost type: replace the incorrect "mutually exclusive with
dedicatedHost" text with "mutually exclusive with id" to reflect the allocation
strategy (Dynamic vs Provided). Locate the dynamicHostAllocation description and
update it to mention mutual exclusivity with the id field (used by the Provided
strategy) so the description accurately describes the relationship between
dynamicHostAllocation and id.
- Around line 23737-23741: Update the OpenAPI schema for the allocationStrategy
field to enforce the valid Go enum values: modify the "allocationStrategy"
property (in openapi.json) to include "enum": ["Provided", "Dynamic"] and remove
or replace the invalid "default": "" (either remove default entirely or set it
to a valid enum value); keep the existing description and "type": "string" and
ensure the property name allocationStrategy is updated accordingly.
♻️ Duplicate comments (1)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
140-144: "Allowed values" list is inconsistent with documented behavior.The description states "Allowed values are AnyAvailable and DedicatedHost" but then documents
DynamicHostbehavior. The allowed values list should includeDynamicHostfor consistency with the subsequent documentation.📝 Suggested fix
- "affinity": "affinity specifies the affinity setting for the instance. Allowed values are AnyAvailable and DedicatedHost. When Affinity is set to DedicatedHost, an instance started onto a specific host always restarts on the same host if stopped. In this scenario, the `dedicatedHost` field must be set. When Affinity is set to DynamicHost, a dedicated host will be allocated and assigned to the instance and the instance will always restart on this host if stopped. In this scenario, the `dynamicHost` field may be set to provide additional settings. When Affinity is set to AnyAvailable, and you stop and restart the instance, it can be restarted on any available host. When Affinity is set to AnyAvailable and the `dedicatedHost` field is defined, it runs on specified Dedicated Host, but may move if stopped.", + "affinity": "affinity specifies the affinity setting for the instance. Allowed values are AnyAvailable, DedicatedHost, and DynamicHost. When Affinity is set to DedicatedHost, an instance started onto a specific host always restarts on the same host if stopped. In this scenario, the `dedicatedHost` field must be set. When Affinity is set to DynamicHost, a dedicated host will be allocated and assigned to the instance and the instance will always restart on this host if stopped. In this scenario, the `dynamicHost` field may be set to provide additional settings. When Affinity is set to AnyAvailable, and you stop and restart the instance, it can be restarted on any available host. When Affinity is set to AnyAvailable and the `dedicatedHost` field is defined, it runs on specified Dedicated Host, but may move if stopped.",
🧹 Nitpick comments (4)
openapi/openapi.json (4)
4575-4587: Missing schema validations fornamefield constraints.The description states
name"must be a non-empty string and must not exceed 256 characters", but the schema lacksminLength: 1andmaxLength: 256constraints to enforce this at the OpenAPI level.🔧 Suggested schema fix
"name": { "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", - "type": "string" + "type": "string", + "minLength": 1, + "maxLength": 256 }
5875-5886: Consider addingmaxItems: 500to match documented constraint.The description states "conditionalUpdateRisks must not contain more than 500 entries" but the schema lacks a
maxItemsconstraint.
6129-6140: Consider addingmaxItems: 1to enforce single-condition constraint.The description states "conditions must not contain more than one entry" but lacks schema enforcement. If this is an invariant, adding
maxItems: 1would catch violations at admission.
11639-11650: AddmaxItems: 1000to enforce documented limit on user input.Since
acceptRisksis in the spec (user-provided), the documented "must not contain more than 1000 entries" limit should be enforced at the schema level to prevent unbounded input.
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@machine/v1beta1/types_awsprovider.go`:
- Around line 476-505: The AllocationStrategy field in DedicatedHost must get a
default of "Provided" to preserve backward compatibility with manifests that
only set id; add a kubebuilder default marker (set default to Provided) on the
AllocationStrategy field (symbol AllocationStrategy in type DedicatedHost) so
the XValidation rules continue to allow legacy manifests that omit
allocationStrategy when ID is present, and ensure the struct tag/comment remains
consistent with the new default.
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 41179-41185: The allocationStrategy field's Description in the
generated OpenAPI contains a run-on sentence; update the source comment that
generates this description (the comment/documentation for the allocationStrategy
property used to build SchemaProps.Description) to insert a period between
"Dynamic" and the following "When" so the sentence reads "...Dynamic. When
AllocationStrategy..."; after updating the source comment, regenerate the
OpenAPI (zz_generated.openapi.go) to propagate the corrected description.
In `@openapi/openapi.json`:
- Around line 5875-5886: The schema for the array property
conditionalUpdateRisks doesn't enforce the documented 500-entry cap; update the
array definition for conditionalUpdateRisks (which references
com.github.openshift.api.config.v1.ConditionalUpdateRisk) to include "maxItems":
500 so the OpenAPI schema enforces the limit described in the description.
- Around line 4575-4585: The OpenAPI schema for
com.github.openshift.api.config.v1.AcceptRisk does not enforce the described
length constraints on AcceptRisk.name; update the properties for "name" in the
AcceptRisk schema to include minLength: 1 and maxLength: 256 so the schema
enforces non-empty and ≤256 character validation for AcceptRisk.name.
- Around line 6094-6102: The schema for the riskNames property currently
documents but does not enforce max entries and per-entry length; update the JSON
schema for "riskNames" (the array property named riskNames) to include
"maxItems": 500 at the array level and add "maxLength": 256 inside the "items"
object for the string entries (the items schema used by riskNames) so entries
are limited to 256 characters and the list to 500 entries, preserving the
existing x-kubernetes-list-type and other fields.
- Around line 11639-11650: The acceptRisks array schema lacks enforcement of the
documented 1000-entry cap—add "maxItems": 1000 to the acceptRisks array
definition (the object with "type": "array" and "items":
{"$ref":"#/definitions/com.github.openshift.api.config.v1.AcceptRisk"}) so the
OpenAPI schema enforces the documented limit; you may also ensure uniqueness
semantics remain consistent with "x-kubernetes-list-type": "map" (unique by
"name").
- Around line 6129-6140: The OpenAPI schema for the "conditions" property
currently documents "no more than one entry" but doesn't enforce it; update the
"conditions" array schema (the "conditions" definition that references
io.k8s.apimachinery.pkg.apis.meta.v1.Condition) to include "maxItems: 1" so the
validator enforces the single-entry constraint, keeping the existing "type":
"array", "items": { "$ref": ... }, and the x-kubernetes-list-* annotations
unchanged.
♻️ Duplicate comments (1)
openapi/openapi.json (1)
23737-23744: Add enum validation forallocationStrategyand remove invalid default.Line 23737 defines
allocationStrategyas a free-form string with default"", but the Go enum isProvided/Dynamic. Please align OpenAPI with the enum and drop the invalid default. (This is the same concern raised in prior review.)✅ Suggested schema fix
"allocationStrategy": { "description": "allocationStrategy specifies if the dedicated host will be provided by the admin through the id field or if the host will be dynamically allocated. Valid values are Provided and Dynamic When AllocationStrategy is set to Provided, an ID of the dedicated host to assign must be provided. When AllocationStrategy is set to Dynamic, a dedicated host will be allocated and used to assign instances. When AllocationStrategy is set to Dynamic, and DynamicHostAllocation is provided, a dedicated host will be allocated and the tags in DynamicHostAllocation will be assigned to that host.", "type": "string", - "default": "" + "enum": [ + "Provided", + "Dynamic" + ] },
| "com.github.openshift.api.config.v1.AcceptRisk": { | ||
| "description": "AcceptRisk represents a risk that is considered acceptable.", | ||
| "type": "object", | ||
| "required": [ | ||
| "name" | ||
| ], | ||
| "properties": { | ||
| "name": { | ||
| "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", | ||
| "type": "string" | ||
| } |
There was a problem hiding this comment.
Add length validation for AcceptRisk.name.
Line 4582 says non-empty and ≤256 chars, but the schema doesn’t enforce it. Consider adding min/max length to avoid invalid values being accepted.
✅ Suggested schema fix
"name": {
"description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.",
- "type": "string"
+ "type": "string",
+ "minLength": 1,
+ "maxLength": 256
}🤖 Prompt for AI Agents
In `@openapi/openapi.json` around lines 4575 - 4585, The OpenAPI schema for
com.github.openshift.api.config.v1.AcceptRisk does not enforce the described
length constraints on AcceptRisk.name; update the properties for "name" in the
AcceptRisk schema to include minLength: 1 and maxLength: 256 so the schema
enforces non-empty and ≤256 character validation for AcceptRisk.name.
vr4manta
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
machine/v1beta1/types_awsprovider.go (2)
436-443: Doc comment foraffinitydoes not documentDynamicHostbehavior.The doc comment states "Allowed values are AnyAvailable and DedicatedHost" but line 433's validation rule suggests
DynamicHostis also an option. IfDynamicHostis intended to be a valid affinity value, update this documentation. If not, remove the XValidation rule referencing it.
431-450: Remove the XValidation rule at line 433 that references non-existentdynamicHostfield and invalidDynamicHostaffinity value.Line 433 contains an XValidation rule that checks for a
dynamicHostfield and validates thatself.affinity == 'DynamicHost'. However:
- The
HostPlacementstruct (lines 435-450) does not have adynamicHostfield—it only hasAffinityandDedicatedHost.- The
HostAffinityenum (lines 453-462) does not defineDynamicHostas a valid value—onlyAnyAvailableandDedicatedHostare allowed.Since your PR comment indicates you're avoiding adding a new affinity value to remain consistent with AWS, this XValidation rule should be removed entirely. If dynamic host allocation is needed, it should be handled through the
DedicatedHoststruct'sAllocationStrategyfield (which already supports aDynamicvalue) rather than introducing a new affinity type.machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
140-144: Update affinity description and resolve DynamicHost inconsistency.The
affinitydescription states "Allowed values are AnyAvailable and DedicatedHost", but the XValidation rule at line 433 referencesDynamicHostas a valid affinity value. However, theHostAffinityenum only definesDedicatedHostandAnyAvailable—DynamicHostis not defined in the enum. Either addDynamicHostto theHostAffinityenum and update the swagger documentation, or remove theDynamicHostreference from the validation rule if it's not intended to be a valid value.
♻️ Duplicate comments (5)
openapi/openapi.json (5)
4582-4585: Enforce AcceptRisk.name length constraints in schema.The description states non-empty and ≤256 chars, but the schema doesn’t enforce it.
✅ Suggested schema fix
"name": { "description": "name is the name of the acceptable risk. It must be a non-empty string and must not exceed 256 characters.", - "type": "string" + "type": "string", + "minLength": 1, + "maxLength": 256 }
5875-5886: Enforce the documented 500-item limit for conditionalUpdateRisks.The description says ≤500 entries, but the schema doesn’t enforce it.
✅ Suggested schema fix
"conditionalUpdateRisks": { "description": "conditionalUpdateRisks contains the list of risks associated with conditionalUpdates. When performing a conditional update, all its associated risks will be compared with the set of accepted risks in the spec.desiredUpdate.acceptRisks field. If all risks for a conditional update are included in the spec.desiredUpdate.acceptRisks set, the conditional update can proceed, otherwise it is blocked. The risk names in the list must be unique. conditionalUpdateRisks must not contain more than 500 entries.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/com.github.openshift.api.config.v1.ConditionalUpdateRisk" }, "x-kubernetes-list-map-keys": [ "name" ], - "x-kubernetes-list-type": "map" + "x-kubernetes-list-type": "map", + "maxItems": 500 },
6094-6102: Enforce riskNames size and entry length constraints.The description mentions ≤500 entries and ≤256 chars per entry, but the schema doesn’t enforce either.
✅ Suggested schema fix
"riskNames": { "description": "riskNames represents the set of the names of conditionalUpdateRisks that are relevant to this update for some clusters. The Applies condition of each conditionalUpdateRisks entry declares if that risk applies to this cluster. A conditional update is accepted only if each of its risks either does not apply to the cluster or is considered acceptable by the cluster administrator. The latter means that the risk names are included in value of the spec.desiredUpdate.acceptRisks field. Entries must be unique and must not exceed 256 characters. riskNames must not contain more than 500 entries.", "type": "array", "items": { "type": "string", - "default": "" + "default": "", + "maxLength": 256 }, - "x-kubernetes-list-type": "set" + "x-kubernetes-list-type": "set", + "maxItems": 500 },
6129-6140: Cap conditions to a single entry as documented.The description says no more than one entry, but the schema doesn’t enforce it.
✅ Suggested schema fix
"conditions": { "description": "conditions represents the observations of the conditional update risk's current status. Known types are: * Applies, for whether the risk applies to the current cluster. The condition's types in the list must be unique. conditions must not contain more than one entry.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/io.k8s.apimachinery.pkg.apis.meta.v1.Condition" }, "x-kubernetes-list-map-keys": [ "type" ], - "x-kubernetes-list-type": "map" + "x-kubernetes-list-type": "map", + "maxItems": 1 },
11639-11650: Enforce the documented 1000-item limit for acceptRisks.The description says ≤1000 entries, but the schema doesn’t enforce it.
✅ Suggested schema fix
"acceptRisks": { "description": "acceptRisks is an optional set of names of conditional update risks that are considered acceptable. A conditional update is performed only if all of its risks are acceptable. This list may contain entries that apply to current, previous or future updates. The entries therefore may not map directly to a risk in .status.conditionalUpdateRisks. acceptRisks must not contain more than 1000 entries. Entries in this list must be unique.", "type": "array", "items": { "default": {}, "$ref": "#/definitions/com.github.openshift.api.config.v1.AcceptRisk" }, "x-kubernetes-list-map-keys": [ "name" ], - "x-kubernetes-list-type": "map" + "x-kubernetes-list-type": "map", + "maxItems": 1000 },
🧹 Nitpick comments (1)
machine/v1beta1/types_awsprovider.go (1)
503-508: Verify mutual exclusivity enforcement betweenidanddynamicHostAllocation.The doc comment states "This field is mutually exclusive with id", but there's no explicit XValidation rule preventing both
idanddynamicHostAllocationfrom being set simultaneously. The current rules only validate that:
idrequiresallocationStrategy == ProvideddynamicHostAllocationrequiresallocationStrategy == DynamicWhile this implicitly enforces mutual exclusivity (since
allocationStrategycan only be one value), consider adding an explicit validation for clarity:💡 Optional: Add explicit mutual exclusivity validation
// +kubebuilder:validation:XValidation:rule="self.allocationStrategy == 'Provided' ? has(self.id) : true",message="id is required when allocationStrategy is Provided, and forbidden otherwise" // +kubebuilder:validation:XValidation:rule="has(self.id) ? self.allocationStrategy == 'Provided' : true",message="id is only allowed when allocationStrategy is Provided" // +kubebuilder:validation:XValidation:rule="has(self.dynamicHostAllocation) ? self.allocationStrategy == 'Dynamic' : true",message="dynamicHostAllocation is only allowed when allocationStrategy is Dynamic" +// +kubebuilder:validation:XValidation:rule="!(has(self.id) && has(self.dynamicHostAllocation))",message="id and dynamicHostAllocation are mutually exclusive" type DedicatedHost struct {
| // +required | ||
| // +optional | ||
| // +unionMember | ||
| ID string `json:"id,omitempty"` |
There was a problem hiding this comment.
ID is already part of 4.21, right? If so, then it's a breaking change:
4.21 will ship:
type DedicatedHost struct {
// +required
ID string `json:"id,omitempty"`
}4.22 will ship:
type DedicatedHost struct {
// +required
AllocationStrategy AllocationStrategy `json:"allocationStrategy,omitempty"`
// +optional (changed from +required)
ID string `json:"id,omitempty"`
// +optional (new field)
DynamicHostAllocation *DynamicHostAllocationSpec `json:"dynamicHostAllocation,omitempty"`
}Means that a valid 4.21 manifest will be invalid in 4.22.
dedicatedHost:
id: "h-1234567890abcdef0"Do we have a webhook or similar solution in the works for this?
There was a problem hiding this comment.
Yes there is webhook changes, that PR is almost ready. For backwards compatability, if its "" (emptry string) it is defaulting to UserProvided.
There was a problem hiding this comment.
For this, I can change the field to be optional but have it default to UserProvided. Just let me know what you prefer.
vr4manta
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@openapi/generated_openapi/zz_generated.openapi.go`:
- Around line 41179-41204: The enum constant name/value for the allocation
strategy is inconsistent: the code defines AllocationStrategyUserProvided =
"UserProvided" while docs/validation expect "Provided"
(AllocationStrategyDynamic = "Dynamic" is correct); fix by making the canonical
value match the docs/validation — either rename the constant to
AllocationStrategyProvided and set it to "Provided" (and update all uses of
AllocationStrategyUserProvided), or update the kubebuilder Enum and any
XValidation rules to accept "UserProvided" instead of "Provided"; update the
AllocationStrategy constant declarations and any related validation
annotations/XValidation (and their references) so the identifier, string value,
documentation, and OpenAPI enum all match.
In `@openapi/openapi.json`:
- Around line 23731-23749: The allocationStrategy property on the DedicatedHost
schema lacks a JSON Schema enum; update the DedicatedHost definition by adding
an "enum": ["Provided", "Dynamic"] to the allocationStrategy property so the
schema enforces the documented valid values, keeping the existing description
and type intact (look for the allocationStrategy property inside the
DedicatedHost object).
♻️ Duplicate comments (6)
machine/v1beta1/types_awsprovider.go (1)
464-474: AllocationStrategy constant value doesn’t match the enum/validation.
AllocationStrategyUserProvidedis"UserProvided"but the enum/validations/docs expect"Provided". Any code using the constant will serialize an invalid value and fail validation.🐛 Suggested fix
-// AllocationStrategyUserProvided specifies that the system should assign instances to a user-provided dedicated host. -AllocationStrategyUserProvided AllocationStrategy = "UserProvided" +// AllocationStrategyProvided specifies that the system should assign instances to a user-provided dedicated host. +AllocationStrategyProvided AllocationStrategy = "Provided"Alternatively, if you want
"UserProvided"to be the canonical value, update the enum/validation/docs accordingly.openapi/openapi.json (5)
4575-4587: Missing length constraints fornamefield.The description states the name "must be a non-empty string and must not exceed 256 characters" but the schema lacks
minLength: 1andmaxLength: 256constraints to enforce this.
5875-5886: MissingmaxItems: 500constraint forconditionalUpdateRisks.The description documents a 500-entry limit that isn't enforced in the schema.
6094-6102: Missing size constraints forriskNames.The description documents max 500 entries and max 256 characters per entry, but the schema lacks
maxItemsandmaxLengthconstraints.
6129-6140: MissingmaxItems: 1constraint forconditions.The description states the array must not contain more than one entry, but the schema doesn't enforce this limit.
11639-11650: MissingmaxItems: 1000constraint foracceptRisks.The description documents a 1000-entry limit that isn't enforced in the schema.
🧹 Nitpick comments (1)
openapi/openapi.json (1)
23771-23784: Consider adding AWS tag constraints for earlier validation.The
tagsproperty has no schema constraints, but AWS enforces limits: 50 tags per resource, key max 128 characters, value max 256 characters. While AWS will reject invalid values, adding schema constraints would provide earlier feedback to users.🔧 Suggested schema enhancement
"tags": { "description": "tags specifies a set of key-value pairs to apply to the allocated dedicated host. When omitted or empty, no additional user-defined tags will be applied to the allocated host.", "type": "object", + "maxProperties": 50, "additionalProperties": { "type": "string", - "default": "" + "default": "", + "maxLength": 256 } }Note: Key length validation (
maxLength: 128) would require usingpropertyNamesschema which may not be supported by all validators.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@machine/v1beta1/types_awsprovider.go`:
- Around line 476-512: The DedicatedHost struct is missing the required union
marker; add the comment marker "// +union" immediately above the DedicatedHost
type declaration so the existing +unionDiscriminator on AllocationStrategy and
+unionMember tags on ID and DynamicHostAllocation are recognized for OpenAPI
schema and union docs (locate the DedicatedHost type and insert the marker above
it).
♻️ Duplicate comments (6)
openapi/openapi.json (6)
4582-4585: Missing length validation fornamefield.The description documents "non-empty string and must not exceed 256 characters" but the schema lacks
minLength: 1andmaxLength: 256constraints to enforce this.
5875-5886: MissingmaxItems: 500constraint.The description documents a 500-entry cap but the schema doesn't enforce it.
6094-6102: Missing size constraints forriskNames.The description documents max 500 entries and max 256 characters per entry, but the schema lacks
maxItems: 500andmaxLength: 256constraints.
6129-6140: MissingmaxItems: 1constraint.The description states "conditions must not contain more than one entry" but the schema doesn't enforce it.
11639-11650: MissingmaxItems: 1000constraint.The description documents a 1000-entry cap but the schema doesn't enforce it.
23737-23740: Missingenumconstraint forallocationStrategy.The description correctly documents "Valid values are UserProvided and Dynamic" but the schema should include
"enum": ["UserProvided", "Dynamic"]to enforce validation.
|
@vr4manta: 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. |
4 participants
SPLAT-2615
Changes
Notes
For dynamic hosts, we are currently allowing admins to provide a set of tags to apply to the new host. Other future enhancements for it can be contained in the new DynamicHostAllocation struct.
This PR is implementing the OCP equivalent to the changes in kubernetes-sigs/cluster-api-provider-aws#5631.