Skip to content

feat: implement container authentication#119

Merged
pyrooka merged 22 commits intomainfrom
add-container-authenticator
Aug 12, 2021
Merged

feat: implement container authentication#119
pyrooka merged 22 commits intomainfrom
add-container-authenticator

Conversation

@pyrooka
Copy link
Copy Markdown
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
Copy Markdown

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

padamstx commented Aug 6, 2021

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

Comment thread ibm_cloud_sdk_core/token_managers/container_token_manager.py
Copy link
Copy Markdown
Contributor

@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 :)

Comment thread Authentication.md Outdated
Comment thread ibm_cloud_sdk_core/authenticators/container_authenticator.py Outdated
Comment thread ibm_cloud_sdk_core/get_authenticator.py Outdated
Comment thread test/test_container_token_manager.py Outdated
Comment thread test/test_container_token_manager.py Outdated
Comment thread test/test_container_token_manager.py Outdated
Comment thread test/test_utils.py Outdated
Comment thread resources/ibm-credentials-container.env Outdated
Copy link
Copy Markdown

@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

Comment thread ibm_cloud_sdk_core/authenticators/container_authenticator.py Outdated
Comment thread ibm_cloud_sdk_core/token_managers/container_token_manager.py Outdated
Comment thread ibm_cloud_sdk_core/token_managers/container_token_manager.py Outdated
@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
Copy Markdown
Contributor

@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 :)

Comment thread ibm_cloud_sdk_core/authenticators/container_authenticator.py Outdated
Comment thread ibm_cloud_sdk_core/authenticators/container_authenticator.py Outdated
Comment thread ibm_cloud_sdk_core/get_authenticator.py
Comment thread ibm_cloud_sdk_core/token_managers/container_token_manager.py Outdated
Comment thread ibm_cloud_sdk_core/token_managers/container_token_manager.py Outdated
Comment thread test/test_container_authenticator.py
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown

@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
Copy Markdown

🎉 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