feat: hide internal/system topics from SHOW TOPICS #4322

spena · 2020-01-15T16:18:00Z

Description

Hide all KSQL, Confluent, Kafka, SR and Connect internal or system topics from the SHOW TOPICS command and prevents writing on streams/tables that are based on an internal/system topic.

Also, it adds the SHOW ALL TOPICS syntax in case users want to list all topics including internal/system topics.

Testing done

Uni tests
Verified manually

ksql> insert into t1(id) values (1);
Cannot insert values into the reserved internal topic: _confluent-ksql-default__command_topic

ksql> show topics extended;

ksql> show topics;

 Kafka Topic                 | Partitions | Partition Replicas 
---------------------------------------------------------------
 default_ksql_processing_log | 1          | 1                  
---------------------------------------------------------------

ksql> show all topics;
sql> show all topics;

 Kafka Topic                                                                                   | Partitions | Partition Replicas 
---------------------------------------------------------------------------------------------------------------------------------
 __confluent.support.metrics                                                                   | 1          | 1                  
 _confluent-command
...
default_ksql_processing_log                                                                   | 1          | 1                  
---------------------------------------------------------------------------------------------------------------------------------

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 #")

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java

big-andy-coates

Thanks @spena

Overall functionality looks good: it's hiding internal topics and not allowing anyone to insert into them, right? i.e. it's treating them as read-only.

As I've said below, I think we should avoid hardcoded lists of internal topics. Instead, we should make this configurable via KsqlConfig.

I see three different types of topics you're hiding / making read-only:

Topics that are specific to KSQL, e.g. command topic, processing log topic, and configs topic (which is missing BTW).
Topics that are internal details of systems KSQL connects to, e.g. Kafka's __consumer_offsets topic, or Connect's connect-configs.
Topics relating to CP, but not related to KSQL OSS.

My thoughts would be:

These should be hard coded within an instance so that it can't corrupt itself, i.e. use the KsqlConfig instance to determine the names of these topics and ensure they are internal / read-only.
We should set up the default config so that these are treated as internal / read-only, but make it configurable. This will be more flexible, e.g. supporting users that aren't running Connect, but have a topic named connect-offsets from some other system.
I don't think these one should be mentioned in any OSS config, unless it's just a total PITA to do it any other way. Ideally, this would be CP specific config for KSQL.

Please add me back in as a reviewer when you want me to take another look.

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConstants.java

ksql-engine/src/main/java/io/confluent/ksql/engine/InsertValuesExecutor.java

ksql-parser/src/test/java/io/confluent/ksql/parser/tree/ListTopicsTest.java

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/ListTopicsExecutor.java

ksql-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4

ksql-engine/src/test/java/io/confluent/ksql/services/KafkaTopicClientImplTest.java

docs-md/developer-guide/ksqldb-reference/show-topics.md

JimGalasyn

LGTM, with a couple of suggestions.

vcrfxia · 2020-01-19T03:17:42Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/DistributingExecutor.java

