feat: classify authorization exception as user error #7061

mjsax · 2021-02-21T20:57:05Z

Description

If we receive a AuthorizationException it indicates as configuration issue, thus, we should classify it as a user error.

Testing done

Added corresponding unit tests.

Reviewer checklist

Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
Ensure relevant issues are linked (description should include text like "Fixes #")

ghost · 2021-02-21T20:57:07Z

@confluentinc It looks like @mjsax just signed our Contributor License Agreement. 👍

Always at your service,

clabot

agavra

LGTM! It might make sense to test this locally to make sure that this is exactly the error type that is raised (and not wrapped in something else)

agavra · 2021-02-22T17:13:37Z

ksqldb-engine/src/main/java/io/confluent/ksql/query/TopicAuthorizationClassifier.java

+
+    if (type == Type.USER) {
+      LOG.info(
+          "Classified error as USER error based on missing topic. Query ID: {} Exception: {}",


I think you meant to update this error message

vcrfxia

+1 to @agavra 's comment on checking that we're catching the right exception -- LGTM otherwise. Thanks @mjsax !

vcrfxia · 2021-02-22T17:32:35Z

ksqldb-engine/src/test/java/io/confluent/ksql/query/TopicAuthorizationClassifierTest.java

+  }
+
+  @Test
+  public void shouldClassifyNoMissingTopicAsUnknownError() {


Suggested change

public void shouldClassifyNoMissingTopicAsUnknownError() {

public void shouldClassifyNoMissingAclAsUnknownError() {

vcrfxia · 2021-02-22T17:32:57Z

ksqldb-engine/src/main/java/io/confluent/ksql/query/TopicAuthorizationClassifier.java

@@ -0,0 +1,52 @@
+/*
+ * Copyright 2020 Confluent Inc.


nit: year (and also for the new test file)

rodesai · 2021-02-22T19:13:46Z

@mjsax can we also knock out https://confluentinc.atlassian.net/browse/KCI-237 in this patch?

Add integration test for error classification

guozhangwang · 2021-02-23T00:52:27Z

Sorry to keep you adding more into this PR, but besides KCI-237, here are a few more exceptions we can knock out :)

ClusterAuthorizationException
AuthenticationException
TransactionalIdAuthorizationException (I know we do not enable EOS atm, but I think it's okay to pre-add it here)

mjsax · 2021-02-23T21:17:40Z

@guozhangwang It seem an AuthenticationException would never be thrown, but only sub-types. Thus I think we could skip this part? Also, for KafkaStreams, ClusterAuthorizationException should never be thrown (only kslqDB itself needs this), but it would be a different code path from my understanding to handle this case.

mjsax · 2021-02-23T21:20:33Z

ksqldb-engine/src/main/java/io/confluent/ksql/query/TransactionAuthorizationClassifier.java

+  @Override
+  public Type classify(final Throwable e) {
+    final Type type =
+        e instanceof StreamsException


@guozhangwang It's unclear to me, why this exception is wrapped with StreamsException while TopicAuthorizationException and GroupAuthorizationException are not?

For MissingSourceTopicException it's clear because it extends StreamsException and is thrown by KS directly.

I think it should not, and it should be fixed via https://cwiki.apache.org/confluence/display/KAFKA/KIP-691%3A+Enhance+Transactional+Producer+Exception+Handling

cc @abbccdda

But the producer should not wrap anything with StreamsException -- so why does KS wrap the one but not the other producer exception? And how would the KIP change it (it seem the KIP is for the producer but not for KS)?

TopicAuthorizationException: this can be thrown from both consumer and producer, and for consumer it could throw from partitionsFor which is used in KS via global store, in that place we just do not wrap it and let it throw all the way up. For producer it may be thrown from the callback, in which we do wrap --- this is an inconsistency.

GroupAuthorizationException: only thrown from producer in latest version with KIP-447, for which we should wrap in KS; in older version it could be thrown from consumer and again we do not wrap in KS.

TransactionalIdAuthorizationException: thrown from producer, in the callback where we always wrap.

In our other classifiers (e.g. the regex classifier) we walk the cause stack and try and find any exception that matches). That would be an approach to make this check more robust.

vcrfxia

Thanks @mjsax -- LGTM with minor suggestions inline. I'm not sure about the answer to your question about why some exceptions are wrapped and others aren't. ksqlDB directly passes the exception (from the call to kafkaStreams.setUncaughtExceptionHandler(...)) to the QueryErrorClassifier so there's no unwrapping done in ksqlDB itself. Would have to dive into the Streams code to understand why some are wrapped and others aren't.

vcrfxia · 2021-02-23T21:28:51Z

ksqldb-engine/src/main/java/io/confluent/ksql/query/MissingTopicClassifier.java

- * {@code MissingTopicClassifier} classifies errors by querying the broker
- * to check that all topics that the query relies on being accessible exist
- * and are accessible
+ * {@code MissingTopicClassifier} classifies missing source topic exceptions as user error


Unrelated to this PR but I'm curious: what happens if an internal (changelog or repartition) topic is missing, or if a sink topic is missing? Does Streams throw a different type of exception in these cases?

Atm, for changelogs and repartition topic they would be created. We would fail if the expected config does not match. (this will "change" with https://cwiki.apache.org/confluence/display/KAFKA/KIP-698%3A+Add+Explicit+User+Initialization+of+Broker-side+State+to+Kafka+Streams) -- Or course, it's only verified in a rebalance, but checking the source topic is also only done during a rebalance.

For output topics, Kafka Streams won't do anything, and thus the producer would fill up its write buffer and eventually block. To eventually Streams would crash.

But as ksqlDB checks for output topic, it should be ok?

For output topics, Kafka Streams won't do anything, and thus the producer would fill up its write buffer and eventually block. To eventually Streams would crash.

this is true for internal topics too right? It seems like right now for internal topics that are deleted we'd block for max.block.ms and then throw an error classified as SYSTEM. Then, retry and recreate. After KIP-698 we'd block and then throw an error we will classify as USER on every retry.

But as ksqlDB checks for output topic, it should be ok?

Where do we check?

this is true for internal topics too right?

Yes. Not sure how the error would be classified atm or after KIP-698. But it might be out-of-scope for this PR. Would like to focus on authorization errors for now.

Where do we check?

Doesn't kslqDB create output topics explicitly if they don't exist?

ksqldb-engine/src/test/java/io/confluent/ksql/query/GroupAuthorizationClassifierTest.java

vcrfxia · 2021-02-23T21:29:38Z

ksqldb-engine/src/test/java/io/confluent/ksql/integration/SecureIntegrationTest.java

+
+    queryMetadata.start();
+    assertThatEventually(
+        "",


nit: empty error message.

vcrfxia · 2021-02-23T21:33:00Z

ksqldb-engine/src/test/java/io/confluent/ksql/integration/SecureIntegrationTest.java

+  @Test
+  public void shouldClassifyMissingSourceTopicExceptionAsUserError() {
+    // Given:
+    final String serviceId = "my-service-id_";  // Defaults to "default_"


These new tests are hard to read since it's not clear to me which parts of the "givens" are relevant for the test and which are just boilerplate. Can we refactor the common parts/boilerplate into a helper method (e.g., givenTestSetupWithAclsForQuery()) so only the relevant parts are left in each test?

vcrfxia · 2021-02-23T21:34:28Z

ksqldb-engine/src/test/java/io/confluent/ksql/integration/SecureIntegrationTest.java

+
+    final Map<String, Object> ksqlConfig = getKsqlConfig(NORMAL_USER);
+    ksqlConfig.put(KSQL_SERVICE_ID_CONFIG, serviceId);
+    ksqlConfig.put(StreamsConfig.PROCESSING_GUARANTEE_CONFIG, StreamsConfig.EXACTLY_ONCE_BETA);


Why does this cause a transaction ID authorization exception? (Can we add a quick comment so readers know that's the purpose of setting this config in this test?)

…rizationClassifierTest.java Co-authored-by: Victoria Xia <victoria.f.xia281@gmail.com>

guozhangwang · 2021-02-24T00:07:43Z

@guozhangwang It seem an AuthenticationException would never be thrown, but only sub-types. Thus I think we could skip this part? Also, for KafkaStreams, ClusterAuthorizationException should never be thrown (only kslqDB itself needs this), but it would be a different code path from my understanding to handle this case.

Why CLUSTER_AUTHORIZATION_FAILED should never be thrown? From javadocs I see it is possible to return in produce response?

rodesai

LGTM! nice integration tests :)

feat: classify authorization exception as user error

bf8f3c6

mjsax requested a review from a team February 21, 2021 20:57

agavra approved these changes Feb 22, 2021

View reviewed changes

vcrfxia approved these changes Feb 22, 2021

View reviewed changes

mjsax added 4 commits February 22, 2021 14:19

Github comments

3bf546f

Add handling for GroupAuthorizationException

b27225a

Add integration test for error classification

Minor cleanup

4adaf97

Clarify test setup

4c801e0

Add tx authorization handling

2cb2e1a

mjsax commented Feb 23, 2021

View reviewed changes

vcrfxia approved these changes Feb 23, 2021

View reviewed changes

mjsax and others added 2 commits February 23, 2021 15:40

fix checkstyle

08b1f72

Update ksqldb-engine/src/test/java/io/confluent/ksql/query/GroupAutho…

d0fdb9b

…rizationClassifierTest.java Co-authored-by: Victoria Xia <victoria.f.xia281@gmail.com>

Github comments

a065ecf

rodesai reviewed Feb 24, 2021

View reviewed changes

mjsax added 2 commits February 23, 2021 19:25

Fix findbugs

1bfee24

Unify error classification based on super-type

7acea03

mjsax merged commit a74b77c into confluentinc:master Feb 25, 2021

mjsax deleted the kse-24-update-error-classification branch February 25, 2021 23:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: classify authorization exception as user error #7061

feat: classify authorization exception as user error #7061

mjsax commented Feb 21, 2021 •

edited

Loading

ghost commented Feb 21, 2021

agavra left a comment

agavra Feb 22, 2021

vcrfxia left a comment

vcrfxia Feb 22, 2021

vcrfxia Feb 22, 2021

rodesai commented Feb 22, 2021

guozhangwang commented Feb 23, 2021 •

edited

Loading

mjsax commented Feb 23, 2021

mjsax Feb 23, 2021

guozhangwang Feb 23, 2021

mjsax Feb 24, 2021

guozhangwang Feb 24, 2021

rodesai Feb 24, 2021 •

edited

Loading

vcrfxia left a comment

vcrfxia Feb 23, 2021

mjsax Feb 24, 2021

rodesai Feb 24, 2021

mjsax Feb 24, 2021

vcrfxia Feb 23, 2021

vcrfxia Feb 23, 2021

vcrfxia Feb 23, 2021

guozhangwang commented Feb 24, 2021

rodesai left a comment

	public void shouldClassifyNoMissingTopicAsUnknownError() {
	public void shouldClassifyNoMissingAclAsUnknownError() {

feat: classify authorization exception as user error #7061

feat: classify authorization exception as user error #7061

Conversation

mjsax commented Feb 21, 2021 • edited Loading

Description

Testing done

Reviewer checklist

ghost commented Feb 21, 2021

agavra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodesai commented Feb 22, 2021

guozhangwang commented Feb 23, 2021 • edited Loading

mjsax commented Feb 23, 2021

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

rodesai Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

vcrfxia 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

guozhangwang commented Feb 24, 2021

rodesai left a comment

Choose a reason for hiding this comment

mjsax commented Feb 21, 2021 •

edited

Loading

guozhangwang commented Feb 23, 2021 •

edited

Loading

rodesai Feb 24, 2021 •

edited

Loading