Skip to content

OIDC config to allow mismatched discovery / issuer #5712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Apr 17, 2025
Merged

OIDC config to allow mismatched discovery / issuer #5712

merged 1 commit into from
Apr 17, 2025

Conversation

ddl-ebrown
Copy link
Contributor

@ddl-ebrown ddl-ebrown commented Aug 31, 2024 •
edited by flyte-bot
Loading

Tracking issue

Why are the changes needed?

  • There are a number of cases where the OIDC discovery url returns one issuer, but its desirable to use a separately configured / named issuer for validation instead.

    There are cases in Azure where this is necessary due to their non-standard OIDC configuration -- which is why this was originally added: oidc: add option to override discovered issuer URL coreos/go-oidc#315

    There are also cases where it's necessary to use an in-cluster service address, but browser clients are using the external ingress address. Due to cluster DNS configuration, it's possible that flyteadmin may be unable to resolve or use the public ingress address for an Idp, but the internal service address is available. This configuration change allows for that.

  • Regenerated flyteadmin code through mise with:

mise x go@1.22 -- make generate

What changes were proposed in this pull request?

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

Summary by Bito

This PR enhances Flyteadmin OIDC configuration by allowing separate issuer URLs for token validation. The changes update authentication context, configuration settings, and command flags to support issuer discrepancies. These updates address challenges with mismatched discovery URLs in environments like Azure or when using internal service addresses.

Unit tests added: True

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

@ddl-ebrown ddl-ebrown force-pushed the add-oidc-issuer-configuration branch from 14474be to d897dfb Compare August 31, 2024 04:30
@codecov Codecov
Copy link

codecov bot commented Aug 31, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.

Project coverage is 59.80%. Comparing base (018ec9c) to head (662cec7).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
flyteadmin/auth/auth_context.go 0.00% 6 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5712      +/-   ##
==========================================
+ Coverage   58.49%   59.80%   +1.31%     
==========================================
  Files         940      639     -301     
  Lines       71555    47344   -24211     
==========================================
- Hits        41854    28316   -13538     
+ Misses      26520    16916    -9604     
+ Partials     3181     2112    -1069
Flag Coverage Δ
unittests-datacatalog ?
unittests-flyteadmin 56.27% <14.28%> (-0.02%) ⬇️
unittests-flytecopilot ?
unittests-flytectl 64.70% <ø> (ø)
unittests-flyteidl ?
unittests-flyteplugins 60.95% <ø> (ø)
unittests-flytepropeller ?
unittests-flytestdlib 64.04% <ø> (+0.01%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

wild-endeavor
wild-endeavor previously approved these changes Dec 19, 2024
View reviewed changes
Copy link
Contributor

@wild-endeavor wild-endeavor left a comment

Choose a reason for hiding this comment

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

Had a chat with @EngHabu. This pr is good to go thank you! There was some discussion about whether this config option needs to be piped in to the resource_server so that the issuer can be verified but it looks like we don't do that. In the self auth server case, the issuer is already read from config or matched against authorized urls so that's okay.

@wild-endeavor
Copy link
Contributor

mind taking care of the checks though @ddl-ebrown

@ddl-ebrown ddl-ebrown force-pushed the add-oidc-issuer-configuration branch from d897dfb to 4c5268d Compare April 17, 2025 16:33
@ddl-ebrown
OIDC config to allow mismatched discovery / issuer
662cec7 
 - There are a number of cases where the OIDC discovery url returns one
   issuer, but its desirable to use a separately configured / named
   issuer for validation instead.

   There are cases in Azure where this is necessary due to their
   non-standard OIDC configuration -- which is why this was originally
   added:
   coreos/go-oidc#315

   There are also cases where it's necessary to use an in-cluster
   service address, but browser clients are using the external ingress
   address. Due to cluster DNS configuration, it's possible that
   flyteadmin may be unable to resolve or use the public ingress
   address for an Idp, but the internal service address is available.
   This configuration change allows for that.

 - Regenerated flyteadmin code through mise with:
   mise x go@1.22 -- make generate

Signed-off-by: ddl-ebrown <ethan.brown@dominodatalab.com>
@ddl-ebrown ddl-ebrown force-pushed the add-oidc-issuer-configuration branch from 4c5268d to 662cec7 Compare April 17, 2025 16:45
@Sovietaced Sovietaced enabled auto-merge (squash) April 17, 2025 16:55
@Sovietaced Sovietaced added the added Merged changes that add new functionality label Apr 17, 2025
@Sovietaced Sovietaced merged commit 42002f4 into flyteorg:master Apr 17, 2025
49 of 50 checks passed
@github-project-automation github-project-automation bot moved this from Approved yet unmerged PRs to Done in Flyte Issues/PRs maintenance Apr 17, 2025
@flyte-bot
Copy link
Collaborator

flyte-bot commented Apr 17, 2025 •
edited
Loading

Code Review Agent Run #b5409e

Actionable Suggestions - 0
Additional Suggestions - 1
  • flyteadmin/auth/config/config_flags.go - 1
    • Potential empty URL string conversion issue · Line 63-63
Review Details
  • Files reviewed - 5 · Commit Range: 662cec7..662cec7
    • flyteadmin/auth/auth_context.go
    • flyteadmin/auth/config/config.go
    • flyteadmin/auth/config/config_flags.go
    • flyteadmin/auth/config/config_flags_test.go
    • flyteadmin/auth/config/config_test.go
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
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 haytham@union.ai.

Documentation & Help

AI Code Review powered by Bito Logo

@flyte-bot
Copy link
Collaborator

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
Feature Improvement - Enhanced OIDC Issuer Configuration

- auth_context.go - Updated OIDC authentication context to conditionally use a custom issuer for token validation when provided.

- config.go - Added an 'IssuerURL' field to configuration to support cases with mismatched discovery URLs.

- config_flags.go - Introduced a new command flag for setting the OIDC issuer URL to enable alternative issuer configuration.
Testing - OIDC Config Testing Enhancements

- config_flags_test.go - Added tests to validate the issuer URL override functionality in command flags.

- config_test.go - Updated tests to verify the default IssuerURL behavior under the new configuration changes.

@ddl-ebrown ddl-ebrown deleted the add-oidc-issuer-configuration branch May 12, 2025 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sovietaced Sovietaced Sovietaced approved these changes

@wild-endeavor wild-endeavor wild-endeavor left review comments

Assignees
No one assigned
Labels
added Merged changes that add new functionality
Projects
Flyte Issues/PRs maintenance
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ddl-ebrown @wild-endeavor @flyte-bot @Sovietaced