Moved bridge configuration setup within the operator #11032

ppatierno · 2025-01-14T14:47:03Z

Type of change

Refactoring

Description

This PR fixes #10959 by moving the configuration of the bridge to be generated by the operator within a ConfigMap (actually using the same one that today is used for the metrics and logging configuratio, but with a different entry in the data section) then mounted on the bridge pod and loaded from there.

NOTE: the temporary change in the .azure/templates/steps/system_test_general.yaml file is for using a custom bridge image which doesn't use its script to generate the configuration anymore. Of course the commit to the file will be reverted before merging the PR. It's there for running STs.

Checklist

Write tests
Make sure all tests pass
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

ppatierno · 2025-01-14T14:47:15Z

This PR comes together with strimzi/strimzi-kafka-bridge#963
I guess the bridge PR should be merged first, and the bridge 0.32.0 released.
After that we can update this PR by setting the new bridge release as well and removing the temporary change in the mentioned azure/templates/steps/system_test_general.yaml to use the right bridge.

see-quick · 2025-01-14T21:19:34Z

/azp run bridge

azure-pipelines · 2025-01-14T21:19:48Z

Azure Pipelines successfully started running 1 pipeline(s).

ppatierno · 2025-01-15T12:56:01Z

/azp run bridge

azure-pipelines · 2025-01-15T12:56:13Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Paolo Patierno <[email protected]>

Set different bridge image for STs Signed-off-by: Paolo Patierno <[email protected]>

Signed-off-by: Paolo Patierno <[email protected]>

ppatierno · 2025-01-15T16:42:30Z

/azp run regression

azure-pipelines · 2025-01-15T16:42:41Z

Azure Pipelines successfully started running 1 pipeline(s).

scholzj

i made just a quick pass ... but left bunch of comments.

scholzj · 2025-01-15T20:43:27Z

systemtest/src/test/java/io/strimzi/systemtest/bridge/HttpBridgeST.java

-            @Label(TestDocsLabels.BRIDGE)
-        }
-    )
-    void testCustomAndUpdatedValues() {


Why are we removing this?

The test was about updating some env vars which are not used anymore because of the PR.

I don't think anything on what the test was doing changed. If anything, I'm pretty sure with this change this test needs to be extended to test the rollout of configuration changes separately.

scholzj · 2025-01-15T20:45:15Z

...src/main/java/io/strimzi/operator/cluster/operator/assembly/KafkaBridgeAssemblyOperator.java

@@ -98,7 +98,7 @@ protected Future<KafkaBridgeStatus> createOrUpdate(Reconciliation reconciliation
            .compose(i -> deploymentOperations.scaleDown(reconciliation, namespace, bridge.getComponentName(), bridge.getReplicas(), operationTimeoutMs))
            .compose(scale -> serviceOperations.reconcile(reconciliation, namespace, KafkaBridgeResources.serviceName(bridge.getCluster()), bridge.generateService()))
            .compose(i -> MetricsAndLoggingUtils.metricsAndLogging(reconciliation, configMapOperations, bridge.logging(), null))
-            .compose(metricsAndLogging -> configMapOperations.reconcile(reconciliation, namespace, KafkaBridgeResources.metricsAndLogConfigMapName(reconciliation.name()), bridge.generateMetricsAndLogConfigMap(metricsAndLogging)))
+            .compose(metricsAndLogging -> configMapOperations.reconcile(reconciliation, namespace, KafkaBridgeResources.metricsAndLogConfigMapName(reconciliation.name()), bridge.generateBridgeConfigMap(metricsAndLogging)))


You will need to add some change tracking I guess?

Wdym? Can you elaborate a little bit more please?

What happens when the configuration changes? How do you reload it?

I see what you mean and you are right. Before we were leveraging the changes on env vars applied to the Deployment in order to have the pod being rolled for us for free to restart the bridge and getting new config. Now there is a ConfigMap changing which will be updated at pod volume level at some point but bridge won't restart of course :-/ I will figure this out.

There isn't really anything to figure out ... do what we use elsewhere. Do a hash of the configuration when updating the config map and pass it as a Pod annotation to the Deployment. You just need to implement it. 😉

"I will figure this out" = "let me see what we do elsewhere, it's not the first time we need doing that" :-)
Yeah I will implement that way.

I have "figured it out" and pushed ;-)

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeConfigurationBuilder.java

