Align configuration with Kafka Operator #36

mstruk · 2020-03-11T09:18:42Z

This PR introduces changes with regards to configuration:

oauth.tokens.not.jwt is now called oauth.access.token.is.jwt and has a reverse meaning
oauth.validation.skip.type.check is now called oauth.check.access.token.type and has a reverse meaning

Having a reverse meaning for both these configuration parameters means that the default value is true whereas before it was false, and when setting them explicitly you usually want to set them to false rather than true as it was the case before.

The old configuration parameter names still work with the old semantics, but have been marked @Deprecated and will be removed in future versions. A Warning is logged if the old parameter names are used.

…g change) Previous attribute defaulted to `false`, the new one defaults to `true`. Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…n.type` (Breaking change) Previous attribute defaulted to `false`, the new one defaults to `true`. Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

tombentley · 2020-03-11T09:28:32Z

I know this is still 0.x stuff, but I don't think it's necessary to make breaking changes. I think it would be better to deprecate oauth.tokens.not.jwt (and log a warning if it's used), and error if oauth.tokens.not.jwt and oauth.tokens.is.jwt are both set. Ditto the other config. Why make life harder for your users? Let them fix their configs in their own time, and don't assume they're going to read the release notes. You can remove the deprecated configs when you hit 1.0, which is when users are more likely to actually read the release notes.

…log a warning if used. Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk · 2020-03-13T08:54:41Z

@tombentley I made the change as you suggested. Old config options are still in place, working as they used to, but marked as deprecated, and a warning is logged if they are used.

examples/consumer/src/main/java/io/strimzi/examples/consumer/ExampleConsumer.java

examples/producer/src/main/java/io/strimzi/examples/producer/ExampleProducer.java

...-client/src/main/java/io/strimzi/kafka/oauth/client/JaasClientOauthLoginCallbackHandler.java

...ver/src/main/java/io/strimzi/kafka/oauth/server/JaasServerOauthValidatorCallbackHandler.java

…o util class Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

tombentley · 2020-03-16T09:44:24Z

...-client/src/main/java/io/strimzi/kafka/oauth/client/JaasClientOauthLoginCallbackHandler.java

-            throw new RuntimeException("Custom username claim (OAUTH_USERNAME_CLAIM) not available, when tokens are configured as opaque (OAUTH_TOKENS_NOT_JWT)");
+        isJwt = isAccessTokenJwt(config, log, null);
+        if (!isJwt && usernameClaim != null) {
+            throw new RuntimeException("Custom username claim (OAUTH_USERNAME_CLAIM) not available, when tokens are configured as opaque (OAUTH_ACCESS_TOKEN_IS_JWT=false)");


Should we have a more specific exception type for this?

We don't handle this exception anywhere. Any exception thrown here is caught by Kafka Broker and results in broker shutting down.

I don't think there is some Kafka runtime exception that should be used here? If that were the case we should replace with it all occurrences of RuntimeException inside configure() method.

I'm just thinking that if your Kafka broker shuts down in these circumstances it's slightly nicer to see OauthConfigurationException in the logs than RuntimeException.

That's very subjective. I personally don't see that as added value. But since it would require changing all RuntimeException throw statements in configure(), if we really want that, I prefer we make a new PR just for that and keep this one focused on configuration alignment. Maybe create a JIRA for the other one if you think we really should introduce a new exception.

tombentley · 2020-03-16T09:44:46Z

oauth-common/src/main/java/io/strimzi/kafka/oauth/common/DeprecationUtil.java

+        if (legacy != null) {
+            log.warn("OAUTH_TOKENS_NOT_JWT is deprecated. Use OAUTH_ACCESS_TOKEN_IS_JWT (with reverse meaning) instead.");
+            if (config.getValue(Config.OAUTH_ACCESS_TOKEN_IS_JWT) != null) {
+                throw new RuntimeException((errorPrefix != null ? errorPrefix : "") + "Can't use both OAUTH_ACCESS_TOKEN_IS_JWT and OAUTH_TOKENS_NOT_JWT");


More specific exception type, maybe? Wdyt?

Same answer as above.

scholzj

Nice PR. I left some comments, but mostly LGTM. Do we need some CHANGELOG.md or RELEASE_NOTES.md to add the deprecations there so that we remember to mention them in the release?

scholzj · 2020-03-23T22:59:41Z

examples/consumer/src/main/java/io/strimzi/examples/consumer/ExampleConsumer.java

+        if (legacy != null) {
+            System.out.println("[WARN] OAUTH_TOKENS_NOT_JWT is deprecated. Use OAUTH_ACCESS_TOKEN_IS_JWT (with reverse meaning) instead.");
+        }
+        return legacy != null ? !Config.isTrue(legacy) :


Is it really correct that the legacy option is prefered over the new one? Shouldn't it be the other way around? Or does it have some other reason?

This is just the ExampleConsumer CLI client - not something anyone will use for more than learning, how to use it on the client. Validation performed by it is more for the user experience than for validation.
Actual validation happens later in the workflow. If both old and new options are present, then the exception is thrown, because it's treated as a misconfiguration.
It happens in OAuth client library that actually handles this config under the hood and throws an exception.

So, if both options are present then the exception will be thrown, just a little later in the code flow.

scholzj · 2020-03-23T23:00:31Z

examples/producer/src/main/java/io/strimzi/examples/producer/ExampleProducer.java

+        if (legacy != null) {
+            System.out.println("[WARN] OAUTH_TOKENS_NOT_JWT is deprecated. Use OAUTH_ACCESS_TOKEN_IS_JWT (with reverse meaning) instead.");
+        }
+        return legacy != null ? !Config.isTrue(legacy) :


Same as above. Is it really correct that the legacy option is prefered over the new one? Shouldn't it be the other way around? Or does it have some other reason?

Same answer.

scholzj · 2020-03-23T23:01:34Z

oauth-common/src/main/java/io/strimzi/kafka/oauth/common/DeprecationUtil.java

+            }
+        }
+
+        return legacy != null ? !Config.isTrue(legacy) :


Same as above ... Is it really correct that the legacy option is prefered over the new one? Shouldn't it be the other way around? Or does it have some other reason?

This code won't be reached if both are present. See previous lines

scholzj · 2020-03-23T23:02:23Z

...ver/src/main/java/io/strimzi/kafka/oauth/server/JaasServerOauthValidatorCallbackHandler.java

+                throw new RuntimeException("OAuth validator configuration error: can't use both OAUTH_CHECK_ACCESS_TOKEN_TYPE and OAUTH_VALIDATION_SKIP_TYPE_CHECK");
+            }
+        }
+        return legacy != null ? !Config.isTrue(legacy) :


Same as above ... Is it really correct that the legacy option is prefered over the new one? Shouldn't it be the other way around? Or does it have some other reason?

Same answer - this code won't be reached if both are present.

ppatierno

LGTM. I left just a question.

ppatierno · 2020-03-24T07:17:35Z

oauth-common/src/main/java/io/strimzi/kafka/oauth/common/Config.java

@@ -128,7 +131,7 @@ public boolean getValueAsBoolean(String key, boolean fallback) {
        }
    }

-    private boolean isTrue(String result) {
+    public static boolean isTrue(String result) {


of course this was already here but why we are supporting all these possible values and not just "true/false"?

The only reason would be usability / user experience. Some people may in some situation prefer 'y' over 'true', or '0' over 'false'. It will be a string in input in any case, and needs to be converted to boolean somehow. This code here makes it both versatile, and specific - as we can always point to the algorithm with a url.

@ppatierno Do you see some negatives of this choice?

in general, I don't like to give too much freedom to users :-)
Anyway I can live with it.

mstruk

Nice PR. I left some comments, but mostly LGTM. Do we need some CHANGELOG.md or RELEASE_NOTES.md to add the deprecations there so that we remember to mention them in the release?

It would be nice to mention these deprecations yes. Funny we don't have CHANGELOG.md yet or RELEASE_NOTES.md. We had no place to put the mention of the new features until now :)

We also have to be careful in strimzi-kafka-operator to not pull in the next release without an accompanying strimzi-kafka-operator PR that rewires to these new properties, so that users won't see deprecation messages in the log. I have the code ready, just didn't open the PR yet.

scholzj · 2020-03-24T09:43:17Z

It would be nice to mention these deprecations yes. Funny we don't have CHANGELOG.md yet or RELEASE_NOTES.md. We had no place to put the mention of the new features until now :)

Yeah, so I think the point of my comment was should add it here in this PR? Or should we merge it without it. :-)

We also have to be careful in strimzi-kafka-operator to not pull in the next release without an accompanying strimzi-kafka-operator PR that rewires to these new properties, so that users won't see deprecation messages in the log. I have the code ready, just didn't open the PR yet.

I think we can do the release soon if you want - up to you.

mstruk · 2020-03-24T09:47:52Z

Ok, I can add CHANGELOG.md, and put in it the changes for the previous releases as well.

I'm not in a hurry with the release I think - 0.17.0 train has left for this one so I don't see any hurry.

mstruk · 2020-03-27T13:06:56Z

Instead of CHANGELOG.md I added RELEASE_NOTES.md as part of a different PR (#34, #44)

mstruk added 2 commits March 10, 2020 17:48

Change oauth.tokens.not.jwt to oauth.access.token.is.jwt (Breakin…

10130b9

…g change) Previous attribute defaulted to `false`, the new one defaults to `true`. Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Change oauth.validation.skip.type.check to `oauth.check.access.toke…

