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

Add V2 support #106

Merged
merged 3 commits into from
Sep 30, 2024
Merged

Add V2 support #106

merged 3 commits into from
Sep 30, 2024

Conversation

ddriddle
Copy link
Collaborator

@ddriddle ddriddle commented Jul 13, 2022

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 configure should clear ~/.aws/credentials #195.
  • Update documentation for V2 (See here for an example)
  • Add Python 3.11 to testing matrix
  • Drop awscli dependency
  • Test with awscli v2 (See Upgrade docs)
  • Split awscli_login into two or three submodules.
    • Need a submodule that contains the plugins login, logout, and configure
  • Add error handler to aws_get_credentials
  • 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:

  • Need unit tests for raise_if_logged_out
  • Update integration tests to also test aws v2 (aws v1 tests added here Add integration tests #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 ddriddle force-pushed the feature/external-script branch 6 times, most recently from 1ba87c0 to d349c84 Compare August 11, 2022 18:18
@tzturner tzturner force-pushed the feature/external-script branch 2 times, most recently from 88c2b59 to eb04120 Compare August 17, 2022 20:48
@tzturner
Copy link
Contributor

tzturner commented Aug 17, 2022

End of session today.

Note: using awscli V2

$ aws login
No module named 'lxml.etree'

@ddriddle ddriddle force-pushed the feature/external-script branch 5 times, most recently from 410abd4 to 211b341 Compare August 29, 2022 15:30
@ddriddle ddriddle force-pushed the feature/external-script branch 2 times, most recently from 137898a to 76e72f0 Compare December 2, 2022 18:01
@edthedev
Copy link
Contributor

edthedev commented Dec 15, 2022

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:
aws/aws-cli#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
Copy link
Contributor

edthedev commented Dec 16, 2022

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
Copy link
Contributor

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
Copy link
Contributor

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 edthedev added the campus enabler Enables better security patterns on campus label Jan 26, 2023
@ddriddle ddriddle force-pushed the feature/external-script branch 2 times, most recently from d2ffc8a to 376480c Compare February 1, 2023 16:58
@edthedev
Copy link
Contributor

edthedev commented Feb 6, 2023

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.

@ddriddle ddriddle force-pushed the feature/external-script branch 8 times, most recently from 8add158 to 19a79ae Compare September 17, 2024 19:35
@ddriddle ddriddle force-pushed the feature/external-script branch 4 times, most recently from 20df0f2 to c328e94 Compare September 27, 2024 19:08
@ddriddle ddriddle changed the title Add V2 support (WIP) Add V2 support Sep 27, 2024
@ddriddle ddriddle marked this pull request as ready for review September 27, 2024 19:28
@ddriddle ddriddle merged commit 0c152c7 into master Sep 30, 2024
47 checks passed
@ddriddle ddriddle deleted the feature/external-script branch September 30, 2024 20:32
@ddriddle ddriddle linked an issue Oct 4, 2024 that may be closed by this pull request
@edthedev edthedev added this to the Strong support of AWS API V2 milestone Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
campus enabler Enables better security patterns on campus good to go Ready to work on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AWS CLI V2 support: is it supported now?
5 participants