scholzj · 2025-01-15T20:47:37Z

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeConfigurationBuilder.java

+    private static final String PLACEHOLDER_CERT_STORE_PASSWORD = "${strimzienv:CERTS_STORE_PASSWORD}";
+    private static final String PLACEHOLDER_SASL_USERNAME = "${strimzienv:KAFKA_BRIDGE_SASL_USERNAME}";
+    private static final String PASSWORD_VOLUME_MOUNT = "/opt/strimzi/bridge-password/";
+    // the SASL password file template includes: <volume_mount>/<secret_name>/<password_file>
+    private static final String PLACEHOLDER_SASL_PASSWORD_FILE_TEMPLATE = "${strimzidir:%s%s:%s}";
+    private static final String PLACEHOLDER_OAUTH_CONFIG = "${strimzienv:KAFKA_BRIDGE_OAUTH_CONFIG}";
+    private static final String PLACEHOLDER_OAUTH_ACCESS_TOKEN = "${strimzienv:KAFKA_BRIDGE_OAUTH_ACCESS_TOKEN}";
+    private static final String PLACEHOLDER_OAUTH_REFRESH_TOKEN = "${strimzienv:KAFKA_BRIDGE_OAUTH_REFRESH_TOKEN}";
+    private static final String PLACEHOLDER_OAUTH_CLIENT_SECRET = "${strimzienv:KAFKA_BRIDGE_OAUTH_CLIENT_SECRET}";
+    private static final String PLACEHOLDER_OAUTH_PASSWORD_GRANT_PASSWORD = "${strimzienv:KAFKA_BRIDGE_OAUTH_PASSWORD_GRANT_PASSWORD}";


Do we really need all of these? For example PLACEHOLDER_CERT_STORE_PASSWORD is used multiple times, so I get it. But some of the others seem to be used only once.

But it would mean having string hardcoded where we use it. I would stick with constant here.

Doesn't that just make the code harder to read? You wanna see what the configuration will look like, but you actually have to jump between the definitions instead of having it inlined. As I said, I think it makes sense to have this for those that are used multiple times. For those used just once it seems like a waste of space.

It's not a big issue jumping on a constant definition as weel as today, most of the IDEs shows the value of the constant straight away.

scholzj · 2025-01-15T20:49:31Z

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeConfigurationBuilder.java

Can we add empty lines between different ifs, after ifs etc.? Some of the methods are really hard to read without the formatting to separate the different code segments.

Hopefully it's now better.

scholzj · 2025-01-15T20:52:08Z

...tor/src/test/java/io/strimzi/operator/cluster/model/KafkaBridgeConfigurationBuilderTest.java

Should we use the IsEquivalent for asserting the configs as we do for the broker configuration?

AFAIU, IsEquivalent matches all the lines. I don't want to check the entire configuration each time for each test but focusing on the parameters I expect for that specific test (i.e. the ssl trustore, ssl keystore, sasl, ...). With IsEquivalent I should always set, for example, bridge id, bootstrap servers as expected lines.

If you look at how it is used in the Broker config, you will see that there it elverages the builder pattern to basically check only the particular segment with minimal overhead (which is basically just node.id=...). Why is this not usable here?

I see what you mean but I also found something weird when looking at it in the KafkaBrokerConfigurationBuilder. But let's start from my doubt ...
As you can see, I am setting up the config providers in the KafkaBridgeConfigurationBuilder ctor so it means that using IsEquivalent I have to put all the 7 lines generated by the configureConfigProviders() method in the expected lines otherwise the test fails (even if I want to test just bridge id and bootstrap servers).
This led me to understand how it was working with broker configuration and I noticed that the config providers are set only when withUserConfiguration method is called but not when, for example, withRackId is called which needs config providers in the end. The same withCruiseControl, it would need config providers. So in the broker config test, when testing the rack id you are not testing that config providers is there (which should be because needed for the configuration to work) ... is it right? At runtime everything works fine because the withUserConfiguration is always called and it sets the config providers (for the rack, for CC, etc etc ...).
This is why I set the config providers in the constructor because they are needed across several withXXXX methods (but of course they could end in the configuration even if not needed).

