Add support for AWS IMDS v2 #10324

bharath-123 · 2020-11-26T19:16:42Z

This PR adds support for enabling instance metadata v2 support on aws kops clusters. This can enable users to allow only IMDv2 on their instances to securely fetch their instance metadata.

I have tested this out by creating kops clusters and ssh into the nodes to check whether IMDv1 was unauthorized or not.

Have added support for launch configuration and launch templates for now.

Fixes: 9970

k8s-ci-robot · 2020-11-26T19:16:50Z

Hi @bharath-123. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 kubernetes/test-infra repository.

bharath-123 · 2020-11-26T19:16:54Z

/assign @rifelpet

bharath-123 · 2020-11-26T19:30:06Z

I will rebase the PR. forgot to run make gofmt and co :)

rifelpet

The new API fields look good to me but I'll let others review as well. Thanks for doing this!

Let me know if you need help with the new cloudmock functions.

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go

bharath-123 · 2020-11-28T13:51:24Z

@rifelpet I think this should be feature complete. Have fixed up the integration tests by mocking CreateLaunchTemplateVersion in the cloudmock. Do review it

rifelpet · 2020-11-28T14:14:12Z

/ok-to-test

olemarkus · 2020-11-28T14:28:03Z

pkg/apis/kops/validation/aws.go

+				" for more details"))
+	}
+
+	if httpPutResponseLimit <= 0 || httpPutResponseLimit > 64 {


Would 1 actually work with kops? Don't we have containers that use the IMDS?

are you referencing to IMDv1 or httpPutResponseLimit?

I believe your referring to the httpPutHopReponseLimit if i m right. I was not aware of the fact that we had some containers using IMDS in kops. I will cross check this with aws documentation.

I believe the IMDS is a special link local address as stated in the aws docs. This may not be an issue. But I am not sure whether overlay networks like Calico will subtract the ttl before sending it out of the cluster.

I believe AWS kept the default ttl 1 to mitigate issues against misconfigured NAT and router instances.
IMDv2

I refer to HttpPutResponseHopLimit yes. TTL of 1 would mean no routing. So pods with hostNetwork will work, but pods using overlay IPs will not. CNIs using ENIs (lyft, cilium-eni, vpc) will probably also work, while those using overlay networks will not.

thanks for this insight.

@olemarkus correct me if I wrong here. when i grepped for 169.254.169.254 ip, i see it being used only here https://github.com/kubernetes/kops/blob/master/upup/models/cloudup/resources/addons/metadata-proxy.addons.k8s.io/v0.1.12.yaml
in the context of a container. hostNetworking is enabled for this container. I think a ttl of 1 should work in this case, if this is the only container in kops which is accessing the instance metadata service.

Fairly sure there are more containers using it, but I cannot think of anything using overlay network.
Easy enough to change later if this fails.

got it. makes sense.

pkg/apis/kops/instancegroup.go

olemarkus · 2020-11-30T07:32:40Z

Could you change References: #9970 to Fixes #9970? Github will link the PR and issue then.

bharath-123 · 2020-12-02T20:44:44Z

@olemarkus apologies for the late reply. We just finished doing an A/B upgrade of our production k8s clusters. had no sleep for 2 days :)

I actually believe it is already linked to the issue. I just felt that this PR does not fix anything as much as it is adding a new feature.

I shall change it regardless.

hakman

@bharath-123 I found a few ways to simplify things and also added various comments changes. After adding these changes, please squash the commits to be easier to do another review pass.

cloudmock/aws/mockec2/launch_templates.go

pkg/apis/kops/instancegroup.go

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_cloudformation.go

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_terraform.go

pkg/apis/kops/validation/instancegroup_test.go

tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml

upup/pkg/fi/cloudup/awstasks/launchconfiguration.go

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_cloudformation.go

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_terraform.go

hakman

Nice. I think these are pretty much the last changes from my point of view. Please squash the code changes into a single commit.

@rifelpet @olemarkus please take another look at this before merging.

