Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Adding configurable audience property for flyte clients #329

Merged
merged 10 commits into from
Jan 18, 2023
Merged

Adding configurable audience property for flyte clients #329

merged 10 commits into from
Jan 18, 2023

Conversation

pmahindrakar-oss
Copy link
Contributor

@pmahindrakar-oss pmahindrakar-oss commented Oct 12, 2022 •
edited
Loading

Signed-off-by: pmahindrakar-oss [email protected]

TL;DR

Most Authorization server provide a way to use default audience which can be used to request auth tokens which is what flyte relied on before this PR.

The current PR allows to send a configurable audience field when requesting auth token from the authorization server.

Follow up PR to return audience also from PublicClientConfig endpoint of flyteadmin

flyteorg/flyteadmin#485

If client config contains the audience field then that would be used
Though if the override to useAudienceFromAdmin is set then whatever audience is set in admin would be used

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

How did you fix the bug, make the feature etc. Link to any design docs etc

Tracking Issue

flyteorg/flyte#2959

Follow-up issue

NA

@pmahindrakar-oss pmahindrakar-oss force-pushed the pass-audience-client-creds branch from 6537f7a to 3643093 Compare November 9, 2022 07:28
@codecov
Copy link

codecov bot commented Nov 9, 2022 •
edited
Loading

Codecov Report

Merging #329 (a022caf) into master (9fbac98) will increase coverage by 2.82%.
The diff coverage is 100.00%.

❗ Current head a022caf differs from pull request most recent head 968544d. Consider uploading reports for the commit 968544d to get more accurate results

@@            Coverage Diff             @@
##           master     #329      +/-   ##
==========================================
+ Coverage   73.12%   75.94%   +2.82%     
==========================================
  Files          18       18              
  Lines        1362     1185     -177     
==========================================
- Hits          996      900      -96     
+ Misses        315      234      -81     
  Partials       51       51
Flag Coverage Δ
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
clients/go/admin/config.go 50.00% <ø> (+33.33%) ⬆️
clients/go/admin/config_flags.go 60.41% <100.00%> (+4.16%) ⬆️
clients/go/admin/token_source_provider.go 37.16% <100.00%> (+6.25%) ⬆️
clients/go/admin/pkce/auth_flow_orchestrator.go 63.04% <0.00%> (-1.96%) ⬇️
clients/go/admin/pkce/handle_app_call_back.go 93.93% <0.00%> (-0.94%) ⬇️
clients/go/admin/client.go 86.91% <0.00%> (-0.31%) ⬇️
clients/go/admin/cert_loader.go 100.00% <0.00%> (ø)
clients/go/admin/atomic_credentials.go 100.00% <0.00%> (ø)
clients/go/admin/pkce/oauth2_client.go 61.11% <0.00%> (+0.24%) ⬆️
clients/go/coreutils/literals.go 93.34% <0.00%> (+1.54%) ⬆️
... and 11 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@pmahindrakar-oss pmahindrakar-oss marked this pull request as ready for review November 9, 2022 08:24
@pmahindrakar-oss pmahindrakar-oss mentioned this pull request Nov 9, 2022
Merged
8 tasks
katrogan
katrogan reviewed Nov 9, 2022
View reviewed changes
clients/go/admin/token_source_provider.go Outdated
@@ -45,16 +50,21 @@ func NewTokenSourceProvider(ctx context.Context, cfg *Config, tokenCache cache.T
tokenURL = metadata.TokenEndpoint
}

clientMetadata, err := authClient.GetPublicClientConfig(ctx, &service.PublicClientAuthConfigRequest{})
Copy link
Contributor

Choose a reason for hiding this comment

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

should we only fetch this if the cfg.Audience is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done., if either scopes or Audience is empty then we call this api

@pmahindrakar-oss pmahindrakar-oss force-pushed the pass-audience-client-creds branch from 982443b to fbc0faa Compare November 10, 2022 07:54
@pmahindrakar-oss pmahindrakar-oss force-pushed the pass-audience-client-creds branch from 18b3904 to 956c7a2 Compare December 20, 2022 18:31
katrogan
katrogan reviewed Dec 20, 2022
View reviewed changes
clients/go/admin/token_source_provider.go Outdated
clientMetadata, err := authClient.GetPublicClientConfig(ctx, &service.PublicClientAuthConfigRequest{})
audienceValue := cfg.Audience

if len(scopes) == 0 || len(audienceValue) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly with this change now we'll always make the call to get the public client config even in the case where the audience property is not actually required? (It isn't always required, right - this is just custom for some deployments?)

Is there anyway to indicate we want to explicitly fetch the audience property so that end users don't have to now update their deployments with a dummy config value for the audience property to avoid this lookup even when scopes are set?

Copy link
Contributor Author

@pmahindrakar-oss pmahindrakar-oss Jan 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Done Added added new config flag useAudienceFromAdmin which will force pull config from Admin only if its set otherwise it wont call the publicEndpoint config
Also added tests for the number of invocattions to the public admin endpoint

@joatkh joatkh mentioned this pull request Jan 5, 2023
Draft
8 tasks
@pmahindrakar-oss
Adding configurable audience property for flyte clients
b8e9edf 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss
changed the const audience to audienceKey
eb8b4c9 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss
fixed unit tests
cde290c 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss
fixed unit test
a705208 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss
nit
2b64c8a 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss
feedback
095c522 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss
refactored unit tests
13b3e90 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss
Added UseAudienceFromAdmin property to force pull audience from admin…
cf832ba 
… config. Default is false and expects clients to pass it

Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss pmahindrakar-oss force-pushed the pass-audience-client-creds branch from 956c7a2 to cf832ba Compare January 12, 2023 16:06
@pmahindrakar-oss
fixed the tests
968544d 
Signed-off-by: pmahindrakar-oss <[email protected]>
@pmahindrakar-oss pmahindrakar-oss merged commit c1c892a into master Jan 18, 2023
@pmahindrakar-oss pmahindrakar-oss deleted the pass-audience-client-creds branch January 18, 2023 01:27
eapolinario pushed a commit that referenced this pull request Sep 8, 2023
@pmahindrakar-oss
Adding configurable audience property for flyte clients (#329)
ce94183 
* Adding configurable audience property for flyte clients

Signed-off-by: pmahindrakar-oss <[email protected]>

* changed the const audience to audienceKey

Signed-off-by: pmahindrakar-oss <[email protected]>

* fixed unit tests

Signed-off-by: pmahindrakar-oss <[email protected]>

* fixed unit test

Signed-off-by: pmahindrakar-oss <[email protected]>

* nit

Signed-off-by: pmahindrakar-oss <[email protected]>

* feedback

Signed-off-by: pmahindrakar-oss <[email protected]>

* refactored unit tests

Signed-off-by: pmahindrakar-oss <[email protected]>

* Added UseAudienceFromAdmin property to force pull audience from admin config. Default is false and expects clients to pass it

Signed-off-by: pmahindrakar-oss <[email protected]>

* Added test for expected number of calls to the public admin endpoint

Signed-off-by: pmahindrakar-oss <[email protected]>

* fixed the tests

Signed-off-by: pmahindrakar-oss <[email protected]>

Signed-off-by: pmahindrakar-oss <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wild-endeavor wild-endeavor wild-endeavor approved these changes

@katrogan katrogan katrogan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pmahindrakar-oss @katrogan @wild-endeavor