This led me to understand how it was working with broker configuration and I noticed that the config providers are set only when withUserConfiguration method is called but not when, for example, withRackId is called which needs config providers in the end. The same withCruiseControl, it would need config providers. So in the broker config test, when testing the rack id you are not testing that config providers is there (which should be because needed for the configuration to work) ... is it right? At runtime everything works fine because the withUserConfiguration is always called and it sets the config providers (for the rack, for CC, etc etc ...).
This is why I set the config providers in the constructor because they are needed across several withXXXX methods (but of course they could end in the configuration even if not needed).

It is unit testing -> you split things into smaller units and test them individually. That is the whole idea. The confgi providers are setup in the user configuration because:

Users might configure their own config providers (not sure if this case is there for the Bridge and if it needs to be handled)

We know the user configuration is always called anyway -> you can test that with a single test in KafkaBridgeClusterTest with single configuration while testing the various options is covered on a smaller units in the KafkaBridgeConfigurationBuilderTest

As you can see, I am setting up the config providers in the KafkaBridgeConfigurationBuilder ctor so it means that using IsEquivalent I have to put all the 7 lines generated by the configureConfigProviders() method in the expected lines otherwise the test fails (even if I want to test just bridge id and bootstrap servers).

That is indeed annoying. But you can split it into a separate with* methods instead of doing it in the constructor I guess?

There is one big advantage with IsEqvivalent -> It tests the exact content. The containsString checks that something is present, but not what else might be present. So you might not catch duplicate or invalid config options for example with the way you have the test now. (The has real meaning, it is eassentially the same as why for example the #11022 bug slipped in)

That is indeed annoying. But you can split it into a separate with* methods instead of doing it in the constructor I guess?

In fact this is the only solution I had in mind with a dedicated withConfigProviders method.
But I would argue that the same could be done for the broker because as I mentioned, even if just unit testing, when set rackid or CC, you would expect the needed config providers being there which is actually not tested instead because they are not set (unless you set the user configuration).

No, you do not care if some other method sets the config providers properly when you test withRackId. And yes, the broker configuration can probably have some method withConfigProviders. But I doubt it would help the code because as I said, you need to reconcile it with any user-set config providers. You might need to do that in the Bridge as well.

I refactored the code by adding a dedicated withConfigProviders method and using isEquivalent (which I moved to the TestUtils class to be used across other test classes).

Signed-off-by: Paolo Patierno <[email protected]>

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeConfigurationBuilder.java

configuration builder Refactored bridge configuration builder to use isEquivalent Signed-off-by: Paolo Patierno <[email protected]>

Signed-off-by: Paolo Patierno <[email protected]>

tinaselenge

LGTM, thanks :)

ppatierno added this to the 0.46.0 milestone Jan 14, 2025

ppatierno mentioned this pull request Jan 14, 2025

Removed the script to generate the configuration strimzi/strimzi-kafka-bridge#963

Open

strimzi deleted a comment from azure-pipelines bot Jan 14, 2025

strimzi deleted a comment from see-quick Jan 15, 2025

strimzi deleted a comment from azure-pipelines bot Jan 15, 2025

ppatierno added 7 commits January 15, 2025 17:31

Moved bridge configuration setup within the operator

3462c5e

Signed-off-by: Paolo Patierno <[email protected]>

Reverted back original operator Deployment

481622a

Set different bridge image for STs Signed-off-by: Paolo Patierno <[email protected]>

Fixed missing env vars to setup the truststore

13ce194

Signed-off-by: Paolo Patierno <[email protected]>

Removed ST using env vars not in place anymore

e741e17

Signed-off-by: Paolo Patierno <[email protected]>

Fixed checkstyle errors

c9abe62

Signed-off-by: Paolo Patierno <[email protected]>

