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

Add flag to enable Simple System Manager (SSM) for easy SSH access #2754

Merged
merged 19 commits into from
Oct 20, 2020
Merged

Add flag to enable Simple System Manager (SSM) for easy SSH access #2754

merged 19 commits into from
Oct 20, 2020

Conversation

aclevername
Copy link
Contributor

@aclevername aclevername commented Oct 19, 2020 •
edited

Description

This PR add the optional to enable Simple Systems Manager (SSM) by passing the --enable-ssm flag. Once SSM is installed you can easily ssh onto nodes in the cluster by running aws ssm start-session --target <node-id> --region <region>

Manually Tested

Related issue #2743

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the userdocs directory)
  • Manually tested
  • Added labels for change area (e.g. area/nodegroup), target version (e.g. version/0.12.0) and kind (e.g. kind/improvement)
  • Make sure the title of the PR is a good description that can go into the release notes

@aclevername aclevername changed the title Enable ssm Add flag to enable Simple System Manager (SSM) for easy SSH access Oct 19, 2020
@aclevername aclevername added area/managed-nodegroup EKS Managed Nodegroups area/nodegroup kind/feature New feature or request labels Oct 19, 2020
@aclevername aclevername requested a review from cPu1 October 19, 2020 10:38
@aclevername aclevername marked this pull request as ready for review October 19, 2020 10:38
cPu1
cPu1 previously requested changes Oct 19, 2020
View reviewed changes
Copy link
Collaborator

@cPu1 cPu1 left a comment

Choose a reason for hiding this comment

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

I have left a few comments but otherwise LGTM.

One thing that's left out though is the SSM agent is not being installed for managed nodegroups. This should be handled in pkg/cfn/builder/managed_launch_template.go in makeUserData. Note that if a user specifies a custom launch template, we should disallow the ssh.enableSsm option.

pkg/cfn/builder/iam.go Outdated Show resolved Hide resolved
pkg/ctl/cmdutils/nodegroup_flags.go Outdated Show resolved Hide resolved
userdocs/src/introduction.md Outdated Show resolved Hide resolved
aclevername and others added 3 commits October 19, 2020 13:12
@aclevername @cPu1
Refactor enableSSM
e732fb1 
Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com>
@aclevername @cPu1
update enable-ssm descriptor
7c4e12c 
Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com>
@aclevername @cPu1
Update enable-ssm docs
650e5a7 
Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com>
@aclevername
Copy link
Contributor Author

aclevername commented Oct 19, 2020 •
edited

I have left a few comments but otherwise LGTM.

Thanks for the comments @cPu1 , some great spots!

One thing that's left out though is the SSM agent is not being installed for managed nodegroups. This should be handled in pkg/cfn/builder/managed_launch_template.go in makeUserData. Note that if a user specifies a custom launch template, we should disallow the ssh.enableSsm option.

Oops! Good spot. I'll update it to handle managed nodegroups and update the docs to note the behaviour for custom launch templates. Thanks 😄

@aclevername aclevername requested a review from cPu1 October 19, 2020 14:57
cPu1
cPu1 reviewed Oct 20, 2020
View reviewed changes
Copy link
Collaborator

@cPu1 cPu1 left a comment

Choose a reason for hiding this comment

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

Support for managed nodegroups looks good 👍

We shouldn't ignore enableSsm if a launch template is specified, but rather disallow its usage.

@aclevername @cPu1
Update pkg/cfn/builder/managed_nodegroup.go
568ae10 
Co-authored-by: Chetan Patwal <cPu1@users.noreply.github.com>
cPu1
cPu1 approved these changes Oct 20, 2020
View reviewed changes
Copy link
Collaborator

@cPu1 cPu1 left a comment

Choose a reason for hiding this comment

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

🎉

michaelbeaumont
michaelbeaumont approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@michaelbeaumont michaelbeaumont left a comment

Choose a reason for hiding this comment

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

🚀

@aclevername aclevername merged commit 87f83d3 into master Oct 20, 2020
@aclevername aclevername deleted the enable-ssm branch October 20, 2020 12:02
This was referenced Oct 20, 2020
Closed
Closed
@michaelbeaumont michaelbeaumont mentioned this pull request Dec 10, 2020
Merged
5 tasks
@aclevername aclevername mentioned this pull request Mar 22, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelbeaumont michaelbeaumont michaelbeaumont approved these changes

@cPu1 cPu1 cPu1 approved these changes

Assignees
No one assigned
Labels
area/managed-nodegroup EKS Managed Nodegroups area/nodegroup kind/feature New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@aclevername @michaelbeaumont @cPu1