Migrate OIDC-based extensions from config classes to @ConfigMapping

[michalvavrik]

Description

I've mentioned extensions migrating to @ConfigMapping along its own axis. Some of them try to solve a bug like Hibernate, but there are other objections like performance, efficiency and possibly new features (config classes are maintained if there is a major bug found, but not developed). Cons is that some config classes can be injected by integrating extensions or users, therefore they can be considered API - #35246. However I don't believe that is a case of OIDC, how many extensions and users outside of Quarkus core extensions really needs to inject configuration classes as CDI beans, or use them in a build step?

I'd like to migrate OIDC config because I think impact will be absolutely minor. I don't know why should users inject OIDC config in general. Waiting probably solves nothing and migration can have positive performance impact (though migrating OIDC alone will hardly be measurable, it is one piece of the puzzle...).

Implementation ideas

No response

[quarkus-bot]

/cc @pedroigor (oidc), @sberyozkin (oidc)

[geoand]

+1

[sberyozkin]

@michalvavrik FYI, OidcTenantConfig and OidcClientConfig extend OidcCommonConfig. Can that work with ConfigMapping ?

Cons is that some config classes can be injected by integrating extensions or users, therefore they can be considered API - #35246. However I don't believe that is a case of OIDC

IMHO it has to be reviewed. I'm quite sure I saw people asking questions about accessing the OIDC config, probably a long while back on Zulip, but I remember it.

FYI, Keycloak Authorization and now OIDC Proxy need access to OidcTenantConfig but the latter accesses it via an instance of TenantConfigBean

[michalvavrik]

Thanks for quick response @sberyozkin .

@michalvavrik FYI, OidcTenantConfig and OidcClientConfig extend OidcCommonConfig. Can that work with ConfigMapping ?

First I tried to detect whether the migration is acceptable. I'll try it, I expect it to work and if it does not, IMO it is candidate for new feature that I'd request from SR Config.

Cons is that some config classes can be injected by integrating extensions or users, therefore they can be considered API - #35246. However I don't believe that is a case of OIDC

IMHO it has to be reviewed. I'm quite sure I saw people asking questions about accessing the OIDC config, probably a long while back on Zulip, but I remember it.

Just because someone asked in past, it doesn't mean that person is still using it. I'm happy to investigate Quarkiverse extensions usage if you suggest concerned extensions (or I can try to look at Mvn central usage of the OIDC extension if it is there and inspect dependencies that are using the OIDC). That's only thing I can investigate.

FYI, Keycloak Authorization and now OIDC Proxy need access to OidcTenantConfig but the latter accesses it via an instance of TenantConfigBean

Hey, but OIDC Proxy extension is not released and Keycloak Authorization is in core, which I can fix as I'll migrate it. I agree that everything has to work as before, just migration from properties/getters to interface methods if that's what you mean by this FYI.

[sberyozkin]

@michalvavrik One more thing I remember.

Dynamic TenantConfigResolver and it is used a lot. Users create the configuration themselves, OidcTenantConfig cfg = new OidcTenantConfig() - we can't break that

[michalvavrik]

@michalvavrik One more thing I remember.

Dynamic TenantConfigResolver and it is used a lot. Users create the configuration themselves, OidcTenantConfig cfg = new OidcTenantConfig() - we can't break that

I can see it is documented and used, alright, we can keep the OidcTenantConfig as POJO and map it to the interface. IMO it is not a good idea for users to manually create config class anyway (there are config builders and other sources and it is better that users use API).

Would it be acceptable @sberyozkin to keep OidcTenantConfig as something used by users, but use in the OidcConfig class some other class. That is fields defaultTenant and namedTenants will use interface TenantConfig instead?

[sberyozkin]

@michalvavrik

IMO it is not a good idea for users to manually create config class anyway (there are config builders and other sources and it is better that users use API).

OIDC config probably covers may be 10+ different aspects, TLS, Proxy, OIDC connections details, a ton of them. And recall, this is created per tenant, dynamically.

I think if we can come up with a good OidcTenantConfig Builder API, @pedroigor was suggesting it, then it can indeed be a path forward, which will let us eventually break from the OidcTenantConfig config = new OidcTenantConfig(); config.setThisOrThat(); pattern that TenantConfigResolver users currently use

[sberyozkin]

Though I'm not sure it is worth the effort, only if we will want to break the current approach. But OidcTenantConfig config = new OidcTenantConfig(); config.setThisOrThat(); is not perfect but it just works... For example: oidc.getAuthentication().setRedirectPath("/callback");

[michalvavrik]

Alright then, I'll (eventually, not right now):

1. investigate OIDC config usage and if it is rare, I'll prepare draft PRs that migrate config for the extension authors
2. migrate current config classes to @ConfigMapping and test it with extension/integration tests
3. prepare builder API for OidcTenantConfig

Then based on concrete proposal there can be discussion whether it's acceptable or not.

[sberyozkin]

@michalvavrik Sure, but lets avoid breaking TenantConfigResolver users - I know it is used a lot, deprecating the current way of creating the config would be the way to go if the builder API becomes available.

[michalvavrik]

@michalvavrik Sure, but lets avoid breaking TenantConfigResolver users - I know it is used a lot, deprecating the current way of creating the config would be the way to go if the builder API becomes available.

Roger That.

[michalvavrik]

FYI this https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Reusing.20ConfigMapping.20from.20other.20JARS is interesting https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Reusing.20ConfigMapping.20from.20other.20JARS

[michalvavrik]

ouch, I punched enter before I finished - it is interesting because there is overall intention to unify Quarkus specific config and SmallRye Config. So I meant to say I'll that OIDC extension migration will be bigger effort that I'm working on, but I'll start slowly with tiny PRs like OIDC token propagation that I already checked has no usage in Quarkiverse. And so on. thx

[michalvavrik]

I made a smoke check in regards of the OIDC and the OIDC client extensions. io.quarkus.oidc.client.OidcClientConfig and io.quarkus.oidc.OidcTenantConfig are part of API, documented, created and used directly by users. We can't touch them. There are only 3 ways I can see, the first 2 unacceptable:

1. keep config classes forever, never migrate
2. break things, just introduce builder, try to write some openrewrite recepies and migrate to configmapping
3. Turn the 2 config classes into POJO, back them with @ConfigMapping interface (called TenantConfig and ClientConfig) and introduce build method on the @ConfigMapping that turns the interface into POJO class if and only if needed.

My understand is that we can keep consistency reliably because the POJOs will implement the @ConfigMapping interfaces, therefore when something changes on the @ConfigMapping it will enforce change in the config POJO class.

Even so, I think it is likely that users are injecting io.quarkus.oidc.runtime.OidcConfig and it will be breaking change in sense of replacing property with method call, but that seems hardly big deal.

[michalvavrik]

Just adding link to the current proposal #41866 (comment). Starting to working on the phase one right now. I think it's the right way to go, but I won't really know till I try it. I also need to look into extensibility of @ConfigMapping which could slow us down. Will report back when there is a progress.

Update: found one test in IT SR Config module that extends @ConfigMapping interface.