CHANGELOG update

024caa3

Signed-off-by: Paolo Patierno <[email protected]>

Fixed updated systemtests bridge MD

d889705

Signed-off-by: Paolo Patierno <[email protected]>

ppatierno force-pushed the bridge-config-via-configmap branch from 76c9c35 to d889705 Compare January 15, 2025 16:32

ppatierno marked this pull request as ready for review January 15, 2025 16:42

ppatierno requested a review from scholzj January 15, 2025 16:44

scholzj reviewed Jan 15, 2025

View reviewed changes

Fixed scholzj comments

f3172de

Signed-off-by: Paolo Patierno <[email protected]>

tinaselenge reviewed Jan 16, 2025

View reviewed changes

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeConfigurationBuilder.java Outdated Show resolved Hide resolved

ppatierno added 3 commits January 16, 2025 16:15

Factored out a dedicated withConfigProviders method for the bridge

07d58de

configuration builder Refactored bridge configuration builder to use isEquivalent Signed-off-by: Paolo Patierno <[email protected]>

Fixed tinaselenge comments

f545cf8

Signed-off-by: Paolo Patierno <[email protected]>

Fixed checkstyle issues

dd5e8f3

Signed-off-by: Paolo Patierno <[email protected]>

scholzj mentioned this pull request Jan 16, 2025

Add proposal for integrating Bridge with Metrics Reporter strimzi/proposals#143

Open

Fixed rolling bridge pod on configuration change

eacdd66

Signed-off-by: Paolo Patierno <[email protected]>

tinaselenge approved these changes Jan 17, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved bridge configuration setup within the operator #11032

Moved bridge configuration setup within the operator #11032

ppatierno commented Jan 14, 2025 •

edited

Loading

ppatierno commented Jan 14, 2025 •

edited

Loading

see-quick commented Jan 14, 2025

azure-pipelines bot commented Jan 14, 2025

ppatierno commented Jan 15, 2025

azure-pipelines bot commented Jan 15, 2025

ppatierno commented Jan 15, 2025

azure-pipelines bot commented Jan 15, 2025

scholzj left a comment

scholzj Jan 15, 2025

ppatierno Jan 16, 2025

scholzj Jan 16, 2025

scholzj Jan 15, 2025

ppatierno Jan 16, 2025 •

edited

Loading

scholzj Jan 16, 2025

ppatierno Jan 16, 2025

scholzj Jan 16, 2025

ppatierno Jan 16, 2025

ppatierno Jan 17, 2025

scholzj Jan 15, 2025

ppatierno Jan 16, 2025

scholzj Jan 16, 2025

ppatierno Jan 16, 2025

scholzj Jan 15, 2025

ppatierno Jan 16, 2025 •

edited

Loading

scholzj Jan 15, 2025

ppatierno Jan 16, 2025

scholzj Jan 16, 2025

ppatierno Jan 16, 2025

scholzj Jan 16, 2025

ppatierno Jan 16, 2025

scholzj Jan 16, 2025

ppatierno Jan 16, 2025

tinaselenge left a comment

Moved bridge configuration setup within the operator #11032

Are you sure you want to change the base?

Moved bridge configuration setup within the operator #11032

Conversation

ppatierno commented Jan 14, 2025 • edited Loading

Type of change

Description

Checklist

ppatierno commented Jan 14, 2025 • edited Loading

see-quick commented Jan 14, 2025

azure-pipelines bot commented Jan 14, 2025

ppatierno commented Jan 15, 2025

azure-pipelines bot commented Jan 15, 2025

ppatierno commented Jan 15, 2025

azure-pipelines bot commented Jan 15, 2025

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

ppatierno Jan 16, 2025 • 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

ppatierno Jan 16, 2025 • 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

tinaselenge left a comment

Choose a reason for hiding this comment

ppatierno commented Jan 14, 2025 •

edited

Loading

ppatierno commented Jan 14, 2025 •

edited

Loading

ppatierno Jan 16, 2025 •

edited

Loading

ppatierno Jan 16, 2025 •

edited

Loading