Auth/prevent lookup per call #5686

wild-endeavor · 2024-08-23T00:59:56Z

Tracking issue

Why are the changes needed?

Clients configured with client credentials secret were calling Admin's auth metadata endpoints for every call. This moves the initialization of all that above the interceptor.

What changes were proposed in this pull request?

Move initialization logic into a new object, synchronized with a Once.
Make the flytectl config command disable the flyte client.

How was this patch tested?

Tested by running locally and hitting our development deployment with client secret.

Setup process

Screenshots

Check all the applicable boxes

I updated the documentation accordingly.
All new and existing tests passed.
All commits are signed-off.

Related PRs

Docs link

Signed-off-by: Yee Hing Tong <[email protected]>

codecov · 2024-08-23T01:05:44Z

Codecov Report

Attention: Patch coverage is 85.18519% with 8 lines in your changes missing coverage. Please review.

Project coverage is 36.18%. Comparing base (e9413c0) to head (98344df).
Report is 153 commits behind head on master.

Files with missing lines	Patch %	Lines
flyteidl/clients/go/admin/auth_interceptor.go	82.22%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #5686   +/-   ##
=======================================
  Coverage   36.17%   36.18%           
=======================================
  Files        1302     1302           
  Lines      109556   109613   +57     
=======================================
+ Hits        39630    39659   +29     
- Misses      65786    65809   +23     
- Partials     4140     4145    +5

Flag	Coverage Δ
unittests-datacatalog	`51.37% <ø> (ø)`
unittests-flyteadmin	`55.33% <ø> (+0.06%)`	⬆️
unittests-flytecopilot	`12.17% <ø> (ø)`
unittests-flytectl	`62.18% <100.00%> (-0.11%)`	⬇️
unittests-flyteidl	`7.12% <82.97%> (+0.04%)`	⬆️
unittests-flyteplugins	`53.34% <ø> (+<0.01%)`	⬆️
unittests-flytepropeller	`41.71% <ø> (-0.05%)`	⬇️
unittests-flytestdlib	`55.35% <ø> (+0.02%)`	⬆️

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

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

🚨 Try these New Features:

Flaky Tests Detection - Detect and resolve failed and flaky tests

Signed-off-by: Yee Hing Tong <[email protected]>

flyteidl/clients/go/admin/auth_interceptor.go

Signed-off-by: Yee Hing Tong <[email protected]>

Sovietaced · 2024-08-27T02:09:16Z

Ideally it would be nice if the metadata was not cached permanently considering it contains trusted public key material. If there is interest I can follow up with a strategy that periodically refreshes the metadata so it will handle key rotations gracefully.

* save values Signed-off-by: Yee Hing Tong <[email protected]> * move things up Signed-off-by: Yee Hing Tong <[email protected]> * tests Signed-off-by: Yee Hing Tong <[email protected]> * unit test Signed-off-by: Yee Hing Tong <[email protected]> * imports for client test Signed-off-by: Yee Hing Tong <[email protected]> * more test Signed-off-by: Yee Hing Tong <[email protected]> * don't test admin connection Signed-off-by: Yee Hing Tong <[email protected]> * disable client for config Signed-off-by: Yee Hing Tong <[email protected]> * make generate Signed-off-by: Yee Hing Tong <[email protected]> * hide behind a once Signed-off-by: Yee Hing Tong <[email protected]> * typo Signed-off-by: Yee Hing Tong <[email protected]> * reset client builder test Signed-off-by: Yee Hing Tong <[email protected]> * reset client test Signed-off-by: Yee Hing Tong <[email protected]> * revert propeller Signed-off-by: Yee Hing Tong <[email protected]> * delay invocation even further Signed-off-by: Yee Hing Tong <[email protected]> --------- Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: pmahindrakar-oss <[email protected]>

* save values Signed-off-by: Yee Hing Tong <[email protected]> * move things up Signed-off-by: Yee Hing Tong <[email protected]> * tests Signed-off-by: Yee Hing Tong <[email protected]> * unit test Signed-off-by: Yee Hing Tong <[email protected]> * imports for client test Signed-off-by: Yee Hing Tong <[email protected]> * more test Signed-off-by: Yee Hing Tong <[email protected]> * don't test admin connection Signed-off-by: Yee Hing Tong <[email protected]> * disable client for config Signed-off-by: Yee Hing Tong <[email protected]> * make generate Signed-off-by: Yee Hing Tong <[email protected]> * hide behind a once Signed-off-by: Yee Hing Tong <[email protected]> * typo Signed-off-by: Yee Hing Tong <[email protected]> * reset client builder test Signed-off-by: Yee Hing Tong <[email protected]> * reset client test Signed-off-by: Yee Hing Tong <[email protected]> * revert propeller Signed-off-by: Yee Hing Tong <[email protected]> * delay invocation even further Signed-off-by: Yee Hing Tong <[email protected]> --------- Signed-off-by: Yee Hing Tong <[email protected]> Signed-off-by: Bugra Gedik <[email protected]>

Cherry-pick the following change to populate oauth metadata once on initialization using Sync.Do ca04314 Tested locally using uctl-admin and fetched projects calling into admin which exercises the auth flow https://buildkite.com/unionai/org-staging-sync/builds/3541 Rollout to all canary and then prod tenants - [x] To be upstreamed to OSS *TODO: Link Linear issue(s) using [magic words](https://linear.app/docs/github#magic-words). `fixes` will move to merged status, while `ref` will only link the PR.* * [ ] Added tests * [ ] Ran a deploy dry run and shared the terraform plan * [ ] Added logging and metrics * [ ] Updated [dashboards](https://unionai.grafana.net/dashboards) and [alerts](https://unionai.grafana.net/alerting/list) * [ ] Updated documentation

