Use OIDC Tenant annotation to resolve tenants

[michalvavrik]

fixes: #34692

Now all that users need to do when they want to link certain resource method with tenant (both in RESTEasy Reactive and RESTEasy Classic) is to annotate the method with @Tenant("my-tenant"). I did that by completely avoiding CDI interceptors, which is necessary for RR scenario. Also adding oidc-wiremock-reactive module as reported issue proved value of that. However I didn't add it to native runs, so additional time required in CI is about 30 seconds per JVM. Using Jakarta REST filter in RESTEasy Classic enabled usage of @Tenant annotation for this purpose (as we avoid CDI interceptors over token customizers).

[github-actions]

🙈 The PR is closed and the preview is expired.

[michalvavrik]

Tests failing in CI didn't fail locally when I run them, I'll have a look.

[michalvavrik]

Alright, Sergey had PRs and made changes in OIDC wiremock module since I started developing. I'll do same changes in OIDC Wiremock reactive.

[sberyozkin]

Hi @michalvavrik Thanks, do we really need to have oidc-wiremock-reactive ? May be you can update the dependency in oidc-wiremock and update the tenant annotation test only ? CDI interceptor test for Resteasy Classic will keep running in 3.2.x branches - but starting from 3.3.0 we'd just recommend using @Tenant("someid") for either stack ?

[michalvavrik]

Hi @michalvavrik Thanks, do we really need to have oidc-wiremock-reactive ?

Issue #34692 would not happen if we tested it with RESTEasy Reactive. Considering RR is our preferred REST stack, it needs to be tested. Also it is additional 30 seconds (on my workstation) per JVM, so no big deal.

May be you can update the dependency in oidc-wiremock and update the tenant annotation test only ?

You mean like change RESTEasy Classic to RESTEasy Reactive in oidc-wiremock? I'm all in :-), just kindly @sberyozkin confirm I got you right, please.

CDI interceptor test for Resteasy Classic will keep running in 3.2.x branches - but starting from 3.3.0 we'd just recommend using @Tenant("someid") for either stack ?

Yeah, I agree with that, but principles how it works under the hood are different. If I change deps RC -> RR, than we don't test whether RC work (more specifically it is just about Jakarta REST filter in RESTEasy Classic, so chance it does not is pretty small!).

@sberyozkin I think I understand and agree, just please confirm, thank you.

[sberyozkin]

@michalvavrik Question about using @Tenant, I added it after the discussion with Georgios - the idea behind it was to mark a given feature such as TokenHeaderCustomizer being applicable to the specific tenant only - here we essentially update the tenant configuration indirectly.

In this PR, @Tenant is meant to mark a JAX-RS class or method to which a given tenant configuration applies.

So we have 2 slightly different cases backed up by the same annotation. I'm not seeing any obvious problem with it right now, but wonder id there could be some problems, if we decide to add some attributes which make sense only for one of these 2 cases...

[michalvavrik]

@michalvavrik Question about using @Tenant, I added it after the discussion with Georgios - the idea behind it was to mark a given feature such as TokenHeaderCustomizer being applicable to the specific tenant only - here we essentially update the tenant configuration indirectly.

In this PR, @Tenant is meant to mark a JAX-RS class or method to which a given tenant configuration applies.

So we have 2 slightly different cases backed up by the same annotation. I'm not seeing any obvious problem with it right now, but wonder id there could be some problems, if we decide to add some attributes which make sense only for one of these 2 cases...

I realize that. I decided to use it because after reading @Tenant javadoc, I didn't feel I need to change a single sentence to make it valid for this case. It speaks about marking feature with tenant id, which is perfect match. It potential attribute are only valid for one of 2 cases, we either need to document them, or add them as standalone annotation instead. Would you prefer to use entirely new annotation for this case instead? It really seems perfect match for the moment being.

[sberyozkin]

@michalvavrik Updating oidc-wiremock dependency will save on introducing another module, so lets update it. oidc-code-flow, oidc-tenancy use RestEasy classic

[sberyozkin]

It potential attribute are only valid for one of 2 cases, we either need to document them, or add them as standalone annotation instead.

Good point, sure lets use @Tenant

[sberyozkin]

@michalvavrik PR is perfect, thanks, lets discuss more though how to deal with this annotation ambiguity

[sberyozkin]

@michalvavrik Would you like to rename this PR to something like Use OIDC Tenant annotation to resolve tenants ? It can be marked then as a release noteworthy feature, otherwise we are only fixing the bug - which is also true :-), but the concept of using @Tenant for resolving tenants is totally new, applies now to both stacks, and is worth getting the users attention to.

[michalvavrik]

@sberyozkin sure thing

[sberyozkin]

Thanks, added a noteworthy feature label

[michalvavrik]

JDK 17 on Windows is IMHO flaky because I saw that before

[michalvavrik]

JDK 11 got cancelled, but in logs I can't see anything related as well (huge logs though..); IMHO it just reached 6 hours Github timeout. cc @gsmet @geoand

[geoand]

Very likely

[sberyozkin]

Thanks @michalvavrik

[sberyozkin]

@michalvavrik Hi Michal, can you please give it one more try and rerun ? This PR updates the vertx-http deployment sequence, I don't see how it can affect the timeout though, but please run one more time

[sberyozkin]

JDK 17 Windows test failed again but it is grpc and reactive messaging, definitely not related

[quarkus-bot]

Failing Jobs - Building a5daa31

Status	Name	Step	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 11
✔️	JVM Tests - JDK 17
✖	JVM Tests - JDK 17 Windows	Build	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 19

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/grpc/deployment extensions/smallrye-reactive-messaging-amqp/deployment 
! Skipped: extensions/micrometer-registry-prometheus/deployment extensions/micrometer/deployment extensions/opentelemetry/deployment and 51 more

📦 extensions/grpc/deployment

✖ io.quarkus.grpc.server.blocking.inheritance.MethodLevelBlockingTest.shouldBlockOnInheritedBlocking line 37 - More details - Source on GitHub

java.util.concurrent.TimeoutException: shouldBlockOnInheritedBlocking() timed out after 5 seconds
	at org.junit.jupiter.engine.extension.TimeoutExceptionFactory.create(TimeoutExceptionFactory.java:29)
	at org.junit.jupiter.engine.extension.SameThreadTimeoutInvocation.proceed(SameThreadTimeoutInvocation.java:58)

📦 extensions/smallrye-reactive-messaging-amqp/deployment

✖ io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest.test line 30 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.smallrye.reactivemessaging.amqp.AnonymousAmqpTest was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.smallrye.reactivemessaging.amqp.devmode.AmqpDevModeTest.testCodeUpdate line 44 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.smallrye.reactivemessaging.amqp.devmode.AmqpDevModeTest was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest.testConsumerUpdate line 77 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest 
Expecting size of:

✖ io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest.testProducerUpdate line 48 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a io.quarkus.smallrye.reactivemessaging.amqp.devmode.nohttp.AmqpDevModeNoHttpTest 
Expecting size of:

✖ io.quarkus.smallrye.reactivemessaging.amqp.SecuredAmqpTest.test line 28 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with io.quarkus.smallrye.reactivemessaging.amqp.SecuredAmqpTest was not fulfilled within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

[sberyozkin]

Should be good to go now