[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #20030: [fix][broker] Fix the reason label of authentication metrics
michaeljmarshall commented on code in PR #20030:
URL: https://github.com/apache/pulsar/pull/20030#discussion_r1185177941
##
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##
@@ -158,19 +164,18 @@ public String getAuthMethodName() {
@Override
public String authenticate(AuthenticationDataSource authData) throws
AuthenticationException {
+String token;
try {
// Get Token
-String token;
token = getToken(authData);
Review Comment:
Thank for your explanation.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #20030: [fix][broker] Fix the reason label of authentication metrics
michaeljmarshall commented on code in PR #20030:
URL: https://github.com/apache/pulsar/pull/20030#discussion_r1176776887
##
pulsar-broker-auth-oidc/src/main/java/org/apache/pulsar/broker/authentication/oidc/AuthenticationProviderOpenID.java:
##
@@ -447,7 +447,7 @@ DecodedJWT verifyJWT(PublicKey publicKey,
}
static void incrementFailureMetric(AuthenticationExceptionCode code) {
-AuthenticationMetrics.authenticateFailure(SIMPLE_NAME, "token",
code.toString());
+AuthenticationMetrics.authenticateFailure(SIMPLE_NAME, "openid", code);
Review Comment:
I don't think this is necessary because the `SIMPLE_NAME` is the class's
name, which has `openid` in it. I wonder if we really even need the auth method
if we also have the class name.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #20030: [fix][broker] Fix the reason label of authentication metrics
michaeljmarshall commented on code in PR #20030:
URL: https://github.com/apache/pulsar/pull/20030#discussion_r1176006164
##
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java:
##
@@ -158,19 +164,18 @@ public String getAuthMethodName() {
@Override
public String authenticate(AuthenticationDataSource authData) throws
AuthenticationException {
+String token;
try {
// Get Token
-String token;
token = getToken(authData);
Review Comment:
Is it possible to increment the `INVALID_AUTH_DATA` error in the `getToken`
method?
##
pulsar-broker-auth-oidc/src/main/java/org/apache/pulsar/broker/authentication/oidc/AuthenticationProviderOpenID.java:
##
@@ -89,7 +89,7 @@ public class AuthenticationProviderOpenID implements
AuthenticationProvider {
private static final String SIMPLE_NAME =
AuthenticationProviderOpenID.class.getSimpleName();
// Must match the value used by the OAuth2 Client Plugin.
-private static final String AUTH_METHOD_NAME = "token";
+private static final String AUTH_METHOD_NAME = "openid";
Review Comment:
This needs to be `token` in order to support the
`AuthenticationProviderList`. Note that the oauth2 client plugin uses `token`
as well. Also, it is valid to use the token authentication data provider in
cases where the oauth2 token is fetched for you, like the k8s service account
token.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [pulsar] michaeljmarshall commented on a diff in pull request #20030: [fix][broker] Fix the reason label of authentication metrics
michaeljmarshall commented on code in PR #20030:
URL: https://github.com/apache/pulsar/pull/20030#discussion_r1170828506
##
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authentication/PulsarAuthenticationException.java:
##
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.pulsar.broker.authentication;
+
+import javax.naming.AuthenticationException;
+import lombok.Getter;
+
+/**
+ * PulsarAuthenticationException indicates Authentication exception in Pulsar,
it contains an error code.
+ */
+public class PulsarAuthenticationException extends AuthenticationException {
+
+public enum ErrorCode {
+UNKNOWN,
+PROVIDER_LIST_AUTH_REQUIRED,
+BASIC_INVALID_TOKEN,
+BASIC_INVALID_AUTH_DATA,
+AUTHZ_NO_CLIENT,
+AUTHZ_NO_TOKEN,
+AUTHZ_NO_PUBLIC_KEY,
+AUTHZ_DOMAIN_MISMATCH,
+AUTHZ_INVALID_TOKEN,
+TLS_NO_CERTS,
+TLS_NO_CN, // cn: common name
+TOKEN_INVALID_HEADER,
+TOKEN_NO_AUTH_DATA,
+TOKEN_EMPTY_TOKEN,
+TOKEN_INVALID_TOKEN,
+TOKEN_INVALID_AUDIENCES,
+}
Review Comment:
My primary concern is that `ErrorCode` has many enums that are specific to
each provider and are not generally useful. What do you think about the
following class?
```java
public class PulsarAuthenticationException extends
AuthenticationException {
private final String errorCode;
// Warning, the errorLabel must not have high cardinality
public PulsarAuthenticationException(String message, String
errorLabel) {
super(message);
this.errorLabel = errorLabel;
}
public String getErrorLabel() {
return errorLabel;
}
}
```
I think it is reasonable to expect maintainers to know that the `errorLabel`
must not be high cardinality, and this way, we don't have enumerations that
combine unrelated labels. If a provider wants to use an enum for its own
tracking, that would be perfectly reasonable, and it would probably work just
like the `AuthenticationProviderOpenID`.
--
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-unsubscr...@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