pkg/apis/kops/v1alpha2/instancegroup.go

upup/pkg/fi/cloudup/awstasks/launchtemplate.go

A new field is add to the InstanceGroup spec with 2 sub fields, HTTPPutResponseHopLimit and HTTPTokens. These fields enable the user to disable IMDv1 for instances within an instance group. By default, both IMDv1 and IMDv2 are enabled in instances in an instance group.

hakman · 2020-12-08T06:12:40Z

/lgtm
/approve

k8s-ci-robot · 2020-12-08T06:13:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bharath-123, 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:

~~OWNERS~~ [hakman]

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

…-upstream-release-1.19 Automated cherry pick of #10324: Add support for AWS IMDS v2

seh · 2020-12-09T13:37:44Z

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_terraform.go

+		MetadataOptions: &terraformLaunchTemplateInstanceMetadata{
+			HTTPTokens:              e.HTTPTokens,
+			HTTPPutResponseHopLimit: e.HTTPPutResponseHopLimit,
+		},


I've found that this breaks using Terraform 0.12.26, with an error message like this:

Error: error creating Launch Template (lt-0f27a1c3cb68aba0f) Version: InvalidParameterValue: A value of ‘’ is not valid for http-endpoint. Valid values are ‘enabled’ or ‘disabled’. status code: 400, request id: fd0c6d8c-ad70-4877-9adf-00e6e35f2edc on kubernetes.tf line 695, in resource "aws_launch_template" "bastions-redacted": 695: resource "aws_launch_template" "bastions-redacted" {

There's an open Terraform issue describing this problem: hashicorp/terraform-provider-aws#12564. If I add the "http_endpoint" attribute in manually, it works. The documentation for that attribute says that it's optional with a reasonable default value, but in practice either Terraform is not sending a value at all, or it's sending the wrong value.

Using terraform is such a joy... Never a dull moment. 😁

Please see #10393.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 26, 2020

k8s-ci-robot assigned rifelpet Nov 26, 2020

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 26, 2020

k8s-ci-robot requested review from gjtempleton and mikesplain November 26, 2020 19:17

k8s-ci-robot added area/api area/provider/aws Issues or PRs related to aws provider labels Nov 26, 2020

rifelpet reviewed Nov 27, 2020

View reviewed changes

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go Outdated Show resolved Hide resolved

bharath-123 force-pushed the feature/aws-imdv2 branch from 04fbe84 to 681ab1d Compare November 28, 2020 13:22

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 28, 2020

bharath-123 changed the title ~~[WIP] Add instance metadata v2 support on aws~~ Add instance metadata v2 support on aws kops clusters Nov 28, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 28, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 28, 2020

olemarkus reviewed Nov 28, 2020

View reviewed changes

hakman requested changes Nov 29, 2020

View reviewed changes

pkg/apis/kops/instancegroup.go Outdated Show resolved Hide resolved

hakman requested changes Dec 2, 2020

View reviewed changes

bharath-123 force-pushed the feature/aws-imdv2 branch from 16f6d5c to 25338c0 Compare December 3, 2020 21:28

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 3, 2020

hakman reviewed Dec 4, 2020

View reviewed changes

tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml Outdated Show resolved Hide resolved

hakman reviewed Dec 4, 2020

View reviewed changes

upup/pkg/fi/cloudup/awstasks/launchconfiguration.go Outdated Show resolved Hide resolved

hakman reviewed Dec 4, 2020

View reviewed changes

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_api.go Outdated Show resolved Hide resolved

hakman reviewed Dec 4, 2020

View reviewed changes

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_cloudformation.go Outdated Show resolved Hide resolved

hakman reviewed Dec 4, 2020

View reviewed changes

upup/pkg/fi/cloudup/awstasks/launchtemplate_target_terraform.go Outdated Show resolved Hide resolved

bharath-123 force-pushed the feature/aws-imdv2 branch from d9b29b9 to 9386119 Compare December 4, 2020 09:21

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 4, 2020

hakman requested changes Dec 4, 2020

View reviewed changes

pkg/apis/kops/v1alpha2/instancegroup.go Outdated Show resolved Hide resolved

pkg/apis/kops/v1alpha2/instancegroup.go Outdated Show resolved Hide resolved

bharath-123 force-pushed the feature/aws-imdv2 branch from 9386119 to eaf5018 Compare December 4, 2020 11:42

hakman reviewed Dec 4, 2020

View reviewed changes

upup/pkg/fi/cloudup/awstasks/launchtemplate.go Outdated Show resolved Hide resolved

bharath-123 force-pushed the feature/aws-imdv2 branch 2 times, most recently from 881df27 to f5a4d19 Compare December 4, 2020 14:16

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

bharath-123 added 2 commits December 7, 2020 02:57

update integration tests

ee5d8a3

bharath-123 force-pushed the feature/aws-imdv2 branch from f5a4d19 to ee5d8a3 Compare December 6, 2020 21:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2020

k8s-ci-robot assigned hakman Dec 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 8, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 8, 2020

hakman changed the title ~~Add instance metadata v2 support on aws kops clusters~~ Add support for AWS IMDS v2 Dec 8, 2020

k8s-ci-robot merged commit bee16c0 into kubernetes:master Dec 8, 2020

k8s-ci-robot added this to the v1.20 milestone Dec 8, 2020

hakman mentioned this pull request Dec 8, 2020

Automated cherry pick of #10324: Add support for AWS IMDS v2 #10384

Merged

k8s-ci-robot added a commit that referenced this pull request Dec 8, 2020

Merge pull request #10384 from hakman/automated-cherry-pick-of-#10324…

6bbc4ba

…-upstream-release-1.19 Automated cherry pick of #10324: Add support for AWS IMDS v2

seh reviewed Dec 9, 2020

View reviewed changes

seh mentioned this pull request Dec 9, 2020

AWS launch template metadata options lacking "http_endpoint" attribute fails with Terraform #10393

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for AWS IMDS v2 #10324

Add support for AWS IMDS v2 #10324

bharath-123 commented Nov 26, 2020 •

edited

Loading

k8s-ci-robot commented Nov 26, 2020

bharath-123 commented Nov 26, 2020

bharath-123 commented Nov 26, 2020

rifelpet left a comment •

edited

Loading

bharath-123 commented Nov 28, 2020

rifelpet commented Nov 28, 2020

olemarkus Nov 28, 2020

bharath-123 Nov 28, 2020

bharath-123 Nov 28, 2020

bharath-123 Nov 28, 2020

olemarkus Nov 30, 2020

bharath-123 Dec 2, 2020

olemarkus Dec 3, 2020

bharath-123 Dec 3, 2020

olemarkus commented Nov 30, 2020

bharath-123 commented Dec 2, 2020

hakman left a comment

hakman left a comment

hakman commented Dec 8, 2020

k8s-ci-robot commented Dec 8, 2020

seh Dec 9, 2020

hakman Dec 9, 2020 •

edited

Loading

seh Dec 9, 2020

Add support for AWS IMDS v2 #10324

Add support for AWS IMDS v2 #10324

Conversation

bharath-123 commented Nov 26, 2020 • edited Loading

k8s-ci-robot commented Nov 26, 2020

bharath-123 commented Nov 26, 2020

bharath-123 commented Nov 26, 2020

rifelpet left a comment • edited Loading

Choose a reason for hiding this comment

bharath-123 commented Nov 28, 2020

rifelpet commented Nov 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olemarkus commented Nov 30, 2020

bharath-123 commented Dec 2, 2020

hakman left a comment

Choose a reason for hiding this comment

hakman left a comment

Choose a reason for hiding this comment

hakman commented Dec 8, 2020

k8s-ci-robot commented Dec 8, 2020

Choose a reason for hiding this comment

hakman Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharath-123 commented Nov 26, 2020 •

edited

Loading

rifelpet left a comment •

edited

Loading

hakman Dec 9, 2020 •

edited

Loading