Skip to content
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.

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

Migrate OIDC, OIDC client and OIDC Client Registration extensions to @ConfigMapping #44140

Conversation

michalvavrik
Copy link
Member

@michalvavrik michalvavrik commented Oct 28, 2024

Migration of OIDC configuration classes to the @ConfigMapping is complicated because:

  • we modify config class @ConfigGroup instances, we change values after runtime config setup has finished
  • we use the config class @ConfigGroup as part of "public API" (meaning: they are in documentation, users directly produce instances of these classes and work with them)
  • abstraction ([ OidcTenantConfig || OidcClientConfig ] -> OidcClientCommonConfig -> OidcCommonConfig and OidcClientRegistrationConfig -> OidcCommonConfig) shared by 3 extensions
  • some OIDC configs are really huge (like OidcTenantConfig)

This PR introduces @ConfigMapping interfaces and makes original config classes POJOs.
These POJOs are still used everywhere by Quarkus OIDC and users.

Regarding affected Quarkiverse extensions:

@michalvavrik michalvavrik changed the title Migrate OIDC, OIDC client and registration to @ConfigMapping Migrate OIDC, OIDC client and OIDC Client Registration extensions to @ConfigMapping Oct 28, 2024
Copy link

github-actions bot commented Oct 28, 2024

🎊 PR Preview 269efae has been successfully built and deployed to https://quarkus-pr-main-44140-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

This comment has been minimized.

@sberyozkin
Copy link
Member

Awesome, thanks @michalvavrik, I'll start reviewing asap, thanks very much

@michalvavrik
Copy link
Member Author

michalvavrik commented Oct 28, 2024

Awesome, thanks @michalvavrik, I'll start reviewing asap, thanks very much

Yeah, I don't want to hurry you as I know you are busy, but there is one thing: I checked all OIDC PRs between I started working on this and now and there were no relevant change, but if anything that touches OIDC config changes, this PR needs to be updated. Like #44114. So we need to sync with changes in main repo (hopefully it would result in a merge conflict, but I am not 100 % certain).

@michalvavrik michalvavrik force-pushed the feature/migrate-oidc-config-classes-to-configmapping branch from 3b01a01 to e9a0833 Compare October 31, 2024 16:54
@michalvavrik
Copy link
Member Author

michalvavrik commented Oct 31, 2024

I have rebased on current main, adapted this PR to changes made in #44114 (AKA added Slack) and checked PRs this week so far, no relevant changes. It's safe so far. I'll re-check in a few days as well. No hurry.

This comment has been minimized.

This comment has been minimized.

@sberyozkin
Copy link
Member

sberyozkin commented Nov 5, 2024

@michalvavrik Michal, can you type some examples here how for example OidcTenantConfig can be built ? The one from the basic properties like the clientid, secret, authserver url and web-app application type only, and more complex ones, where in addition to the basics, users set up some authentication properties, and use JWT private key client authentication... For the latter part, it would be great to have an option to do all not only in a single sequence, but create individual parts and then assemble together...

@sberyozkin
Copy link
Member

Sorry @michalvavrik, I forgot that phase 1 is only about supporting @ConfigMapping

@michalvavrik michalvavrik force-pushed the feature/migrate-oidc-config-classes-to-configmapping branch from e9a0833 to 981f8a6 Compare November 5, 2024 13:40
@michalvavrik
Copy link
Member Author

FYI - I just rebased and checked commits. Looks like nothing relevant changed so I didn't have to do anything.

Copy link

quarkus-bot bot commented Nov 5, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 981f8a6.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Nov 5, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 981f8a6.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

@sberyozkin sberyozkin self-requested a review November 5, 2024 14:45
Copy link
Member

@sberyozkin sberyozkin left a comment

Choose a reason for hiding this comment

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

Let's start the migration process, thanks @michalvavrik

@sberyozkin sberyozkin merged commit cb47d8f into quarkusio:main Nov 5, 2024
25 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.17 - main milestone Nov 5, 2024
@michalvavrik michalvavrik deleted the feature/migrate-oidc-config-classes-to-configmapping branch November 5, 2024 14:46
@michalvavrik
Copy link
Member Author

Thank you @sberyozkin , I'll open Renarde PR to address this change. (these 2 bracket scenarios)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants