[fix][broker] Fix the reason label of authentication metrics #20030

gaoran10 · 2023-04-06T13:11:46Z

Motivation

The reason label of authentication metrics is a potential infinity string.
The PR #19758 also tries to fix this problem.

Use an enum ErrorCode as the authentication failure reason.

Modifications

Add an enum class ErrorCode for Pulsar authentication, it indicates the authentication failure reason.
Add a new exception PulsarAuthenticationException which contains the error code.

Verifying this change

Change the test to match this change.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository: gaoran10#26

mattisonchao · 2023-04-10T14:10:15Z

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderBasic.java


        try {
            if (users.get(userId) == null) {
-                throw new AuthenticationException(msg);
+                throw new PulsarAuthenticationException(msg, errorCode);


IMO, use ErrorCode.BASIC_INVALID_AUTH_DATA directly will be more clear. :)

Ok, I'll fix this.

mattisonchao · 2023-04-10T14:13:55Z

@michaeljmarshall would you mind to taking a look here?

codelipenghui

The change looks good me.
And I have added the release labels. Since it's a metrics changes that might impact the Pulsar operators, maybe not, I don't know someone will add alert rules or something based on the long reason message. But it still worth to start a discussion for cherry-picking the changes to release branches.

codelipenghui · 2023-04-10T23:18:10Z

And please also update the documentation after the RP get merged.

codelipenghui · 2023-04-10T23:18:16Z

/pulsarbot run-failure-checks

michaeljmarshall

Thanks for working on this @gaoran10, I just noticed this problem last week. It's definitely one we need to fix.

My primary concern is with the PulsarAuthenticationException class because it isn't really extensible for custom implementations of the AuthenticationProvider interface.

I also wonder if it would be better to add a new class that makes it so we can avoid each implementation from needing to increment the metrics on its own. As long as a provider throws a PulsarAuthenticationException, we could increment a metric in the broker/proxy/function worker/websocket proxy.

Let me know what you think. Thanks!

michaeljmarshall · 2023-04-11T01:42:23Z

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

@@ -157,8 +158,7 @@ public String authenticate(AuthenticationDataSource authData) throws Authenticat
            AuthenticationMetrics.authenticateSuccess(getClass().getSimpleName(), getAuthMethodName());
            return role;
        } catch (AuthenticationException exception) {
-            AuthenticationMetrics.authenticateFailure(getClass().getSimpleName(), getAuthMethodName(),
-                    exception.getMessage());
+            AuthenticationMetrics.authenticateFailure(getClass().getSimpleName(), getAuthMethodName(), exception);


I think it would be valuable to increment the metric outside of the implementations of the AuthenticationProvider so that all custom implementations can integrate with this feature.

Good idea, I think we can calculate the metric in the AuthenticationService.

It seems it's hard to find a unified entrance to increment the authentication metric. Maybe we can try to improve this in another PR. WDYT

michaeljmarshall · 2023-04-11T01:45:38Z

...mon/src/main/java/org/apache/pulsar/broker/authentication/PulsarAuthenticationException.java

+    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,
+    }


My primary concern with relying on this enum is that it is not easily extended. For example, I just merged the OIDC Provider with the following enum:

pulsar/pulsar-broker-auth-oidc/src/main/java/org/apache/pulsar/broker/authentication/oidc/AuthenticationExceptionCode.java

Lines 25 to 38 in bafecb2

public enum AuthenticationExceptionCode {

UNSUPPORTED_ISSUER,

UNSUPPORTED_ALGORITHM,

ISSUER_MISMATCH,

ALGORITHM_MISMATCH,

INVALID_PUBLIC_KEY,

ERROR_RETRIEVING_PROVIDER_METADATA,

ERROR_RETRIEVING_PUBLIC_KEY,

ERROR_DECODING_JWT,

ERROR_VERIFYING_JWT,

ERROR_VERIFYING_JWT_SIGNATURE,

INVALID_JWT_CLAIM,

EXPIRED_JWT,

}

