-
Notifications
You must be signed in to change notification settings - Fork 177
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
🎨 Release 2.0 cleanup #280
Conversation
- Breaking: clear credentials from AWS credentials file on logout - Breaking: make awscli use default (not generated) profile name - Breaking: remove config.properties from source - Breaking: remove log4j and its configuration - Clean up MFA handling code (rat nest -> mouse nest) - Allow fast reuse of existing, unexpired named sessions - Allow okta-aws {profilename} logout on macOS/Linux/*nix-likes - Address most SonarLint findings - Update third party notices (IANAL) - Update copyright to 2019 - Apply Apache 2.0 header to most files - Update dependencies to latest stable versions
@bogeylnj @aparkerlue @froshyfrosh @Joeskyyy mh-psaad mind giving this a once-over? Focus on pieces of it you are familiar with or have strong opinions about. This is a non-trivial change, but I've been pretty thorough in my manual testing outside of certain MFA factors like Okta Push, Yubikeys, and hardware tokens which I don't have to test with. @robkinyon-roivant @jeugene @duhaas2015 @au457415 @mrbelowski @tbenshalom you folks appear to be using this pretty heavily for production environments. Do you anticipate problems from the breaking changes I listed? |
@AlainODea We started using this tool in the last 4 weeks, so were not suffered from breaking-changes. |
@AlainODea we don't use any logout functionality, so the clearing won't matter - we just let credentials expire and, for the most part, re-run this tool every time a new action is happening. The log4j change won't matter to us because we don't look at the log messages in an automated way. I'd like to know more about "make awscli use default (not generated) profile name" and "remove config.properties from source" - what exactly would those do? |
@robkinyon-roivant thank you for your feedback.
awscli used to generate a profile name and save it to the credentials file if OKTA_PROFILE was not supplied. Now it will update the default profile in AWS CLI instead. This will overwrite the default profile credentials if someone was depending on those. Removing config.properties from source should have no impact on people unless they are installing it by copy-pasting config.properties from the repo and updating it instead of using the installer. |
@tbenshalom thank you.
None of these changes have been released yet. This is a proposal to go to 2.0 with breaking changes. I wanted to gather feedback on the breaking changes first to avoid seriously frustrating people :) |
So, does this mean if we supply a OKTA_PROFILE, that awscli will set that to the default profile? Will the other profiles (e.g., I have 7 in one of our AWS accounts) still be accessible? |
Yikes, is this configurable? I like to avoid having a default AWS profile in my configs to avoid accidentally running awscli commands against unintended accounts. |
@Joeskyyy thank you for your feedback.
It is not configurable in the proposed changes. This can still be changed and leaving that option open is a big motivator for having this consultation with you folks before making these radical changes. My recommendation is to use OKTA_PROFILE instead. My rationale for getting rid of awscli.java and the command is because it leads to ~/.aws/credentials clutter that it never cleans up. Is using okta-aws {profilename} a workable alternative to awscli for you? |
@AlainODea I suppose I understand the rationale. I think it's probably going to cause a lot of pain down the line, especially since most of the power users of this are going to have multiple profiles they'll be working with. I think, personally, using specific profile names is going to be much better in terms of support. Or at least being able to override not having the That being said, as long as one could do |
@robkinyon-roivant good question!
The proposed awscli command will override the OKTA_PROFILE environment variable. You will still be able to use okta-aws {profilename} ... to choose a specific profile by name. This bit is unchanged. The documentation is a bit misleading as it documents what WithOkta.java does with the configuration and environment, not what the bundled commands do. All of the existing bundled commands override OKTA_PROFILE, usually replacing it with a positional parameter. |
@bogeylnj @aparkerlue @froshyfrosh @jeugene @duhaas2015 @au457415 @mrbelowski I will be keeping this open until at least March 18th. After March 18th, I will check if there are any deal-breakers. If there are, I will work to address them. If not, I'll merge and publish v2.0.0 with these changes. |
@AlainODea Sorry I don't have the time for a deeper review, maybe next week I can have a closer look. So let me put it like this: I am using the the library only programmatically like this:
The On Monday I will ask some of my colleagues to also have a look at your proposal (maybe it's possible to wait a few more days - sorry I'm a bit late to the party). |
@froshyfrosh I can wait a few extra days if your team needs it. I can’t imagine your use case will be impacted by the breaking changes. |
@AlainODea I asked my colleagues and there are no complains from our side. Thank you very much and good luck for the big update. |
@froshyfrosh thank you for letting me know. |
Problem Statement
This codebase hasn't seen a refactor and cleanup in a while. In the interest of maintainability one should be performed.
Solution
Breaking: clear credentials from AWS credentials file on logout
Breaking: make awscli use default (not generated) profile name
Breaking: remove config.properties from source
Breaking: remove log4j and its configuration
Clean up MFA handling code (rat nest -> mouse nest)
Allow fast reuse of existing, unexpired named sessions
Allow okta-aws {profilename} logout on macOS/Linux/*nix-likes
Address most SonarLint findings
Update third party notices (IANAL)
Update copyright to 2019
Apply Apache 2.0 header to most files
Update dependencies to latest stable versions