MirrorMaker2: Add warning for dangerously invalid configuration #9923
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes strimzi#9905 Signed-off-by: Markus Vuorio <markus.vuorio@upsert.fi>
|
This is a trivial change, so I don't think any documentation changes are needed. I am wondering whether the test coverage this way is sufficient, or would it be preferable to add another test case. I have not run this yet on real clusters, just with test suite. |
scholzj
reviewed
Apr 5, 2024
There was a problem hiding this comment.
I left one nit. As for the tests:
- Would it make sense to separate the test for the non-existing alias from the test from the connect cluster not being target cluster?
- Maybe it would be also worth having a test with multiple Mirror connectors where some of them have the right target cluster and some do not to see that it validates properly in that case as well.
Also, it seems like some other tests are failing now:
[ERROR] Errors:
[ERROR] io.strimzi.operator.cluster.operator.assembly.KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsBlacklist(VertxTestContext)
[ERROR] Run 1: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsBlacklist:1115 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 2: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsBlacklist:1115 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 3: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsBlacklist:1115 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 4: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsBlacklist:1115 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 5: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsBlacklist:1115 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 6: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsBlacklist:1115 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[INFO]
[ERROR] io.strimzi.operator.cluster.operator.assembly.KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsExclude(VertxTestContext)
[ERROR] Run 1: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsExclude:1065 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 2: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsExclude:1065 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 3: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsExclude:1065 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 4: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsExclude:1065 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 5: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsExclude:1065 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
[ERROR] Run 6: KafkaMirrorMaker2AssemblyOperatorPodSetTest.testTopicsGroupsExclude:1065 » InvalidResource KafkaMirrorMaker2 resource validation failed: [Target cluster alias my-cluster-tgt is used in a mirror definition, but it is not the same as the connect cluster alias null]
You probably just need to fix the custom resource used by these tests that test something different and are now failing this validation.
| @@ -114,6 +114,11 @@ public static KafkaMirrorMaker2Connectors fromCrd(Reconciliation reconciliation, | |||
| } else if (!existingClusterAliases.contains(mirror.getTargetCluster())) { | |||
| errorMessages.add("Target cluster alias " + mirror.getTargetCluster() + " is used in a mirror definition, but cluster with this alias does not exist in cluster definitions"); | |||
| } | |||
|
|
|||
| String connectCluster = kafkaMirrorMaker2.getSpec().getConnectCluster(); | |||
There was a problem hiding this comment.
If there will be multiple sets of mirrors, you will be extracting this again and again. Would it make sense to get it only once before we start the for loop?
The non-existing-alias will also keep failing because this added test, but maybe it does make sense to add another test just for this check.
This makes sense. |
… comments Fixes strimzi#9905 Signed-off-by: Markus Vuorio <markus.vuorio@upsert.fi>
Signed-off-by: Markus Vuorio <markus.vuorio@upsert.fi>
maakuth
force-pushed
the
mm2-add-target-cluster-error
branch
from
721d7a0
to
7217596
Compare
scholzj
reviewed
Apr 12, 2024
...er-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaMirrorMaker2Connectors.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Markus Vuorio <markus.vuorio@upsert.fi>
|
/azp run regression |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #9905
Type of change
Description
In MirrorMaker2 configuration model, add check to ensure all mirrors have target set to the connect cluster. This is the only sane option anyway, but instead of raising an error message, it did start and behaved badly. See #9905.
Checklist