Integrate additional OAuth 0.15.0 config options #9970

mstruk · 2024-04-16T07:47:32Z

Type of change

Enhancement / new feature

Description

The following options have been added to authentication section of type oauth of Kafka CR:

serverBearerTokenLocation
userNamePrefix

The following options have been added to authentication section of type oauth of KafkaMirrorMaker, KafkaMirrorMaker2, KafkaConnectCluster, and KafkaBridge CRs:

accessTokenLocation
clientAssertion
clientAssertionLocation
clientAssertionType
saslExtensions

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
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

scholzj

I thought we would include docs on how to configure it for the Servic Accoutn configuration. Or will that be a separate Pr?

CHANGELOG.md

scholzj · 2024-07-04T18:20:28Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeCluster.java

@@ -99,6 +99,7 @@ public class KafkaBridgeCluster extends AbstractModel implements SupportsLogging
    protected static final String ENV_VAR_KAFKA_BRIDGE_OAUTH_ACCESS_TOKEN = "KAFKA_BRIDGE_OAUTH_ACCESS_TOKEN";
    protected static final String ENV_VAR_KAFKA_BRIDGE_OAUTH_REFRESH_TOKEN = "KAFKA_BRIDGE_OAUTH_REFRESH_TOKEN";
    protected static final String ENV_VAR_KAFKA_BRIDGE_OAUTH_PASSWORD_GRANT_PASSWORD = "KAFKA_BRIDGE_OAUTH_PASSWORD_GRANT_PASSWORD";
+    protected static final String ENV_VAR_KAFKA_BRIDGE_OAUTH_CLIENT_ASSERTION = "KAFKA_BRIDGE_OAUTH_CLIENT_ASSERTION";


This seems to be used only in tests. So why do we need it? Should we move it to the test class? Or should it be actually set somewhere? The same applies to some of the other classes as well.

I'm just following a pattern here. It's the kafka_bridge_config_generator.sh shell script that uses these same env vars to configure the bridge. I assume they are in this class as they are considered part of the contract. What does @ppatierno think?

So, I'm a bit curios how you set the environment variable if you don't use the name anywhere in the production code. Is it assembled somewhere on the fly from some prefix and suffix? But I'm not aware of any contract that we add there a variable not referenced from the Java code itself - I'm certainly not following such contract in any of my PRs.

Most of these constants are indeed only used in tests and could be moved into some test class. But I'll defer to @ppatierno to give an opinion.

So, the above env vars related constant were used here in the past as well IIRC but then we moved to this approach to build them https://github.com/strimzi/strimzi-kafka-operator/blob/main/cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeCluster.java#L458
At this point, I guess we can removed them from here and move to test classes.

A separate PR is fine with me. Thanks.

I think it is fine to move the others in a separate PR. But we should not introduce new variables like this in this PR.

@mstruk I saw you moved the env vars within this PR. Wasn't the plan to do that in a different one as stated above?

@ppatierno Given that I had to move one constant based on additional feedback, it was actually trivial to move the others as well and avoid the overhead of doing another PR. The idea of another PR was to get this one merged ASAP. But at this point doing another PR is an overhead.

Ok makes sense thanks

mstruk · 2024-07-05T10:09:00Z

I thought we would include docs on how to configure it for the Servic Accoutn configuration. Or will that be a separate Pr?

I'm making a separate PR for that. But I can also add it to this one if you prefer.

scholzj · 2024-07-05T10:34:49Z

I thought we would include docs on how to configure it for the Servic Accoutn configuration. Or will that be a separate Pr?

I'm making a separate PR for that. But I can also add it to this one if you prefer.

Doing it in a separate PR is fine. It was just not clear to me. Thanks.

scholzj

@mstruk This looks good to me now. Thanks. Could you please rebase it so that I can run the regression tests?

The following options have been added to `authentication` section of type `oauth` of Kafka CR: - `userNamePrefix` - `serverBearerTokenLocation` The following options have been added to `authentication` section of type `oauth` of KafkaMirrorMaker, KafkaMirrorMaker2, KafkaConnectCluster, and KafkaBridge CRs: - `clientAssertion` - `clientAssertionLocation` - `clientAssertionType` - `saslExtensions` Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

