Skip to content

feat: implement container authentication #119

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

Merged
merged 22 commits into from
Aug 12, 2021
Merged

Conversation

pyrooka
Copy link
Member

@pyrooka pyrooka commented Aug 6, 2021

This PR implements the new container authentication flow, which is highly based on the IAM auth. This means a new token manager and authenticator have been created and covered with tests.

NOTE: this PR/branch is based on the IAM refactoring branch(shared-iam-related-code)/PR(#118), so SHOULD NOT be merged in before the other one.

@codecov
Copy link

codecov bot commented Aug 6, 2021

Codecov Report

Merging #119 (5fba59b) into main (e0aeed7) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   98.92%   99.01%   +0.09%     
==========================================
  Files          20       22       +2     
  Lines         746      816      +70     
==========================================
+ Hits          738      808      +70     
  Misses          8        8              
Impacted Files Coverage Δ
.../authenticators/iam_request_based_authenticator.py 100.00% <ø> (ø)
ibm_cloud_sdk_core/__init__.py 100.00% <100.00%> (ø)
ibm_cloud_sdk_core/authenticators/__init__.py 100.00% <100.00%> (ø)
...sdk_core/authenticators/container_authenticator.py 100.00% <100.00%> (ø)
...loud_sdk_core/authenticators/cp4d_authenticator.py 100.00% <100.00%> (ø)
...cloud_sdk_core/authenticators/iam_authenticator.py 100.00% <100.00%> (ø)
ibm_cloud_sdk_core/get_authenticator.py 100.00% <100.00%> (ø)
...sdk_core/token_managers/container_token_manager.py 100.00% <100.00%> (ø)
ibm_cloud_sdk_core/token_managers/token_manager.py 100.00% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0aeed7...5fba59b. Read the comment docs.

@padamstx
Copy link
Member

padamstx commented Aug 6, 2021

@pyrooka Looks like there's a linter error in the builds :)

Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

Overall, these changes look good, but I found a problem related to our support of the AUTH_DISABLE_SSL config property that has most likely existed for quite some time.
I guess now's a good time to fix it :)

Copy link

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good once Phil's comments are addressed (I'll wait until then to officially approve)

I just found what I think are a few typos

@pyrooka pyrooka force-pushed the add-container-authenticator branch from f17b94d to 60f49f2 Compare August 10, 2021 20:47
@pyrooka pyrooka force-pushed the add-container-authenticator branch from 1cf9817 to fce4dd1 Compare August 10, 2021 21:12
@pyrooka pyrooka requested a review from padamstx August 10, 2021 21:31
Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

Getting pretty close, but a few small changes needed.
Plus I'm sneaking in a request to support "AUTHTYPE" as an alias for the "AUTH_TYPE" config property :)

Copy link
Member

@padamstx padamstx left a comment

Choose a reason for hiding this comment

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

I've pushed a commit to address the changes I requested in my previous review. So I approve of those changes :)

@pyrooka
Copy link
Member Author

pyrooka commented Aug 12, 2021

Thank you for fixing these issues @padamstx !

@pyrooka pyrooka requested a review from dpopp07 August 12, 2021 13:28
Copy link

@dpopp07 dpopp07 left a comment

Choose a reason for hiding this comment

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

Looks good! 👍

@pyrooka pyrooka merged commit 5237277 into main Aug 12, 2021
@pyrooka pyrooka deleted the add-container-authenticator branch August 12, 2021 15:36
ibm-devx-sdk pushed a commit that referenced this pull request Aug 12, 2021
# [3.11.0](v3.10.1...v3.11.0) (2021-08-12)

### Features

* implement container authentication ([#119](#119)) ([5237277](5237277))
@ibm-devx-sdk
Copy link

🎉 This PR is included in version 3.11.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

4 participants