Cherry-pick the following change to populate oauth metadata once on initialization using Sync.Do ca04314 Tested locally using uctl-admin and fetched projects calling into admin which exercises the auth flow https://buildkite.com/unionai/org-staging-sync/builds/3541 Rollout to all canary and then prod tenants - [x] To be upstreamed to OSS *TODO: Link Linear issue(s) using [magic words](https://linear.app/docs/github#magic-words). `fixes` will move to merged status, while `ref` will only link the PR.* * [ ] Added tests * [ ] Ran a deploy dry run and shared the terraform plan * [ ] Added logging and metrics * [ ] Updated [dashboards](https://unionai.grafana.net/dashboards) and [alerts](https://unionai.grafana.net/alerting/list) * [ ] Updated documentation Signed-off-by: pmahindrakar-oss <[email protected]>

…ource (#6001) * Auth/prevent lookup per call (#5686) (#555) Cherry-pick the following change to populate oauth metadata once on initialization using Sync.Do ca04314 Tested locally using uctl-admin and fetched projects calling into admin which exercises the auth flow https://buildkite.com/unionai/org-staging-sync/builds/3541 Rollout to all canary and then prod tenants - [x] To be upstreamed to OSS *TODO: Link Linear issue(s) using [magic words](https://linear.app/docs/github#magic-words). `fixes` will move to merged status, while `ref` will only link the PR.* * [ ] Added tests * [ ] Ran a deploy dry run and shared the terraform plan * [ ] Added logging and metrics * [ ] Updated [dashboards](https://unionai.grafana.net/dashboards) and [alerts](https://unionai.grafana.net/alerting/list) * [ ] Updated documentation Signed-off-by: pmahindrakar-oss <[email protected]> * [COR-1114] Fix token validity check logic to use exp field in access token (#330) * Add logs for token * add logs * Fixing the validity check logic for token * nit * nit * Adding in memory token source provider * nit * changed Valid method to log and ignore parseDateClaim error * nit * Fix unit tests * lint * fix unit tests Signed-off-by: pmahindrakar-oss <[email protected]> * remove debug logs Signed-off-by: pmahindrakar-oss <[email protected]> --------- Signed-off-by: pmahindrakar-oss <[email protected]>

wild-endeavor added 2 commits August 22, 2024 13:19

save values

37395e5

Signed-off-by: Yee Hing Tong <[email protected]>

move things up

5c87e34

Signed-off-by: Yee Hing Tong <[email protected]>

tests

b2977e8

Signed-off-by: Yee Hing Tong <[email protected]>

wild-endeavor marked this pull request as ready for review August 23, 2024 17:21

wild-endeavor changed the title ~~wip Auth/prevent lookup per call~~ Auth/prevent lookup per call Aug 23, 2024

unit test

a00bbc0

Signed-off-by: Yee Hing Tong <[email protected]>

eapolinario previously approved these changes Aug 23, 2024

View reviewed changes

imports for client test

0d12519

Signed-off-by: Yee Hing Tong <[email protected]>

wild-endeavor dismissed eapolinario’s stale review via 0d12519 August 23, 2024 19:23

wild-endeavor added 4 commits August 23, 2024 12:35

more test

ae30628

Signed-off-by: Yee Hing Tong <[email protected]>

don't test admin connection

c6b767a

Signed-off-by: Yee Hing Tong <[email protected]>

disable client for config

fd1afd0

Signed-off-by: Yee Hing Tong <[email protected]>

make generate

4fab08d

Signed-off-by: Yee Hing Tong <[email protected]>

eapolinario reviewed Aug 23, 2024

View reviewed changes

flyteidl/clients/go/admin/auth_interceptor.go Outdated Show resolved Hide resolved

wild-endeavor added 6 commits August 23, 2024 14:40

hide behind a once

99e5ef9

Signed-off-by: Yee Hing Tong <[email protected]>

typo

e04eece

Signed-off-by: Yee Hing Tong <[email protected]>

reset client builder test

d352a2e

Signed-off-by: Yee Hing Tong <[email protected]>

reset client test

2a75b12

Signed-off-by: Yee Hing Tong <[email protected]>

revert propeller

7a8e627

Signed-off-by: Yee Hing Tong <[email protected]>

delay invocation even further

98344df

Signed-off-by: Yee Hing Tong <[email protected]>

eapolinario approved these changes Aug 23, 2024

View reviewed changes

eapolinario enabled auto-merge (squash) August 23, 2024 22:23

eapolinario merged commit ca04314 into master Aug 23, 2024
52 of 53 checks passed

eapolinario deleted the auth/prevent-lookup-per-call branch August 23, 2024 22:24

eapolinario mentioned this pull request Aug 27, 2024

[BUG] FlytePropeller 1.13.0 is crashing Flyteadmin and making the overall service in a bad state #5606

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auth/prevent lookup per call #5686

Auth/prevent lookup per call #5686

wild-endeavor commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading

Sovietaced commented Aug 27, 2024

Auth/prevent lookup per call #5686

Auth/prevent lookup per call #5686

Conversation

wild-endeavor commented Aug 23, 2024 • edited Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Setup process

Screenshots

Check all the applicable boxes

Related PRs

Docs link

codecov bot commented Aug 23, 2024 • edited Loading

Codecov Report

Sovietaced commented Aug 27, 2024

wild-endeavor commented Aug 23, 2024 •

edited

Loading

codecov bot commented Aug 23, 2024 •

edited

Loading