Skip to content
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

fix(ec2): Check if IMDS url is IPv6 before enabling ephemeral ipv6 #5826

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jinkkkang
Copy link
Contributor

@jinkkkang jinkkkang commented Oct 15, 2024

Proposed Commit Message

fix(ec2): Check if IMDS url is IPv6 before enabling ephemeral ipv6

EphemeralIPNetwork sets up networking in the init-local stage but 
enables IPv6 without verifying if the data source supports it. 
Blindly enabling IPv6 on the network card on Alibaba Cloud 
(which uses ec2 as datasource) causes continuous failures to obtain
metadata due to missing network settings. This commit adds a check to
ensure the IMDS urls being used are IPv6 before trying to bring up
ephemeral ipv6 networking.

Additional Context

Test Steps

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@jinkkkang
Copy link
Contributor Author

alibabacloud-cloud-init.log

cloud-init log

@a-dubs
Copy link
Collaborator

a-dubs commented Oct 15, 2024

Thank you for contributing this code and for providing a log for further context! Would you mind cleaning up the proposed commit message to be wrapped at <72 chars wide, have proper punctuation and to remove any mention of the attaching of the log file to the PR?

Here is my suggestion for the proposed commit message:

fix(ec2): Check if IMDS url is IPv6 before enabling ephemeral ipv6

EphemeralIPNetwork sets up networking in the init-local stage but 
enables IPv6 without verifying if the data source supports it. 
Blindly enabling IPv6 on the network card on Alibaba Cloud 
(which uses ec2 as datasource) causes continuous failures to obtain
metadata due to missing network settings. This commit adds a check to
ensure the IMDS urls being used are IPv6 before trying to bring up
ephemeral ipv6 networking.

@jinkkkang
Copy link
Contributor Author

Thank you for your reply, commit updated @a-dubs

@jinkkkang jinkkkang changed the title fix: Add check if the data source supports IPv6 when perform dhcp fix(ec2): Check if IMDS url is IPv6 before enabling ephemeral ipv6 Oct 15, 2024
@a-dubs
Copy link
Collaborator

a-dubs commented Oct 15, 2024

Awesome, thank you! @jinkkkang

@holmanb
Copy link
Member

holmanb commented Oct 15, 2024

@jinkkkang Thanks for this contribution!

I think that this would break if a metadata url is provided that is a domain name which points to an IPv6 address. I don't think that this is the case with Ec2, but given the many lookalikes it is hard to tell in advance who this could break.

@jinkkkang
Copy link
Contributor Author

alibaba_cloud_without_dhclient_cloud-init.log

alibaba_cloud_without_dhclient_with_this_commit_cloud-init.log

aws_ec2_without_dhclient_cloud-init.log

The original problem was that on Alibaba Cloud, when the dhclient was not installed,
it would take a long time to fail in the init local phase. Another log is the modified log
of this commit. I can also reproduce this issue by testing on AWS. Although the IMDS url in EC2
supports IPv6, it is actually not available (document shows The instance must be a Nitro-based
instance launched in a subnet that supports IPv6. ).
This commit can solve the problem with Alibaba Cloud (not support ipv6 IMDS url),
but it cannot solve the problem with AWZ EC2 init-local fails too long
(without dhclient and not support ipv6 instance).

The ideal solution is to check the real access address to see if IPv6 url is accessible to determine this parameter, but this will cause the action of accessing meta data to be advanced. Therefore, I think it is sufficient to only determine whether the URL address is IPv6 to determine this parameter

@jinkkkang
Copy link
Contributor Author

Please help me review again and let me know if there are any better optimization suggestions @holmanb

@jinkkkang
Copy link
Contributor Author

@holmanb @TheRealFalcon hello, Please help merge if there are no issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants