Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add config for enabling topic access validator #3079

Merged

Conversation

@rodesai
Copy link
Contributor

commented Jul 13, 2019

Adds a config called ksql.access.validator.enable for enabling the
topic access validator. The possible values for this config are "on",
"off", and "auto". "on" is used to enable the validator, "off"
is used to disable it, and "auto" can be set to have ksql query kafka
properties to auto-discover whether or not the validator can be used (
this is the behaviour before this patch, and is the current default
value for this setting). This config is useful for enabling the validator
when using ksql against multi-tenant kafka, where ksql will not have
access to the broker's configs.

This patch also switches the topic validator factory test to use mockito
for mocking, instead of easymock.

@rodesai rodesai requested a review from confluentinc/ksql as a code owner Jul 13, 2019

@vcrfxia
Copy link
Contributor

left a comment

LGTM -- thanks @rodesai !

KSQL_ACCESS_VALIDATOR_AUTO
),
ConfigDef.Importance.LOW,
""

This comment has been minimized.

Copy link
@vcrfxia

vcrfxia Jul 15, 2019

Contributor

Should we add a doc for this?

This comment has been minimized.

Copy link
@spena

spena Jul 15, 2019

Member

Agree on the doc +1

This comment has been minimized.

Copy link
@rodesai

rodesai Jul 15, 2019

Author Contributor

I want this to be an internal config for now. I thought that the way you do that was with an empty docstring, but you actually have to call defineInternal. But then you can't specify a validator. I'm going to choose to not define this as an internal config, so I'll add a doc string. This only matters once we start using auto-doc-generation. And even then, there isn't really any harm in exposing the config, other than adding noise.

@vcrfxia vcrfxia requested a review from confluentinc/ksql Jul 15, 2019

@spena
spena approved these changes Jul 15, 2019
Copy link
Member

left a comment

The patch looks good. Just the minor two comments I left.

) {
final String enabled = ksqlConfig.getString(KsqlConfig.KSQL_ENABLE_TOPIC_ACCESS_VALIDATOR);
if (enabled.equals(KsqlConfig.KSQL_ACCESS_VALIDATOR_ON)) {

This comment has been minimized.

Copy link
@spena

spena Jul 15, 2019

Member

Optional:
Is case-sensitive a requirement in all KSQL configurations? Should we allow the use of any case?

This comment has been minimized.

Copy link
@rodesai

rodesai Jul 23, 2019

Author Contributor

The only other similar config (ksql.named.internal.topics) is case-sensitive. I don't feel particularly strongly one way or another.

KSQL_ACCESS_VALIDATOR_AUTO
),
ConfigDef.Importance.LOW,
""

This comment has been minimized.

Copy link
@spena

spena Jul 15, 2019

Member

Agree on the doc +1

feat: add config for enabling topic access validator
Adds a config called ksql.access.validator.enable for enabling the
topic access validator. The possible values for this config are "on",
"off", and "auto". "on" is used to enable the validator, "off"
is used to disable it, and "auto" can be set to have ksql query kafka
properties to auto-discover whether or not the validator can be used (
this is the behaviour before this patch, and is the current default
value for this setting). This config is useful for enabling the validator
when using ksql against multi-tenant kafka, where ksql will not have
access to the broker's configs.

This patch also switches the topic validator factory test to use mockito
for mocking, instead of easymock.

@rodesai rodesai force-pushed the rodesai:add-topic-access-validator-config branch from a71f587 to 31d29c1 Jul 23, 2019

@rodesai rodesai merged commit 440e247 into confluentinc:5.3.x Jul 23, 2019

2 checks passed

Semantic Pull Request ready to be squashed
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.