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

Use Uni to resolve TenantContextConfig #13121

Merged
merged 1 commit into from
Nov 10, 2020

Conversation

sberyozkin
Copy link
Member

@sberyozkin sberyozkin commented Nov 4, 2020

Fixes #12754
Fixes #13040

This PR is big enough but it simply properly addresses #12754 with the help from Clement. So in this PR:

  • TenantConfigContext is resolved as Uni in OidcRecorder which avoids various blocked Vert.x issues when OIDC connections are established dynamically and also should make it faster since only part of the call runs on the executor thread. This is why PR affects so much code as now Uni<TenantConfigContext> gets transformed into another Uni completing the call.
  • Cleaned up DefaultTenantConfigResolver: 1) instead of resolve(vertxContext, true/false) it is now either resolveConfig or resolveTenant 2) isBlocking has been moved out which now only blocks if user info or refreshing the tokens is required (this will be optimized to the just in time blocking call) 3) updated it to avoid calling the resolvers more than once during the current request

I hope it make it to 10.0.0.CR1 as it has to settle a bit and it brings some good improvements

CC @cescoffier

@sberyozkin sberyozkin added this to the 1.10 - master milestone Nov 4, 2020
@sberyozkin sberyozkin force-pushed the uni_tenant_context_resolver branch from dd74d09 to 57e7eb9 Compare November 5, 2020 09:31
@sberyozkin
Copy link
Member Author

@stuartwdouglas @pedroigor PR build was green but I've rebased and force-pushed to make sure it is definitely green :-) I'm feeling much more confident about this code now though...

@sberyozkin
Copy link
Member Author

JDK15: Caused by: io.fabric8.maven.docker.access.DockerAccessException: Unable to pull 'quay.io/keycloak/keycloak:11.0.2' from registry 'quay.io' : {"message":"Get https://quay.io/v2/: EOF"} (Internal Server Error: 500)

@sberyozkin sberyozkin force-pushed the uni_tenant_context_resolver branch from 57e7eb9 to 53ad0b2 Compare November 5, 2020 10:38
@sberyozkin sberyozkin force-pushed the uni_tenant_context_resolver branch from 53ad0b2 to 5cab288 Compare November 9, 2020 16:59
@sberyozkin sberyozkin added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Nov 9, 2020
@sberyozkin
Copy link
Member Author

[INFO] 
Error:  Failures: 
Error:    FormAuthCookiesTestCase.testFormBasedAuthSuccess:117 1 expectation failed.
Expected status code <200> but was <302>.

This is unrelated, looks like this test is brittle though I've never seen it failing before

@sberyozkin sberyozkin force-pushed the uni_tenant_context_resolver branch from 5cab288 to a868d08 Compare November 9, 2020 22:07
@gastaldi gastaldi merged commit 53a8784 into quarkusio:master Nov 10, 2020
@sberyozkin sberyozkin deleted the uni_tenant_context_resolver branch November 10, 2020 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
3 participants