Query IMDS over IPv6 if no IPv4 interface address #2453

johngmyers · 2022-01-10T06:03:59Z

Issue

Description

Queries IMDS over IPv6 if there is no non-loopback IPv4 interface address.

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

codecov-commenter · 2022-01-10T06:44:04Z

Codecov Report

Base: 54.12% // Head: 54.12% // No change to project coverage 👍

Coverage data is based on head (3c8839c) compared to base (b20f899).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2453   +/-   ##
=======================================
  Coverage   54.12%   54.12%           
=======================================
  Files         144      144           
  Lines        8268     8268           
=======================================
  Hits         4475     4475           
  Misses       3469     3469           
  Partials      324      324

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

kishorj · 2022-02-08T00:51:38Z

pkg/aws/cloud.go

@@ -44,9 +47,28 @@ type Cloud interface {

 // NewCloud constructs new Cloud implementation.
 func NewCloud(cfg CloudConfig, metricsRegisterer prometheus.Registerer) (Cloud, error) {
+	hasIPv4 := true
+	addrs, err := net.InterfaceAddrs()
+	if err == nil {


in case of error, why not return right away?
Are there situations where the net.InterfaceAddrs() return error, but we still want to continue processing?

The idea was to avoid any negative effects in existing cases. So if the process doesn't have permission to enumerate the interfaces, it would assume IPv4.

M00nF1sh · 2022-02-09T19:29:30Z

pkg/aws/cloud.go

+		hasIPv4 = false
+		for _, addr := range addrs {
+			str := addr.String()
+			if !strings.HasPrefix(str, "127.") && !strings.Contains(str, ":") {


i think this check is a bit hacky.
maybe just upgrade the sdk and let users specify AWS_EC2_METADATA_SERVICE_ENDPOINT_MODE env variable via helm chart?

Why should users have to specify AWS_EC2_METADATA_SERVICE_ENDPOINT_MODE at all?

M00nF1sh · 2022-02-09T19:30:17Z

pkg/deploy/elbv2/load_balancer_synthesizer.go

@@ -99,7 +98,7 @@ func (s *loadBalancerSynthesizer) disableDeletionProtection(lb *elbv2sdk.LoadBal
 		},
 		LoadBalancerArn: lb.LoadBalancerArn,
 	}
-	_, err := svc.ModifyLoadBalancerAttributes(input)
+	_, err := s.elbv2Client.ModifyLoadBalancerAttributes(input)


this is a good catch :D

k8s-triage-robot · 2022-08-05T06:20:26Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-09-04T06:38:29Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

johngmyers · 2022-09-10T02:55:34Z

/remove-lifecycle rotten

olemarkus · 2022-10-01T19:18:51Z

/lgtm

k8s-ci-robot · 2022-10-01T19:18:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, kishorj, olemarkus

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:

~~OWNERS~~ [kishorj]

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

M00nF1sh · 2022-10-05T22:59:16Z

to be honest, i still don't think we should do this from the controller. It should be AWSSDK-GO to automatically detect IMDS if necessary.

And this detection feels hacky to me as well, e.g. you might have IMDS only available in ipv6 but the container have some other ipv4 addresses as well(e.g. cni's dummy ipv4 addresses for ipv6 only mode).
Would be better to follow AWSSDK's practice to have the explicitly configure knob before AWSSDK have automatic detection mechanism.

johngmyers · 2022-10-05T23:27:44Z

I would also prefer this be done in AWSSDK-GO, but aws-sdk has stated, without any explanation or discussion, that they are not going to do that. So this would need to be done client-by-client. The PR aws/aws-sdk-go#4234 has gotten no attention in months.

The container having non-loopback IPv4 addresses is an obscure corner case and is not negatively impacted by this change. Such unusual situations can still be handled by setting the environment variable.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 10, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh January 10, 2022 06:04

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 10, 2022

johngmyers force-pushed the ipv6-metadata branch from bb69646 to 71a1613 Compare January 10, 2022 06:20

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2022

johngmyers force-pushed the ipv6-metadata branch from 71a1613 to a6ee3e4 Compare January 10, 2022 06:35

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2022

kishorj reviewed Feb 8, 2022

View reviewed changes

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 8, 2022

kishorj approved these changes Feb 9, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2022

kishorj removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2022

M00nF1sh reviewed Feb 9, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 7, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 5, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 4, 2022

Query IMDS over IPv6 if no IPv4 interface address

3c8839c

johngmyers force-pushed the ipv6-metadata branch from a6ee3e4 to 3c8839c Compare September 10, 2022 02:52

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 10, 2022

olemarkus approved these changes Oct 1, 2022

View reviewed changes

k8s-ci-robot assigned olemarkus Oct 1, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2022

k8s-ci-robot merged commit 18019d0 into kubernetes-sigs:main Oct 1, 2022

johngmyers deleted the ipv6-metadata branch October 23, 2022 02:49

johngmyers mentioned this pull request Jan 24, 2023

Add johngmyers as reviewer #2999

Merged

johngmyers mentioned this pull request Aug 23, 2023

Add johngmyers as approver #3356

Merged

12 tasks

Timothy-Dougherty pushed a commit to adammw/aws-load-balancer-controller that referenced this pull request Nov 9, 2023

Query IMDS over IPv6 if no IPv4 interface address (kubernetes-sigs#2453)

8a41cdf

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query IMDS over IPv6 if no IPv4 interface address #2453

Query IMDS over IPv6 if no IPv4 interface address #2453

johngmyers commented Jan 10, 2022

codecov-commenter commented Jan 10, 2022 •

edited

Loading

kishorj Feb 8, 2022

johngmyers Feb 8, 2022

M00nF1sh Feb 9, 2022

johngmyers Feb 9, 2022

M00nF1sh Feb 9, 2022

k8s-triage-robot commented Aug 5, 2022

k8s-triage-robot commented Sep 4, 2022

johngmyers commented Sep 10, 2022

olemarkus commented Oct 1, 2022

k8s-ci-robot commented Oct 1, 2022

M00nF1sh commented Oct 5, 2022 •

edited

Loading

johngmyers commented Oct 5, 2022

Query IMDS over IPv6 if no IPv4 interface address #2453

Query IMDS over IPv6 if no IPv4 interface address #2453

Conversation

johngmyers commented Jan 10, 2022

Issue

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

codecov-commenter commented Jan 10, 2022 • edited Loading

Codecov Report

kishorj Feb 8, 2022

Choose a reason for hiding this comment

johngmyers Feb 8, 2022

Choose a reason for hiding this comment

M00nF1sh Feb 9, 2022

Choose a reason for hiding this comment

johngmyers Feb 9, 2022

Choose a reason for hiding this comment

M00nF1sh Feb 9, 2022

Choose a reason for hiding this comment

k8s-triage-robot commented Aug 5, 2022

k8s-triage-robot commented Sep 4, 2022

johngmyers commented Sep 10, 2022

olemarkus commented Oct 1, 2022

k8s-ci-robot commented Oct 1, 2022

M00nF1sh commented Oct 5, 2022 • edited Loading

johngmyers commented Oct 5, 2022

codecov-commenter commented Jan 10, 2022 •

edited

Loading

M00nF1sh commented Oct 5, 2022 •

edited

Loading