The public method getDynamicTenantsConfig in OIDC TenantConfigBean not available in 3.16.0.CR1

[sschellh]

Describe the bug

The public method getDynamicTenantsConfig in io.quarkus.oidc.runtime.TenantConfigBean is not available in 3.16.0.RC1 making it incompatible with previous version.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

Quarkus version or git rev

3.16.0.CR1

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

[quarkus-bot]

/cc @radcortez (config)

[gsmet]

/cc @sberyozkin @michalvavrik

[michalvavrik]

Yes, that comes from this PR #43590. This issue has 2 sides:

1. we could have done it better
2. why it changed:

• whatever is in runtime module we usually do not consider to be part of the API
• it wasn't documented
• I don't think we expected users need this

My conclusion is that we should be more careful about methods visibility to avoid confusion. There is getDynamicTenant that can be used for same purpose. Can you please mention your use case so maybe we can consider it in the future?

[sberyozkin]

@sschellh The actual map holding dynamic tenants has been encapsulated, otherwise we can't plan any work related to managing this map, such as restricting a number of dynamic tenants.
You can do tenantConfigBean.getDynamicTenant("tenantid") to get the individual tenant.

FYI everything directly in the io.quarkus.oidc. package is considered a public API and we'd add migration notes for any breaking changes there.
What is in the io.quarkus.oidc.runtime.* is not really a public API, we have so much code there so trying to avoid any public modifiers to prevent users accessing it is quite hard.

Can you please switch to tenantConfigBean.getDynamicTenant("tenantid") ? I don't think we can provide direct access to that map again, like I said we need to start working on encapsulating that map even further into an in memory cache

[sberyozkin]

@michalvavrik Michal, I'm not sure we could've avoided that change, if users are allowed a direct access to the map then it is really impossible to manage it internally, that dynamic map was all over the place :-)

[michalvavrik]

@michalvavrik Michal, I'm not sure we could've avoided that change, if users are allowed a direct access to the map then it is really impossible to manage it internally, that dynamic map was all over the place :-)

We were in no hurry, I just say we could deprecate it for one release or so; that said, I wouldn't do it unless I knew there is more affected users. Personally I thought users don't need this method. I don't think we disagree @sberyozkin , changes was positive.

[sberyozkin]

@michalvavrik

We were in no hurry, I just say we could deprecate it for one release or so;

Yeah, definitely for what we consider to be a public API. Also, I was hoping we would get the initial cache support in time for 3.16.0, IMHO the fact we don't have a solution yet for restricting how far the dynamic map can grow is something which we should look at sooner rather than later :-). I don't expect any but the most complex deployment to have 1K+ OIDC providers, but there should be some cap in place for our piece of mind :-)

[sberyozkin]

We could reintroduce that method and mark as deprecated, but it could delay the solution for 6 months or something like that and that is way too long.

But in any case, I guess we will need to review all public methods in io.quatrkus.oidc.runtime.* and mark those methods which we really don't want users to access as deprecated, as you recommend, and then restrict them to the package soon afterwards

@sschellh Can you let us know, does tenantConfigBean.getDynamicTenant("tenantid") work for you ?

[sberyozkin]

@sschellh And if you need access to this map to actually remove some tenants then I think we can easily add tenantConfigBean.removeDynamicTenant("tenantid") while keeping the encapsulation in place, let us know please

[sschellh]

Hi all,
yes tenantConfigBean.getDynamicTenant("tenantid") also works for me.
I was not sure if the method was removed by accident or by purpose. Especially as getStaticTenantsConfig() is still available.
Hence I created this issue.

[sberyozkin]

@sschellh the static map size is unmodifiable, restricted to whatever providers have been configured in the properties, but indeed, it would be consistent to wrap it too though...
Are you Ok then if we close this issue ? We can keep reviewing our internal code and start making it package restricted, step by step

[sschellh]

good for me