Ensure token is refreshed on Unauthenticated #5388

pmahindrakar-oss · 2024-05-17T21:36:37Z

Tracking issue

#5387

Why are the changes needed?

Fix the unsafe threading behavior of multiple parallel client api trying to get tokens

What changes were proposed in this pull request?

Adding condition variables and mutex locks to serialize the calls

How was this patch tested?

Tested this on internal tenant by sending multiple parallel client requests and verified no issue were observed

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

codecov · 2024-05-17T21:44:04Z

Codecov Report

Attention: Patch coverage is 36.00000% with 48 lines in your changes are missing coverage. Please review.

Project coverage is 61.07%. Comparing base (2f1f813) to head (dd47b45).

Files	Patch %	Lines
flytectl/pkg/pkce/token_cache_keyring.go	6.89%	27 Missing ⚠️
flyteidl/clients/go/admin/auth_interceptor.go	48.27%	10 Missing and 5 partials ⚠️
...admin/tokenorchestrator/base_token_orchestrator.go	55.55%	2 Missing and 2 partials ⚠️
flyteidl/clients/go/admin/token_source_provider.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5388      +/-   ##
==========================================
- Coverage   61.08%   61.07%   -0.02%     
==========================================
  Files         793      793              
  Lines       51156    51210      +54     
==========================================
+ Hits        31251    31275      +24     
- Misses      17033    17059      +26     
- Partials     2872     2876       +4

Flag	Coverage Δ
unittests-datacatalog	`69.31% <ø> (ø)`
unittests-flyteadmin	`58.90% <ø> (+0.04%)`	⬆️
unittests-flytecopilot	`17.79% <ø> (ø)`
unittests-flytectl	`67.98% <18.18%> (-0.34%)`	⬇️
unittests-flyteidl	`79.04% <50.00%> (-0.26%)`	⬇️
unittests-flyteplugins	`61.94% <ø> (ø)`
unittests-flytepropeller	`57.32% <ø> (ø)`
unittests-flytestdlib	`65.75% <ø> (ø)`

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.

flytectl/go.mod

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

eapolinario · 2024-05-20T18:01:51Z

The docs failures are unrelated.

eapolinario · 2024-05-21T19:37:29Z

~~@pmahindrakar-oss , can you also revert this one?~~

edit: I clicked the revert button on github. 😂

This reverts commit 1384b32.

…#5404)" This reverts commit 7d2f0d0.

…#5404)" This reverts commit 7d2f0d0. Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

