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.

Sign up for GitHub

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

Jump to bottom

Default IMDSv2 to "optional" for AWS #10655

Merged
merged 3 commits into from
Jan 29, 2021
Merged

Default IMDSv2 to "optional" for AWS #10655

merged 3 commits into from
Jan 29, 2021

Conversation

hakman
Copy link
Member

@hakman hakman commented Jan 25, 2021

IMDSv2 breaks any setup using Flatcar or Debian 9 as base image and seems to be the reason kube-router tests stoped working. The change, even though recommended by AWS seems to be highly disruptive in our case and for now should remain optional.

/cc @rifelpet @olemarkus

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 25, 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 area/documentation area/provider/aws Issues or PRs related to aws provider approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 25, 2021
@rifelpet
Copy link
Member

Maybe we should default to required for new clusters only. I believe that would involve setting it in setupMasters and setupNodes here

@hakman
Copy link
Member Author

hakman commented Jan 25, 2021

@rifelpet This would mean adding the setting to each IG on creation and may look a bit odd:

  instanceMetadata:
    httpTokens: required

@hakman hakman force-pushed the imdsv2-optional branch 2 times, most recently from ce65f8e to 37a674e Compare January 25, 2021 06:35
@olemarkus
Copy link
Member

It would be interesting to know why it fails. It may be one rather wants to increase allowed number of hops.

@hakman
Copy link
Member Author

hakman commented Jan 25, 2021

It would be interesting to know why it fails. It may be one rather wants to increase allowed number of hops.

I think kube-router is because of the number of hops.
Debian 9 fails because of very old cloud-init version.
Flatcar fails because of strange way of getting userdata from Ignition: flatcar/Flatcar#220.

rifelpet
rifelpet reviewed Jan 26, 2021
View reviewed changes
upup/pkg/fi/cloudup/new_cluster.go Outdated
if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderAWS && (g.Spec.InstanceMetadata == nil || g.Spec.InstanceMetadata.HTTPTokens == nil) {
k8sVersion, err := version.ParseKubernetesVersion(cluster.Spec.KubernetesVersion)
if err == nil && version.IsKubernetesGTE("1.20", *k8sVersion) {
g.Spec.InstanceMetadata = &api.InstanceMetadataOptions{
Copy link
Member

Choose a reason for hiding this comment

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

this will overwrite InstanceMetadata if it already exists but HTTPTokens is nil. We should probably only create InstanceMetadata if it is nil, otherwise just set HTTPTokens and HTTPPutResponseHopLimit if they arent set.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I think this last change should do it.

olemarkus
olemarkus reviewed Jan 26, 2021
View reviewed changes
upup/pkg/fi/cloudup/new_cluster.go Outdated
@@ -674,6 +674,16 @@ func setupMasters(opt *NewClusterOptions, cluster *api.Cluster, zoneToSubnetMap
g.Spec.Zones = []string{zone}
}

if api.CloudProviderID(cluster.Spec.CloudProvider) == api.CloudProviderAWS && (g.Spec.InstanceMetadata == nil || g.Spec.InstanceMetadata.HTTPTokens == nil) {
k8sVersion, err := version.ParseKubernetesVersion(cluster.Spec.KubernetesVersion)
if err == nil && version.IsKubernetesGTE("1.20", *k8sVersion) {
Copy link
Member

Choose a reason for hiding this comment

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

Looks like it will be enabled on new clusters with k8s 1.20+. Do we really need the k8s check here when it is only for new clusters?

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 discussed a bit with @rifelpet and he is worried that this may break some existing clusters if operators don't read the release notes.
I would question a bit the requirement for 1.20+ and change to skip for debian 9 and flatcar images.

Copy link
Member

Choose a reason for hiding this comment

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

If the intention is to only do this for new clusters, it won't break any existing ones (I hope)

Copy link
Member

@rifelpet rifelpet Jan 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think historically we've made node-level cloud provider changes by only affecting new clusters and the newly supported k8s version. That way someone creating k8s 1.18 clusters wont have a change in behavior when upgrading from kops 1.18 to 1.19.

This might be especially important for changes that could break workloads like this one.

I don't nt have a particularly strong opinion about this though as long as the release notes make it abundantly clear. Given that this is for 1.20 perhaps we get more input at the mext office hours?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, Friday it is then. 😄

@kubernetes kubernetes deleted a comment from bharath-123 Jan 27, 2021
@kubernetes kubernetes deleted a comment from k8s-ci-robot Jan 27, 2021
@kubernetes kubernetes deleted a comment from bharath-123 Jan 27, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 28, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 29, 2021
@k8s-ci-robot k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 29, 2021
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 29, 2021
@rifelpet
Copy link
Member

as discussed in office hours:
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021
@k8s-ci-robot k8s-ci-robot merged commit dda4fb1 into kubernetes:master Jan 29, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.20 milestone Jan 29, 2021
@hakman hakman deleted the imdsv2-optional branch February 18, 2021 08:35
@rifelpet rifelpet mentioned this pull request Apr 27, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olemarkus olemarkus olemarkus left review comments

@rifelpet rifelpet rifelpet left review comments

Assignees

@rifelpet rifelpet

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation 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/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.20
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@hakman @k8s-ci-robot @rifelpet @olemarkus