Retrieve VPC ID from EC2 instance the controller is running on if possible #2824

olemarkus · 2022-10-03T13:35:41Z

Issue

Description

Right now either metadata has to be available or one has to pass VPC ID as a flag.
This PR tries to fetch the VPC ID from the EC2 API if it can infer the instance ID from the node name.

It uses the "new" node name convention from cloud controller manager, which works well for my use case. A follow up would be to try to fetch the node object and extract the instance ID from the provider ID.

This approach was tested running LBC on instances with IMDS max-hop-limit 1 and IRSA enabled and it ran successfully without setting the vpc id flag.

The LBC Deployment requires the following:

spec:
  template:
    spec:
      containers:
      - env:
        - name: NODENAME
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: spec.nodeName

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

If this approach looks good, I can do any of the above deemed necessary.

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 🌟

M00nF1sh · 2022-10-06T17:26:06Z

pkg/aws/cloud.go

+				cfg.VpcID = *vpcID
+			}
+
+		} else {


i think it's better to use metadata first, and then fallback to use nodeName given metadata is cheaper.
We might change our default manifest to have this NODEName env injected always, doing so can save an AWS api call.

Agree! Done.

M00nF1sh · 2022-10-06T17:27:52Z

pkg/aws/cloud.go

+			output, err := ec2Service.DescribeInstances(&ec2.DescribeInstancesInput{
+				InstanceIds: []*string{&nodeName},
+			})
+			if err != nil {


we should change our logic here, if we don't have permission to describe the instance, we still should be able to fallback to metadata path.
similarly, if we check for metadata first then instance id(see my previous comment), we should change the logic to not fall when metadata is not available

Also done.
Means less meaningful errors returned, but perhaps not very important anyway.

M00nF1sh · 2022-10-06T17:30:33Z

pkg/aws/cloud.go

+	ec2Service := services.NewEC2(sess)
+
+	if len(cfg.VpcID) == 0 {
+		nodeName := os.Getenv("NODENAME")


a nit comment, given this function is a bit long,
maybe created a dedicated file like pkg/aws/utils/vpc_id.go, and have functions like

inferVPCID(sess, metadata), which invokes inferVPCIDFromNodeName inferVPCIDFromNodeName(sess, nodeName)

Didn't spot this comment, but ended up doing pretty much the same you describe here as it makes the fallback logic easier. I put the function below NewCloud though.

M00nF1sh

overall looks good to me with some nit comments

codecov-commenter · 2022-10-06T20:30:00Z

Codecov Report

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

Coverage data is based on head (331f509) compared to base (18019d0).
Patch has no changes to coverable lines.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2824   +/-   ##
=======================================
  Coverage   54.05%   54.05%           
=======================================
  Files         144      144           
  Lines        8278     8278           
=======================================
  Hits         4475     4475           
  Misses       3479     3479           
  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.

M00nF1sh · 2022-10-12T22:20:41Z

pkg/aws/cloud.go

@@ -114,9 +108,19 @@ func NewCloud(cfg CloudConfig, metricsRegisterer prometheus.Registerer) (Cloud,
 		metricsCollector.InjectHandlers(&sess.Handlers)
 	}

+	ec2Service := services.NewEC2(sess)
+
+	if len(cfg.VpcID) == 0 {


nit: i think we should use same check here, either len(cfg.VpcID) ==0 or vpcID == ""

You were referring to this one and the one two lines below? The latter was replaced by an err check now that the infer function returns errors.

M00nF1sh · 2022-10-12T22:31:41Z

pkg/aws/cloud.go

@@ -126,6 +130,37 @@ func NewCloud(cfg CloudConfig, metricsRegisterer prometheus.Registerer) (Cloud,
 	}, nil
 }

+func getVPC(metadata services.EC2Metadata, ec2Service services.EC2) string {


this breaks the existing experience as it hide the error if metadata is not available since it hides the error.
I think change to below will be better:

inferVPCID(metadata, ec2Service) (vpcID, error) { var errors []err vpcID, err := metadata.vpcID() if err == nil { return vpcID } errors = append(errors, errors.Wrap(err, "cannot infer VPCID from metadata")) vpcID, err := inferVPCIDFromNodeName(...) if err == nil { return vpcID } errors = append(errors, err) return "", errors.NewAggregate(errors) # https://github.com/kubernetes/apimachinery/blob/478dd6ed1ec9b4cc3459dcd4f1d772013ac0103c/pkg/util/errors/errors.go#L46 }

Ouh. Apimachinery has it's own error aggregate? Today I learnt. Will fix.

pkg/aws/cloud.go

…sible

olemarkus · 2022-10-19T18:12:12Z

Anything more needed here?

M00nF1sh

/lgtm
/approve

Thanks :D now it looks good to me, we can include this in next release.

k8s-ci-robot · 2022-10-21T22:54:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh, 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~~ [M00nF1sh]

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 3, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh October 3, 2022 13:36

M00nF1sh reviewed Oct 6, 2022

View reviewed changes

olemarkus force-pushed the vpc-from-nodename branch from 868c9ee to 331f509 Compare October 6, 2022 20:22

olemarkus requested review from M00nF1sh and removed request for kishorj October 6, 2022 20:27

M00nF1sh reviewed Oct 12, 2022

View reviewed changes

pkg/aws/cloud.go Show resolved Hide resolved

Retrieve VPC ID from EC2 instance the controller is running on if pos…

882a301

…sible

olemarkus force-pushed the vpc-from-nodename branch from 331f509 to 882a301 Compare October 13, 2022 06:50

olemarkus requested a review from M00nF1sh October 13, 2022 07:19

M00nF1sh approved these changes Oct 21, 2022

View reviewed changes

k8s-ci-robot assigned M00nF1sh Oct 21, 2022

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

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

k8s-ci-robot merged commit fb1f93b into kubernetes-sigs:main Oct 22, 2022

olemarkus mentioned this pull request Nov 13, 2022

Bump AWS Load Balancer Controller to 2.4.5 kubernetes/kops#14540

Merged

niooss-ledger mentioned this pull request Mar 31, 2025

Set http_put_response_hop_limit = 1 by default terraform-aws-modules/terraform-aws-eks#3333

Open

Retrieve VPC ID from EC2 instance the controller is running on if possible #2824

Retrieve VPC ID from EC2 instance the controller is running on if possible #2824

Uh oh!

Conversation

olemarkus commented Oct 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue

Description

Checklist

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

Uh oh!

M00nF1sh Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Oct 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

M00nF1sh left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 6, 2022

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

olemarkus commented Oct 19, 2022

Uh oh!

M00nF1sh left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Oct 21, 2022

Uh oh!

Uh oh!

olemarkus commented Oct 3, 2022 •

edited

Loading

M00nF1sh Oct 6, 2022 •

edited

Loading

M00nF1sh Oct 6, 2022 •

edited

Loading