d76aecd

…n.type` (Breaking change) Previous attribute defaulted to `false`, the new one defaults to `true`. Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk added this to the 0.4.0 milestone Mar 11, 2020

Keep the old config options working, but mark them as deprecated and …

010c7dc

…log a warning if used. Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

tombentley reviewed Mar 13, 2020

View reviewed changes

Handle presence of old and new config property and move shared code t…

f706a24

…o util class Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

tombentley reviewed Mar 16, 2020

View reviewed changes

tombentley approved these changes Mar 16, 2020

View reviewed changes

tombentley requested a review from ppatierno March 16, 2020 12:01

scholzj self-requested a review March 16, 2020 13:47

scholzj reviewed Mar 23, 2020

View reviewed changes

ppatierno approved these changes Mar 24, 2020

View reviewed changes

mstruk commented Mar 24, 2020

View reviewed changes

mstruk mentioned this pull request Mar 27, 2020

Adjust deprecated OAuth authentication parameters strimzi/strimzi-kafka-operator#2754

Merged

8 tasks

scholzj approved these changes Mar 27, 2020

View reviewed changes

scholzj merged commit d350a31 into strimzi:master Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align configuration with Kafka Operator #36

Align configuration with Kafka Operator #36

mstruk commented Mar 11, 2020 •

edited

Loading

tombentley commented Mar 11, 2020

mstruk commented Mar 13, 2020

tombentley Mar 16, 2020

mstruk Mar 16, 2020

tombentley Mar 16, 2020

mstruk Mar 16, 2020

tombentley Mar 16, 2020

mstruk Mar 16, 2020

scholzj left a comment

scholzj Mar 23, 2020

mstruk Mar 24, 2020

scholzj Mar 23, 2020

mstruk Mar 24, 2020

scholzj Mar 23, 2020

mstruk Mar 24, 2020

scholzj Mar 23, 2020

mstruk Mar 24, 2020

ppatierno left a comment

ppatierno Mar 24, 2020

mstruk Mar 24, 2020

ppatierno Mar 24, 2020

mstruk left a comment

scholzj commented Mar 24, 2020

mstruk commented Mar 24, 2020

mstruk commented Mar 27, 2020

Align configuration with Kafka Operator #36

Align configuration with Kafka Operator #36

Conversation

mstruk commented Mar 11, 2020 • edited Loading

tombentley commented Mar 11, 2020

mstruk commented Mar 13, 2020

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

scholzj 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

ppatierno 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

mstruk left a comment

Choose a reason for hiding this comment

scholzj commented Mar 24, 2020

mstruk commented Mar 24, 2020

mstruk commented Mar 27, 2020

mstruk commented Mar 11, 2020 •

edited

Loading