…checkAccessTokenType` Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

scholzj · 2024-07-16T15:34:39Z

/azp run regression

azure-pipelines · 2024-07-16T15:34:49Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj · 2024-07-16T20:54:16Z

Thanks @mstruk

mstruk added this to the 0.41.0 milestone Apr 16, 2024

scholzj modified the milestones: 0.41.0, 0.42.0 May 7, 2024

mstruk force-pushed the oauth-0.15.0 branch from c26b51b to e872b34 Compare May 9, 2024 13:40

scholzj modified the milestones: 0.42.0, 0.43.0 Jul 1, 2024

mstruk force-pushed the oauth-0.15.0 branch 2 times, most recently from 8eef877 to 94337b9 Compare July 2, 2024 11:59

mstruk mentioned this pull request Jul 4, 2024

Additional authentication type 'serviceaccount-oauth' that extends 'oauth' #9657

Closed

8 tasks

mstruk marked this pull request as ready for review July 4, 2024 10:54

mstruk requested a review from scholzj July 4, 2024 10:54

scholzj reviewed Jul 4, 2024

View reviewed changes

scholzj requested a review from ppatierno July 4, 2024 18:21

mstruk mentioned this pull request Jul 8, 2024

Additional documentation for OAuth 0.15.0 features and how to use Service Accounts #10317

Merged

8 tasks

mstruk force-pushed the oauth-0.15.0 branch from d2b8646 to 55f7500 Compare July 15, 2024 10:08

mstruk requested a review from scholzj July 15, 2024 15:38

scholzj reviewed Jul 16, 2024

View reviewed changes

ppatierno approved these changes Jul 16, 2024

View reviewed changes

mstruk added 10 commits July 16, 2024 16:05

Cleanup the docs and the tests

bdcab99

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Cleanup the docs and the tests

349d27b

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Fix CHANGELOG.md

4511183

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Fix checkstyle violations

b0c7bda

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Remove unnecessary condition

fb68185

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Undo boolean to Boolean conversion on includeAcceptHeader and `…

0c540a3

…checkAccessTokenType` Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Small fixes (CHANGELOG, code)

7eba87f

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Revert invalid CHANGELOG 'fix'

29cd320

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Move ENV var constants only used in tests to KafkaBridgeClusterTests

b4b125c

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

Rebase and update CRDs

4f94a73

Signed-off-by: Marko Strukelj <marko.strukelj@gmail.com>

mstruk force-pushed the oauth-0.15.0 branch from 3e46f17 to 4f94a73 Compare July 16, 2024 14:36

scholzj approved these changes Jul 16, 2024

View reviewed changes

scholzj merged commit 4aa2d4b into strimzi:main Jul 16, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate additional OAuth 0.15.0 config options #9970

Integrate additional OAuth 0.15.0 config options #9970

mstruk commented Apr 16, 2024 •

edited

Loading

scholzj left a comment

scholzj Jul 4, 2024

mstruk Jul 5, 2024

scholzj Jul 5, 2024

mstruk Jul 8, 2024

ppatierno Jul 15, 2024

ppatierno Jul 15, 2024

scholzj Jul 15, 2024

ppatierno Jul 16, 2024

mstruk Jul 16, 2024

ppatierno Jul 16, 2024

mstruk commented Jul 5, 2024

scholzj commented Jul 5, 2024

scholzj left a comment

scholzj commented Jul 16, 2024

azure-pipelines bot commented Jul 16, 2024

scholzj commented Jul 16, 2024

Integrate additional OAuth 0.15.0 config options #9970

Integrate additional OAuth 0.15.0 config options #9970

Conversation

mstruk commented Apr 16, 2024 • edited Loading

Type of change

Description

Checklist

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstruk commented Jul 5, 2024

scholzj commented Jul 5, 2024

scholzj left a comment

Choose a reason for hiding this comment

scholzj commented Jul 16, 2024

azure-pipelines bot commented Jul 16, 2024

scholzj commented Jul 16, 2024

mstruk commented Apr 16, 2024 •

edited

Loading