-
Notifications
You must be signed in to change notification settings - Fork 584
OCPCLOUD-2710: Extend AWS metadata service options in MAPA #2654
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Add httpEndpoint, httpPutResponseHopLimit, and instanceMetadataTags fields to MetadataServiceOptions to bring parity with CAPA. This allows users to configure additional aspects of the AWS Instance Metadata Service (IMDS) on machine instances, improving backwards compatibility and reducing conversion errors when these options are used in CAPA. New fields: - httpEndpoint: Enable/disable HTTP metadata endpoint (Enabled/Disabled) - httpPutResponseHopLimit: Configure metadata token hop limit (1-64) - instanceMetadataTags: Enable/disable instance tag access from metadata
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@RadekManak: This pull request references OCPCLOUD-2710 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. |
|
Hello @RadekManak! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThis pull request extends the MetadataServiceOptions structure in the AWS machine provider configuration by introducing three new fields and two new enum types. HTTPEndpointState and InstanceMetadataTagsState enums are added with Disabled and Enabled constants. The MetadataServiceOptions struct gains three new fields: HTTPEndpoint (HTTPEndpointState pointer), HTTPPutResponseHopLimit (int64 pointer), and InstanceMetadataTags (InstanceMetadataTagsState pointer). Corresponding generated files for deep copying, Swagger documentation, and OpenAPI schema definitions are updated to reflect these additions. 🚥 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
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration 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 (4)
🧰 Additional context used🧬 Code graph analysis (1)machine/v1beta1/zz_generated.deepcopy.go (1)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting 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 |
|
[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 |
|
@RadekManak: 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. |
yuqi-zhang
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally seems fine, a couple of comments inline
Also to double check, there is no other reference to these objects in the api repo correct? i.e. the actual CRDs would be deployed via e.g. openshift/machine-api-provider-aws#170 ?
| // When set to Disabled, you cannot access your instance metadata. | ||
| // When omitted, the value is determined by account-level settings in the AWS Region, or the AWS service default if not configured at the account level. | ||
| // The typical AWS service default is Enabled. | ||
| // +kubebuilder:validation:Enum=Enabled;Disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since we are defining a new type (HTTPEndpointState), we should probably validate on the type itself, so we don't have to duplicate validation if other places want to use it.
That said since this is the only occurrence, and we do similar validation for e.g. MetadataServiceAuthentication, we can keep it for consistency purposes.
| // The typical AWS service default is Enabled. | ||
| // +kubebuilder:validation:Enum=Enabled;Disabled | ||
| // +optional | ||
| HTTPEndpoint *HTTPEndpointState `json:"httpEndpoint,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that all 3 new fields do not need to be pointers. There's no need to distinguish between an omittited and empty value (they cannot be empty anyways due to the validations)
Add httpEndpoint, httpPutResponseHopLimit, and instanceMetadataTags fields to MetadataServiceOptions to bring parity with CAPA. This allows users to configure additional aspects of the AWS Instance Metadata Service (IMDS) on machine instances, improving backwards compatibility and reducing conversion errors when these options are used in CAPA.
New fields: