Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MirrorMaker2: Check if bootstrap servers are the same for different cluster aliases #10632

Merged
merged 5 commits into from
Oct 21, 2024

Conversation

varada-sunanda-ibm
Copy link
Contributor

Type of change

Select the type of your PR

  • Bugfix
  • Refactoring

Description

After the changes we made in the PR #9923, the current condition prevents users from independently managing the connections between Kafka Connect and Kafka, as well as between the MM2 connectors and Kafka.
Specifically, we’re using two distinct KafkaUsers, each assigned with minimal set of ACLs that are required.

Our proposed solution is to modify the condition to permit scenarios where the connectCluster bootstrapServer and the mirror.targetCluster bootstrapServer are identical.

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

@varada-sunanda-ibm
Copy link
Contributor Author

@scholzj scholzj added this to the 0.44.0 milestone Sep 24, 2024
@scholzj scholzj requested a review from ppatierno September 24, 2024 08:29
@ppatierno
Copy link
Member

I had a pass with no additional comments to the ones already there. I will have another one when comments are addressed.

Copy link
Contributor

@katheris katheris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 334 to 337
private static boolean hasMatchingBootstrapServers(KafkaMirrorMaker2 kafkaMirrorMaker2, String connectClusterAlias, String targetClusterAlias) {
List<String> configs = Optional.ofNullable(kafkaMirrorMaker2)
.map(KafkaMirrorMaker2::getSpec)
.map(KafkaMirrorMaker2Spec::getClusters)
.orElse(Collections.emptyList())
.stream()
.filter(cluster -> connectClusterAlias.equals(cluster.getAlias()) || targetClusterAlias.equals(cluster.getAlias()))
.map(KafkaMirrorMaker2ClusterSpec::getBootstrapServers)
.toList();

return configs.size() == 2 && configs.get(0).equals(configs.get(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks super complex and hard to read. Why don't you pass the list of clusters here, find the one for the first alias, find the one for the second alias and then compare them?

Comment on lines 156 to 165
.editSpec()
.withConnectCluster("source")
.addToClusters(new KafkaMirrorMaker2ClusterSpecBuilder()
.withAlias("third")
.withBootstrapServers("source:9092")
.build())
.editMirror(0)
.withTargetCluster("third")
.endMirror()
.endSpec()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be aligned according to the edit / end methods. Same applies to other two tests below.

Suggested change
.editSpec()
.withConnectCluster("source")
.addToClusters(new KafkaMirrorMaker2ClusterSpecBuilder()
.withAlias("third")
.withBootstrapServers("source:9092")
.build())
.editMirror(0)
.withTargetCluster("third")
.endMirror()
.endSpec()
.editSpec()
.withConnectCluster("source")
.addToClusters(new KafkaMirrorMaker2ClusterSpecBuilder()
.withAlias("third")
.withBootstrapServers("source:9092")
.build())
.editMirror(0)
.withTargetCluster("third")
.endMirror()
.endSpec()

Comment on lines 331 to 337
List<String> configs = clusterList
.stream()
.filter(cluster -> connectClusterAlias.equals(cluster.getAlias()) || targetClusterAlias.equals(cluster.getAlias()))
.map(KafkaMirrorMaker2ClusterSpec::getBootstrapServers)
.toList();

return configs.size() == 2 && configs.get(0).equals(configs.get(1));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you insist on doing it this comúplicated way despite my comments, maybe you can add at least some comment/JavaDoc to what it does and how.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies @scholzj, I missed that earlier comment. Did you mean something like this?

private static boolean hasMatchingBootstrapServers(List<KafkaMirrorMaker2ClusterSpec> clusterList, String connectClusterAlias, String targetClusterAlias) {
    // Find the cluster for the connectClusterAlias
    String connectClusterBootstrap = clusterList.stream()
            .filter(cluster -> connectClusterAlias.equals(cluster.getAlias()))
            .map(KafkaMirrorMaker2ClusterSpec::getBootstrapServers)
            .findFirst()
            .orElse(null);

    // Find the cluster for the targetClusterAlias
    String targetClusterBootstrap = clusterList.stream()
            .filter(cluster -> targetClusterAlias.equals(cluster.getAlias()))
            .map(KafkaMirrorMaker2ClusterSpec::getBootstrapServers)
            .findFirst()
            .orElse(null);

    // Return true if both are found and have matching bootstrap servers
    return connectClusterBootstrap != null && connectClusterBootstrap.equals(targetClusterBootstrap);
}

Let me know if this is what you were suggesting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is what I would have used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I have pushed in the latest changes. Please can you re-review.

@scholzj
Copy link
Member

scholzj commented Oct 21, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

Copy link
Member

@ppatierno ppatierno left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks!

@scholzj scholzj merged commit 41372de into strimzi:main Oct 21, 2024
21 checks passed
@varada-sunanda-ibm varada-sunanda-ibm deleted the mm2-bootstrapserver-fix branch October 21, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants