You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/03/19 11:58:55 UTC
[GitHub] [pulsar] liudezhi2098 opened a new pull request #14756: [metrics] Metrics http interface unified authentication configuration
liudezhi2098 opened a new pull request #14756:
URL: https://github.com/apache/pulsar/pull/14756
### Motivation
When proxy and broker enable authentication, I can get broker metrics, but I can't get proxy metrics, because of server returned HTTP status 401 Unauthorized。
* broker
```
// Add metrics servlet
webService.addServlet("/metrics",
new ServletHolder(metricsServlet),
false, attributeMap);
```
`http://brokerIp:port/metrics` **normal**
* proxy
```
server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
Collections.emptyList(), config.isAuthenticateMetricsEndpoint());
```
`http://proxyIp:port/metrics` **abnormal**
### Modifications
unified authentication configuration authenticateMetricsEndpoint and default false.
```
server.addServlet("/metrics", new ServletHolder(MetricsServlet.class),
Collections.emptyList(), config.isAuthenticateMetricsEndpoint());
```
### Verifying this change
*If `yes` was chosen, please highlight the changes*
- Dependencies (does it add or upgrade a dependency): (yes / no)
- The public API: (no)
- The schema: (no )
- The default values of configurations: (yes)
- The wire protocol: (no)
- The rest endpoints: (no)
- The admin cli options: (no)
- Anything that affects deployment: (no)
### Documentation
Check the box below or label this PR directly (if you have committer privilege).
Need to update docs?
- [x] `no-need-doc`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liudezhi2098 commented on a change in pull request #14756: [metrics] Metrics http interface unified authentication configuration
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on a change in pull request #14756:
URL: https://github.com/apache/pulsar/pull/14756#discussion_r830485161
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -864,7 +864,7 @@ private void addWebServerHandlers(WebService webService,
// Add metrics servlet
webService.addServlet("/metrics",
new ServletHolder(metricsServlet),
- false, attributeMap);
+ config.isAuthenticateMetricsEndpoint(), attributeMap);
Review comment:
I will add.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liudezhi2098 commented on pull request #14756: [metrics] Metrics http interface unified authentication configuration
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on pull request #14756:
URL: https://github.com/apache/pulsar/pull/14756#issuecomment-1073173073
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liudezhi2098 commented on pull request #14756: [metrics] Metrics http interface unified authentication configuration
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on pull request #14756:
URL: https://github.com/apache/pulsar/pull/14756#issuecomment-1075795918
@eolivelli PTAL
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] eolivelli commented on a change in pull request #14756: [metrics] Metrics http interface unified authentication configuration
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #14756:
URL: https://github.com/apache/pulsar/pull/14756#discussion_r830483636
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java
##########
@@ -332,10 +332,10 @@
private boolean forwardAuthorizationCredentials = false;
@FieldContext(
category = CATEGORY_AUTHENTICATION,
- doc = "Whether the '/metrics' endpoint requires authentication. Defaults to true."
+ doc = "Whether the '/metrics' endpoint requires authentication. Defaults to false."
+ "'authenticationEnabled' must also be set for this to take effect."
)
- private boolean authenticateMetricsEndpoint = true;
+ private boolean authenticateMetricsEndpoint = false;
Review comment:
This is a breaking change, related to security.
It is fine to make configuration more strict but not less strict otherwise users who upgrade may not notice the change.
It is fine to be to default to requiring auth on the broker by default but not to change this default value in the proxy
##########
File path: pulsar-broker/src/main/java/org/apache/pulsar/broker/PulsarService.java
##########
@@ -864,7 +864,7 @@ private void addWebServerHandlers(WebService webService,
// Add metrics servlet
webService.addServlet("/metrics",
new ServletHolder(metricsServlet),
- false, attributeMap);
+ config.isAuthenticateMetricsEndpoint(), attributeMap);
Review comment:
It looks like this change is not covered by tests
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] liudezhi2098 commented on a change in pull request #14756: [metrics] Metrics http interface unified authentication configuration
Posted by GitBox <gi...@apache.org>.
liudezhi2098 commented on a change in pull request #14756:
URL: https://github.com/apache/pulsar/pull/14756#discussion_r830485002
##########
File path: pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConfiguration.java
##########
@@ -332,10 +332,10 @@
private boolean forwardAuthorizationCredentials = false;
@FieldContext(
category = CATEGORY_AUTHENTICATION,
- doc = "Whether the '/metrics' endpoint requires authentication. Defaults to true."
+ doc = "Whether the '/metrics' endpoint requires authentication. Defaults to false."
+ "'authenticationEnabled' must also be set for this to take effect."
)
- private boolean authenticateMetricsEndpoint = true;
+ private boolean authenticateMetricsEndpoint = false;
Review comment:
ok
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org