From your list and from mine, my primary conclusion is that the types of errors are often provider specific, so a static enum that cannot be extended feels restrictive.

I agree that it is extremely important to limit the cardinality of these metrics, but I also think we want to make it easily extensible by users.

I think an alternative solution could work by making PulsarAuthenticationException abstract and letting each provider implement their own exception. Then, the provider itself could have its own enum, and when the framework calls getErrorCode, the implementation will convert its own enum into a string.

Good idea! I'll change this.

I add an abstract class BaseAuthenticationException, which extends AuthenticationException and has an abstract method public abstract Enum<?> getErrorCode(), I think developers can custom their exceptions based on the BaseAuthenticationException, the custom exception can provide the custom error code. PTAL

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?

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.

I removed the PulsarAuthenticationException, and only added some error codes for every authentication provider, like AuthenticationProviderOpenID . PTAL

michaeljmarshall · 2023-04-11T01:51:34Z

...mon/src/main/java/org/apache/pulsar/broker/authentication/metrics/AuthenticationMetrics.java

+    public static void authenticateFailure(String providerName, String authMethod,
+                                           AuthenticationException exception) {


Because this is a generic metrics class meant to be useful to all authentication provider implementations, I think we should provide an easy way to provide custom "reasons". I respect that it is easy to provide a reason that is high cardinality, but I think we should expect each implementation to handle those details.

I add an abstract class BaseAuthenticationException. PTAL

gaoran10 · 2023-04-17T05:43:20Z

@michaeljmarshall PTAL

gaoran10 · 2023-04-18T04:50:53Z

/pulsarbot run-failure-checks

michaeljmarshall

Thanks for making the changes @gaoran10. I left a couple comments.

michaeljmarshall · 2023-04-24T22:27:19Z

...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";


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.

I revert this change.

michaeljmarshall · 2023-04-25T04:57:28Z

...ommon/src/main/java/org/apache/pulsar/broker/authentication/AuthenticationProviderToken.java

        try {
            // Get Token
-            String token;
            token = getToken(authData);


Is it possible to increment the INVALID_AUTH_DATA error in the getToken method?

The method getToken is static, some methods use it and catch exceptions to record failure metrics, such as OpenID provider, if we increment the INVALID_AUTH_DATA error in the getToken method, this will generate repeat failure records.

OpenID provider

CompletableFuture<DecodedJWT> authenticateTokenAsync(AuthenticationDataSource authData) { String token; try { token = AuthenticationProviderToken.getToken(authData); } catch (AuthenticationException e) { incrementFailureMetric(AuthenticationExceptionCode.ERROR_DECODING_JWT); return CompletableFuture.failedFuture(e); } return authenticateToken(token) .whenComplete((jwt, e) -> { if (jwt != null) { AuthenticationMetrics.authenticateSuccess(getClass().getSimpleName(), getAuthMethodName()); } // Failure metrics are incremented within methods above }); }

Thank for your explanation.

gaoran10 · 2023-04-25T07:50:35Z

...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);


@michaeljmarshall I changed the method name label to openid, what do you think?

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.

Ok, I changed to use the AUTH_METHOD_NAME(token). Thanks.

michaeljmarshall

Thanks for addressing my feedback @gaoran10. Would you mind getting the build to pass? Then I'll re-review it. Thanks!

gaoran10 · 2023-05-04T04:35:52Z

/pulsarbot rerun-failure-checks

lhotari · 2023-05-04T07:48:08Z

Thanks for addressing my feedback @gaoran10. Would you mind getting the build to pass? Then I'll re-review it. Thanks!

@michaeljmarshall when changes are requested, the build in upstream won't run at all. I'll dismiss the "changes requested" to get the tests running.

to run CI pipeline

lhotari · 2023-05-04T07:48:50Z

/pulsarbot rerun-failure-checks

lhotari · 2023-05-04T14:57:33Z

Thanks for addressing my feedback @gaoran10. Would you mind getting the build to pass? Then I'll re-review it. Thanks!

@michaeljmarshall when changes are requested, the build in upstream won't run at all. I'll dismiss the "changes requested" to get the tests running.

I should have just added the "ready-to-test" label to the PR instead of dismissing the review. I forgot that it's the option that I should have used.

michaeljmarshall · 2023-05-04T15:07:44Z

/pulsarbot rerun-failure-checks

michaeljmarshall

LGTM

codecov-commenter · 2023-05-04T16:27:20Z

Codecov Report

Merging #20030 (613f2c6) into master (9b72302) will increase coverage by 39.74%.
The diff coverage is 67.60%.

@@              Coverage Diff              @@
##             master   #20030       +/-   ##
=============================================
+ Coverage     33.17%   72.92%   +39.74%     
- Complexity    12236    31967    +19731     
=============================================
  Files          1499     1868      +369     
  Lines        114413   138590    +24177     
  Branches      12431    15246     +2815     
=============================================
+ Hits          37962   101071    +63109     
+ Misses        71499    29461    -42038     
- Partials       4952     8058     +3106

Flag	Coverage Δ
inttests	`24.17% <0.00%> (?)`
systests	`24.77% <21.81%> (?)`
unittests	`72.21% <67.60%> (+39.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...e/pulsar/broker/authentication/oidc/JwksCache.java	`56.62% <0.00%> (ø)`
...thentication/oidc/OpenIDProviderMetadataCache.java	`78.48% <0.00%> (ø)`
...sar/broker/authorization/AuthorizationService.java	`57.20% <0.00%> (+47.20%)`	⬆️
...er/authentication/AuthenticationProviderBasic.java	`59.09% <50.00%> (+59.09%)`	⬆️
...r/authentication/AuthenticationProviderAthenz.java	`68.57% <68.75%> (ø)`
...ker/authentication/AuthenticationProviderList.java	`48.66% <71.42%> (+48.66%)`	⬆️
...er/authentication/AuthenticationProviderToken.java	`92.35% <80.00%> (+84.89%)`	⬆️
...oker/authentication/AuthenticationProviderTls.java	`83.33% <87.50%> (+29.16%)`	⬆️
...hentication/oidc/AuthenticationProviderOpenID.java	`75.86% <100.00%> (ø)`
.../broker/authentication/AuthenticationProvider.java	`72.72% <100.00%> (+32.72%)`	⬆️
... and 1 more

... and 1535 files with indirect coverage changes

(cherry picked from commit 2b515ff)

…20030) (cherry picked from commit 2b515ff) (cherry picked from commit 80dcea7)

michaeljmarshall · 2023-06-27T21:55:58Z

As discussed on the mailing list https://lists.apache.org/thread/w4jzk27qhtosgsz7l9bmhf1t7o9mxjhp, there is no plan to release 2.9.6, so I am going to remove the release/2.9.6 label

github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 6, 2023

gaoran10 self-assigned this Apr 6, 2023

gaoran10 added the area/metrics label Apr 6, 2023

gaoran10 requested review from tuteng, RobertIndie, michaeljmarshall, Technoboy-, dao-jun, poorbarcode, congbobo184 and mattisonchao April 6, 2023 17:47

mattisonchao reviewed Apr 10, 2023

View reviewed changes

mattisonchao approved these changes Apr 10, 2023

View reviewed changes

codelipenghui added the release/important-notice The changes which are important should be mentioned in the release note label Apr 10, 2023

codelipenghui added this to the 3.0.0 milestone Apr 10, 2023

codelipenghui added release/2.11.2 release/2.10.5 release/2.9.6 doc-required Your PR changes impact docs and you will update later. labels Apr 10, 2023

github-actions bot removed the doc-required Your PR changes impact docs and you will update later. label Apr 10, 2023

codelipenghui approved these changes Apr 10, 2023

View reviewed changes

michaeljmarshall requested changes Apr 11, 2023

View reviewed changes

cbornet changed the title ~~[fix][metrics] Fix the reason label of authentication metrics~~ [fix][broker] Fix the reason label of authentication metrics Apr 11, 2023

cbornet modified the milestones: 3.0.0, 3.1.0 Apr 11, 2023

Remove PulsarAuthenticationException

6612405

gaoran10 force-pushed the fix-authentication-reason-label branch from e434b54 to 6612405 Compare April 24, 2023 02:47

michaeljmarshall previously requested changes Apr 25, 2023

View reviewed changes

revert auth method name for AuthenticationProviderOpenID

ea3ae40

gaoran10 commented Apr 25, 2023

View reviewed changes

fix

613f2c6

michaeljmarshall reviewed May 4, 2023

View reviewed changes

gaoran10 closed this May 4, 2023

gaoran10 reopened this May 4, 2023

michaeljmarshall approved these changes May 4, 2023

View reviewed changes

gaoran10 merged commit 2b515ff into apache:master May 5, 2023

poorbarcode added the release/3.0.1 label May 6, 2023

poorbarcode pushed a commit that referenced this pull request May 7, 2023

[fix][broker] Fix the reason label of authentication metrics (#20030)

21c2edd

(cherry picked from commit 2b515ff)

poorbarcode added the cherry-picked/branch-3.0 label May 7, 2023

gaoran10 added a commit that referenced this pull request May 8, 2023

[fix][broker] Fix the reason label of authentication metrics (#20030)

80dcea7

(cherry picked from commit 2b515ff)

gaoran10 added the cherry-picked/branch-2.10 label May 8, 2023

gaoran10 added a commit that referenced this pull request May 9, 2023

[fix][broker] Fix the reason label of authentication metrics (#20030)

2ab530c

(cherry picked from commit 2b515ff)

Technoboy- added the cherry-picked/branch-2.11 label May 9, 2023

gaoran10 added a commit that referenced this pull request May 10, 2023

[fix][broker] Fix the reason label of authentication metrics (#20030)

f2f49f6

(cherry picked from commit 2b515ff)

gaoran10 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label May 11, 2023

nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request May 11, 2023

[fix][broker] Fix the reason label of authentication metrics (apache#…

c00eaa6

…20030) (cherry picked from commit 2b515ff) (cherry picked from commit 80dcea7)

michaeljmarshall removed the release/2.9.6 label Jun 27, 2023

	public enum AuthenticationExceptionCode {
	UNSUPPORTED_ISSUER,
	UNSUPPORTED_ALGORITHM,
	ISSUER_MISMATCH,
	ALGORITHM_MISMATCH,
	INVALID_PUBLIC_KEY,
	ERROR_RETRIEVING_PROVIDER_METADATA,
	ERROR_RETRIEVING_PUBLIC_KEY,
	ERROR_DECODING_JWT,
	ERROR_VERIFYING_JWT,
	ERROR_VERIFYING_JWT_SIGNATURE,
	INVALID_JWT_CLAIM,
	EXPIRED_JWT,
	}

		public static void authenticateFailure(String providerName, String authMethod,
		AuthenticationException exception) {

[fix][broker] Fix the reason label of authentication metrics #20030

[fix][broker] Fix the reason label of authentication metrics #20030

Conversation

gaoran10 commented Apr 6, 2023 • edited Loading

Motivation

Modifications

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattisonchao commented Apr 10, 2023

codelipenghui left a comment

Choose a reason for hiding this comment

codelipenghui commented Apr 10, 2023

codelipenghui commented Apr 10, 2023

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoran10 Apr 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaoran10 Apr 12, 2023 • edited Loading

Choose a reason for hiding this comment

gaoran10 commented Apr 17, 2023

gaoran10 commented Apr 18, 2023

michaeljmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaeljmarshall left a comment

Choose a reason for hiding this comment

gaoran10 commented May 4, 2023

lhotari commented May 4, 2023

lhotari commented May 4, 2023

lhotari commented May 4, 2023

michaeljmarshall commented May 4, 2023

michaeljmarshall left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 4, 2023

Codecov Report

michaeljmarshall commented Jun 27, 2023

gaoran10 commented Apr 6, 2023 •

edited

Loading

gaoran10 Apr 11, 2023 •

edited

Loading

gaoran10 Apr 12, 2023 •

edited

Loading