awscli-login icon indicating copy to clipboard operation
awscli-login copied to clipboard

Add V2 support (WIP)

Open ddriddle opened this issue 2 years ago • 9 comments

Plan is to drop the daemon, and use an external script for rotating the credentials.

Things left TODO:

  • [ ] Fix failure to overwrite existing keys when setting credential_process in ~/.aws/credentials during aws configure
  • [ ] Update documentation for V2 (See here for an example)
  • [x] Add Python 3.11 to testing matrix
  • [x] Drop awscli dependency
  • [x] Test with awscli v2 (See Upgrade docs)
  • [x] Split awscli_login into two or three submodules.
    • [x] Need a submodule that contains the plugins login, logout, and configure
  • [x] Add error handler to aws_get_credentials
  • [x] Rename aws_get_credentials to aws-login-credentials
  • [ ] Add warning to user if keys are set in profile. Warn on login and configure (Should this be in a separate PR? Need to think about this.)
  • [ ] Add unit test for remove_credentials
  • [ ] Improve integration test for logout to reflect new behavior.

Documentation:

  • [ ] Update README.md documentation (Need to mention V2, need upgrade instructions tested with both V1 and V2)
  • [ ] Review aws logon help, aws login configure help, and aws logout help
  • [ ] Update login, configure, and logout online documentation as needed

Testing left TODO:

  • [x] Need unit tests for raise_if_logged_out
  • [ ] Update integration tests to also test aws v2 (aws v1 tests added here https://github.com/techservicesillinois/awscli-login/pull/120)
    • [ ] Add new section in github actions to run aws v2 integration tests. I recommend right before publishing but after unit tests and integration tests for aws v1. Only one run is needed (not per version of Python) since aws v2 bundles its own Python.

Stretch Goals (DO NOT DO IN THIS PR):

  • [ ] Add flag --all so that aws logout --all clears all credentials & roles in ~/.aws-logon/credentials.
  • [ ] Add flag to remove cookies to force reauth with Shibboleth. Maybe --force to aws logout?
  • [ ] Add error handling to login role selection
  • [ ] Add optional aliases for account numbers
  • [ ] Add default value for ECP value for aws login configure. Pull from default or other existing profile.
  • [ ] Tab completion for aws login configure would be awesome. Pull from any available profile.

ddriddle avatar Jul 13 '22 19:07 ddriddle

End of session today.

Note: using awscli V2

$ aws login
No module named 'lxml.etree'

tzturner avatar Aug 17 '22 21:08 tzturner

We may have hit a bug in awscli plugin support.

PS C:\src\aws-cli> git diff 5cdf69f5c562f1adc07ba95aa033c69dfe5e12f1 d3053cf6429bcffa0507e4aaf411486f61bd89d3 -- .\awscli\plugin.py 
diff --git a/awscli/plugin.py b/awscli/plugin.py
index f993ec1f7..d47128a44 100644
--- a/awscli/plugin.py
+++ b/awscli/plugin.py
@@ -46,6 +46,10 @@ def _import_plugins(plugin_names):
     plugins = []
     for name, path in plugin_names.items():
         log.debug("Importing plugin %s: %s", name, path)
-        if '.' not in name:
+        if '.' not in path:
             plugins.append(__import__(path))
+        else:
+            package, module = path.rsplit('.', 1)
+            module = __import__(path, fromlist=[module])
+            plugins.append(module)
     return plugins

Note that this change from name to path doesn't look intentional, and appears to be at least the immediate reason our automated test fails. Not sure at this time why local testing can pass, while testing on CI/CD fails. So this might not be the true reason.

-        if '.' not in name:
+        if '.' not in path:

But the closest open issue I could find is: https://github.com/aws/aws-cli/issues/2350

Need to take a closer look before opening a pull request with awscli, as this might just be the surface level place whatever is happening in our tests goes sideways.'

Update: Stepping through, no bug in AWS CLI here. name is set to login, path is set to aswcli_login.plugin.

Then it calls plugins.append(__import__('awscli_login.plugin', fromlist=['plugin']))

(confirmed this with an equality check in pdb)

Runs fine on @zdc217's machine, but goes boom in the CI/CD.

Update with carefully considered technical conclusion: ¯\_(ツ)_/¯

edthedev avatar Dec 15 '22 21:12 edthedev

We're going to add this to our CI/CD to check if there's an import issue being buried by other layers of code:

PYTHONPATH=/home/zdc/dev/projects/awscli-login/venv2/lib/python3.11/site-packages `head -n1 $(which aws) | sed 's/#!//'` -c "print(__import__('awscli_login.plugin', fromlist=['plugin']))"

Result: the distributed version cannot be head -n1-ed because it's compiled.

edthedev avatar Dec 16 '22 17:12 edthedev

Probably next step: do a manually install of AWS into our CI/CD to test more directly. https://statics.teams.cdn.office.net/evergreen-assets/safelinks/1/atp-safelinks.html

edthedev avatar Dec 16 '22 17:12 edthedev

Team discussion update - to close this out, we will need to pivot away from any reliance on plugin mode, since the official support from AWS is not there on V2. Don't anticipate significant usability impact, just need to think through and make the code changes.

  • Make use as a plugin completely optional.
  • Clarify in README that use in plugin mode with AWS CLI V2 is not officially supported.

edthedev avatar Jan 26 '23 16:01 edthedev

Per discussion today, we're looking to pivot toward a version that does not rely on the plugin system for V2. Let's refocus #106 on dropping the refresh-daemon.

edthedev avatar Feb 06 '23 19:02 edthedev

This pull request - https://github.com/techservicesillinois/awscli-login/pull/149 targets this branch, in order to speed up debugging test issues here.

edthedev avatar Aug 14 '23 20:08 edthedev

Nota bene the config step needs to cleanup empty aws_access_key_id fields otherwise errors are given by aws. For example the following credentials file:

[default]
aws_access_key_id = 
aws_secret_access_key = 
aws_session_token = 
aws_security_token = 
credential_process = aws-login --profile default

will cause the following error:

 aws s3 ls

An error occurred (AuthorizationHeaderMalformed) when calling the ListBuckets operation: The authorization header is malformed; a non-empty Access Key (AKID) must be provided in the credential.

Removing the empty fields clears the error.

ddriddle avatar Apr 30 '24 15:04 ddriddle

During ensemble today, we verified the updated install instructions with a fresh install of AWS CLI V2 on Windows.

edthedev avatar Jul 02 '24 20:07 edthedev