Re: [PR] [metrics][docs] Support Basic Auth for Prometheus PushGateway reporter [fluss]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #3552:
URL: https://github.com/apache/fluss/pull/3552#discussion_r3518094463


##########
fluss-metrics/fluss-metrics-prometheus/src/main/java/org/apache/fluss/metrics/prometheus/PrometheusPushGatewayReporter.java:
##########
@@ -46,12 +52,18 @@ public PrometheusPushGatewayReporter(
             String jobName,
             Map<String, String> groupingKey,
             final boolean deleteOnShutdown,
-            Duration pushInterval) {
+            Duration pushInterval,
+            @Nullable String username,
+            @Nullable String password) {
         this.pushGateway = new PushGateway(hostUrl);
         this.jobName = jobName;
         this.groupingKey = groupingKey;
         this.deleteOnShutdown = deleteOnShutdown;
         this.pushInterval = pushInterval;
+        if (username != null && !username.isEmpty()) {
+            this.pushGateway.setConnectionFactory(
+                    basicAuthConnectionFactory(username, password == null ? "" : password));
+        }

Review Comment:
   `username` is treated as configured when it is non-empty, but whitespace-only values (e.g. " ") will still enable Basic Auth and send an invalid `Authorization` header. This is inconsistent with the plugin, which treats whitespace-only usernames as disabled. Consider using the same `StringUtils.isNullOrWhitespaceOnly` check here as well.



##########
fluss-common/src/main/java/org/apache/fluss/config/ConfigOptions.java:
##########
@@ -2171,6 +2171,22 @@ public class ConfigOptions {
                             .withDescription(
                                     "The interval of pushing metrics to Prometheus PushGateway.");
 
+    public static final ConfigOption<String> METRICS_REPORTER_PROMETHEUS_PUSHGATEWAY_USERNAME =
+            key("metrics.reporter.prometheus-push.username")
+                    .stringType()
+                    .noDefaultValue()
+                    .withDescription(
+                            "The username for Basic Auth of the Prometheus PushGateway. "
+                                    + "Leave it unset to disable authentication.");
+
+    public static final ConfigOption<String> METRICS_REPORTER_PROMETHEUS_PUSHGATEWAY_PASSWORD =
+            key("metrics.reporter.prometheus-push.password")
+                    .stringType()
+                    .noDefaultValue()
+                    .withDescription(
+                            "The password for Basic Auth of the Prometheus PushGateway. "
+                                    + "Only takes effect when username is configured.");

Review Comment:
   `metrics.reporter.prometheus-push.password` is defined as a plain `String` option, which means it won’t be treated as sensitive by the configuration subsystem (e.g., it may be included in config dumps or error messages). Consider defining it as a `Password` option via `.passwordType()` so it is automatically redacted when configurations are logged or displayed.



##########
fluss-metrics/fluss-metrics-prometheus/src/main/java/org/apache/fluss/metrics/prometheus/PrometheusPushGatewayReporter.java:
##########
@@ -80,4 +92,17 @@ public void report() {
             LOG.warn("Could not push metrics to PushGateway.", e);
         }
     }
+
+    private static HttpConnectionFactory basicAuthConnectionFactory(String user, String password) {
+        final String header =
+                "Basic "
+                        + Base64.getEncoder()
+                                .encodeToString(
+                                        (user + ":" + password).getBytes(StandardCharsets.UTF_8));

Review Comment:
   The PR description says `BasicAuthHttpConnectionFactory` is injected into `PushGateway`, but the implementation here uses a custom `HttpConnectionFactory`. Please either update the PR description to match the implementation, or switch to the Prometheus client’s built-in factory if it is available in the dependency version used by this module.



-- 
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: issues-unsubscribe@fluss.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org