AUTOSCALE-570: Expose MetadataOptions on OpenShiftEC2NodeClass - #7958
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
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:
📝 WalkthroughWalkthroughThis PR adds an optional Sequence Diagram(s)sequenceDiagram
participant User
participant OpenShiftAPI as "OpenShift API\n(OpenshiftEC2NodeClass)"
participant Controller as "karpenter-operator\nController"
participant Converter as "util.KarpenterMetadataOptions()"
participant Downstream as "EC2NodeClass\n(awskarpenterv1)"
participant Karpenter as "Karpenter"
User->>OpenShiftAPI: Create/Update OpenshiftEC2NodeClass (spec.MetadataOptions)
OpenShiftAPI->>Controller: Notify resource change / Reconcile
Controller->>Converter: Call KarpenterMetadataOptions() on spec
Converter-->>Controller: Return awskarpenterv1.MetadataOptions (or nil)
Controller->>Downstream: Create/Update EC2NodeClass with Spec.MetadataOptions
Downstream-->>Controller: Confirm created/updated
Karpenter->>Downstream: Read EC2NodeClass.Spec (including MetadataOptions) during provisioning
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/test unit |
openshift-ci
Bot
added
area/api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go (1)
213-242: Minor: This test case is redundant.This test case ("When MetadataOptions is nil") has identical
specandexpectedSpecto the first test case ("When OpenshiftEC2NodeClassSpec.spec is empty"). Consider removing it or differentiating it if the intent was to test something distinct.🤖 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 213 - 242, The test "When MetadataOptions is nil it should not be set on EC2NodeClass" is redundant with "When OpenshiftEC2NodeClassSpec.spec is empty"; either remove the redundant test case or make it assert the distinct behavior by explicitly setting MetadataOptions to nil on the OpenshiftEC2NodeClassSpec and verifying EC2NodeClassSpec does not include any MetadataOptions field—update the test case in ec2_nodeclass_controller_test.go (referencing the test name string, OpenshiftEC2NodeClassSpec, and awskarpenterv1.EC2NodeClassSpec) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go`:
- Around line 213-242: The test "When MetadataOptions is nil it should not be
set on EC2NodeClass" is redundant with "When OpenshiftEC2NodeClassSpec.spec is
empty"; either remove the redundant test case or make it assert the distinct
behavior by explicitly setting MetadataOptions to nil on the
OpenshiftEC2NodeClassSpec and verifying EC2NodeClassSpec does not include any
MetadataOptions field—update the test case in ec2_nodeclass_controller_test.go
(referencing the test name string, OpenshiftEC2NodeClassSpec, and
awskarpenterv1.EC2NodeClassSpec) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 3f432988-0a53-457a-ab22-f6494111e3e8
⛔ Files ignored due to path filters (7)
api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goclient/applyconfiguration/karpenter/v1beta1/metadataoptions.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassspec.gois excluded by!client/**client/applyconfiguration/utils.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/**
📒 Files selected for processing (5)
api/karpenter/v1beta1/karpenter_types.goapi/karpenter/v1beta1/util.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.gotest/e2e/karpenter_test.go
bbf15fc to
7df98f5
Compare
|
/test unit |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e/karpenter_test.go (1)
534-546: Consider asserting the launched EC2 instance’s metadata options too.This proves propagation into
EC2NodeClass, but not that the node actually launches with the requested IMDS settings. A smallDescribeInstancescheck here—similar to the instance-profile test above—would cover the security-critical behavior end to end.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/e2e/karpenter_test.go` around lines 534 - 546, The test currently asserts MetadataOptions on the EC2NodeClass but not on the actual launched EC2 instance; add a DescribeInstances check (similar to the instance-profile test) after the EC2NodeClass assertions to find the created instance by node/instance-id and call EC2 DescribeInstances to retrieve Instance.MetadataOptions and assert HTTPEndpoint == "enabled", HTTPProtocolIPv6 == "disabled", HTTPPutResponseHopLimit == 2, and HTTPTokens == "required"; reuse the existing AWS client setup used in the instance-profile test and ensure the lookup ties the instance to the Karpenter node created in this test (e.g., via node labels or the instance-id recorded when the node becomes Ready).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/e2e/karpenter_test.go`:
- Around line 534-546: The test currently asserts MetadataOptions on the
EC2NodeClass but not on the actual launched EC2 instance; add a
DescribeInstances check (similar to the instance-profile test) after the
EC2NodeClass assertions to find the created instance by node/instance-id and
call EC2 DescribeInstances to retrieve Instance.MetadataOptions and assert
HTTPEndpoint == "enabled", HTTPProtocolIPv6 == "disabled",
HTTPPutResponseHopLimit == 2, and HTTPTokens == "required"; reuse the existing
AWS client setup used in the instance-profile test and ensure the lookup ties
the instance to the Karpenter node created in this test (e.g., via node labels
or the instance-id recorded when the node becomes Ready).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a4c12405-4046-4be0-8caf-dca55ca7e7e9
⛔ Files ignored due to path filters (8)
api/karpenter/v1beta1/zz_generated.deepcopy.gois excluded by!**/zz_generated*.goclient/applyconfiguration/karpenter/v1beta1/metadataoptions.gois excluded by!client/**client/applyconfiguration/karpenter/v1beta1/openshiftec2nodeclassspec.gois excluded by!client/**client/applyconfiguration/utils.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
📒 Files selected for processing (5)
api/karpenter/v1beta1/karpenter_types.goapi/karpenter/v1beta1/util.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.gokarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.gotest/e2e/karpenter_test.go
|
/test e2e-aws-autonode |
|
/retest |
7df98f5 to
ee37000
Compare
|
/test e2e-aws-autonode |
|
/retest |
1 similar comment
|
/retest |
|
/test e2e-aws-autonode |
e950e58 to
fb95269
Compare
|
/test e2e-aws-autonode |
|
/test e2e-aws |
Test Resultse2e-aks
e2e-aws
|
| g.Eventually(func(g Gomega) { | ||
| ec2NodeClass := &awskarpenterv1.EC2NodeClass{} | ||
| err := guestClient.Get(ctx, crclient.ObjectKey{Name: nc.Name}, ec2NodeClass) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(ec2NodeClass.Spec.MetadataOptions).NotTo(BeNil(), "MetadataOptions should be set on EC2NodeClass") | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPEndpoint).To(Equal("enabled")) | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPProtocolIPv6).To(Equal("disabled")) | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPPutResponseHopLimit).To(Equal(int64(2))) | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPTokens).To(Equal("required")) | ||
| }).WithTimeout(2 * time.Minute).WithPolling(5 * time.Second).Should(Succeed()) | ||
| t.Log("MetadataOptions propagated correctly to EC2NodeClass") |
There was a problem hiding this comment.
This test just verifies that we propagate the openshiftec2nodeclass fields to ec2nodeclass. Can we have part of this test which is to just make sure that if we set up the config right, that the metadata options actually get set on the instance?
Like with ec2:DescribeInstances API call?
| g.Eventually(func(g Gomega) { | ||
| ec2NodeClass := &awskarpenterv1.EC2NodeClass{} | ||
| err := guestClient.Get(ctx, crclient.ObjectKey{Name: nc.Name}, ec2NodeClass) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(ec2NodeClass.Spec.MetadataOptions).NotTo(BeNil(), "MetadataOptions should be set on EC2NodeClass") | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPEndpoint).To(Equal("enabled")) | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPProtocolIPv6).To(Equal("disabled")) | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPPutResponseHopLimit).To(Equal(int64(2))) | ||
| g.Expect(*ec2NodeClass.Spec.MetadataOptions.HTTPTokens).To(Equal("required")) | ||
| }).WithTimeout(2 * time.Minute).WithPolling(5 * time.Second).Should(Succeed()) |
There was a problem hiding this comment.
Can we use e2eutil.EventuallyObject here?
|
/test verify Should now be seeing an updated linter config |
fb95269 to
7d749a7
Compare
|
/test e2e-aws-autonode |
- Rename httpProtocolIP to httpIPProtocol for readability - Remove redundant omitempty tag from metadataOptions (omitzero suffices) - Document valid values and their meanings on all fields - Document hop limit use cases (1=host only, 2=pods, 3+=nested) - Clarify httpTokens doc to avoid Required/optional confusion - Update field-level defaults doc to use current field names
|
/test e2e-aws-autonode |
|
/test e2e-aks-4-21 |
|
/test e2e-aws |
|
/test e2e-aws |
|
/test e2e-aws |
|
/retest-required |
| // accessible on the node. | ||
| // When omitted, the platform defaults to HTTP endpoint access. | ||
| // +optional | ||
| Access MetadataAccess `json:"access,omitempty"` |
There was a problem hiding this comment.
Do we need cel to prevent the rest of the fields from being set, when Access is None?
|
/lgtm |
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/test e2e-aws-4-21 |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/verified by e2e @rafael-azevedo |
openshift-ci-robot
added
the
verified
|
@enxebre: This PR has been marked as verified by 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. |
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
rafael-azevedo
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@rafael-azevedo: This pull request references AUTOSCALE-570 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@rafael-azevedo: 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. |
…hift#7958) * feat(karpenter): expose MetadataOptions on OpenshiftEC2NodeClass Allow customers to configure EC2 IMDS settings (HTTPTokens, HTTPEndpoint, HTTPProtocolIPv6, HTTPPutResponseHopLimit) on Karpenter-managed nodes by piping MetadataOptions through OpenshiftEC2NodeClass to EC2NodeClass. Fixes AUTOSCALE-570 * fix(api): address review feedback for MetadataOptions API types - Use non-pointer enum types (MetadataEndpointState, MetadataProtocolIPv6State, MetadataHTTPTokensState) following OpenShift API conventions - Use uppercase enum values (Enabled/Disabled, Required/Optional) - Change MetadataOptions to value type with omitzero - Remove pointer types for HTTPPutResponseHopLimit (use int64) - Add MinProperties=1 validation on MetadataOptions type - Use lowercase godoc comments matching serialized field format - Convert to lowercase when mapping to upstream karpenter types * fix(api): use semantic enum values for MetadataOptions fields - Rename HTTPEndpoint to access with values HTTPEndpoint/None - Rename HTTPProtocolIPv6 to httpProtocolIP with values IPv4/IPv6 - Remove kubebuilder:default markers, platform controls defaults - Document defaults are subject to change over time - Convert semantic values to upstream karpenter lowercase in util * fix(api): address API review feedback for MetadataOptions - Rename httpProtocolIP to httpIPProtocol for readability - Remove redundant omitempty tag from metadataOptions (omitzero suffices) - Document valid values and their meanings on all fields - Document hop limit use cases (1=host only, 2=pods, 3+=nested) - Clarify httpTokens doc to avoid Required/optional confusion - Update field-level defaults doc to use current field names
6 participants
What this PR does / why we need it:
Adds
MetadataOptionstoOpenshiftEC2NodeClassso customers can configure EC2 IMDS (v1/v2) on Karpenter-managed nodes in HyperShift.This pipes
HTTPEndpoint,HTTPProtocolIPv6,HTTPPutResponseHopLimit, andHTTPTokensthrough fromOpenshiftEC2NodeClassto the upstream KarpenterEC2NodeClass, following the same pattern as existing fields likeDetailedMonitoringandAssociatePublicIPAddress.Changes:
api/karpenter/v1beta1/karpenter_types.go— AddedMetadataOptionsstruct and field toOpenshiftEC2NodeClassSpecapi/karpenter/v1beta1/util.go— AddedKarpenterMetadataOptions()conversion helperkarpenter-operator/controllers/nodeclass/ec2_nodeclass_controller.go— MapMetadataOptionsinreconcileEC2NodeClass()karpenter-operator/controllers/nodeclass/ec2_nodeclass_controller_test.go— Unit tests for full, nil, and partialMetadataOptionspropagationtest/e2e/karpenter_test.go— E2E verification thatMetadataOptionspropagates to the downstreamEC2NodeClassmake update)Which issue(s) this PR fixes:
Fixes AUTOSCALE-570
Special notes for your reviewer:
The
MetadataOptionstype mirrors the upstream Karpenter AWS provider definition (vendor/github.com/aws/karpenter-provider-aws/pkg/apis/v1/ec2nodeclass.go) with the same fields, validation markers, and defaults. This is consistent with how other types (e.g.,BlockDeviceMapping,SubnetSelectorTerm) are defined locally in the HyperShift API rather than importing from upstream, sinceapi/is a separate Go module consumed by external clients.Checklist:
Summary by CodeRabbit
New Features
Behavior
Tests