Added a custom SASL config option to the Topic Operator #5473 #9606

john-mcpeek · 2024-01-27T21:31:16Z

Type of change

Enhancement

Description

I added a custom SASL_MECHANISM so that the Topic Operator in standalone can be used to manage topics in MSK with IAM for example.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

Write tests
Make sure all tests pass
Update documentation
Check RBAC rights for Kubernetes / OpenShift roles - NA
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md
Supply screenshots for visual changes, such as Grafana dashboards - NA

scholzj · 2024-01-27T21:35:11Z

Thanks for the PR. @tombentley @fvaleri please have a look at it as you are the SMEs for the Topic Operator.

Signed-off-by: John McPeek <[email protected]>

Signed-off-by: john-mcpeek <[email protected]> Signed-off-by: John McPeek <[email protected]>

Signed-off-by: John McPeek <[email protected]>

…okers still in use (strimzi#9585) Signed-off-by: John McPeek <[email protected]>

Signed-off-by: john-mcpeek <[email protected]>

Signed-off-by: John McPeek <[email protected]>

scholzj · 2024-01-28T17:45:50Z

/azp run regression

azure-pipelines · 2024-01-28T17:46:02Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-01-28T17:46:16Z

/azp run feature-gates-regression

azure-pipelines · 2024-01-28T17:46:25Z

Azure Pipelines successfully started running 1 pipeline(s).

fvaleri

Hi @john-mcpeek, I think this is a useful feature, thanks. Left some comments.

topic-operator/src/main/java/io/strimzi/operator/topic/Session.java

fvaleri · 2024-01-29T08:10:27Z

documentation/modules/deploying/proc-deploy-topic-operator-standalone.adoc

@@ -144,6 +144,28 @@ env:
 <3> The keystore contains the private key for mTLS authentication.
 <4> The password for accessing the keystore.

+. If you need a custom `SASL` configuration. For example, to access a cluster in a cloud provider with a custom login module, the `custom` `SASL_MECHANISM` allows for `SASL_CUSTOM_CONFIG`. +


You need to add "custom" to the SASL mechanism list on line 118, and rework the documentation about mandatory fields (STRIMZI_SASL_USERNAME and STRIMZI_SASL_PASSWORD are not mandatory with custom).

The same custom SASL config is available for the BTO, so we should also update that documentation.

In order to use the new "custom" SASL mechanism, I guess you would need to extend the default Strimzi operator image to add the required AWS libraries, but there is no mention in the documentation. It would be good to provide an example for AWS_MSK_IAM.

I expected that if you need AWS IAM or AZURE, etc. you would create an image with those so we don't have to include every possible jar. I would like an standard mechanism for external jars other than a new image, but that may be a different conversation.

Sorry, I don't know what you mean by BTO?

I would like an standard mechanism for external jars other than a new image, but that may be a different conversation.

As you say, we can discuss that in a separate thread, but for now, we should include a Dockerfile example for this specific use case, so that other users can easily adapt to their needs.

Sorry, I don't know what you mean by BTO?

Bidirectional Topic Operator, it's the old deprecated implementation. In the latest Strimzi release you have the UTO (Unidirectional Topic Operator) enabled by default. https://strimzi.io/blog/2023/11/02/unidirectional-topic-operator

Roger that, I get that in Dockerfile example.

Ah, I see, I'll get on that.

On another note, the details of a working example might be a lot for the docs. What do you all think of a separate doc to explain the AWS example with a link in the Standalone doc?

This is a repo I did to show the example. MSK-IAM-Example

I think a generic Dockerfile example with placeholders is enough. As mentioned before, we should also update the BTO documentation. Alternatively, we can only implement and document this feature for the UTO, as the BTO is going away in two releases.

Actually, BTO is scheduled for removal in 0.41. So unless we change it it will be gone in few weeks.

BTO is gone in the meantime, so less work to do :)

documentation/modules/deploying/proc-deploy-topic-operator-standalone.adoc

fvaleri · 2024-01-29T09:40:37Z

topic-operator/src/main/java/io/strimzi/operator/topic/v2/TopicOperatorConfig.java

