OCPCLOUD-2710: Extend AWS metadata service options in MAPA - #2654
OCPCLOUD-2710: Extend AWS metadata service options in MAPA#2654RadekManak wants to merge 1 commit into
Conversation
|
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: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded IMDS state types with enabled and disabled constants. Expanded Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.12.2)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 |
yuqi-zhang
left a comment
There was a problem hiding this comment.
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.
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.
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)
|
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
f685436 to
cf20a50
Compare
cf20a50 to
47d84d5
Compare
|
/remove-lifecycle stale Rebased onto current master. @yuqi-zhang on pointers: these live in Your note matches the default Could you advise which way we should go?
Enum validation is on the types themselves (your other nit). |
|
/retest |
Add four new optional fields to MetadataServiceOptions in the AWS provider config, exposing the remaining EC2 instance metadata service (IMDS) settings that MAPA manages: - httpEndpoint: enable or disable the HTTP metadata endpoint - httpProtocolIPv6: enable or disable the IPv6 IMDS endpoint - httpPutResponseHopLimit: maximum hop limit for metadata tokens (1-64) - instanceMetadataTags: enable or disable access to instance tags via IMDS New types (HTTPEndpointState, HTTPProtocolIPv6State, InstanceMetadataTagsState) and their constants are introduced alongside the fields. All new fields use pointer types, following the convention for Go-validated provider specs where the zero value must be distinguishable from an unset field. The existing authentication field comment is updated to match the consistent style of the new fields. Generated deepcopy and OpenAPI docs are updated accordingly.
47d84d5 to
f5f2578
Compare
|
@RadekManak: 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. |
|
Ack, thanks for the update, will check with the API team for the suggested behaviour here |
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: