Skip to content

ZOOKEEPER-5029: Port unification for PrometheusMetricsProvider #2362

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PDavid wants to merge 1 commit into
base: master
Choose a base branch
from
+73 −17
Open

ZOOKEEPER-5029: Port unification for PrometheusMetricsProvider #2362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 53 additions & 13 deletions ...rics/src/main/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,11 @@
import org.apache.zookeeper.metrics.MetricsProviderLifeCycleException;
import org.apache.zookeeper.metrics.Summary;
import org.apache.zookeeper.metrics.SummarySet;
import org.apache.zookeeper.server.admin.UnifiedConnectionFactory;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.server.HttpConfiguration;
import org.eclipse.jetty.server.HttpConnectionFactory;
import org.eclipse.jetty.server.SecureRequestCustomizer;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.ServerConnector;
import org.eclipse.jetty.servlet.ServletContextHandler;
Expand Down Expand Up @@ -84,6 +89,7 @@ public class PrometheusMetricsProvider implements MetricsProvider {
private boolean wantClientAuth = true; // Secure default
private String enabledProtocols;
private String cipherSuites;
private int httpVersion;

// Constants for configuration
public static final String HTTP_HOST = "httpHost";
Expand All @@ -101,7 +107,10 @@ public class PrometheusMetricsProvider implements MetricsProvider {
public static final String SSL_WANT_CLIENT_AUTH = "ssl.want.client.auth";
public static final String SSL_ENABLED_PROTOCOLS = "ssl.enabledProtocols";
public static final String SSL_ENABLED_CIPHERS = "ssl.ciphersuites";
public static final String HTTP_VERSION = "httpVersion";
public static final int SCAN_INTERVAL = 60 * 10; // 10 minutes
public static final int DEFAULT_HTTP_VERSION = 11; // based on HttpVersion.java in jetty
public static final int DEFAULT_STS_MAX_AGE = 1 * 24 * 60 * 60; // seconds in a day

/**
* Custom servlet to disable the TRACE method for security reasons.
Expand Down Expand Up @@ -139,6 +148,7 @@ public void configure(Properties configuration) throws MetricsProviderLifeCycleE
this.wantClientAuth = Boolean.parseBoolean(configuration.getProperty(SSL_WANT_CLIENT_AUTH, "true"));
this.enabledProtocols = configuration.getProperty(SSL_ENABLED_PROTOCOLS);
this.cipherSuites = configuration.getProperty(SSL_ENABLED_CIPHERS);
this.httpVersion = Integer.getInteger(HTTP_VERSION, DEFAULT_HTTP_VERSION);
}

// Validate that at least one port is configured.
Expand Down Expand Up @@ -171,23 +181,47 @@ public void start() throws MetricsProviderLifeCycleException {
int acceptors = 1;
int selectors = 1;

// Configure HTTP connector if enabled
if (this.httpPort != -1) {
ServerConnector httpConnector = new ServerConnector(server, acceptors, selectors);
httpConnector.setPort(this.httpPort);
httpConnector.setHost(this.host);
server.addConnector(httpConnector);
}
ServerConnector connector = null;

if (this.httpPort != -1 && this.httpsPort != -1 && this.httpPort == this.httpsPort) {
SecureRequestCustomizer customizer = new SecureRequestCustomizer();
customizer.setStsMaxAge(DEFAULT_STS_MAX_AGE);
Copy link
Copy Markdown
Contributor

@anmolnar anmolnar Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is STS?

Copy link
Copy Markdown
Contributor Author

@PDavid PDavid Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

STS means the Strict-Transport-Security HTTP response header.

The HTTP Strict-Transport-Security response header (often abbreviated as HSTS) informs browsers that the host should only be accessed using HTTPS, and that any future attempts to access it using HTTP should automatically be upgraded to HTTPS. Additionally, on future connections to the host, the browser will not allow the user to bypass secure connection errors, such as an invalid certificate. HSTS identifies a host by its domain name only.

...

max-age=<expire-time>
The time, in seconds, that the browser should remember that a host is only to be accessed using HTTPS.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Strict-Transport-Security

This is implemented here the same way as we have it in JettyAdminServer:
https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java#L119

customizer.setStsIncludeSubDomains(true);
Copy link
Copy Markdown
Contributor

@anmolnar anmolnar Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to add some comments to briefly explain what is this about.

Copy link
Copy Markdown
Contributor Author

@PDavid PDavid Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sets a directive of the Strict-Transport-Security HTTP response header.

includeSubDomains Optional
If this directive is specified, the HSTS policy applies to all subdomains of the host's domain as well.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Strict-Transport-Security#directives

Same as we have in JettyAdminServer here:

https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/server/admin/JettyAdminServer.java#L120

Copy link
Copy Markdown
Contributor

@anmolnar anmolnar Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation. I think it'd be beneficial to briefly mention these in a Javadoc.


HttpConfiguration config = new HttpConfiguration();
config.setSecureScheme("https");
config.addCustomizer(customizer);

// Configure HTTPS connector if enabled
if (this.httpsPort != -1) {
SslContextFactory.Server sslContextFactory = createSslContextFactory();
KeyStoreScanner keystoreScanner = new KeyStoreScanner(sslContextFactory);
keystoreScanner.setScanInterval(SCAN_INTERVAL);
server.addBean(keystoreScanner);
server.addConnector(createSslConnector(server, acceptors, selectors, sslContextFactory));
setKeyStoreScanner(sslContextFactory);

String nextProtocol = HttpVersion.fromVersion(httpVersion).asString();
connector = new ServerConnector(server,
new UnifiedConnectionFactory(sslContextFactory, nextProtocol),
new HttpConnectionFactory(config));
connector.setPort(this.httpPort);
connector.setHost(this.host);
LOG.debug("Created unified ServerConnector for host: {}, httpPort: {}", host, httpPort);
Copy link
Copy Markdown
Contributor

@anmolnar anmolnar Apr 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can move these messages to INFO level. We should see clearly which type of server has been initialized.

} else {
// Configure HTTP connector if enabled
if (this.httpPort != -1) {
connector = new ServerConnector(server, acceptors, selectors);
connector.setPort(this.httpPort);
connector.setHost(this.host);
LOG.debug("Created ServerConnector for host: {}, httpPort: {}", host, httpPort);
}

// Configure HTTPS connector if enabled
if (this.httpsPort != -1) {
SslContextFactory.Server sslContextFactory = createSslContextFactory();
setKeyStoreScanner(sslContextFactory);
connector = createSslConnector(server, acceptors, selectors, sslContextFactory);
LOG.debug("Created HTTPS ServerConnector for host: {}, httpsPort: {}", host, httpsPort);
}
}

server.addConnector(connector);

// Set up the servlet context handler
ServletContextHandler context = new ServletContextHandler();
context.setContextPath("/");
Expand All @@ -207,6 +241,12 @@ public void start() throws MetricsProviderLifeCycleException {
}
}

private void setKeyStoreScanner(SslContextFactory.Server sslContextFactory) {
KeyStoreScanner keystoreScanner = new KeyStoreScanner(sslContextFactory);
keystoreScanner.setScanInterval(SCAN_INTERVAL);
server.addBean(keystoreScanner);
}

/**
* Creates and configures the SslContextFactory for the server.
*
Expand Down
8 changes: 4 additions & 4 deletions ...test/java/org/apache/zookeeper/metrics/prometheus/PrometheusHttpsMetricsProviderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,10 @@
public class PrometheusHttpsMetricsProviderTest extends PrometheusMetricsTestBase {

private PrometheusMetricsProvider provider;
private String httpHost = "127.0.0.1";
private int httpsPort = 4443;
private int httpPort = 4000;
private String testDataPath = System.getProperty("test.data.dir", "src/test/resources/data");
private final String httpHost = "127.0.0.1";
private final int httpsPort = 4443;
private final int httpPort = 4000;
private final String testDataPath = System.getProperty("test.data.dir", "src/test/resources/data");

public void initializeProviderWithCustomConfig(Properties inputConfiguration) throws Exception {
provider = new PrometheusMetricsProvider();
Expand Down
16 changes: 16 additions & 0 deletions ...est/java/org/apache/zookeeper/metrics/prometheus/PrometheusMetricsProviderConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,4 +292,20 @@ public void checkServerTrusted(X509Certificate[] certs, String authType) {
}
};
}

@Test
public void testPortUnification() throws Exception {
int unifiedPort = 5400;
Properties configuration = new Properties();
configuration.setProperty("httpsPort", String.valueOf(unifiedPort));
configuration.setProperty("httpPort", String.valueOf(unifiedPort));
String testDataPath = System.getProperty("test.data.dir", "src/test/resources/data");
configuration.setProperty("ssl.keyStore.location", testDataPath + "/ssl/server_keystore.jks");
configuration.setProperty("ssl.keyStore.password", "testpass");
configuration.setProperty("ssl.trustStore.location", testDataPath + "/ssl/server_truststore.jks");
configuration.setProperty("ssl.trustStore.password", "testpass");
PrometheusMetricsProvider provider = new PrometheusMetricsProvider();
provider.configure(configuration);
provider.start();
}
}
Loading