* Revert "Revert "Ensure token is refreshed on Unauthenticated (#5388)" (#5404)" This reverts commit 7d2f0d0. Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Using same mutex for condition variable Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Lock the locker in the wait to adher to cond.Wait() semantics Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * comments Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * using noop locker as waitlist add is atomic operation Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Replace Azure AD OIDC URL with correct one (#4075) Signed-off-by: Erwin de Haan <erwin.de.haan@calcasa.nl> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Update the example Dockerfile to run on k8s (#5412) Signed-off-by: Jason Parraga <sovietaced@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * docs(kubeflow): Fix kubeflow webhook error (#5410) Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * update flytekit version to 1.12.1b2 in monodocs requirements (#5411) Signed-off-by: Samhita Alla <aallasamhita@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Add supported task types to agent service config and rename (#5402) Signed-off-by: Jason Parraga <sovietaced@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * update lock file (#5416) Signed-off-by: Samhita Alla <aallasamhita@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * [monorepo] Fix flytectl install script (#5405) Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * bring in changes for flytecl keyring from PR flytectl/pull/488 Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * typo fix Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> --------- Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> Signed-off-by: Erwin de Haan <erwin.de.haan@calcasa.nl> Signed-off-by: Jason Parraga <sovietaced@gmail.com> Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com> Signed-off-by: Samhita Alla <aallasamhita@gmail.com> Co-authored-by: Erwin de Haan <1627021+EraYaN@users.noreply.github.com> Co-authored-by: Jason Parraga <Sovietaced@gmail.com> Co-authored-by: Chi-Sheng Liu <chishengliu@chishengliu.com> Co-authored-by: Samhita Alla <aallasamhita@gmail.com> Co-authored-by: Eduardo Apolinario <653394+eapolinario@users.noreply.github.com>

Sovietaced · 2024-06-06T21:20:35Z

Why change the interface instead of just making the implementation thread safe?

* Ensure token is refreshed on Unauthenticated Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Using condition broadcast Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * fix unit tests Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * nit Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * nit Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * nit Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * fix go mod entries in flytectl Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * adding a log line when materializing creds Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> --------- Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

…flyteorg#5404) This reverts commit 1384b32.

* Revert "Revert "Ensure token is refreshed on Unauthenticated (flyteorg#5388)" (flyteorg#5404)" This reverts commit 7d2f0d0. Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Using same mutex for condition variable Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Lock the locker in the wait to adher to cond.Wait() semantics Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * comments Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * using noop locker as waitlist add is atomic operation Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Replace Azure AD OIDC URL with correct one (flyteorg#4075) Signed-off-by: Erwin de Haan <erwin.de.haan@calcasa.nl> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Update the example Dockerfile to run on k8s (flyteorg#5412) Signed-off-by: Jason Parraga <sovietaced@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * docs(kubeflow): Fix kubeflow webhook error (flyteorg#5410) Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * update flytekit version to 1.12.1b2 in monodocs requirements (flyteorg#5411) Signed-off-by: Samhita Alla <aallasamhita@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * Add supported task types to agent service config and rename (flyteorg#5402) Signed-off-by: Jason Parraga <sovietaced@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * update lock file (flyteorg#5416) Signed-off-by: Samhita Alla <aallasamhita@gmail.com> Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * [monorepo] Fix flytectl install script (flyteorg#5405) Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * bring in changes for flytecl keyring from PR flytectl/pull/488 Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> * typo fix Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> --------- Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com> Signed-off-by: Erwin de Haan <erwin.de.haan@calcasa.nl> Signed-off-by: Jason Parraga <sovietaced@gmail.com> Signed-off-by: Chi-Sheng Liu <chishengliu@chishengliu.com> Signed-off-by: Samhita Alla <aallasamhita@gmail.com> Co-authored-by: Erwin de Haan <1627021+EraYaN@users.noreply.github.com> Co-authored-by: Jason Parraga <Sovietaced@gmail.com> Co-authored-by: Chi-Sheng Liu <chishengliu@chishengliu.com> Co-authored-by: Samhita Alla <aallasamhita@gmail.com> Co-authored-by: Eduardo Apolinario <653394+eapolinario@users.noreply.github.com>

pmahindrakar-oss force-pushed the upstream-auth-token-cache-issue branch 3 times, most recently from 111ee95 to 8d6e360 Compare May 20, 2024 15:09

eapolinario reviewed May 20, 2024

View reviewed changes

flytectl/go.mod Outdated Show resolved Hide resolved

pmahindrakar-oss requested a review from eapolinario May 20, 2024 17:26

eapolinario previously approved these changes May 20, 2024

View reviewed changes

pmahindrakar-oss dismissed eapolinario’s stale review via 513b12b May 20, 2024 17:29

pmahindrakar-oss added 8 commits May 20, 2024 10:31

Ensure token is refreshed on Unauthenticated

8fafb40

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

Using condition broadcast

e95cc21

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

fix unit tests

4bc1bcb

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

nit

4248274

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

nit

7bbca78

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

nit

2579d20

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

fix go mod entries in flytectl

efcbdc1

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

adding a log line when materializing creds

dd47b45

Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

pmahindrakar-oss force-pushed the upstream-auth-token-cache-issue branch from 513b12b to dd47b45 Compare May 20, 2024 17:31

eapolinario approved these changes May 20, 2024

View reviewed changes

pmahindrakar-oss enabled auto-merge (squash) May 20, 2024 17:52

pmahindrakar-oss disabled auto-merge May 20, 2024 17:52

eapolinario merged commit 1384b32 into master May 20, 2024
47 of 50 checks passed

eapolinario deleted the upstream-auth-token-cache-issue branch May 20, 2024 18:02

eapolinario added a commit that referenced this pull request May 21, 2024

Revert "Ensure token is refreshed on Unauthenticated (#5388)"

796cfee

This reverts commit 1384b32.

eapolinario mentioned this pull request May 21, 2024

Revert "Ensure token is refreshed on Unauthenticated" #5404

Merged

eapolinario added a commit that referenced this pull request May 21, 2024

Revert "Ensure token is refreshed on Unauthenticated (#5388)" (#5404)

7d2f0d0

This reverts commit 1384b32.

pmahindrakar-oss added a commit that referenced this pull request May 22, 2024

Revert "Revert "Ensure token is refreshed on Unauthenticated (#5388)" (…

45c3db5

…#5404)" This reverts commit 7d2f0d0.

pmahindrakar-oss added a commit that referenced this pull request May 23, 2024

Revert "Revert "Ensure token is refreshed on Unauthenticated (#5388)" (…

e7714c7

…#5404)" This reverts commit 7d2f0d0. Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

pmahindrakar-oss added a commit that referenced this pull request May 31, 2024

Revert "Revert "Ensure token is refreshed on Unauthenticated (#5388)" (…

a13f698

…#5404)" This reverts commit 7d2f0d0. Signed-off-by: pmahindrakar-oss <prafulla.mahindrakar@gmail.com>

robert-ulbrich-mercedes-benz pushed a commit to robert-ulbrich-mercedes-benz/flyte that referenced this pull request Jul 2, 2024

Revert "Ensure token is refreshed on Unauthenticated (flyteorg#5388)" (…

db58edb

…flyteorg#5404) This reverts commit 1384b32.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure token is refreshed on Unauthenticated #5388

Ensure token is refreshed on Unauthenticated #5388

pmahindrakar-oss commented May 17, 2024

codecov bot commented May 17, 2024 •

edited

Loading

eapolinario commented May 20, 2024

eapolinario commented May 21, 2024 •

edited

Loading

Sovietaced commented Jun 6, 2024

Ensure token is refreshed on Unauthenticated #5388

Ensure token is refreshed on Unauthenticated #5388

Conversation

pmahindrakar-oss commented May 17, 2024

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 May 17, 2024 • edited Loading

Codecov Report

eapolinario commented May 20, 2024

eapolinario commented May 21, 2024 • edited Loading

Sovietaced commented Jun 6, 2024

codecov bot commented May 17, 2024 •

edited

Loading

eapolinario commented May 21, 2024 •

edited

Loading