Move external command execution from token source provider to token source #6255

andrei-trandafir · 2025-02-18T13:19:52Z

Tracking issue

Why are the changes needed?

External command doesn't get called again if authentication starts failing due to the returned token.

What changes were proposed in this pull request?

Create a new externalCommandTokenSource that runs the external command instead of the token source provider.
This results in:

The external command is called as part of the MaterialiseCredentials function instead of the getTokenSourceAndMetadata function that is synchronized (in the flyteidl/go/client/admin/auth_interceptor.go).
If the token in the token cache results in an Unauthenticated response from the flyte admin, the external command is called again.

How was this patch tested?

Deployed the change to a flytepropeller instance using ExternalCommand auth that returns an expiring token
Waited for the expiry of the token and checked whether authentication still works

Check all the applicable boxes

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

Related PRs

The issue resulted from the following PR: #5686

Summary by Bito

This PR refactors the external command token source mechanism with improved token caching and refresh functionality. It introduces a dedicated ExternalCommandTokenSource implementation with mutex-protection to prevent unnecessary command executions, enhances authentication with proper token expiration checks, and removes legacy code while cleaning up dependencies.

Unit tests added: True

Estimated effort to review (1-5, lower is better): 4

…ource Signed-off-by: Andrei Trandafir <[email protected]>

welcome · 2025-02-18T13:19:55Z

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
Sign off your commits (Reference: DCO Guide).

flyte-bot · 2025-02-18T13:20:16Z

Code Review Agent Run #cbee27

Actionable Suggestions - 0

Review Details

Files reviewed - 1 · Commit Range: dc31a56..dc31a56
- flyteidl/clients/go/admin/token_source_provider.go
Files skipped - 0
Tools
- Golangci-lint (Linter) - ✖︎ Failed
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful

AI Code Review powered by

flyte-bot · 2025-02-18T13:22:56Z

Changelist by Bito

This pull request implements the following key changes.

Key Change	Files Impacted
Feature Improvement - External Command Execution Refactor	- `token_source.go` - Introduced ExternalCommandTokenSource with a new Token() method to execute an external command for token retrieval. - `token_source_provider.go` - Refactored the provider to delegate token generation to the new ExternalCommandTokenSource, removing redundant logic.
Testing - Token Source Unit Test Addition	- `token_source_test.go` - Added a test function to verify the behavior of ExternalCommandTokenSource and ensure correct token extraction and error handling.
Other Improvements - Dependency and Cleanup Updates	- `go.mod` - Removed unused module dependencies to streamline the project configuration. - `go.sum` - Cleaned up redundant and outdated dependency entries for better maintainability.

codecov · 2025-02-18T16:03:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.87%. Comparing base (a8b9db2) to head (13278eb).
Report is 3 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6255   +/-   ##
=======================================
  Coverage   36.86%   36.87%           
=======================================
  Files        1318     1318           
  Lines      134767   134771    +4     
=======================================
+ Hits        49682    49692   +10     
+ Misses      80755    80749    -6     
  Partials     4330     4330

Flag	Coverage Δ
unittests-datacatalog	`51.58% <ø> (ø)`
unittests-flyteadmin	`51.87% <ø> (+0.02%)`	⬆️
unittests-flytecopilot	`30.99% <ø> (ø)`
unittests-flytectl	`62.33% <ø> (ø)`
unittests-flyteidl	`7.23% <100.00%> (+<0.01%)`	⬆️
unittests-flyteplugins	`54.03% <ø> (ø)`
unittests-flytepropeller	`42.78% <ø> (ø)`
unittests-flytestdlib	`55.33% <ø> (ø)`

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.

flyte-bot · 2025-02-18T19:31:03Z

Code Review Agent Run Status

Limitations and other issues: ❌ Failure - The AI Code Review Agent skipped reviewing this change because it is configured to exclude certain pull requests based on the source/target branch or the pull request status. You can change the settings here, or contact the agent instance creator at [email protected].

eapolinario

Looks reasonable, I have one small comment. Also, can you add a test to auth_interceptor_test.go?

eapolinario · 2025-02-21T20:07:52Z

flyteidl/clients/go/admin/token_source_provider.go

+type externalCommandTokenSource struct {
+	command []string
+}


Do we really need to define externalCommandTokenSource? We could implement Token in ExternalTokenSourceProvider, right?

We could implement Token() in ExternalTokenSourceProvider, but I feel it would potentially feel a bit confusing to follow.
I don't personally mind either way.

wild-endeavor · 2025-03-07T21:23:09Z

@andrei-trandafir would you mind adding a unit test? is that simple to do?

Signed-off-by: Andrei Trandafir <[email protected]>

andrei-trandafir · 2025-03-13T16:21:18Z

@wild-endeavor
I've added a unit test to the token source for the external command.
I've tried adding a test to the auth_interceptor.go checking if in the case that we're using ExternalCommand, the external command is called during the MaterializeCredentials, but I haven't managed to get it working. Not sure if it's possible

flyte-bot · 2025-03-13T17:23:07Z

Code Review Agent Run #26b31b

Actionable Suggestions - 0

Filtered by Review Rules

Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.

flyteidl/clients/go/admin/token_source.go - 2
- Consider validating token after trimming · Line 65-65
- Consider validating command before execution · Line 60-60

Review Details

Files reviewed - 5 · Commit Range: dc31a56..4ecfdb1
- flyteidl/clients/go/admin/token_source.go
- flyteidl/clients/go/admin/token_source_provider.go
- flyteidl/clients/go/admin/token_source_test.go
- go.mod
- go.sum
Files skipped - 5
- .github/workflows/checks.yml - Reason: Filter setting
- .github/workflows/component_docker_build.yml - Reason: Filter setting
- .github/workflows/integration.yml - Reason: Filter setting
- .github/workflows/single-binary.yml - Reason: Filter setting
- docs/user_guide/development_lifecycle/decks.md - Reason: Filter setting
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- SNYK (Security Vulnerability) - ✔︎ Successful
- OWASP (Security Vulnerability) - ✔︎ Successful
- GOVULNCHECK (Security Vulnerability) - ✖︎ Failed

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

/review - Manually triggers a full AI review.

Refer to the documentation for additional commands.

Configuration

This repository uses code_review_bito You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by

Move external command execution from token source provider to token s…

dc31a56

…ource Signed-off-by: Andrei Trandafir <[email protected]>

davidmirror-ops requested review from EngHabu and wild-endeavor February 18, 2025 15:58

Merge branch 'flyteorg:master' into master

13278eb

eapolinario reviewed Feb 21, 2025

View reviewed changes

andrei-trandafir mentioned this pull request Feb 25, 2025

[BUG] External command only gets called once #6254

Open

2 tasks

Move external cmd token source to token_source.go and add unit test

4ecfdb1

Signed-off-by: Andrei Trandafir <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move external command execution from token source provider to token source #6255

Move external command execution from token source provider to token source #6255

andrei-trandafir commented Feb 18, 2025 •

edited by flyte-bot

Loading

welcome bot commented Feb 18, 2025

flyte-bot commented Feb 18, 2025 •

edited

Loading

Code Review Agent Run #cbee27

flyte-bot commented Feb 18, 2025 •

edited

Loading

Changelist by Bito

codecov bot commented Feb 18, 2025 •

edited

Loading

flyte-bot commented Feb 18, 2025

eapolinario left a comment

eapolinario Feb 21, 2025

andrei-trandafir Feb 26, 2025

wild-endeavor commented Mar 7, 2025

andrei-trandafir commented Mar 13, 2025

flyte-bot commented Mar 13, 2025 •

edited

Loading

Code Review Agent Run #26b31b

Bito Usage Guide

Move external command execution from token source provider to token source #6255

Are you sure you want to change the base?

Move external command execution from token source provider to token source #6255

Conversation

andrei-trandafir commented Feb 18, 2025 • edited by flyte-bot Loading

Tracking issue

Why are the changes needed?

What changes were proposed in this pull request?

How was this patch tested?

Check all the applicable boxes

Related PRs

Summary by Bito

welcome bot commented Feb 18, 2025

flyte-bot commented Feb 18, 2025 • edited Loading

Code Review Agent Run #cbee27

flyte-bot commented Feb 18, 2025 • edited Loading

Changelist by Bito

codecov bot commented Feb 18, 2025 • edited Loading

Codecov Report

flyte-bot commented Feb 18, 2025

Code Review Agent Run Status

eapolinario left a comment

Choose a reason for hiding this comment

eapolinario Feb 21, 2025

Choose a reason for hiding this comment

andrei-trandafir Feb 26, 2025

Choose a reason for hiding this comment

wild-endeavor commented Mar 7, 2025

andrei-trandafir commented Mar 13, 2025

flyte-bot commented Mar 13, 2025 • edited Loading

Code Review Agent Run #26b31b

Bito Usage Guide

andrei-trandafir commented Feb 18, 2025 •

edited by flyte-bot

Loading

flyte-bot commented Feb 18, 2025 •

edited

Loading

flyte-bot commented Feb 18, 2025 •

edited

Loading

codecov bot commented Feb 18, 2025 •

edited

Loading

flyte-bot commented Mar 13, 2025 •

edited

Loading