feat: New ksql.properties.overrides.denylist to deny clients configs overrides #5877

spena · 2020-07-24T17:19:16Z

Description

Added a new configuration ksql.properties.overrides.denylist to specify which server properties cannot be overridden by the KSQL clients/users.

There is currently an internal hard-coded list of properties that cannot be overriden. However, there are KSQL admins that also want other properties to be in the list. To make this dynamic, this new denylist property allow those admins to specify which properties should not be overridden, without making changes in the code.

The only disadvantage on this dynamic property over the hard-coded is the validation happens only until executing a DDL or query statements.

Testing done

Added unit tests
Verify the CLI manually

ksql> set 'ksql.streams.num.stream.threads'='4';
Successfully changed local property 'ksql.streams.num.stream.threads' from '4' to '4'.

ksql> show streams;
Cannot override property 'ksql.streams.num.stream.threads'

ksql> unset 'ksql.streams.num.stream.threads';
Successfully unset local property 'ksql.streams.num.stream.threads' (value was '4').

ksql> show streams;

 Stream Name         | Kafka Topic                 | Format 
------------------------------------------------------------
 KSQL_PROCESSING_LOG | default_ksql_processing_log | JSON   
------------------------------------------------------------

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

ksqldb-common/src/test/java/io/confluent/ksql/properties/DenyListPropertyValidatorTest.java

agavra · 2020-07-28T16:47:28Z

...p/src/main/java/io/confluent/ksql/rest/server/resources/streaming/StreamedQueryResource.java

@@ -214,15 +219,21 @@ private EndpointResponse handleStatement(
              statement.getStatement())
      );

+      final Map<String, Object> requestProperties = request.getRequestProperties();


it seems weird to need to do this validation at each endpoint, what if we add another one in the future? is there a place in the engine we can do this validation in one place that would work for all endpoints?

It's hard to find a good place for this. I initially wanted the validation to happen inside the getRequestProperties, but I need a KsqlConfig inside the request which is created by the Vert.x endpoint code. Will require some refactoring just to pass the KsqlConfig.

I looked doing it in the engine.execute(), but it's risky too if the request properties are obtained before calling the execute. See the below code as an example:

final KsqlEntityList entities = handler.execute( securityContext, statements, new SessionProperties( configProperties, localHost, localUrl, requestConfig.getBoolean(KsqlRequestConfig.KSQL_REQUEST_INTERNAL_REQUEST) ) );

The requestConfig.getBoolean(KsqlRequestConfig.KSQL_REQUEST_INTERNAL_REQUEST) won't be validated if we do the properties validation inside the Engine. If we encapsulate it in the Engine, then developers won't know there is some validation done inside. Here at least, if someone looks at the endpoint to add another endpoint, they could see there is some validation to do in the properties.

The engine is littered with code that passes the KsqlConfig ksqlConfig and Map<String, ?> overrides pair around. I think the overrides is now mostly being correctly applied before the config is used... but... it's pretty ugly and unclear if the config you've got has overrides or not.

I've thought for a while that it would be much cleaner to have:

KsqlConfig which is always the server config.

SessionConfig which wraps a KsqlConfig and a map of overrides.

Headless mode just has a single session, where as each user request can build its own SessionConfig and pass this around, rather than the two params.

Ideally, the engine would take SessionConfig, not KsqlConfig. SessionConfig can have a very similar / same interface as KsqlConfig.

The SessionConfig constructor, which takes the server's KsqlConfig and the map of overrides can then validate the overrides, i.e. only one place for override validation.

The SessionConfig would allow access to the serverConfig, then overrides and then 'mergedConfig`. Different parts of the code need each of these, if memory serves me right.

Of course, this is a much bigger change. Simple, but wide ranging. However, I wonder if this PR could start us down this road (Assuming we agree it's a good idea). For example, this PR could add the SessionConfig class, and pass this around in the rest-server model, before converting to a KsqlConfig that's passed to the engine. If you want to also switch the engine to use SessionConfig, then awesome - but maybe in follow up PR.

I addressed the rest of the feedback except this. I found there is an existing SessionProperties that is passed to the KsqlEngine to execute the request and we could use it. I will create a Builder for it that accepts the DenyListPropertyValidator on its constructor, and then build the SessionProperties on every request. The build will fail if the request contains overrides that are prohibited.

I will added in a another commit.

@big-andy-coates I'd like to do this change in a follow-up PR to close this deny list task. The SessionProperties is only supported in the KsqlResource, and I need changes in the StreamedQueryResource and WSQueryEndpoint and other internal methods, so better doing it as a different patch.

If you're good with this current PR, could you approve it?

big-andy-coates

I like this idea @spena. I think it'll be very useful.

Couple of suggestions below.

ksqldb-common/src/main/java/io/confluent/ksql/properties/DenyListPropertyValidator.java

big-andy-coates · 2020-08-03T17:54:55Z

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

@@ -316,6 +316,12 @@
      .put(KSQL_STREAMS_PREFIX + StreamsConfig.MAX_TASK_IDLE_MS_CONFIG, 500L)
      .build();

+  public static final String KSQL_PROPERTIES_OVERRIDES_DENYLIST =
+      "ksql.properties.overrides.denylist";
+  public static final String KSQL_PROPERTIES_OVERRIDES_DENYLIST_DEFAULT = "";


nit: this can be inlined. It isn't referenced from else where.

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

ksqldb-common/src/test/java/io/confluent/ksql/properties/DenyListPropertyValidatorTest.java

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

big-andy-coates · 2020-08-03T18:09:28Z

...p/src/main/java/io/confluent/ksql/rest/server/resources/streaming/StreamedQueryResource.java

@@ -214,15 +219,21 @@ private EndpointResponse handleStatement(
              statement.getStatement())
      );

