OCPSTRAT-2336: Adjust karpenter API to OpenShift conventions#7952
OCPSTRAT-2336: Adjust karpenter API to OpenShift conventions#7952enxebre merged 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@enxebre: This pull request references OCPSTRAT-2336 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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThe PR adds five enum-like public types (PublicIPAddressAssignment, DetailedMonitoringState, RootVolumeDesignation, DeleteOnTerminationPolicy, EncryptionState), replaces several pointer/bool fields with these enums, tightens validations (MinProperties/MinItems/MinLength/MaxLength/Enum) across Subnet, SecurityGroup, BlockDeviceMapping, selector terms, and OpenshiftEC2NodeClass APIs, and updates conversions and spec accessors (deleteOnTerminationToBool, encryptionStateToBool, ptrIfNonEmpty, KarpenterAssociatePublicIPAddress, KarpenterDetailedMonitoring, KarpenterBlockDeviceMapping). Controllers now call the new accessors when reconciling Karpenter EC2NodeClass resources. Sequence Diagram(s)mermaid Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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 |
openshift-ci
bot
added
area/api
|
@enxebre: This pull request references OCPSTRAT-2336 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. |
|
@enxebre: This pull request references OCPSTRAT-2336 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.
🧹 Nitpick comments (3)
api/karpenter/v1beta1/karpenter_types.go (3)
428-430: Inconsistency:+requiredSpec field withomitzerotag.The
Specfield is marked+requiredbut usesomitzero, which will omit zero-value structs during serialization. This could cause the field to be absent in serialized output even though it's required. SinceOpenshiftEC2NodeClassSpechasMinProperties=1, validation will fail on empty specs anyway, but the serialization behavior may not match the required semantics.Consider using just
json:"spec"withoutomitzerofor required fields.♻️ Proposed fix
// spec defines the desired state of the OpenshiftEC2NodeClass. // +required - Spec OpenshiftEC2NodeClassSpec `json:"spec,omitzero"` + Spec OpenshiftEC2NodeClassSpec `json:"spec"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/karpenter/v1beta1/karpenter_types.go` around lines 428 - 430, The Spec field on the OpenshiftEC2NodeClass struct is marked +required but uses json:"spec,omitzero", which can omit a zero-value Spec during serialization; update the struct tag for Spec (the Spec OpenshiftEC2NodeClassSpec field) to remove omitzero (use json:"spec") so the required semantics match serialization while keeping the +required marker and existing OpenshiftEC2NodeClassSpec type/validation.
135-139: Same inconsistency:+requiredwithomitemptyonIDfield.Same issue as
Subnet- consider removingomitemptyfor the requiredIDfield to maintain consistency with the+requiredmarker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/karpenter/v1beta1/karpenter_types.go` around lines 135 - 139, The ID field has a +required marker but is declared with json:"id,omitempty", causing inconsistency; update the struct tag for the ID field in karpenter_types.go (the ID string `json` tag) to remove "omitempty" so it matches the +required validation, i.e., change `json:"id,omitempty"` to `json:"id"` to enforce presence in marshaled output.
116-125: Inconsistency between+requiredmarker andomitemptyJSON tag.Fields
ID(line 120) andZone(line 125) are marked+requiredbut haveomitemptyin their JSON tags. WhileMinLength=1provides runtime validation, theomitemptytag will omit these fields during serialization when empty, which contradicts the+requiredsemantics. Consider removingomitemptyfor required fields to ensure proper OpenAPI schema generation.♻️ Proposed fix
type Subnet struct { // id is the ID of the subnet. // +required // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=256 - ID string `json:"id,omitempty"` + ID string `json:"id"` // zone is the associated availability zone. // +required // +kubebuilder:validation:MinLength=1 // +kubebuilder:validation:MaxLength=256 - Zone string `json:"zone,omitempty"` + Zone string `json:"zone"`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/karpenter/v1beta1/karpenter_types.go` around lines 116 - 125, The struct fields ID and Zone are annotated with +required and MinLength validations but their JSON tags use `omitempty`, which contradicts required semantics; update the JSON tags for the ID and Zone fields in karpenter_types.go to remove `omitempty` so the fields are always serialized (ensuring OpenAPI/kubebuilder generates them as required) while keeping the existing `+required` and `+kubebuilder:validation:MinLength=1`/MaxLength attributes intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/karpenter/v1beta1/karpenter_types.go`:
- Around line 428-430: The Spec field on the OpenshiftEC2NodeClass struct is
marked +required but uses json:"spec,omitzero", which can omit a zero-value Spec
during serialization; update the struct tag for Spec (the Spec
OpenshiftEC2NodeClassSpec field) to remove omitzero (use json:"spec") so the
required semantics match serialization while keeping the +required marker and
existing OpenshiftEC2NodeClassSpec type/validation.
- Around line 135-139: The ID field has a +required marker but is declared with
json:"id,omitempty", causing inconsistency; update the struct tag for the ID
field in karpenter_types.go (the ID string `json` tag) to remove "omitempty" so
it matches the +required validation, i.e., change `json:"id,omitempty"` to
`json:"id"` to enforce presence in marshaled output.
- Around line 116-125: The struct fields ID and Zone are annotated with
+required and MinLength validations but their JSON tags use `omitempty`, which
contradicts required semantics; update the JSON tags for the ID and Zone fields
in karpenter_types.go to remove `omitempty` so the fields are always serialized
(ensuring OpenAPI/kubebuilder generates them as required) while keeping the
existing `+required` and `+kubebuilder:validation:MinLength=1`/MaxLength
attributes intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 4e9b9bd5-9c5d-4e48-b6aa-4d1e2fc9bfc0
⛔ Files ignored due to path filters (1)
vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/karpenter_types.gois excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
api/karpenter/v1beta1/karpenter_types.go
enxebre
force-pushed
the
adjust-karpenter-api-to-ocp-convention
branch
from
8a769c2 to
f457c9e
Compare
|
@enxebre: This pull request references OCPSTRAT-2336 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. |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/karpenter/v1beta1/karpenter_types.go (1)
354-356:⚠️ Potential issue | 🟠 MajorThe
volumeSizeregex now accepts bareTiandT.The doubled
||in theTiandTbranches creates an empty alternative, so unit-only values match this pattern.Suggested change
- // +kubebuilder:validation:Pattern:="^((?:[1-9][0-9]{0,3}|[1-4][0-9]{4}|[5][0-8][0-9]{3}|59000)Gi|(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|[6][0-3][0-9]{3}|64000)G|([1-9]||[1-5][0-7]|58)Ti|([1-9]||[1-5][0-9]|6[0-3]|64)T)$" + // +kubebuilder:validation:Pattern:="^((?:[1-9][0-9]{0,3}|[1-4][0-9]{4}|[5][0-8][0-9]{3}|59000)Gi|(?:[1-9][0-9]{0,3}|[1-5][0-9]{4}|[6][0-3][0-9]{3}|64000)G|([1-9]|[1-5][0-7]|58)Ti|([1-9]|[1-5][0-9]|6[0-3]|64)T)$"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/karpenter/v1beta1/karpenter_types.go` around lines 354 - 356, The kubebuilder validation regex for the volumeSize field contains accidental empty alternatives ("||") in the Ti and T branches which allows unit-only values; fix the pattern in the kubebuilder:validation:Pattern string by removing the stray empty alternatives so those groups read e.g. ([1-9]|[1-5][0-7]|58)Ti and ([1-9]|[1-5][0-9]|6[0-3]|64)T (update the existing kubebuilder:validation:Pattern line that defines the volumeSize regex).
🧹 Nitpick comments (1)
karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go (1)
91-122: Use a schema-valid fixture in this test.This case now builds a subnet term with both
tagsandid, a security-group term with bothtagsandname, and a subnet ID ("testID") that no longer matches the newsubnet-...pattern. Because the unit test bypasses admission, it can still pass on an object the API server would reject. Split those into separate OR terms if you want to keep covering both selection modes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go` around lines 91 - 122, The fixture is invalid because SubnetSelectorTerm and SecurityGroupSelectorTerm are populated with both tags and an ID/Name and the subnet ID "testID" doesn't match the expected "subnet-..." pattern; update the OpenshiftEC2NodeClassSpec fixture so each selector term is schema-valid by splitting selection modes into separate OR terms: for SubnetSelectorTerms create one term with ID only using a valid ID like "subnet-abc123" and another term with Tags only, and for SecurityGroupSelectorTerms create one term with Name only ("testName") and another with Tags only; modify the SubnetSelectorTerm and SecurityGroupSelectorTerm entries in the test fixture accordingly so no term contains both tags and id/name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/karpenter/v1beta1/karpenter_types.go`:
- Around line 161-162: The MinProperties=1 validation on
OpenshiftEC2NodeClassSpec makes an empty spec object invalid even though the
controller function reconcileEC2NodeClass() expects and handles spec: {} by
applying defaults; remove the kubebuilder:validation:MinProperties=1 marker (or
replace it with a comment explaining defaults) from the
OpenshiftEC2NodeClassSpec type so OpenshiftEC2NodeClass.Spec can be empty and
reconcilers can populate defaults at runtime.
- Around line 164-166: The CEL for subnetSelectorTerms currently uses
"!self.all(x, has(x.id) && has(x.tags))" which still allows a list where one
term sets both id and tags; update the mutual-exclusion rule for
subnetSelectorTerms to assert every term does not contain both fields, e.g.
replace that rule with one that uses either "self.all(x, !(has(x.id) &&
has(x.tags)))" or "!self.any(x, has(x.id) && has(x.tags))" so no single term may
set both id and tags (update the kubebuilder XValidation rule associated with
subnetSelectorTerms accordingly).
- Around line 174-177: The XValidation exclusivity rules on
securityGroupSelectorTerms currently use "!self.all(...)" which only rejects
when every term is invalid; change both rules so they reject if any term
contains the forbidden combination. Replace the rule expressions in the two
lines that reference "!self.all(x, has(x.id) && (has(x.tags) || has(x.name)))"
and "!self.all(x, has(x.name) && (has(x.tags) || has(x.id)))" with either
"not(self.any(x, has(x.id) && (has(x.tags) || has(x.name))))" and
"not(self.any(x, has(x.name) && (has(x.tags) || has(x.id))))" or equivalently
"self.all(x, !(has(x.id) && (has(x.tags) || has(x.name))))" and "self.all(x,
!(has(x.name) && (has(x.tags) || has(x.id))))" so any single invalid term causes
validation to fail.
---
Outside diff comments:
In `@api/karpenter/v1beta1/karpenter_types.go`:
- Around line 354-356: The kubebuilder validation regex for the volumeSize field
contains accidental empty alternatives ("||") in the Ti and T branches which
allows unit-only values; fix the pattern in the kubebuilder:validation:Pattern
string by removing the stray empty alternatives so those groups read e.g.
([1-9]|[1-5][0-7]|58)Ti and ([1-9]|[1-5][0-9]|6[0-3]|64)T (update the existing
kubebuilder:validation:Pattern line that defines the volumeSize regex).
---
Nitpick comments:
In `@karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go`:
- Around line 91-122: The fixture is invalid because SubnetSelectorTerm and
SecurityGroupSelectorTerm are populated with both tags and an ID/Name and the
subnet ID "testID" doesn't match the expected "subnet-..." pattern; update the
OpenshiftEC2NodeClassSpec fixture so each selector term is schema-valid by
splitting selection modes into separate OR terms: for SubnetSelectorTerms create
one term with ID only using a valid ID like "subnet-abc123" and another term
with Tags only, and for SecurityGroupSelectorTerms create one term with Name
only ("testName") and another with Tags only; modify the SubnetSelectorTerm and
SecurityGroupSelectorTerm entries in the test fixture accordingly so no term
contains both tags and id/name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2e7a8836-d367-4e39-83fc-abf73fd6c359
⛔ Files ignored due to path filters (9)
api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*client/applyconfiguration/karpenter/v1beta1/blockdevice.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/blockdevicemapping.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassspec.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassstatus.gois excluded by!client/**karpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftec2nodeclasses.yamlis excluded by!karpenter-operator/controllers/karpenter/assets/*.yamlvendor/github.com/openshift/hypershift/api/karpenter/v1beta1/karpenter_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (4)
api/karpenter/v1beta1/karpenter_types.gokarpenter-operator/controllers/karpenter/karpenter_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- karpenter-operator/controllers/karpenter/karpenter_controller.go
| // +kubebuilder:validation:MinProperties=1 | ||
| type OpenshiftEC2NodeClassSpec struct { |
There was a problem hiding this comment.
Don't turn the default-only spec into an invalid object.
OpenshiftEC2NodeClass.Spec is already required on the root type. Adding MinProperties=1 goes further and rejects spec: {}, even though reconcileEC2NodeClass() still supports that shape by filling in default selectors and the default root volume when fields are unset.
Suggested change
-// +kubebuilder:validation:MinProperties=1
type OpenshiftEC2NodeClassSpec struct {📝 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.
| // +kubebuilder:validation:MinProperties=1 | |
| type OpenshiftEC2NodeClassSpec struct { | |
| type OpenshiftEC2NodeClassSpec struct { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/karpenter/v1beta1/karpenter_types.go` around lines 161 - 162, The
MinProperties=1 validation on OpenshiftEC2NodeClassSpec makes an empty spec
object invalid even though the controller function reconcileEC2NodeClass()
expects and handles spec: {} by applying defaults; remove the
kubebuilder:validation:MinProperties=1 marker (or replace it with a comment
explaining defaults) from the OpenshiftEC2NodeClassSpec type so
OpenshiftEC2NodeClass.Spec can be empty and reconcilers can populate defaults at
runtime.
| // +kubebuilder:validation:XValidation:message="subnetSelectorTerms cannot be empty",rule="self.size() != 0" | ||
| // +kubebuilder:validation:XValidation:message="expected at least one, got none, ['tags', 'id']",rule="self.all(x, has(x.tags) || has(x.id))" | ||
| // +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in subnetSelectorTerms",rule="!self.all(x, has(x.id) && has(x.tags))" |
There was a problem hiding this comment.
This subnet-selector CEL only fails when every term is invalid.
!self.all(x, has(x.id) && has(x.tags)) lets a list through as soon as one other item is valid, so a single term can still set both id and tags and pass schema validation.
Suggested change
-// +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in subnetSelectorTerms",rule="!self.all(x, has(x.id) && has(x.tags))"
+// +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in subnetSelectorTerms",rule="self.all(x, !(has(x.id) && has(x.tags)))"📝 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.
| // +kubebuilder:validation:XValidation:message="subnetSelectorTerms cannot be empty",rule="self.size() != 0" | |
| // +kubebuilder:validation:XValidation:message="expected at least one, got none, ['tags', 'id']",rule="self.all(x, has(x.tags) || has(x.id))" | |
| // +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in subnetSelectorTerms",rule="!self.all(x, has(x.id) && has(x.tags))" | |
| // +kubebuilder:validation:XValidation:message="subnetSelectorTerms cannot be empty",rule="self.size() != 0" | |
| // +kubebuilder:validation:XValidation:message="expected at least one, got none, ['tags', 'id']",rule="self.all(x, has(x.tags) || has(x.id))" | |
| // +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in subnetSelectorTerms",rule="self.all(x, !(has(x.id) && has(x.tags)))" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/karpenter/v1beta1/karpenter_types.go` around lines 164 - 166, The CEL for
subnetSelectorTerms currently uses "!self.all(x, has(x.id) && has(x.tags))"
which still allows a list where one term sets both id and tags; update the
mutual-exclusion rule for subnetSelectorTerms to assert every term does not
contain both fields, e.g. replace that rule with one that uses either
"self.all(x, !(has(x.id) && has(x.tags)))" or "!self.any(x, has(x.id) &&
has(x.tags))" so no single term may set both id and tags (update the kubebuilder
XValidation rule associated with subnetSelectorTerms accordingly).
| // +kubebuilder:validation:XValidation:message="securityGroupSelectorTerms cannot be empty",rule="self.size() != 0" | ||
| // +kubebuilder:validation:XValidation:message="expected at least one, got none, ['tags', 'id', 'name']",rule="self.all(x, has(x.tags) || has(x.id) || has(x.name))" | ||
| // +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in securityGroupSelectorTerms",rule="!self.all(x, has(x.id) && (has(x.tags) || has(x.name)))" | ||
| // +kubebuilder:validation:XValidation:message="'name' is mutually exclusive, cannot be set with a combination of other fields in securityGroupSelectorTerms",rule="!self.all(x, has(x.name) && (has(x.tags) || has(x.id)))" |
There was a problem hiding this comment.
These security-group exclusivity rules have the same !self.all(...) bug.
Both rules only reject the list when all entries are invalid. A mixed list can still include one term with id+tags or name+tags and pass validation.
Suggested change
-// +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in securityGroupSelectorTerms",rule="!self.all(x, has(x.id) && (has(x.tags) || has(x.name)))"
-// +kubebuilder:validation:XValidation:message="'name' is mutually exclusive, cannot be set with a combination of other fields in securityGroupSelectorTerms",rule="!self.all(x, has(x.name) && (has(x.tags) || has(x.id)))"
+// +kubebuilder:validation:XValidation:message="'id' is mutually exclusive, cannot be set with a combination of other fields in securityGroupSelectorTerms",rule="self.all(x, !(has(x.id) && (has(x.tags) || has(x.name))))"
+// +kubebuilder:validation:XValidation:message="'name' is mutually exclusive, cannot be set with a combination of other fields in securityGroupSelectorTerms",rule="self.all(x, !(has(x.name) && (has(x.tags) || has(x.id))))"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/karpenter/v1beta1/karpenter_types.go` around lines 174 - 177, The
XValidation exclusivity rules on securityGroupSelectorTerms currently use
"!self.all(...)" which only rejects when every term is invalid; change both
rules so they reject if any term contains the forbidden combination. Replace the
rule expressions in the two lines that reference "!self.all(x, has(x.id) &&
(has(x.tags) || has(x.name)))" and "!self.all(x, has(x.name) && (has(x.tags) ||
has(x.id)))" with either "not(self.any(x, has(x.id) && (has(x.tags) ||
has(x.name))))" and "not(self.any(x, has(x.name) && (has(x.tags) ||
has(x.id))))" or equivalently "self.all(x, !(has(x.id) && (has(x.tags) ||
has(x.name))))" and "self.all(x, !(has(x.name) && (has(x.tags) || has(x.id))))"
so any single invalid term causes validation to fail.
f457c9e to
b72eff1
Compare
|
@enxebre: This pull request references OCPSTRAT-2336 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
api/karpenter/v1beta1/karpenter_types.go (1)
114-145: Minor inconsistency:+requiredmarkers withomitemptytags.The
idandzonefields are marked+requiredbut useomitemptyJSON tags. While this works because these structs appear in status fields that are always populated by the controller, the mixed semantics could cause confusion. Consider documenting that these are "required when present" rather than schema-required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api/karpenter/v1beta1/karpenter_types.go` around lines 114 - 145, The +required kubebuilder markers on Subnet.ID and Subnet.Zone (and the +optional markers on ZoneID and SecurityGroup.Name) conflict with their json:"...,omitempty" tags and can be confusing; update the declarations for the Subnet and SecurityGroup structs (symbols: Subnet, SecurityGroup, fields ID, Zone, ZoneID, Name) by either removing the +required annotations or changing the field comments to explicitly state "required when present / set by controller in status" (i.e., document that these are not schema-required but are populated by the controller), and ensure the kubebuilder markers and json tags are consistent with that choice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@api/karpenter/v1beta1/karpenter_types.go`:
- Around line 114-145: The +required kubebuilder markers on Subnet.ID and
Subnet.Zone (and the +optional markers on ZoneID and SecurityGroup.Name)
conflict with their json:"...,omitempty" tags and can be confusing; update the
declarations for the Subnet and SecurityGroup structs (symbols: Subnet,
SecurityGroup, fields ID, Zone, ZoneID, Name) by either removing the +required
annotations or changing the field comments to explicitly state "required when
present / set by controller in status" (i.e., document that these are not
schema-required but are populated by the controller), and ensure the kubebuilder
markers and json tags are consistent with that choice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: b96520f4-7b34-4c96-ba0f-66a499d67e1a
⛔ Files ignored due to path filters (9)
api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*client/applyconfiguration/karpenter/v1beta1/blockdevice.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/blockdevicemapping.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassspec.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassstatus.gois excluded by!client/**karpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftec2nodeclasses.yamlis excluded by!karpenter-operator/controllers/karpenter/assets/*.yamlvendor/github.com/openshift/hypershift/api/karpenter/v1beta1/karpenter_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (5)
api/karpenter/v1beta1/karpenter_types.goapi/karpenter/v1beta1/util.gokarpenter-operator/controllers/karpenter/karpenter_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- api/karpenter/v1beta1/util.go
- karpenter-operator/controllers/karpenter/karpenter_controller.go
- karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go
enxebre
force-pushed
the
adjust-karpenter-api-to-ocp-convention
branch
from
b72eff1 to
d5b46ab
Compare
|
@enxebre: This pull request references OCPSTRAT-2336 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/karpenter/v1beta1/karpenter_types.go`:
- Around line 373-383: The kubebuilder marker `MinProperties=1` on the
OpenshiftEC2NodeClassStatus struct can reject valid initial/partial status
updates; remove (or comment out) the `//
+kubebuilder:validation:MinProperties=1` marker from the
OpenshiftEC2NodeClassStatus definition so the controller can create an
initially-empty status and later populate the Conditions field (which already
enforces MinItems=1); ensure only the Conditions slice retains its validation
and that status remains controller-managed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3eb72468-2686-45ce-837b-95168dc793f1
⛔ Files ignored due to path filters (9)
api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.go,!**/zz_generated*client/applyconfiguration/karpenter/v1beta1/blockdevice.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/blockdevicemapping.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassspec.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassstatus.gois excluded by!client/**karpenter-operator/controllers/karpenter/assets/karpenter.hypershift.openshift.io_openshiftec2nodeclasses.yamlis excluded by!karpenter-operator/controllers/karpenter/assets/*.yamlvendor/github.com/openshift/hypershift/api/karpenter/v1beta1/karpenter_types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/util.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/hypershift/api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*.go,!**/zz_generated*
📒 Files selected for processing (5)
api/karpenter/v1beta1/karpenter_types.goapi/karpenter/v1beta1/util.gokarpenter-operator/controllers/karpenter/karpenter_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go
- karpenter-operator/controllers/karpenter/karpenter_controller.go
| // +kubebuilder:validation:MinProperties=1 | ||
| type OpenshiftEC2NodeClassStatus struct { | ||
| // Subnets contains the current Subnet values that are available to the | ||
| // conditions contain signals for health and readiness. | ||
| // +optional | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| // +patchMergeKey=type | ||
| // +patchStrategy=merge | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=100 | ||
| Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type"` |
There was a problem hiding this comment.
MinProperties=1 on status may reject valid intermediate states.
When an OpenshiftEC2NodeClass is first created, its status will be empty until the controller reconciles it. The MinProperties=1 constraint could reject valid status updates if the controller tries to set status with only conditions (which itself has MinItems=1), potentially causing issues during the initial reconciliation cycle.
Consider whether this constraint is necessary given that status is always controller-managed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@api/karpenter/v1beta1/karpenter_types.go` around lines 373 - 383, The
kubebuilder marker `MinProperties=1` on the OpenshiftEC2NodeClassStatus struct
can reject valid initial/partial status updates; remove (or comment out) the `//
+kubebuilder:validation:MinProperties=1` marker from the
OpenshiftEC2NodeClassStatus definition so the controller can create an
initially-empty status and later populate the Conditions field (which already
enforces MinItems=1); ensure only the Conditions slice retains its validation
and that status remains controller-managed.
| // associatePublicIPAddress controls if public IP addresses are assigned to instances that are launched with the nodeclass. | ||
| // +optional | ||
| AssociatePublicIPAddress *bool `json:"associatePublicIPAddress,omitempty"` | ||
| AssociatePublicIPAddress PublicIPAddressAssignment `json:"associatePublicIPAddress,omitempty"` |
There was a problem hiding this comment.
Can we change the field name? What if it were ipAddressAssociation: Private | Public or maybe Public should be PublicAndPrivate?
There was a problem hiding this comment.
I think we should actually only allow Public here. If omitted default to the subnet behaviour
There was a problem hiding this comment.
I'd probably allow an explicit Private option here to be honest. It makes it predictable for users who want to set this explicitly in case for some reason we wanted to change what omitted behaviour in the future
There was a problem hiding this comment.
actually I'm not sure now, we can guarantee Public because is a cloud provider knob, but when not Public, the visibility is dictated by the subnet setup which is a service consumer infra choice not owned/controlled by us
There was a problem hiding this comment.
What happens If we allowed Private here or Basic in monitoring and then the cloud provider change their bool negative value behaviour? they technically shouldn't and it's unlikely, but their bool semantic leave room for ambiguity https://docs.aws.amazon.com/AWSEC2/latest/APIReference/API_LaunchTemplatesMonitoringRequest.html#API_LaunchTemplatesMonitoringRequest_Contents
There was a problem hiding this comment.
actually I'm not sure now, we can guarantee Public because is a cloud provider knob, but when not Public, the visibility is dictated by the subnet setup which is a service consumer infra choice not owned/controlled by us
Good point
So the behaviour is Public or SubnetDefault? And then you'd need to just document that behaviour as
When set to SubnetDefault, the instance IP addresses will match the configuration of the instance subnet
On the monitoring front, I suspect Basic is fine and we will have bigger issues if Amazon changed their API around this (if it were enabled/disabled you'd still get the same breaking change)
| // associatePublicIPAddress controls if public IP addresses are assigned to instances that are launched with the nodeclass. | ||
| // +optional | ||
| AssociatePublicIPAddress *bool `json:"associatePublicIPAddress,omitempty"` | ||
| AssociatePublicIPAddress PublicIPAddressAssignment `json:"associatePublicIPAddress,omitempty"` |
There was a problem hiding this comment.
Technically this is a breaking change, any existing client/object will have serialization issues if you change this away from a bool. Who is this going to impact?
There was a problem hiding this comment.
this is still not published. There's no real use case for an out of band client to serialize this today. If the do, they must bump the deps.
| // +patchMergeKey=type | ||
| // +patchStrategy=merge |
There was a problem hiding this comment.
PatchStrategy doesn't do anything for CRDs so you can drop these
enxebre
force-pushed
the
adjust-karpenter-api-to-ocp-convention
branch
from
d5b46ab to
3be64ff
Compare
| InstanceStorePolicy InstanceStorePolicy `json:"instanceStorePolicy,omitempty"` | ||
|
|
||
| // DetailedMonitoring controls if detailed monitoring is enabled for instances that are launched | ||
| // detailedMonitoring controls if detailed monitoring is enabled for instances that are launched. |
There was a problem hiding this comment.
Maybe this wants to be monitoring: Basic | Detailed?
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we can only take detailed and default to the provider behaviour
| SecurityGroupSelectorTerms []SecurityGroupSelectorTerm `json:"securityGroupSelectorTerms,omitempty"` | ||
|
|
||
| // AssociatePublicIPAddress controls if public IP addresses are assigned to instances that are launched with the nodeclass. | ||
| // ipAddressAssociation controls the IP address assignment for instances launched with the nodeclass. |
There was a problem hiding this comment.
Should document valid values from the enum and what each valid value means
|
|
||
| // ID is the subnet id in EC2 | ||
| // +kubebuilder:validation:Pattern="subnet-[0-9a-z]+" | ||
| // id is the subnet id in EC2. |
There was a problem hiding this comment.
Should document in the godoc the expected format
|
|
||
| // ID is the security group id in EC2 | ||
| // +kubebuilder:validation:Pattern:="sg-[0-9a-z]+" | ||
| // id is the security group id in EC2. |
There was a problem hiding this comment.
Should document in the godoc the expected format
| Throughput *int64 `json:"throughput,omitempty"` | ||
|
|
||
| // VolumeSize in `Gi`, `G`, `Ti`, or `T`. You must specify either a snapshot ID or | ||
| // volumeSize in `Gi`, `G`, `Ti`, or `T`. You must specify either a snapshot ID or |
There was a problem hiding this comment.
Using resource.Quantity is generally not advised in most cases.
It can cause parsing issues for non-go clients
Best to make this something like VolumeSizeGiB and make it an integer value
| // volumeType is the type of the block device. | ||
| // For more information, see Amazon EBS volume types (https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/EBSVolumeTypes.html) | ||
| // in the Amazon Elastic Compute Cloud User Guide. | ||
| // +kubebuilder:validation:Enum:={standard,io1,io2,gp2,sc1,st1,gp3} |
There was a problem hiding this comment.
Nit, should use PascalCase for enum values
| // +kubebuilder:validation:Enum:={standard,io1,io2,gp2,sc1,st1,gp3} | |
| // +kubebuilder:validation:Enum:={Standard,IO1,IO2,GP2,SC1,ST1,GP3} |
enxebre
force-pushed
the
adjust-karpenter-api-to-ocp-convention
branch
3 times, most recently
from
83366ce to
88c0211
Compare
enxebre
force-pushed
the
adjust-karpenter-api-to-ocp-convention
branch
3 times, most recently
from
bddce3d to
de6bf17
Compare
| // +kubebuilder:validation:Schemaless | ||
| // +kubebuilder:validation:Type:=string | ||
| // +kubebuilder:validation:Minimum=1 | ||
| // +kubebuilder:validation:Maximum=64000 |
There was a problem hiding this comment.
This is 64TiB? Are we working as binary sizes here? If so wouldn't this be 65536?
enxebre
force-pushed
the
adjust-karpenter-api-to-ocp-convention
branch
from
de6bf17 to
70afd8f
Compare
|
@enxebre: This pull request references OCPSTRAT-2336 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. |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
LGTM from an API perspective
|
/test e2e-aws-autonode |
- Replace *bool fields with string enum types to comply with OpenShift API conventions - Rename associatePublicIPAddress to ipAddressAssociation with Public/SubnetDefault values - Rename detailedMonitoring to monitoring with Detailed/Basic values - Replace volumeType string with VolumeType enum using PascalCase values - Replace volumeSize *resource.Quantity with volumeSizeGiB int64 (max 65536 = 64 TiB) - Change BlockDeviceMappings from []*BlockDeviceMapping to []BlockDeviceMapping - Make OpenshiftEC2NodeClass spec field required - Add conversion helpers to translate enum types back to *bool and lowercase values for upstream Karpenter compatibility - Document expected formats for tags and id fields in selector terms - Fix CEL rule referencing renamed volumeSizeGiB field Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The karpenter API types changed from *bool/pointer fields to string enum types (IPAddressAssociation, MonitoringState, VolumeType, etc.). Update controller code and tests to use conversion helpers and enum constants. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Regenerated deepcopy, apply configurations, CRDs, and vendor copies to reflect the karpenter API enum type and required spec changes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
enxebre
force-pushed
the
adjust-karpenter-api-to-ocp-convention
branch
from
70afd8f to
b3e2a10
Compare
|
/test e2e-aws-autonode |
|
/label tide/merge-method-squash |
openshift-ci
bot
added
the
tide/merge-method-squash
|
/test e2e-aws-autonode |
|
/test e2e-aws-techpreview |
1 similar comment
|
/test e2e-aws-techpreview |
|
@enxebre: all tests passed! 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. |
5 participants
Summary
*booland pointer fields in karpenter API types with string enum types (PublicIPAddressAssignment,DetailedMonitoringState,EncryptionState,DeleteOnTerminationPolicy,RootVolumeDesignation) to comply with OpenShift API conventions and kube-api-linter rules*boolfor upstream Karpenter compatibilityTest plan
make buildpassesmake verifypasses (excluding unrelated gitlint commit body warning)🤖 Generated with Claude Code
Summary by CodeRabbit