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.

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

[WIP] Add option for Role session name to EKS subcommands #8994

Open
wants to merge 2 commits into
base: v2
Choose a base branch
from

Conversation

murshed-panorama
Copy link

@murshed-panorama murshed-panorama commented Oct 19, 2024

This PR is a work in progress (WIP)

Issue #, if available: #8976

Description of changes:

This PR adds a --session-name flag for the aws eks get-token and aws eks update-kubeconfig commands. This flag is meant to be used with --role-arn to give a custom Role Session Name that overrides the default session name of EKSGetTokenAuth.

Commit Summary

This commit adds a --session-name option to the eks get-token and eks
update-kubeconfig commands. Prior to this, when creating a kubeconfig
file with eks update-kubeconfig and specifying a --role-arn, there was
no way to set a custom Role Session Name. This session name was
hardcoded to "EKSGetTokenAuth". Role session names are included in the
output of commands like kubectl auth whoami and can be used to
distinguish users connecting to the EKS cluster that are using the same
--role-arn.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@@ -95,6 +95,14 @@ class GetTokenCommand(BasicCommand):
),
'required': False,
},
{
'name': 'session-name',
Copy link
Author

Choose a reason for hiding this comment

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

I would have preferred to name this option --role-session-name instead of --session-name for clarity, but that would mean having two options with names that begin with --role.

This causes an ambiguity with this section of the file update_kubeconfig.py, where it adds an option --role, which normally gets matched to --role-arn in this file.

To minimize changes and preserve backwards compatibility, I instead use --session-name for the option.

Copy link
Member

Choose a reason for hiding this comment

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

This is consistent with aws-iam-authenticators flag name, no objection to the name

@@ -174,6 +175,28 @@ def test_url_with_arn(self):
)
self.assert_url_correct(response, has_session_token=True)

def test_url_with_arn_and_session_name(self):
Copy link
Author

Choose a reason for hiding this comment

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

This is mostly copied from test_url_with_arn, with a few minor changes to accomodate the new --session-name option.

This commit adds a --session-name option to the eks get-token and eks
update-kubeconfig commands. Prior to this, when creating a kubeconfig
file with eks update-kubeconfig and specifying a --role-arn, there was
no way to set a custom Role Session Name. This session name was
hardcoded to "EKSGetTokenAuth". Role session names are included in the
output of commands like `kubectl auth whoami` and can be used to
distinguish users connecting to the EKS cluster that are using the same
--role-arn.
Copy link
Member

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -95,6 +95,14 @@ class GetTokenCommand(BasicCommand):
),
'required': False,
},
{
'name': 'session-name',
Copy link
Member

Choose a reason for hiding this comment

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

This is consistent with aws-iam-authenticators flag name, no objection to the name

awscli/customizations/eks/update_kubeconfig.py Outdated Show resolved Hide resolved
Applying changes recommended by @micahhausler

Co-authored-by: Micah Hausler <micahhausler@users.noreply.github.com>
@murshed-panorama
Copy link
Author

Another open question is whether this should be merged to the v2 branch or the develop branch. I created this branch off v2, but I would like to get confirmation if that was the right course of action.

@murshed-panorama murshed-panorama marked this pull request as ready for review October 31, 2024 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants