fix: Modify the callback URL string in auth flow, to support custom base URLs in deployments

[ddl-rliu]

Why are the changes needed?

In the auth flow, it seems that Flyteadmin always generates a callback URL, that looks like foo.tech/callback

However, when Flyte is configured on a custom subpath, this causes an issue, and we actually want the callback to look like foo.tech/custom-subpath/callback

What changes were proposed in this pull request?

By changing the callbackURL variable to "callback", the logic is fixed. Note that this also may require a change to the flyte config, since e.g. foo.tech/custom-subpath/login should be set as an AuthorizedURI in the config.yaml

How was this patch tested?

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

[ddl-rliu]

I believe this is necessary, because when Flyteadmin generates the callback URL, the logic looks like URL("foo.tech").ResolveReference(URL("/callback")) -> "foo.tech/callback"

In fact, the base URL can have any path after it, but the result is the same. URL("foo.tech/any/path").ResolveReference(URL("/callback")) -> "foo.tech/callback"

This causes issues when flyte is configured on a custom subpath. To fix this...

By adding foo.tech/custom-subpath/login as an AuthorizedURI, and changing the reference to "callback", the logic becomes URL("foo.tech/custom-subpath/login").ResolveReference(URL("callback")) -> "foo.tech/custom-subpath/callback"

[ddl-ebrown]

Should any tests be updated in

flyte/flyteadmin/auth/handlers_test.go

Line 117 in 2528de7

func TestGetCallbackHandler(t *testing.T) {

to help verify this change?

[ddl-rliu]

Added the test here https://github.com/flyteorg/flyte/pull/5192/files#diff-aa5ac252b2ec45c95649a3a874e727e436be5c932e3d93b5b2aadcfcc83d57feR12

Although I could also merge this into handlers_test.go, no strong preference.

[ddl-ebrown]

I think what you did is better / clearer - thanks!

[ddl-ebrown]

Just FYI - we are successfully running this in our environment with flyte hosted under /flyte. That requires us to do a custom build of flyteadmin though, so we're hoping to see this one make it in as its backwards compatible

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.98%. Comparing base (653ca85) to head (48719d7).
Report is 134 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5192      +/-   ##
==========================================
+ Coverage   60.97%   60.98%   +0.01%     
==========================================
  Files         793      793              
  Lines       51331    51331              
==========================================
+ Hits        31299    31306       +7     
+ Misses      17147    17139       -8     
- Partials     2885     2886       +1     

Flag	Coverage Δ
unittests-datacatalog	69.31% <ø> (ø)
unittests-flyteadmin	58.71% <ø> (+0.04%)	⬆️
unittests-flytecopilot	17.79% <ø> (ø)
unittests-flytectl	67.97% <ø> (ø)
unittests-flyteidl	79.04% <ø> (ø)
unittests-flyteplugins	61.78% <ø> (ø)
unittests-flytepropeller	57.32% <ø> (ø)
unittests-flytestdlib	65.82% <ø> (ø)

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.