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 2021/01/26 08:25:47 UTC
[GitHub] [pulsar] RobertIndie opened a new pull request #9321: [pulsar-broker]Add alerts for expired tokens
RobertIndie opened a new pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321
### Motivation
Currently, pulsar-broker does not provide additional alerts about expired tokens.
### Modifications
Add additional alerts metrics for expired tokens.
### Verifying this change
This change is already covered by existing tests, such as *testExpiredTokenMetrics*.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#issuecomment-770379593
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#discussion_r565094843
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -202,6 +211,10 @@ private static String validateToken(final String token) throws AuthenticationExc
return jwt;
} catch (JwtException e) {
+ if (e instanceof ExpiredJwtException) {
+ String expiredTime = new Date(1000L * (Integer) ((ExpiredJwtException) e).getClaims().get("exp")).toString();
+ expiredTokenMetrics.labels(expiredTime).inc();
Review comment:
Thanks. It seems unnecessary to record expired time here. Here I made some changes: Remove the `expiredTime` label in `expiredTokenMetrics`, use `Histogram` to record tokens that are about to expire and the corresponding remaining time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#issuecomment-770370921
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #9321: [pulsar-broker]Add alerts for expired tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#issuecomment-767555792
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#issuecomment-768301106
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on a change in pull request #9321: [pulsar-broker]Add alerts for expired tokens
Posted by GitBox <gi...@apache.org>.
merlimat commented on a change in pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#discussion_r564687640
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -202,6 +211,10 @@ private static String validateToken(final String token) throws AuthenticationExc
return jwt;
} catch (JwtException e) {
+ if (e instanceof ExpiredJwtException) {
+ String expiredTime = new Date(1000L * (Integer) ((ExpiredJwtException) e).getClaims().get("exp")).toString();
+ expiredTokenMetrics.labels(expiredTime).inc();
Review comment:
This will create an unbound cardinality in the labels values. What's the rational to include the absolute time in the metric?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#issuecomment-768921716
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#issuecomment-768676817
/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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a change in pull request #9321: [pulsar-broker]Add alerts for expired tokens
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on a change in pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#discussion_r565094843
##########
File path: pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java
##########
@@ -202,6 +211,10 @@ private static String validateToken(final String token) throws AuthenticationExc
return jwt;
} catch (JwtException e) {
+ if (e instanceof ExpiredJwtException) {
+ String expiredTime = new Date(1000L * (Integer) ((ExpiredJwtException) e).getClaims().get("exp")).toString();
+ expiredTokenMetrics.labels(expiredTime).inc();
Review comment:
Thanks. It seems unnecessary to record expired time here. Here I made some changes: Remove the `expiredTime` label in `expiredTokenMetrics`, use `Histogram` to record tokens that are about to expire and the corresponding expiration time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] sijie commented on pull request #9321: [pulsar-broker]Add alerts for expired/expiring soon tokens
Posted by GitBox <gi...@apache.org>.
sijie commented on pull request #9321:
URL: https://github.com/apache/pulsar/pull/9321#issuecomment-770522873
@merlimat Can you review this again?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org