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

aws: Set IMDS defaults for existing clusters #14879

Merged
merged 3 commits into from
Jan 11, 2023
Merged

aws: Set IMDS defaults for existing clusters #14879

merged 3 commits into from
Jan 11, 2023

Conversation

hakman
Copy link
Member

@hakman hakman commented Dec 26, 2022

IMDSv2 has been enabled for newly created clusters since k8s 1.22, which will be the minimum supported version in kOps 1.27. I think it's time to enable IMDSv2 by default, unless configured otherwise.

/cc @olemarkus @johngmyers

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. area/documentation area/provider/aws Issues or PRs related to aws provider labels Dec 26, 2022
@johngmyers
Copy link
Member

I think I'm leaning towards soft-changing the default. Keep the default the same for k8s <= 1.26, make the default IMDS hop 1 for k8s >= 1.27.

@hakman
Copy link
Member Author

hakman commented Dec 26, 2022 •
edited
Loading

I think I'm leaning towards soft-changing the default. Keep the default the same for k8s <= 1.26, make the default IMDS hop 1 for k8s >= 1.27.

So, make it default to optional until k8s 1.26?

@hakman hakman force-pushed the aws_imds_all_clusters branch from 4953472 to f29ff2b Compare December 26, 2022 06:32
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 26, 2022
olemarkus
olemarkus reviewed Dec 26, 2022
View reviewed changes
pkg/model/awsmodel/autoscalinggroup.go Outdated
@@ -281,10 +281,14 @@ func (b *AutoscalingGroupModelBuilder) buildLaunchTemplateTask(c *fi.CloudupMode

if ig.Spec.InstanceMetadata != nil && ig.Spec.InstanceMetadata.HTTPPutResponseHopLimit != nil {
lt.HTTPPutResponseHopLimit = ig.Spec.InstanceMetadata.HTTPPutResponseHopLimit
} else if ig.IsControlPlane() && (b.Cluster.IsKubernetesLT("1.27") || !(b.Cluster.Spec.IAM != nil && fi.ValueOf(b.Cluster.Spec.IAM.UseServiceAccountExternalPermissions))) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this to a function? My head hurts a bit from trying to read this expression :)

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 think latest change makes it a little more readable. Even if it were a function it would still be hard to even name it. :))

@hakman hakman force-pushed the aws_imds_all_clusters branch from f29ff2b to 961ae76 Compare December 26, 2022 08:12
@hakman
Copy link
Member Author

hakman commented Dec 26, 2022

/retest

johngmyers
johngmyers requested changes Dec 26, 2022
View reviewed changes
@johngmyers
Copy link
Member

I think we need a higher-bandwidth discussion.
/kind office-hours
/hold for office-hours

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/office-hours labels Dec 27, 2022
@hakman hakman force-pushed the aws_imds_all_clusters branch from 961ae76 to f490b27 Compare January 3, 2023 17:41
@hakman hakman requested a review from johngmyers January 4, 2023 05:40
@hakman
Copy link
Member Author

hakman commented Jan 4, 2023

@johngmyers I think this is ready for another pass.

johngmyers
johngmyers approved these changes Jan 7, 2023
View reviewed changes
Copy link
Member

@johngmyers johngmyers left a comment

Choose a reason for hiding this comment

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

If we backport #14913 to 1.26, this logic could be simpler.
/hold for consideration of #14913

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers

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 Jan 7, 2023
@hakman
Copy link
Member Author

hakman commented Jan 10, 2023

@johngmyers Could you remove the hold? We can improve this when we merge #14913.

@johngmyers
Copy link
Member

/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 Jan 11, 2023
@k8s-ci-robot k8s-ci-robot merged commit d247c0b into kubernetes:master Jan 11, 2023
@hakman hakman deleted the aws_imds_all_clusters branch August 7, 2023 06:45
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

@johngmyers johngmyers johngmyers approved these changes

Assignees

@johngmyers johngmyers

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. kind/office-hours lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hakman @johngmyers @k8s-ci-robot @olemarkus