+                String key = entry.getKey();
+                String value = entry.getValue();
+
+                if (key == null || key.isBlank() || !key.startsWith("sasl.")) {


We should also detect the use of one of the standard sasl.mechanism that we support (plain, scram-sha-256, scram-sha-512).

fvaleri · 2024-01-29T09:40:57Z

topic-operator/src/main/java/io/strimzi/operator/topic/Session.java

+                String key = entry.getKey();
+                String value = entry.getValue();
+
+                if (key == null || key.isBlank() || !key.startsWith("sasl.")) {


We should also detect the use of one of the standard sasl.mechanism that we support (plain, scram-sha-256, scram-sha-512).

We already do that is setSaslConfigs(). If they need this then I think limiting them could be counter productive.

Why should we provide two ways to do the same thing? What's the use case?

It seems to me like they could need a custom JAAS login module and a standard sasl mechanism. We don't know what they will need.

If you are dead set on it, I'll add the check.

I think @john-mcpeek has a point here: There are use cases where people want to do funky things with SASL-PLAIN and a custom module, so let's allow this.

topic-operator/src/main/java/io/strimzi/operator/topic/v2/TopicOperatorConfig.java

fvaleri · 2024-01-29T12:53:06Z

topic-operator/src/main/java/io/strimzi/operator/topic/Session.java

@@ -406,12 +413,62 @@ private TopicStore createTopicStore(Zk zk, Config config) {
    }

    private void setSaslConfigs(Properties kafkaClientProps) {
-        String saslMechanism;
-        String jaasConfig;
+        String configSaslMechanism = config.get(Config.SASL_MECHANISM);


We should check for empty config, which is different from invalid mechanism.

Empty or invalid is the same result. Are you meaning a different error message like "SASL_MECHANISM is empty"?

Yes, but Tom proposed to drop this, and I agree.

topic-operator/src/main/java/io/strimzi/operator/topic/Session.java

CHANGELOG.md

fvaleri · 2024-01-29T13:18:54Z

topic-operator/src/main/java/io/strimzi/operator/topic/v2/TopicOperatorConfig.java

        return kafkaClientProps;
    }

    private void putSaslConfigs(Map<String, Object> kafkaClientProps) {
+        TopicOperatorConfig config = this;
+        String configSaslMechanism = config.saslMechanism();


We should check for empty config, which is different from invalid mechanism.

Co-authored-by: Federico Valeri <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Signed-off-by: john-mcpeek <[email protected]>

….java Co-authored-by: Federico Valeri <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

…cOperatorConfig.java Co-authored-by: Federico Valeri <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

tombentley

Thanks for the PR, this would be a useful addition.

tombentley · 2024-01-31T21:14:13Z

documentation/modules/deploying/proc-deploy-topic-operator-standalone.adoc

@@ -144,6 +144,28 @@ env:
 <3> The keystore contains the private key for mTLS authentication.
 <4> The password for accessing the keystore.

+. If you need a custom `SASL` configuration. For example, to access a cluster in a cloud provider with a custom login module, the `custom` `SASL_MECHANISM` allows for `SASL_CUSTOM_CONFIG`. +


I expected that if you need AWS IAM or AZURE, etc. you would create an image with those so we don't have to include every possible jar.

You should explain this in the documentation. Right now someone reading it might not guess all the steps you're expecting them to make.

tombentley · 2024-01-31T21:25:43Z

documentation/modules/deploying/proc-deploy-topic-operator-standalone.adoc

@@ -144,6 +144,28 @@ env:
 <3> The keystore contains the private key for mTLS authentication.
 <4> The password for accessing the keystore.

+. If you need a custom `SASL` configuration. For example, to access a cluster in a cloud provider with a custom login module, the `custom` `SASL_MECHANISM` allows for `SASL_CUSTOM_CONFIG`. +


org.apache.kafka.common.errors.InvalidRequestException: Amazon MSK Serverless doesn't support fetching broker or broker-logger configurations

Perhaps one way to handle this would be to allow the skipping of this check with an env var, e.g. ASSUME_AUTO_CREATE_TOPICS=FALSE. If this was not set we'd do the check which we currently do. If it was =FALSE then we just trust that the person configuring the operator is correct and skip the check. And in the unlikely case where it's =TRUE we fail in the same way as if we'd done the check ourselves.

topic-operator/src/main/java/io/strimzi/operator/topic/Session.java

topic-operator/src/main/java/io/strimzi/operator/topic/v2/TopicOperatorConfig.java

tombentley · 2024-01-31T21:43:28Z

documentation/modules/deploying/proc-deploy-topic-operator-standalone.adoc

+env:
+  - name: SASL_MECHANISM # <1>
+    value: custom
+  - name: SASL_CUSTOM_CONFIG # <2>


I wonder if SASL_CUSTOM_CONFIG_JSON would be clearer.

But also, did you consider using Properties format rather than JSON? Users would likely be familiar with this because it's what the Kafka Java client's use, so it might avoid them having to convert something they already have as Properties format into JSON.

I like adding JSON to the name.

JSONs easy to parse and I was worried about how it could be a mess getting people to understand the value as properties, but taking properties shouldn't be a big change if you would prefer that.

I was worried about how it could be a mess getting people to understand the value as properties, but taking properties shouldn't be a big change if you would prefer that.

That's a good point. The case of accidentally using folded style block strings (>), like this, bothers me

- name: SASL_CUSTOM_CONFIG_PROPERTIES value: > sasl.mechanism: AWS_MSK_IAM sasl.jaas.config: software.amazon.msk.auth.iam.IAMLoginModule required; sasl.client.callback.handler.class: software.amazon.msk.auth.iam.IAMClientCallbackHandler

That's legal YAML and legal .properties format, but it's not what the user intended because it's just the single key sasl.mechanism with a malformed value made up of the real value and the remaining keys and values. Kafka itself would catch that when/if there was sufficient validation on the first property defined. I think there's a similar issue with plain flow scalar strings. (I guess this kind of problem is worse than other common YAML problems, like writing foo: true [where the value is expected to be a string not a boolean, so needs to be quoted] precisely because in this case the expected and actual types are the same, so it's less likely to be caught.)

So I agree, on balance, that although the JSON is ugly it's less likely to be parsed wrongly.

Can we though:

Call it SASL_CUSTOM_CONFIG_JSON

Configure the ObjectMapper to support comments. It's non-standard, but I think people might appreciate the possibility to comment on why/how they're setting individual properties.

This is what I'm working on now. Is SASL_CUSTOM_CONFIG_JSON and not having a custom type the solution we are going with?

@john-mcpeek yes, I think that's what we landed on, right @fvaleri ?

topic-operator/src/main/java/io/strimzi/operator/topic/Session.java

….java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

…cOperatorConfig.java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Signed-off-by: John McPeek <[email protected]>

john-mcpeek · 2024-04-12T14:50:42Z

@PaulRMellor I like the re-wording. Thank you.

scholzj · 2024-04-12T17:36:02Z

/azp run kraft-regression

azure-pipelines · 2024-04-12T17:36:15Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-04-12T23:03:28Z

@john-mcpeek Just FYI: you do not have to rebase the PR all the time. It would be needed only if there would be some conflict.

scholzj · 2024-04-12T23:12:29Z

/azp run kraft regression

azure-pipelines · 2024-04-12T23:12:35Z

No pipelines are associated with this pull request.

scholzj · 2024-04-12T23:24:31Z

/azp run kraft-regression

azure-pipelines · 2024-04-12T23:24:42Z

Azure Pipelines successfully started running 1 pipeline(s).

john-mcpeek · 2024-04-13T00:19:40Z

@john-mcpeek Just FYI: you do not have to rebase the PR all the time. It would be needed only if there would be some conflict.

Roger that, thank you.

john-mcpeek · 2024-04-15T13:36:04Z

@scholzj What's next?

fvaleri · 2024-04-15T13:42:31Z

@john-mcpeek, regression tests are all green, so I think we are just waiting for the final reviews from @tombentley and @PaulRMellor.

PaulRMellor

Changes look good. A couple of nits and I also think we should add something to the intro, as suggested

documentation/modules/deploying/proc-deploy-topic-operator-standalone.adoc

…ndalone.adoc Co-authored-by: PaulRMellor <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Signed-off-by: John McPeek <[email protected]>

PaulRMellor

Great!

tombentley

Many thanks @john-mcpeek!

ppatierno · 2024-04-17T06:47:37Z

topic-operator/src/main/java/io/strimzi/operator/topic/BatchingTopicController.java

+    private void skipNonAlterableConfigs(Set<AlterConfigOp> alterConfigOps) {
+        var alterableConfigs = config.alterableTopicConfig();
+        if (alterableConfigs != null && alterConfigOps != null && !alterableConfigs.isEmpty()) {
+            if (alterableConfigs.equalsIgnoreCase("none")) {


despite there is an ignore case, you are mentioning "none" (lower case) here but "NONE" (upper case) in the previous doc. I would stick with the same on both, i.e. "none" lower case.

right, let's keep them aligned

Ok, so, "none" everywhere including the documentation?

I would say yes

What do we do for other similar options? Didn't @fvaleri before suggest to use uppercase? This should be also consistent with the ALL I guess?

I am just for consistency so even the upper case is fine with me.

Guys, I just need an answer. Upper or lower? Please.

@john-mcpeek Well, it makes no sense to have none and ALL. So one way or another there will be another change needed. It was actually @tombentley who suggested upper case in #9606 (comment) and I think his logic makes sense. So if Paolo doesn't care you should go with uppercase everywhere.

Tom originally suggested to have all uppercase as it is more evident comparing to regular configuration, which is lowercase. So let's use UPPERCASE (NONE and ALL).

Ok, I think we are NONE and ALL everywhere now.

topic-operator/src/main/java/io/strimzi/operator/topic/BatchingTopicController.java

ppatierno · 2024-04-17T06:55:11Z

topic-operator/src/main/java/io/strimzi/operator/topic/BatchingTopicController.java

+
+        if (reconcilableTopic != null && reconcilableTopic.kt() != null
+              && hasConfig(reconcilableTopic.kt()) && alterableConfigs != null) {
+            if (alterableConfigs.equalsIgnoreCase("NONE")) {


again despite the ignore case, why are you using "NONE" here instead of "none" as before? I would make consistency everywhere, even across the documentation part.

Signed-off-by: john-mcpeek <[email protected]>

scholzj · 2024-04-17T14:46:20Z

Thanks a lot for the contribution @john-mcpeek.

fvaleri · 2024-04-17T14:55:37Z

Thanks @john-mcpeek, and congrats for your first contribution. Well done.

john-mcpeek · 2024-04-17T15:13:44Z

Thanks @scholzj @fvaleri @tombentley @ppatierno @PaulRMellor, it was an adventure :)

scholzj added this to the 0.40.0 milestone Jan 27, 2024

scholzj requested review from tombentley and fvaleri January 27, 2024 21:33

john-mcpeek and others added 4 commits January 28, 2024 09:00

Added a custom SASL config option to the Topic Operator

a07507f

Signed-off-by: John McPeek <[email protected]>

Update CHANGELOG.md adding custom SASL support

b8bbe5f

Signed-off-by: john-mcpeek <[email protected]> Signed-off-by: John McPeek <[email protected]>

Added back the else to ensure no null properties get set.

8faa578

Signed-off-by: John McPeek <[email protected]>

Allow reconciliation to continue when scale-down is blocked due to br…

3a50659

…okers still in use (strimzi#9585) Signed-off-by: John McPeek <[email protected]>

john-mcpeek force-pushed the custom-topic-operator-sasl-config-5473 branch from 3fbec7a to 3a50659 Compare January 28, 2024 14:10

john-mcpeek added 3 commits January 28, 2024 09:12

Merge branch 'main' into custom-topic-operator-sasl-config-5473

cce0581

Signed-off-by: john-mcpeek <[email protected]>

Cleanup SpotBugs issues.

68c576e

Signed-off-by: John McPeek <[email protected]>

Cleanup formatting issues.

79427ab

Signed-off-by: John McPeek <[email protected]>

fvaleri suggested changes Jan 29, 2024

View reviewed changes

john-mcpeek and others added 5 commits January 29, 2024 08:25

Apply suggestions from code review

5ba77f5

Co-authored-by: Federico Valeri <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update CHANGELOG.md

e9f237d

Signed-off-by: john-mcpeek <[email protected]>

Update topic-operator/src/main/java/io/strimzi/operator/topic/Session…

a2431bc

….java Co-authored-by: Federico Valeri <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update topic-operator/src/main/java/io/strimzi/operator/topic/v2/Topi…

cdbb681

…cOperatorConfig.java Co-authored-by: Federico Valeri <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update topic-operator/src/main/java/io/strimzi/operator/topic/v2/Topi…

b2458fb

…cOperatorConfig.java Co-authored-by: Federico Valeri <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

tombentley reviewed Jan 31, 2024

View reviewed changes

john-mcpeek and others added 7 commits January 31, 2024 19:01

Update topic-operator/src/main/java/io/strimzi/operator/topic/Session…

50b1a03

….java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update topic-operator/src/main/java/io/strimzi/operator/topic/Session…

0cb145c

….java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update topic-operator/src/main/java/io/strimzi/operator/topic/Session…

5c1605c

….java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update topic-operator/src/main/java/io/strimzi/operator/topic/Session…

8d0de96

….java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update topic-operator/src/main/java/io/strimzi/operator/topic/v2/Topi…

b03be6d

…cOperatorConfig.java Co-authored-by: Tom Bentley <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Used the SASL_TYPE_CUSTOM and update error message.

615517a

Signed-off-by: John McPeek <[email protected]>

Removed logs used during development.

c3b4fc0

Signed-off-by: John McPeek <[email protected]>

Merge branch 'strimzi:main' into custom-topic-operator-sasl-config-5473

149cb17

fvaleri requested a review from PaulRMellor April 15, 2024 12:31

PaulRMellor reviewed Apr 15, 2024

View reviewed changes

john-mcpeek and others added 3 commits April 15, 2024 13:03

Update documentation/modules/deploying/proc-deploy-topic-operator-sta…

299f2d9

…ndalone.adoc Co-authored-by: PaulRMellor <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Update documentation/modules/deploying/proc-deploy-topic-operator-sta…

9b6a206

…ndalone.adoc Co-authored-by: PaulRMellor <[email protected]> Signed-off-by: john-mcpeek <[email protected]>

Updated the doc to clarify

9794bc4

Signed-off-by: John McPeek <[email protected]>

PaulRMellor approved these changes Apr 16, 2024

View reviewed changes

tombentley approved these changes Apr 17, 2024

View reviewed changes

ppatierno reviewed Apr 17, 2024

View reviewed changes

john-mcpeek added 5 commits April 17, 2024 05:28

Merge branch 'main' into custom-topic-operator-sasl-config-5473

1f151cc

Signed-off-by: john-mcpeek <[email protected]>

Update BatchingTopicController.java

da51009

Signed-off-by: john-mcpeek <[email protected]>

Update proc-deploy-topic-operator-standalone.adoc

98918ff

Signed-off-by: john-mcpeek <[email protected]>

Update proc-deploy-topic-operator-standalone.adoc

b3fb404

Signed-off-by: john-mcpeek <[email protected]>

Update BatchingTopicController.java

3a6afea

Signed-off-by: john-mcpeek <[email protected]>

ppatierno approved these changes Apr 17, 2024

View reviewed changes

scholzj merged commit 14d5ed1 into strimzi:main Apr 17, 2024
13 checks passed

Added a custom SASL config option to the Topic Operator #5473 #9606

Added a custom SASL config option to the Topic Operator #5473 #9606

Conversation

john-mcpeek commented Jan 27, 2024

Type of change

Description

Checklist

scholzj commented Jan 27, 2024

scholzj commented Jan 28, 2024

azure-pipelines bot commented Jan 28, 2024

scholzj commented Jan 28, 2024

azure-pipelines bot commented Jan 28, 2024

fvaleri 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

fvaleri Feb 8, 2024 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvaleri Feb 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombentley 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

john-mcpeek commented Apr 12, 2024

scholzj commented Apr 12, 2024

azure-pipelines bot commented Apr 12, 2024

scholzj commented Apr 12, 2024

scholzj commented Apr 12, 2024

azure-pipelines bot commented Apr 12, 2024

scholzj commented Apr 12, 2024

azure-pipelines bot commented Apr 12, 2024

john-mcpeek commented Apr 13, 2024

john-mcpeek commented Apr 15, 2024

fvaleri commented Apr 15, 2024 • edited Loading

PaulRMellor left a comment

Choose a reason for hiding this comment

PaulRMellor left a comment

Choose a reason for hiding this comment

tombentley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvaleri Apr 17, 2024 • 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

fvaleri Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj commented Apr 17, 2024

fvaleri commented Apr 17, 2024

john-mcpeek commented Apr 17, 2024

fvaleri Feb 8, 2024 •

edited

Loading

fvaleri Feb 2, 2024 •

edited

Loading

fvaleri commented Apr 15, 2024 •

edited

Loading

fvaleri Apr 17, 2024 •

edited

Loading

fvaleri Apr 17, 2024 •

edited

Loading