KAFKA-21113: Apply response.http.headers.config to the Connect admin listener - #23494
Open
ashwinpankaj wants to merge 2 commits into
Open
ashwinpankaj wants to merge 2 commits into
ashwinpankaj wants to merge 2 commits into
Conversation
…listener case Addresses a review comment on apache#23150 (the REST-extension-registration refactor): when admin.listeners is unset, admin resources fall back to sharing the regular ResourceConfig, so /admin/loggers should be reachable via advertisedUrl() with REST extension filters applied, same as any other resource on that listener. testRestExtensionsWithoutAdminListener only checked /connectors; add the equivalent assertion for /admin/loggers.
RestServer.initializeResources() only ever called configureHttpResponseHeaderFilter on the regular ServletContextHandler, never on the admin one. Same bug shape as the REST-extension-registration fix in the previous commit and apache#23150: when admin.listeners points at a distinct listener, response.http.headers.config -- commonly used for security headers like X-XSS-Protection, X-Frame-Options, Cache-Control -- was silently missing on all admin responses. Guard on adminContext != null, mirroring the existing pattern already used elsewhere in this method for admin resources/extensions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes KAFKA-21113:
RestServer.initializeResources()builds a separateServletContextHandler adminContextwheneveradmin.listenersis configured to a distinct value, butconfigureHttpResponseHeaderFilterwas only ever called on the regular context.response.http.headers.configis commonly used for security-hardening headers (the existing test suite itself usesX-XSS-Protection,Cache-Control, and referencesX-Frame-Options), so these were silently missing from all admin endpoint responses whenever a distinct admin listener was configured.This is the same "regular-only, admin left out" bug shape already fixed for REST extension registration in #23150. Fix: call
configureHttpResponseHeaderFilter(adminContext, headerConfig)too, guarded onadminContext != null, mirroring the guard already used elsewhere in the same method (adminContext.start(), extension registration). AddedtestCustomizedHttpResponseHeadersAppliedToAdminContextasserting the configured headers appear onserver.adminUrl()responses.Also includes a small, related test fix raised in review on #23150: this comment pointed out that
testRestExtensionsWithoutAdminListeneronly checked/connectorsviaadvertisedUrl(), not/admin/loggers, even though in the no-separate-admin-listener case admin resources fall back to sharing the regularResourceConfigand should be reachable the same way. Bundling it here since it's the same file and the same reviewer thread.Committer Checklist (excluded from commit message)