Myracle commented on code in PR #27576:
URL: https://github.com/apache/flink/pull/27576#discussion_r2797271869
##########
flink-metrics/flink-metrics-prometheus/src/test/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporterTest.java:
##########
@@ -68,4 +77,62 @@ void
testConnectToPushGatewayThrowsExceptionWithoutHostInformation() {
assertThatThrownBy(() -> factory.createMetricReporter(metricConfig))
.isInstanceOf(IllegalArgumentException.class);
}
+
+ @Test
+ void testBasicAuthNotEnabledWithoutCredentials() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "http://localhost:9091";);
+ // No authentication configured - should create reporter successfully
without Basic Auth
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ assertThat(reporter).isNotNull();
+
assertThat(reporter.hostUrl.toString()).isEqualTo("http://localhost:9091";);
+ }
+
+ @Test
+ void testBasicAuthNotEnabledWithOnlyUsername() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "http://localhost:9091";);
+ metricConfig.setProperty(USERNAME.key(), "flink-user");
+ // Only username configured - should create reporter successfully
without Basic Auth
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ assertThat(reporter).isNotNull();
+ // Verify warning log is emitted
+ assertThat(loggerExtension.getMessages())
+ .anyMatch(
+ msg ->
+ msg.contains("Both username and password must
be configured")
+ && msg.contains("Currently only
username is configured"));
+ }
+
+ @Test
+ void testBasicAuthNotEnabledWithOnlyPassword() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "http://localhost:9091";);
+ metricConfig.setProperty(PASSWORD.key(), "flink-password");
+ // Only password configured - should create reporter successfully
without Basic Auth
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
+ assertThat(reporter).isNotNull();
+ // Verify warning log is emitted
+ assertThat(loggerExtension.getMessages())
+ .anyMatch(
+ msg ->
+ msg.contains("Both username and password must
be configured")
+ && msg.contains("Currently only
password is configured"));
+ }
+
+ @Test
+ void testBasicAuthEnabledWithBothCredentials() {
+ PrometheusPushGatewayReporterFactory factory = new
PrometheusPushGatewayReporterFactory();
+ MetricConfig metricConfig = new MetricConfig();
+ metricConfig.setProperty(HOST_URL.key(), "http://localhost:9091";);
+ metricConfig.setProperty(USERNAME.key(), "flink-user");
+ metricConfig.setProperty(PASSWORD.key(), "flink-password");
+ // Both username and password configured - Basic Auth should be enabled
+ PrometheusPushGatewayReporter reporter =
factory.createMetricReporter(metricConfig);
Review Comment:
Thanks for the suggestion! I've added a @VisibleForTesting boolean
basicAuthEnabled field following the same pattern as hostUrl. Now all four
basic auth test cases verify this property:
testBasicAuthNotEnabledWithoutCredentials - asserts basicAuthEnabled is false
testBasicAuthNotEnabledWithOnlyUsername - asserts basicAuthEnabled is false
testBasicAuthNotEnabledWithOnlyPassword - asserts basicAuthEnabled is false
testBasicAuthEnabledWithBothCredentials - asserts basicAuthEnabled is true
##########
flink-metrics/flink-metrics-prometheus/src/main/java/org/apache/flink/metrics/prometheus/PrometheusPushGatewayReporterFactory.java:
##########
@@ -70,17 +72,28 @@ public PrometheusPushGatewayReporter
createMetricReporter(Properties properties)
jobName = configuredJobName + new AbstractID();
}
+ String username = metricConfig.getString(USERNAME.key(), null);
Review Comment:
Thanks for catching this! You're absolutely right - I've updated the code to
use USERNAME.defaultValue() and PASSWORD.defaultValue() instead of hardcoded
null to maintain consistency with other configuration options like HOST_URL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