+      final Map<String, Object> requestProperties = request.getRequestProperties();


The engine is littered with code that passes the KsqlConfig ksqlConfig and Map<String, ?> overrides pair around. I think the overrides is now mostly being correctly applied before the config is used... but... it's pretty ugly and unclear if the config you've got has overrides or not.

I've thought for a while that it would be much cleaner to have:

KsqlConfig which is always the server config.

SessionConfig which wraps a KsqlConfig and a map of overrides.

Headless mode just has a single session, where as each user request can build its own SessionConfig and pass this around, rather than the two params.

Ideally, the engine would take SessionConfig, not KsqlConfig. SessionConfig can have a very similar / same interface as KsqlConfig.

The SessionConfig constructor, which takes the server's KsqlConfig and the map of overrides can then validate the overrides, i.e. only one place for override validation.

The SessionConfig would allow access to the serverConfig, then overrides and then 'mergedConfig`. Different parts of the code need each of these, if memory serves me right.

Of course, this is a much bigger change. Simple, but wide ranging. However, I wonder if this PR could start us down this road (Assuming we agree it's a good idea). For example, this PR could add the SessionConfig class, and pass this around in the rest-server model, before converting to a KsqlConfig that's passed to the engine. If you want to also switch the engine to use SessionConfig, then awesome - but maybe in follow up PR.

ksqldb-rest-app/src/test/java/io/confluent/ksql/rest/server/resources/KsqlResourceTest.java

big-andy-coates · 2020-08-03T18:16:17Z

...est-app/src/main/java/io/confluent/ksql/rest/server/resources/streaming/WSQueryEndpoint.java

@@ -228,7 +233,7 @@ private KsqlRequest parseRequest(final MultiMap requestParams) {
        throw new IllegalArgumentException("\"ksql\" field of \"request\" must be populated");
      }
      // To validate props:
-      request.getConfigOverrides();
+      denyListPropertyValidator.validateAll(request.getConfigOverrides());


missing a unit test.

big-andy-coates · 2020-08-03T18:16:25Z

...est-app/src/main/java/io/confluent/ksql/rest/server/resources/streaming/WSQueryEndpoint.java

@@ -245,6 +250,7 @@ private KsqlRequest parseRequest(final MultiMap requestParams) {

  private void handleQuery(final RequestContext info, final Query query) {
    final Map<String, Object> clientLocalProperties = info.request.getConfigOverrides();
+    denyListPropertyValidator.validateAll(clientLocalProperties);


missing a unit test.

ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/resources/KsqlResource.java

…rrides

- change error message for a more descriptive error

- inject DenyListPropertyValidator to Resource classes - add unit tests for WSQueryEndpoint - validate only streamProperties (no requestProperties) - minor fixes

big-andy-coates

LGTM. Looks forward to the follow up PR!

spena requested review from big-andy-coates and a team July 24, 2020 17:19

spena mentioned this pull request Jul 24, 2020

chore: make ksql.streams.num.stream.threads immutable #5512

Closed

2 tasks

spena requested a review from rodesai July 27, 2020 16:10

agavra reviewed Jul 28, 2020

View reviewed changes

spena force-pushed the blacklist_client_configs branch from 2a6e624 to d4e555d Compare July 31, 2020 19:04

big-andy-coates reviewed Aug 3, 2020

View reviewed changes

spena force-pushed the blacklist_client_configs branch from d4e555d to 27d2643 Compare August 4, 2020 22:07

spena added 3 commits August 5, 2020 09:34

feat: Add ksql.client.properties.denylist to deny clients configs ove…

c432e43

…rrides

fix: Address 1st round of reviews

71bbe6f

- change error message for a more descriptive error

fix: address Andy's comments

978c79a

- inject DenyListPropertyValidator to Resource classes - add unit tests for WSQueryEndpoint - validate only streamProperties (no requestProperties) - minor fixes

spena force-pushed the blacklist_client_configs branch from 27d2643 to 978c79a Compare August 5, 2020 14:34

big-andy-coates approved these changes Aug 6, 2020

View reviewed changes

spena merged commit 7d1ad25 into confluentinc:master Aug 6, 2020

spena deleted the blacklist_client_configs branch August 6, 2020 17:40

JimGalasyn mentioned this pull request Jul 21, 2021

docs: doc the ksql.properties.overrides.denylist config (DOCS-6882) #7853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New ksql.properties.overrides.denylist to deny clients configs overrides #5877

feat: New ksql.properties.overrides.denylist to deny clients configs overrides #5877

spena commented Jul 24, 2020

agavra Jul 28, 2020

spena Jul 31, 2020

big-andy-coates Aug 3, 2020

spena Aug 4, 2020

spena Aug 5, 2020

big-andy-coates left a comment

big-andy-coates Aug 3, 2020

spena Aug 4, 2020

big-andy-coates Aug 3, 2020

big-andy-coates Aug 3, 2020

spena Aug 4, 2020

big-andy-coates Aug 3, 2020

spena Aug 4, 2020

big-andy-coates left a comment

feat: New ksql.properties.overrides.denylist to deny clients configs overrides #5877

feat: New ksql.properties.overrides.denylist to deny clients configs overrides #5877

Conversation

spena commented Jul 24, 2020

Description

Testing done

Reviewer checklist

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

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