CORS-4166: enable IPv6 IMDS endpoint and private DNS configuration for dualstack#177
CORS-4166: enable IPv6 IMDS endpoint and private DNS configuration for dualstack#177tthvo wants to merge 2 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@tthvo: This pull request references CORS-4166 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. |
WalkthroughInstance metadata and Private DNS name options for EC2 RunInstances are now derived from the cluster Infrastructure IP family. New helper isAWSDualStack and getPrivateDNSNameOptionsRequest(infra) were added; getInstanceMetadataOptionsRequest now takes infra. launchInstance passes infra and conditionally applies these options. Changes
sequenceDiagram
participant Actuator as MachineActuator
participant Infra as Infrastructure
participant Helper as OptionsHelpers
participant EC2 as AWS EC2 API
Actuator->>Infra: read IP family (infra.Status.PlatformStatus...)
Actuator->>Helper: getInstanceMetadataOptionsRequest(providerConfig, infra)
Helper->>Infra: inspect IP family
Helper-->>Actuator: InstanceMetadataOptionsRequest (or nil)
Actuator->>Helper: getPrivateDNSNameOptionsRequest(infra)
Helper->>Infra: inspect IP family
Helper-->>Actuator: PrivateDnsNameOptionsRequest (or nil)
Actuator->>EC2: RunInstances(Input with MetadataOptions?, PrivateDnsNameOptions?)
EC2-->>Actuator: RunInstancesOutput / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
[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 |
|
@tthvo: This pull request references CORS-4166 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 (2)
pkg/actuators/machine/instances.go (1)
791-795: Extract dual-stack infra detection to a shared helper.The same nested infra/IPFamily check appears twice. Consolidating it avoids future drift and keeps both option builders in lockstep.
♻️ Suggested refactor
+func isDualStackAWSIPFamily(infra *configv1.Infrastructure) bool { + return infra != nil && + infra.Status.PlatformStatus != nil && + infra.Status.PlatformStatus.AWS != nil && + (infra.Status.PlatformStatus.AWS.IPFamily == configv1.DualStackIPv6Primary || + infra.Status.PlatformStatus.AWS.IPFamily == configv1.DualStackIPv4Primary) +} + func getInstanceMetadataOptionsRequest(providerConfig *machinev1beta1.AWSMachineProviderConfig, infra *configv1.Infrastructure) *ec2.InstanceMetadataOptionsRequest { imdsOptions := &ec2.InstanceMetadataOptionsRequest{} @@ - if infra != nil && infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.AWS != nil { - if infra.Status.PlatformStatus.AWS.IPFamily == configv1.DualStackIPv6Primary || infra.Status.PlatformStatus.AWS.IPFamily == configv1.DualStackIPv4Primary { - imdsOptions.HttpProtocolIpv6 = ptr.To("enabled") - } - } + if isDualStackAWSIPFamily(infra) { + imdsOptions.HttpProtocolIpv6 = ptr.To("enabled") + } @@ func getPrivateDnsNameOptionsRequest(providerConfig *machinev1beta1.AWSMachineProviderConfig, infra *configv1.Infrastructure) *ec2.PrivateDnsNameOptionsRequest { privateDnsNameOptions := &ec2.PrivateDnsNameOptionsRequest{} - if infra != nil && infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.AWS != nil { - if infra.Status.PlatformStatus.AWS.IPFamily == configv1.DualStackIPv6Primary || infra.Status.PlatformStatus.AWS.IPFamily == configv1.DualStackIPv4Primary { - privateDnsNameOptions.EnableResourceNameDnsARecord = ptr.To(true) - privateDnsNameOptions.EnableResourceNameDnsAAAARecord = ptr.To(true) - privateDnsNameOptions.HostnameType = ptr.To("resource-name") - } - } + if isDualStackAWSIPFamily(infra) { + privateDnsNameOptions.EnableResourceNameDnsARecord = ptr.To(true) + privateDnsNameOptions.EnableResourceNameDnsAAAARecord = ptr.To(true) + privateDnsNameOptions.HostnameType = ptr.To("resource-name") + }Also applies to: 807-815
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/actuators/machine/instances.go` around lines 791 - 795, Extract the nested infra/IPFamily check into a shared helper (e.g., infraIsAWSDualStack or supportsAWSDualStack) that accepts the infra object and returns a bool if infra != nil && infra.Status.PlatformStatus != nil && infra.Status.PlatformStatus.AWS != nil and AWS.IPFamily equals configv1.DualStackIPv6Primary or configv1.DualStackIPv4Primary; then replace the duplicate blocks that set imdsOptions.HttpProtocolIpv6 and the other option builder (the second block around lines 807-815) to call this helper instead so both code paths use the same centralized check.pkg/actuators/machine/instances_test.go (1)
1480-1567: Consider one launch-path test that asserts both options are propagated together.These helper tests are good; adding a single
launchInstancedualstack case would catch regressions where one of the two helper outputs stops being wired intoRunInstancesInput.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/actuators/machine/instances_test.go` around lines 1480 - 1567, Add a test that ensures the dual-stack DNS options from getPrivateDnsNameOptionsRequest are actually passed into the EC2 RunInstancesInput during launch; specifically add a LaunchInstance/launchInstance test case that uses an infra with AWS.IPFamily set to DualStackIPv6Primary or DualStackIPv4Primary, calls the function that builds RunInstancesInput (the code path around launchInstance or whatever constructs ec2.RunInstancesInput), and asserts that RunInstancesInput.PrivateDnsNameOptions contains EnableResourceNameDnsARecord=true and EnableResourceNameDnsAAAARecord=true and HostnameType="resource-name", so both helper outputs are propagated together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/actuators/machine/instances_test.go`:
- Around line 1480-1567: Add a test that ensures the dual-stack DNS options from
getPrivateDnsNameOptionsRequest are actually passed into the EC2
RunInstancesInput during launch; specifically add a
LaunchInstance/launchInstance test case that uses an infra with AWS.IPFamily set
to DualStackIPv6Primary or DualStackIPv4Primary, calls the function that builds
RunInstancesInput (the code path around launchInstance or whatever constructs
ec2.RunInstancesInput), and asserts that RunInstancesInput.PrivateDnsNameOptions
contains EnableResourceNameDnsARecord=true and
EnableResourceNameDnsAAAARecord=true and HostnameType="resource-name", so both
helper outputs are propagated together.
In `@pkg/actuators/machine/instances.go`:
- Around line 791-795: Extract the nested infra/IPFamily check into a shared
helper (e.g., infraIsAWSDualStack or supportsAWSDualStack) that accepts the
infra object and returns a bool if infra != nil && infra.Status.PlatformStatus
!= nil && infra.Status.PlatformStatus.AWS != nil and AWS.IPFamily equals
configv1.DualStackIPv6Primary or configv1.DualStackIPv4Primary; then replace the
duplicate blocks that set imdsOptions.HttpProtocolIpv6 and the other option
builder (the second block around lines 807-815) to call this helper instead so
both code paths use the same centralized check.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.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 (2)
pkg/actuators/machine/instances.gopkg/actuators/machine/instances_test.go
|
@tthvo: This pull request references CORS-4166 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)
pkg/actuators/machine/instances.go (1)
812-828: UnusedproviderConfigparameter.The
providerConfigparameter is accepted but never used in this function. If it's intended for future extensibility, consider documenting that intent. Otherwise, it could be removed to simplify the signature.♻️ Optional: Remove unused parameter if not needed for future use
-func getPrivateDNSNameOptionsRequest(providerConfig *machinev1beta1.AWSMachineProviderConfig, infra *configv1.Infrastructure) *ec2.PrivateDnsNameOptionsRequest { +func getPrivateDNSNameOptionsRequest(infra *configv1.Infrastructure) *ec2.PrivateDnsNameOptionsRequest {This would also require updating the call site at line 522.
Note: If the parameter is kept for API consistency with
getInstanceMetadataOptionsRequestor future extensibility, that's a valid reason to keep it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/actuators/machine/instances.go` around lines 812 - 828, The function getPrivateDNSNameOptionsRequest has an unused parameter providerConfig; either remove providerConfig from the signature and update all callers to call getPrivateDNSNameOptionsRequest() (adjust any call sites that currently pass a providerConfig), or retain providerConfig but add a clear comment above getPrivateDNSNameOptionsRequest explaining it is intentionally unused for API consistency/future extensibility (and mark it with a linter-suppress or underscore name if your linters require). Ensure any caller changes compile and run tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/actuators/machine/instances.go`:
- Around line 812-828: The function getPrivateDNSNameOptionsRequest has an
unused parameter providerConfig; either remove providerConfig from the signature
and update all callers to call getPrivateDNSNameOptionsRequest() (adjust any
call sites that currently pass a providerConfig), or retain providerConfig but
add a clear comment above getPrivateDNSNameOptionsRequest explaining it is
intentionally unused for API consistency/future extensibility (and mark it with
a linter-suppress or underscore name if your linters require). Ensure any caller
changes compile and run tests.
ℹ️ Review info
Configuration used: Repository: openshift/coderabbit/.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 (2)
pkg/actuators/machine/instances.gopkg/actuators/machine/instances_test.go
…r dualstack networking Configure IPv6 IMDS and private DNS configuration when infrastructure CR status reports dualstack IP family variants.
|
@tthvo: This pull request references CORS-4166 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. |
|
/retest |
|
@tthvo: 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. |
tthvo
changed the title
|
/verified by @tthvo See #177 (comment) |
openshift-ci-robot
added
the
verified
|
@tthvo: 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. |
5 participants
Thus, this PR configures IPv6 IMDS and private DNS configuration when infrastructure CR status reports dualstack IP family variants. Since we don't add new features to MAPI, these changes follow the pattern in #171.
Motivation
Every EC2 instance receives a private hostname, which should provide both A and AAAA records for the IPv4 and IPv6 addresses of the instance. This is only supported via "resource-name" hostname type.
Additionally, any pods that needs to assume the node assigned IAM role should be able to contact the IMDS endpoint over IPv6. The endpoint must be enabled explicitly.
Summary by CodeRabbit
New Features
Refactor
Tests