Refacoring test case for MisdirectedFilter - #51443
Conversation
Closes keycloak#51442 Signed-off-by: Alexander Schwartz <alexander.schwartz@gmx.net>
7d8f053 to
0ff60bd
Compare
There was a problem hiding this comment.
Pull request overview
Moves MisdirectedFilter coverage into the regular test suite for easier debugging and faster execution.
Changes:
- Adds test-framework-based HTTP/2 misdirection coverage.
- Removes equivalent distribution test code.
- Documents the filter’s purpose and behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/base/.../MisdirectedRequestTest.java |
Adds the migrated TLS test. |
quarkus/tests/.../HttpDistTest.java |
Removes the former distribution test. |
quarkus/runtime/.../MisdirectedFilter.java |
Adds class-level JavaDoc. |
Suppressed comments (1)
tests/base/src/test/java/org/keycloak/tests/ssl/MisdirectedRequestTest.java:74
- A hung Vert.x shutdown can add another five-minute delay after the client cleanup timeout, so one failure may hold a test worker for roughly ten minutes. Restore the prior seconds-based bound for automated runs.
// Timeout set to minutes to allow manual debugging
vertx.close().toCompletionStage().toCompletableFuture().get(5, TimeUnit.MINUTES);
|
@shawkins - please review when you have the time. Thanks! |
| * <li>No SNI sent ({@code indicatedServerName()} is null) — happens with health checks | ||
| * or non-FQDN connections; rejecting these would break monitoring.</li> | ||
| * <li>SNI matches authority — the connection was established for this hostname. | ||
| * This also allows health checks that connect with an arbitrary hostname | ||
| * not configured in Keycloak.</li> |
There was a problem hiding this comment.
Not sure if you want to mention it, but these account for all "backend" requests made directly to the Keycloak instance, not just monitoring.
| * <li>Authority matches a configured Keycloak hostname ({@code hostname} or | ||
| * {@code hostname-admin}) — the request is legitimately for this server, even if | ||
| * the connection was originally established for a different hostname.</li> |
There was a problem hiding this comment.
Might be worth mentioning here that we aren't yet trying to account for the realm front-end urls - and that 421s may erroneously occur then.
There was a problem hiding this comment.
What are "realm front-end urls"? It seems that I missed that discussion :D
If I know what they are, I'm happy to add them.
There was a problem hiding this comment.
Each realm can be configured to have its own frontend url -
The hostname provider looks for it here
|
@shawkins - given the discussion in the parent issue, I'd like to know if you want to keep the test case in the original place, or move it. I'll leave it you and the cloud native dist team to decide. |
I'd just like to capture additional issues like adding embedded mode for quarkus integration tests, or moving additional tests to the new test suite, to capture more of the motivation behind doing this. After that, it doesn't really matter to me where the test lives. |
@shawkins - can you please make this more actionable for me? Should I keep the changes about the test, or should I remove it? |
I wasn't aware of this concept as I never used it. I thought the current configuration was already hard enough, but it is then even more complex...
This makes me feel that I'm not the right person to document that. If I think about your comment, 421s will erroneously occur. And if I understand it correctly, it would occur by default once someone uses TLS and Keycloak assumes it is passthrough. This sounds to me like a bug. I'd say this is better handled in the cloud-native team. Feel free to reuse any part of this PR, but my knowledge seems to be too limited here. |
Again I don't have a strong opinion on where the test is - if you want it in the new test suite that's fine. What I'm looking for on the issue is to make sure we capture more behind the intent for that change. Otherwise it just seems like a one-off. It sounds like we'll be capturing one issue to add embedded mode for the quarkus integration tests. Then there is already an issue on reusing the quarkus integration test optimization for the new test suite if performance is a concern. Finally since we already had some discussions on this, I was wondering if we should have a parent issue on migrating more of the quarkus integration tests to the new tests |
If and only if realm frontend urls are in use and they are using a different host from the hostname / hostname admin. This should not be a common configuration.
It could be seen as bug, but out initial perspective is that this would be a small performance regression for an uncommon setup. The browser is expected to automatically retry with a new connection to the realm frontend should it get a 421 trying to switch from another realm frontend, the hostname, or hostname admin. |
Closes #51442