@@ -89,6 +95,14 @@ public DistributingExecutor(
        .apply(executionContext, securityContext.getServiceContext())
        .inject(statement);

+    if (injected.getStatement() instanceof InsertInto) {
+      throwIfInsertOnInternalTopic(
+          statement.getConfig(),


This ignores property overrides associated with the request, right? I'm wondering about the situation where a user has a valid reason to INSERT INTO an topic marked as internal by default, issues the request and sees an error, and now wants to override the SYSTEM_INTERNAL_TOPICS_CONFIG config to enable their use case. If this code ignores the statement override (which I believe it does), then they can't achieve the override without setting the config at a server level and restarting the KSQL server, which feels unnecessarily heavy-weight.

I don't think we should allow users to override this setting at the console.

If it is an actual KSQL topic, e.g. the command topic, then we really don't want to allow them to produce to it!

If it is a topic the admin of the system has configured, via the new config, to be internal/read-only, then again, we should IMHO allow the CLI user to override this.

Agree with @big-andy-coates, we should not let users override the setting. If the KSQL admin decided to hide and make those topics read-only, then is up to the admin to change the setting.

That's fine, but can we be more explicit about it and add the property to https://github.com/confluentinc/ksql/blob/master/ksql-common/src/main/java/io/confluent/ksql/config/ImmutableProperties.java in that case? The current implementation of this PR is inconsistent in that it does not allow overrides for INSERT INTO but it does for INSERT VALUES.

big-andy-coates

Thanks @spena for adding the config, I think this is much more flexible. However, it's also made it clear to me that we're actually trying to control two things with one config! I've called this out below. There's also some code duplication that I think we can clean up, and a change in the way the ProcessingLogConfig is being created that I think can be avoided.

Aside from that it's looking good! There's a few suggestions and nits below.

This will cover internal changelog and repartition topics as well, right?

I've not approved yet as I'd appreciate another look after you've read through my comments and made any changes. However, if I'm holding you up then feel free to merge after addressing my concerns. We can always pick up anything I feel strongly about later.

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java

big-andy-coates · 2020-01-20T15:37:06Z

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java

+      final String message = "Cannot get a list of system internal topics due to an invalid " +
+          "configuration in '" + KsqlConfig.SYSTEM_INTERNAL_TOPICS_CONFIG + "'";
+
+      LOG.error(message + ": " + e.getMessage());


Generally, avoid "log and throw". It's an anti-pattern, which results in the same error resulting in multiple errors being logged for the same cause. Leave it up to the calling code to determine if it wants to log the error, ignore it or throw it up the stack.

I thought that initially, but while I was running some tests, I found the exception was only displayed on the client side. The server log didn't have any error. Being this a server configuration, I wanted to warn the KSQL server admin about this error too so they can fix it instead of relying on the client to send the error to the server admin.

I didn't change this in the updated PR because of above reasons. Let me know what you think.

Log and throw really is a anti pattern. Better to work out why the exception was not being logged on startup, as that omission could be causing other failures to not be logged, i.e. its a bug that needs fixing.

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/DistributingExecutor.java

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/ListTopicsExecutor.java

big-andy-coates

Thanks @spena - LGTM!

Couple of suggestions below to give us cleaner code, but nothing major except that I'm still concerned about the change in how we construct ProcessingLogConfig.

docs-md/developer-guide/ksqldb-reference/show-topics.md

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

big-andy-coates · 2020-01-22T10:35:03Z

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java

+      final String message = "Cannot get a list of system internal topics due to an invalid " +
+          "configuration in '" + KsqlConfig.SYSTEM_INTERNAL_TOPICS_CONFIG + "'";
+
+      LOG.error(message + ": " + e.getMessage());


Log and throw really is a anti pattern. Better to work out why the exception was not being logged on startup, as that omission could be causing other failures to not be logged, i.e. its a bug that needs fixing.

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java

big-andy-coates · 2020-01-22T10:42:31Z

ksql-engine/src/main/java/io/confluent/ksql/engine/InsertValuesExecutor.java

@@ -192,11 +189,41 @@ public void execute(
      throw new KsqlException("Cannot insert values into windowed stream/table!");
    }

+    final ReservedInternalTopics internalTopics = new ReservedInternalTopics(ksqlConfig);


Don't mind. I don't think this PR is too large yet; adding a few more files where a param is injected and passed through won't, IMHO, make this PR unwieldy. Up to you.

On the flip side, I don't think we should merge this with the change in how you create the ProcessingLoggerConfig, which may necessitate passing more things around anyway.

IMHO, wiring this up is part of this change. But I'll leave it up to you.

big-andy-coates · 2020-01-22T10:43:46Z

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/DistributingExecutor.java

@@ -43,20 +48,24 @@
 * {@code distributedCmdResponseTimeout}.
 */
 public class DistributingExecutor {
+  private final KsqlConfig ksqlConfig;


not needed.

ksql-rest-app/src/main/java/io/confluent/ksql/rest/server/computation/DistributingExecutor.java

big-andy-coates

Still LGTM; but spotted a few minors.

big-andy-coates · 2020-01-26T18:54:48Z

ksql-common/src/main/java/io/confluent/ksql/util/KsqlConfig.java

@@ -532,6 +564,20 @@ private static ConfigDef buildConfigDef(final ConfigGeneration generation) {
            KSQL_AUTH_CACHE_MAX_ENTRIES_DEFAULT,
            Importance.LOW,
            KSQL_AUTH_CACHE_MAX_ENTRIES_DOC
+        ).define(
+            KSQL_HIDDEN_TOPICS_CONFIG,
+            Type.STRING,


This should be Type.LIST`, which will be a list of strings.

Of course, you'll need to update the validator as well. The most future proof would be to have a validator that validates each element, and pass the regex one to that. But a short cut, for now, would just be to have the regex one validate a List.

Done. I was having some issues with the casting initially, so I decided to convert it to a Type.STRING. But I put more logic in the validator to verify each list item is a String too and used a StringBuilder to build the final String now. That allowed me to use a Type.LIST this time.

big-andy-coates · 2020-01-26T18:55:49Z

docs-md/developer-guide/ksqldb-reference/show-topics.md

+`SHOW TOPICS` does not display hidden topics by default, such as:
+* KSQL internal topics, like the KSQL command topic or changelog & repartition topics.
+* Topics that match any pattern in the `ksql.hidden.topics` configuration.


Last sentence doesn't read right. Did you mean to have it end with:

like the KSQL command topic or changelog & repartition topics, or topics that match any pattern in the ksql.hidden.topics configuration.

I see both formats correctly. I used 2 bullets to mention both types of hidden topics, but your sentence is also correct. I'm not inclined to any sentence, though.

I'll make the change if I find other changes in the code to make after the rebase.

big-andy-coates · 2020-01-26T18:58:03Z

ksql-common/src/main/java/io/confluent/ksql/configdef/ConfigValidators.java

+      final String regex = Arrays.stream(((String) val).split(","))
+          .collect(Collectors.joining("|"));
+


The regex should be a list, not a comma separated string. So this logic will need to change.

- added configs to immutable properties - removed 'internal' word from config names - modified show-topics.md - list changelog/partition topics with ALL syntax - add regex validator to ConfigValidators and KsqlConfig

spena added this to the 0.7.0 milestone Jan 15, 2020

spena requested review from big-andy-coates and a team January 15, 2020 16:18

spena assigned big-andy-coates Jan 15, 2020

spena commented Jan 15, 2020

View reviewed changes

ksql-common/src/main/java/io/confluent/ksql/util/ReservedInternalTopics.java Outdated Show resolved Hide resolved

big-andy-coates reviewed Jan 16, 2020

View reviewed changes

spena force-pushed the fix_show_topics_internal branch from dab9bd6 to 284ace5 Compare January 17, 2020 17:55

spena requested a review from JimGalasyn as a code owner January 17, 2020 17:55

JimGalasyn reviewed Jan 17, 2020

View reviewed changes

docs-md/developer-guide/ksqldb-reference/show-topics.md Outdated Show resolved Hide resolved

JimGalasyn reviewed Jan 17, 2020

View reviewed changes

docs-md/developer-guide/ksqldb-reference/show-topics.md Outdated Show resolved Hide resolved

JimGalasyn reviewed Jan 17, 2020

View reviewed changes

docs-md/developer-guide/ksqldb-reference/show-topics.md Outdated Show resolved Hide resolved

JimGalasyn approved these changes Jan 17, 2020

View reviewed changes

vcrfxia reviewed Jan 19, 2020

View reviewed changes

big-andy-coates reviewed Jan 20, 2020

View reviewed changes

spena force-pushed the fix_show_topics_internal branch from 284ace5 to 11f97a1 Compare January 21, 2020 22:58

big-andy-coates approved these changes Jan 22, 2020

View reviewed changes

spena force-pushed the fix_show_topics_internal branch 2 times, most recently from 2dc3c0e to a3a1a18 Compare January 24, 2020 20:34

big-andy-coates approved these changes Jan 26, 2020

View reviewed changes

spena added 10 commits February 3, 2020 10:08

fix: avoid writing data to reserved internal topics

3a746ed

feat: add 'show all topics' to include internal topics

c4c9054

refactor: move KSQL_INTERNAL_TOPIC_PREFIX to new class

44016ac

refactor: use configurable SYSTEM_INTERNAL_TOPICS

9214ed5

refactor: create command/configs topics from new class

ea83fd0

feat: do not allow writing to the processing log topic

4fc068f

fix: address review feedback from Andy and Jim

d36f00d

fix: address 3rd round of feedback

79fcb18

- added configs to immutable properties - removed 'internal' word from config names - modified show-topics.md - list changelog/partition topics with ALL syntax - add regex validator to ConfigValidators and KsqlConfig

fix: add KsqlConfig method to get ProcessingLogConfig props

d4f53c6

fix: rebase + convert hidden config names from string to list

84ebde3

spena force-pushed the fix_show_topics_internal branch from a3a1a18 to 84ebde3 Compare February 3, 2020 17:02

spena merged commit 075fed3 into confluentinc:master Feb 3, 2020

spena deleted the fix_show_topics_internal branch February 3, 2020 18:55

spena mentioned this pull request Aug 16, 2020

SHOW TOPICS should have option to hide internal topics #3552

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hide internal/system topics from SHOW TOPICS #4322

feat: hide internal/system topics from SHOW TOPICS #4322

spena commented Jan 15, 2020

big-andy-coates left a comment

JimGalasyn left a comment

vcrfxia Jan 19, 2020

big-andy-coates Jan 20, 2020

spena Jan 21, 2020

vcrfxia Jan 22, 2020 •

edited

Loading

big-andy-coates left a comment

big-andy-coates Jan 20, 2020

spena Jan 21, 2020

spena Jan 21, 2020

big-andy-coates Jan 22, 2020

big-andy-coates left a comment

big-andy-coates Jan 22, 2020

big-andy-coates Jan 22, 2020

big-andy-coates Jan 22, 2020

big-andy-coates left a comment

big-andy-coates Jan 26, 2020

spena Feb 3, 2020

big-andy-coates Jan 26, 2020

spena Feb 3, 2020

big-andy-coates Jan 26, 2020

		final String regex = Arrays.stream(((String) val).split(","))
		.collect(Collectors.joining("\|"));

feat: hide internal/system topics from SHOW TOPICS #4322

feat: hide internal/system topics from SHOW TOPICS #4322

Conversation

spena commented Jan 15, 2020

Description

Testing done

Reviewer checklist

big-andy-coates left a comment

Choose a reason for hiding this comment

JimGalasyn 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

vcrfxia Jan 22, 2020 • edited Loading

Choose a reason for hiding this comment

big-andy-coates 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

big-andy-coates 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

big-andy-coates 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

vcrfxia Jan 22, 2020 •

edited

Loading