Skip to content
This repository has been archived by the owner on May 18, 2021. It is now read-only.

feat: role flag for exec #202

Closed

Conversation

aadityasondhi
Copy link
Contributor

This lets the user specify a role ARN with the -r flag to assume the
specified role.

cmd/exec.go Outdated
@@ -44,6 +45,7 @@ func init() {
RootCmd.AddCommand(execCmd)
execCmd.Flags().DurationVarP(&sessionTTL, "session-ttl", "t", time.Hour, "Expiration time for okta role session")
execCmd.Flags().DurationVarP(&assumeRoleTTL, "assume-role-ttl", "a", time.Hour, "Expiration time for assumed role")
execCmd.Flags().StringVarP(&assumeRole, "assume-role", "r", "", "Assume a specified role using the role ARN")

Choose a reason for hiding this comment

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

I would make this assume-role-arn to be more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made the change

Choose a reason for hiding this comment

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

I was thinking the CLI flag should be --assume-role-arn for clarity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I misunderstood your comment. Will change to --assume-role-arn

This lets the user specify a role ARN with the `-r` flag to assume the
specified role.
@@ -124,7 +126,9 @@ func execRun(cmd *cobra.Command, args []string) error {
return err
}

if _, ok := profiles[profile]; !ok {
if _, ok := profiles[profile]; ok {
profiles[profile]["role_arn"] = assumeRoleARN
Copy link
Contributor

Choose a reason for hiding this comment

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

This feels kind of weird? You're modifying the in-memory config loaded from disk.

Also doesn't this break if -r is not set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. The testing we did was with #165 also enabled, so it did not break.

Could do non-empty check and skip if empty. Thoughts?

@nickatsegment
Copy link
Contributor

What's the use case here? Why not just write a new profile?

@aadityasondhi
Copy link
Contributor Author

The intended use case is when using aws-okta through automation scripts rather than a user. A lot easier to pass a role than creating a new profile.

@switj
Copy link
Contributor

switj commented Sep 24, 2019

This PR has been replaced by #219 and can be closed.

@nickatsegment I'll respond to your question on the usecase in the other PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants