[FLINK-40592][metrics] Fix PushGateway basic authentication without JAXB - #29151
[FLINK-40592][metrics] Fix PushGateway basic authentication without JAXB#29151weicheng-07 wants to merge 1 commit into
Conversation
| } | ||
| super.close(); | ||
| } | ||
|
|
There was a problem hiding this comment.
My AI found:
-
com.sun.net.httpserver.HttpServer in the test
The test uses com.sun.net.httpserver.HttpServer — a JDK internal API. While it's technically stable and available in JDK 8–21, it's in com.sun.* which convention discourages. The Flink codebase already has prior usages, so this likely won't be a blocker, but worth flagging. -
The null credential cases silently drop auth with no warning
Looking at the test cases ", ,", "user, ,", ", password," — when username or password is null, the expectedAuthorization is also null (no header is set). The existing upstream behaviour is preserved here, but there is no log warning emitted when only one of the two credentials is provided. A half-configured auth (e.g. username but no password) silently results in unauthenticated requests. This is arguably a pre-existing issue, but the PR is touching this code path. -
Class name shadowing
The new inner class is named BasicAuthHttpConnectionFactory, which is the same simple name as the upstream class being replaced (io.prometheus.client.exporter.BasicAuthHttpConnectionFactory). The old import is removed so there's no actual conflict, but it could cause confusion if the upstream library ever gets imported again in the future. A name like JdkBasicAuthHttpConnectionFactory would be clearer. -
Minor: assertThat(reporter.getClass().getClassLoader()).isSameAs(classLoader) is overly strict
This assertion verifies that the reporter class itself was loaded by the custom classloader. While correct by construction, it's testing implementation internals of the classloader setup rather than the behaviour under test (auth header correctness). If the factory loading mechanism ever changes, this assertion will fail for non-auth-related reasons.
There was a problem hiding this comment.
Updated in f99b044.
-
HttpServeris a documented API exported by the JDK'sjdk.httpservermodule, despite thecom.sunpackage name. It is JDK-specific and is also used by other Flink tests, so I retained it here. -
Incomplete credentials already trigger a warning in the reporter factory. Existing username-only and password-only tests assert those warnings. The new test uses that factory and preserves unauthenticated mode when neither credential is set.
-
Renamed the private helper to
JdkBasicAuthHttpConnectionFactoryto distinguish it from the upstream class. -
Replaced the classloader identity assertion with a check that the reporter's actual defining classloader cannot load
javax.xml.bind.DatatypeConverter, expectingClassNotFoundException. This protects the regression test against JAXB on the test classpath masking its absence from the reporter's runtime. The HTTP Authorization assertions for PUT and DELETE remain.
The Prometheus module clean verify passed locally: 34 tests, including all six authentication cases. The updated regression test still reproduces the three JAXB initialization errors with the unfixed reporter; the three control cases pass.
MartijnVisser
left a comment
There was a problem hiding this comment.
Please squash the second commit into the first, it only reworks what the first one adds. I reproduced the failure on master and confirmed the six cases pass here, Azure is green on f99b044.
| private final String authorizationHeader; | ||
|
|
||
| private JdkBasicAuthHttpConnectionFactory(String username, String password) { | ||
| // Use the JDK encoder because simpleclient's implementation requires JAXB. |
There was a problem hiding this comment.
PushGateway.base64url calls DatatypeConverter too, so a slash in the job name or a grouping key value still fails without JAXB. Here push throws NoClassDefFoundError, which report() misses because it is an Error. Out of scope, that one needs FLINK-29623.
There was a problem hiding this comment.
Confirmed: URL encoding through PushGateway.base64url still depends on JAXB in simpleclient 0.8.1. This PR remains scoped to Basic Auth header encoding; the client upgrade is tracked in FLINK-29623.
| }); | ||
| server.start(); | ||
| try (URLClassLoader classLoader = createClassLoaderWithoutJaxb()) { | ||
| assertThatThrownBy(() -> classLoader.loadClass("javax.xml.bind.DatatypeConverter")) |
There was a problem hiding this comment.
The check below on the reporter's own classloader resolves to the same loader, is this one still needed?
There was a problem hiding this comment.
Removed the earlier assertion and retained the check on the reporter's defining classloader. Squashed the changes into 0a074852, leaving one commit. The module clean verify passes all 34 tests; the updated regression still produces the three expected JAXB errors with the unfixed reporter.
Encode the Basic Authorization header with JDK Base64 and UTF-8 while preserving the default connection factory and credential configuration. Cover PUT and DELETE requests with regression tests that isolate JAXB. Generated-by: OpenAI Codex 0.153.4
f99b044 to
0a07485
Compare
What is the purpose of the change
Fixes FLINK-40592.
Configuring both credentials causes
PrometheusPushGatewayReporterto fail during initialization when JAXB is absent. Use JDK Base64 encoding so Basic authentication no longer requires JAXB.Brief change log
java.util.Base64, preserving the default connection factory and existing credential behavior.Verifying this change
./mvnw -Djdk17 -Pjava17-target -pl flink-metrics/flink-metrics-prometheus clean verifypassed for commit0a074852on macOS aarch64 with JDK 17 and Maven 3.9.16: 34 tests, including all six authentication cases; Checkstyle, Spotless, and japicmp passed.f99b0448. Community CI for the squashed commit0a074852, which removes the redundant test assertion, is pending.Does this pull request potentially affect one of the following parts:
@Public(Evolving): yes — the reporter is@PublicEvolving; only private implementation changes, with no public signature or configuration changes.Documentation
Was generative AI tooling used to co-author this PR?
Generated-by: OpenAI Codex 0.153.4