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

Enable IMDS IPv6 endpoint #12290

Merged
merged 2 commits into from
Sep 9, 2021
Merged

Conversation

rifelpet
Copy link
Member

@rifelpet rifelpet commented Sep 9, 2021

This is unconditionally enabled.

There is an IPv4-equivlanet "HttpEndpoint" field that defaults to enabled and we dont expose a way to disable it.

HttpProtocolIpv6 defaults to disabled, so to have it match the same behavior of HttpEndpoint we hardcode it to enabled.

Since we dont allow the ipv4 endpoint to be disabled, I dont think we need to expose a new API field to allow this to be overridden either.~

I chose not to expose it through the model since any kops upgrade to include this functionality will always have other LaunchTemplate changes that will prompt a new LT version to be written which will include this change, so we dont need to find and detect any necessary LT updates for this field.

This now enables the IPv6 endpoint whenever IPv6AddressCount is > 0

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 9, 2021
@k8s-ci-robot k8s-ci-robot added the area/provider/aws Issues or PRs related to aws provider label Sep 9, 2021
@rifelpet
Copy link
Member Author

rifelpet commented Sep 9, 2021

/test pull-kops-e2e-ipv6-conformance

/hold for comment on whether we should only enable this on ipv6 / dualstack clusters. I also havent done any extensive testing around this field and its behavior on ipv4 / dualstack / ipv6 instances

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2021
@hakman
Copy link
Member

hakman commented Sep 9, 2021

Wonderful :)

Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/hold for comment on whether we should only enable this on ipv6 / dualstack clusters. I also havent done any extensive testing around this field and its behavior on ipv4 / dualstack / ipv6 instances

I think we should enable it for now only when t.IPv6AddressCount > 0.
Usually operators want to limit access to IMDS for security reasons.

@@ -45,6 +45,7 @@ func (t *LaunchTemplate) RenderAWS(c *awsup.AWSAPITarget, a, e, changes *LaunchT
MetadataOptions: &ec2.LaunchTemplateInstanceMetadataOptionsRequest{
HttpPutResponseHopLimit: t.HTTPPutResponseHopLimit,
HttpTokens: t.HTTPTokens,
HttpProtocolIpv6: aws.String(ec2.LaunchTemplateInstanceMetadataProtocolIpv6Enabled),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state should also be read back in Find()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up adding it to the model so it can mimic IPv6AddressCount, so it is now in Find().

@olemarkus
Copy link
Member

I agree that it makes sense to only do this only for ipv6. But I'd use the IPv6Only function as that is what we do everywhere else. dualstack isn't a thing yet, and if we do dualstack many of the things behind ipv6only should change condition anyway.

@hakman
Copy link
Member

hakman commented Sep 9, 2021

I agree that it makes sense to only do this only for ipv6. But I'd use the IPv6Only function as that is what we do everywhere else. dualstack isn't a thing yet, and if we do dualstack many of the things behind ipv6only should change condition anyway.

Let's say same way it's done for the IPv6 address count.

@rifelpet
Copy link
Member Author

rifelpet commented Sep 9, 2021

/test pull-kops-e2e-ipv6-conformance
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 9, 2021
Copy link
Member

@olemarkus olemarkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2021
@k8s-ci-robot k8s-ci-robot merged commit 39eb930 into kubernetes:master Sep 9, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Sep 9, 2021
k8s-ci-robot added a commit that referenced this pull request Sep 10, 2021
…290-origin-release-1.22

Automated cherry pick of #12290: Update aws-sdk-go
@johngmyers johngmyers modified the milestones: v1.22, v1.23 Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/aws Issues or PRs related to aws provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants