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

Fix read authorization_metadata_key from public_client_config in _refresh_credentials_from_command #1065

Merged
merged 17 commits into from
Jun 16, 2022
Merged

Fix read authorization_metadata_key from public_client_config in _refresh_credentials_from_command #1065

merged 17 commits into from
Jun 16, 2022

Conversation

sonjaer
Copy link
Contributor

@sonjaer sonjaer commented Jun 15, 2022

TL;DR

FLYTE_CREDENTIALS_AUTHORIZATION_METADATA_KEY environment variable was removed in this PR #858 in favor of reading the authorization metadata key from Admin.

There was a miss in the PR where the _refresh_credentials_from_command did not switch to get the authorization key from self.public_client_config.authorization_metadata_key but instead used the default value authorization.

Spotify uses flyte-authorization so we received an error about calls being UNAUTHENTICATED since the header name didn't correspond to the one expected by admin.

This PR adds support for reading authorization_metadata_key from public_client_config in _refresh_credentials_from_command

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

Tracking Issue

https://github.com/flyteorg/flyte/issues/

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

sonjaer and others added 4 commits June 15, 2022 11:09
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
sonjaer added 2 commits June 15, 2022 11:30
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Attention: Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 86.52%. Comparing base (14b3a4c) to head (af9b033).
Report is 1057 commits behind head on master.

Files Patch % Lines
flytekit/clients/raw.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1065      +/-   ##
==========================================
+ Coverage   86.51%   86.52%   +0.01%     
==========================================
  Files         268      268              
  Lines       24864    24893      +29     
  Branches     2799     2803       +4     
==========================================
+ Hits        21510    21539      +29     
+ Misses       2877     2875       -2     
- Partials      477      479       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sonjaer and others added 4 commits June 15, 2022 12:49
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
@sonjaer sonjaer changed the title Add support for custom auth from env Add support for configuring custom auth from environment variables Jun 15, 2022
sonjaer added 3 commits June 15, 2022 16:37
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
Copy link
Collaborator

@eapolinario eapolinario left a comment

Choose a reason for hiding this comment

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

Thanks for this fix, the change to pass the authorization key in the case of external commands was missing.

flytekit/clients/raw.py Show resolved Hide resolved
flytekit/clients/raw.py Outdated Show resolved Hide resolved
tests/flytekit/unit/clients/test_raw.py Outdated Show resolved Hide resolved
sonjaer added 3 commits June 16, 2022 10:33
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
Signed-off-by: Sonja Ericsson <[email protected]>
@sonjaer sonjaer changed the title Add support for configuring custom auth from environment variables Add support for reading authorization_metadata_key form public_client_config in custom auth from external process Jun 16, 2022
@sonjaer sonjaer changed the title Add support for reading authorization_metadata_key form public_client_config in custom auth from external process Add support for reading authorization_metadata_key form public_client_config in _refresh_credentials_from_command Jun 16, 2022
@sonjaer sonjaer changed the title Add support for reading authorization_metadata_key form public_client_config in _refresh_credentials_from_command Add support for reading authorization_metadata_key from public_client_config in _refresh_credentials_from_command Jun 16, 2022
@sonjaer sonjaer changed the title Add support for reading authorization_metadata_key from public_client_config in _refresh_credentials_from_command Fix read authorization_metadata_key from public_client_config in _refresh_credentials_from_command Jun 16, 2022
Signed-off-by: Sonja Ericsson <[email protected]>
@sonjaer
Copy link
Contributor Author

sonjaer commented Jun 16, 2022

@eapolinario what takes precedence from config, env and passing a flag?

@eapolinario
Copy link
Collaborator

@eapolinario what takes precedence from config, env and passing a flag?

flyte-cli rules are a bit more complicated, but in general, for configuration settings, environment variables take precedence over config data, with the exception of the host and insecure flags.

@eapolinario eapolinario merged commit ed048c8 into flyteorg:master Jun 16, 2022
@sonjaer sonjaer deleted the add-support-for-custom-auth-from-env-variables branch June 17, 2022 09:20
eapolinario pushed a commit that referenced this pull request Jun 17, 2022
…resh_credentials_from_command (#1065)

* add support for custom auth from env

Signed-off-by: Sonja Ericsson <[email protected]>

* Update flytekit/clis/flyte_cli/main.py

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* Update test_raw.py

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>

* fix

Signed-off-by: Sonja Ericsson <[email protected]>
@eapolinario eapolinario mentioned this pull request Jun 17, 2022
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants