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

Add Controller listener to the advertised.listeners #111

Merged
merged 9 commits into from
Nov 13, 2024
Merged

Conversation

see-quick
Copy link
Member

@see-quick see-quick commented Nov 12, 2024

This PR adds the CONTROLLER to advertised.listeners.

@see-quick see-quick added the enhancement New feature or request label Nov 12, 2024
@see-quick see-quick added this to the 0.109.0 milestone Nov 12, 2024
@see-quick see-quick requested a review from a team November 12, 2024 14:18
@see-quick see-quick self-assigned this Nov 12, 2024
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.

I think you need to be careful with this. You want to listen on 0.0.0.0. But you do not want to advertise it. That does not seem to be what you are doing here.

@see-quick
Copy link
Member Author

I think you need to be careful with this. You want to listen on 0.0.0.0. But you do not want to advertise it. That does not seem to be what you are doing here.

I see, currently, we have in single-node this configuration

advertised.listeners=PLAINTEXT\://localhost\:33219,BROKER1\://10.90.209.2\:9091
broker.id=1
controller.listener.names=CONTROLLER
controller.quorum.voters=1@broker-1\:9094
inter.broker.listener.name=BROKER1
listener.security.protocol.map=PLAINTEXT\:PLAINTEXT,CONTROLLER\:PLAINTEXT,BROKER1\:PLAINTEXT
listeners=PLAINTEXT\://0.0.0.0\:9092,BROKER1\://0.0.0.0\:9091,CONTROLLER\://broker-1\:9094
...

for the controller listener, I had to change it to DNS [1] i.e., CONTROLLER\://broker-1\:9094. But I assume instead of BROKER1\://10.90.209.2\:9091 I can use BROKER1\://broker-1\:9091. That way for instance producer or consumer would easily connect to the internal docker network by just specifying BROKER1\://broker-1\:9091 as bootstrap servers. External clients have to use PLAINTEXT\://localhost\:33219.

[1] - apache/kafka@9be27e7

@scholzj
Copy link
Member

scholzj commented Nov 12, 2024

No. You need to add the controller to the advertised hosts.

@see-quick see-quick changed the title Use DNS instead of 0.0.0.0 Add Controller listener to the advertised.listeners Nov 12, 2024
Signed-off-by: see-quick <[email protected]>

# Conflicts:
#	src/test/java/io/strimzi/test/container/AbstractIT.java
public static String extractVersionFromImageName(final String imageName) {
final Matcher matcher = KAFKA_VERSION_PATTERN.matcher(imageName);
if (matcher.find()) {
return matcher.group(1); // Returns the Kafka version string like "3.9.0"
Copy link
Member

Choose a reason for hiding this comment

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

This is pretty unreliable. I think we need to have better way to configure KAfka version than through the container image. If you do not want to solve it in this PR it might be fine, but we should have issues to track this.

This is pretty likel to fail when using custom images when in various ofline / disconnected environments, when you want to fix to use some specific image through digest etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was thinking about the same with custom images. I can create an issue when I solve that also for custom images or at least log a warning when using custom images.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should work like this:

  • The builders should have a withVersion field/method
  • If not set, the default version (the latest Kafka version) should be used
  • The image should be by the users always set to correspond to the version

I think that is the way it should work. (As I said, I'm happy to leave it for later and not block this PR.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I think I have come up with a more clear solution. Let me know what you think

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, that doesn't seem to work :/. I will have to think about that.....created issue [1] to track it. And I rollback that solution I had previously.

[1] - #112

Signed-off-by: see-quick <[email protected]>
Signed-off-by: see-quick <[email protected]>
@see-quick see-quick merged commit 5622649 into main Nov 13, 2024
7 checks passed
@see-quick see-quick deleted the use-aliases branch November 14, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants