Fix kafka internal docker connection #2490

catinapoke · 2024-04-19T11:48:04Z

What does this PR do?

I fixed passing alias name in startup script when it's possible.

Why is it important?

You can't connect to kafka container from other container at the same docker network.
Details: https://www.confluent.io/blog/kafka-listeners-explained/

Related issues

I have stumbled into issue with connecting from app docker container to kafka docker container at the same network. I tried many options including altering KAFKA_ADVERTISED_LISTENERS, but none of them worked. Finally, I found out that you overwrite KAFKA_ADVERTISED_LISTENERS in startup script.

Options I tried:

Alter KAFKA_ADVERTISED_LISTENERS
Change network type from bridge to host (it's impossible)
Add new listeners

How to test this PR

I have tested it in my ProofOfConcept repo https://github.com/catinapoke/testcontainers-poc in kafka_test.go file
You can run it go test -v ./tests/integration/kafka_test.go

netlify · 2024-04-19T11:48:19Z

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	`b51be12`
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/66e9b99aaad7a100089ab570
😎 Deploy Preview	https://deploy-preview-2490--testcontainers-go.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

catinapoke · 2024-04-22T09:41:36Z

@mdelapenya hi, will you make code review?

mdelapenya · 2024-04-22T10:51:32Z

@eddumelendez wdyt about this one? It LGTM from the code point of view, but want to double check with you if you see anything else 🙏

eddumelendez · 2024-04-22T13:21:57Z

Hi, I think the approach should be consistent between Redpanda and Kafka regarding adding additional listeners https://golang.testcontainers.org/modules/redpanda/#additional-listener

catinapoke · 2024-04-22T13:39:46Z

Okay, I will check this

mdelapenya · 2024-05-08T12:52:14Z

I think that, after #1989, we can close this one. @catinapoke could you double check it? 🙏

mdelapenya · 2024-05-08T12:52:39Z

BTW I'm going to add support for registering listeners, like in the redpanda implementation.

catinapoke · 2024-05-09T11:20:18Z

I think that, after #1989, we can close this one. @catinapoke could you double check it? 🙏

I checked and it is doing different things, so not yet

mdelapenya · 2024-05-09T15:20:16Z

The code to register listeners will shadow what the redpanda module is currently doing (see https://github.com/testcontainers/testcontainers-go/blob/main/modules/redpanda/redpanda.go#L295), but instead of using the YAML template to render the listeners, we are going to build the proper environment variables for kafka: KAFKA_ADVERTISED_LISTENERS, KAFKA_LISTENERS, etc

catinapoke · 2024-05-10T18:19:02Z

@mdelapenya I added listeners and you can check it's working with this branch of my test repo, but you will have to clone my kafka module locally

@eddumelendez fyi

Old tests are passed btw and fixed assertAdvertisedListeners cause I use Host instead of Inspect.Hostname

That's how it is used:

KafkaContainer, err = kafka.RunContainer(ctx,
		kafka.WithClusterID("test-cluster"),
		testcontainers.WithImage("confluentinc/confluent-local:7.6.1"),
		network.WithNetwork([]string{"kafka"}, Network),
		kafka.WithListener([]kafka.KafkaListener{
			{
				Name: "INTERNAL",
				Ip:   "kafka",
				Port: "9092",
			},
		}),
	)

There is console output for convenience

=== RUN   TestKafkaTests
2024/05/10 21:12:03 github.com/testcontainers/testcontainers-go - Connected to docker:
  Server Version: 25.0.4
  API Version: 1.44
  Operating System: Debian GNU/Linux 11 (bullseye)
  Total Memory: 7918 MB
  Resolved Docker Host: unix:///var/run/docker.sock
  Resolved Docker Socket Path: /var/run/docker.sock
  Test SessionID: 46d84fc1f454f00ea235cd3704cf2a9cd34e7f0ad278632b6845237407425ab2
  Test ProcessID: 7bf1657e-427e-4328-a767-915f2073b99e
2024/05/10 21:12:03 🐳 Creating container for image testcontainers/ryuk:0.7.0
2024/05/10 21:12:04 ✅ Container created: bc124ab81e28
2024/05/10 21:12:04 🐳 Starting container: bc124ab81e28
2024/05/10 21:12:04 ✅ Container started: bc124ab81e28
2024/05/10 21:12:04 🚧 Waiting for container id bc124ab81e28 image: testcontainers/ryuk:0.7.0. Waiting for: &{Port:8080/tcp timeout:<nil> PollInterval:100ms}
2024/05/10 21:12:04 🔔 Container is ready: bc124ab81e28
2024/05/10 21:12:04 🐳 Creating container for image confluentinc/confluent-local:7.6.1
2024/05/10 21:12:05 ✅ Container created: 18074a289352
2024/05/10 21:12:05 🐳 Starting container: 18074a289352
2024/05/10 21:12:10 ✅ Container started: 18074a289352
2024/05/10 21:12:10 🔔 Container is ready: 18074a289352
2024/05/10 21:12:10 🐳 Creating container for image postgres:16.2-alpine3.19
2024/05/10 21:12:10 ✅ Container created: d1d7187c9c31
2024/05/10 21:12:10 🐳 Starting container: d1d7187c9c31
2024/05/10 21:12:10 ✅ Container started: d1d7187c9c31
2024/05/10 21:12:10 🚧 Waiting for container id d1d7187c9c31 image: postgres:16.2-alpine3.19. Waiting for: &{timeout:<nil> deadline:0xc0002de300 Strategies:[0xc000478090]}
2024/05/10 21:12:11 🔔 Container is ready: d1d7187c9c31
2024/05/10 21:12:11 true
succesfully created topics
time="2024-05-10T21:12:12+03:00" level=error msg="Tar: Can't stat file /home/impossible/vkusvill/testcontainers/tests/docker/kafka_rw_test to tar: lstat /home/impossible/vkusvill/testcontainers/tests/docker/kafka_rw_test//home/impossible/vkusvill/testcontainers/tests/docker/kafka_rw_test/.dockerignore: no such file or directory"
Step 1/8 : FROM golang:bullseye
 ---> 6023337f69a5
Step 2/8 : WORKDIR /app
 ---> Using cache
 ---> b4dc351741d0
Step 3/8 : COPY go.mod .
 ---> Using cache
 ---> babb994d22b5
Step 4/8 : COPY go.sum .
 ---> Using cache
 ---> 6800ebf38938
Step 5/8 : RUN go mod tidy
 ---> Using cache
 ---> b577bdb6096a
Step 6/8 : COPY . .
 ---> Using cache
 ---> 9b726456719e
Step 7/8 : RUN go build -o ./pg_test
 ---> Using cache
 ---> 1b06125cd391
Step 8/8 : CMD /app/pg_test
 ---> Using cache
 ---> 0f76970b95dc
Successfully built 0f76970b95dc
Successfully tagged d3d5b7cc-3010-40fb-8dc9-4d7e65445ba5:a840a8f1-5151-4857-81d9-a36add67b840
2024/05/10 21:12:12 🐳 Creating container for image
2024/05/10 21:12:12 ✅ Container created: 6101fa184022
2024/05/10 21:12:12 🐳 Starting container: 6101fa184022
2024/05/10 21:12:12 ✅ Container started: 6101fa184022
2024/05/10 21:12:12 🚧 Waiting for container id 6101fa184022 image: d3d5b7cc-3010-40fb-8dc9-4d7e65445ba5:a840a8f1-5151-4857-81d9-a36add67b840. Waiting for: &{timeout:<nil> Log:start consuming events IsRegexp:false Occurrence:1 PollInterval:100ms}
2024/05/10 21:12:12 🔔 Container is ready: 6101fa184022
=== RUN   TestKafkaTests/TestKafkaConnectivity
2024/05/10 21:12:12 INFO kafka producer connectivity-producer created
2024/05/10 21:12:12 INFO kafka consumer connectivity-consumer created
--- PASS: TestKafkaTests/TestKafkaConnectivity (1.24s)
2024/05/10 21:12:14 🐳 Terminating container: 18074a289352
2024/05/10 21:12:15 🚫 Container terminated: 18074a289352
2024/05/10 21:12:15 🐳 Terminating container: d1d7187c9c31
2024/05/10 21:12:16 🚫 Container terminated: d1d7187c9c31
2024/05/10 21:12:16 🐳 Terminating container: 6101fa184022
2024/05/10 21:12:19 🚫 Container terminated: 6101fa184022
--- PASS: TestKafkaTests (16.40s)
PASS
ok  	testcontainers/tests/integration	16.418s

modules/kafka/options.go

mdelapenya

Added some comments regarding docs format (my fault, as it's not documented at all)

Thanks!

docs/modules/kafka.md

Co-authored-by: Manuel de la Peña <[email protected]>

catinapoke · 2024-05-16T13:49:15Z

@mdelapenya I have fixed docs and added new test that shows connectivity with option WithListeners

modules/kafka/kafka_test.go

catinapoke · 2024-05-17T06:26:00Z

Btw looks like failed tests are the ones that I didn't touch and couldn't touch as I only renamed folder and test

mdelapenya

@catinapoke I think this PR looks good to me. Let's address my comments and wait for @eddumelendez's approval.

@eddumelendez could you do a final review? 🙏

modules/kafka/kafka.go

catinapoke · 2024-05-28T17:19:09Z

@mdelapenya got several minutes and added unit test
btw do you have rights to approve this PR?

mdelapenya · 2024-05-29T08:11:40Z

Some day @eddumelendez will approve this PR, but probably not today....

He is currently in a conference, so not sure how available is he. Will double check with him today.

do you have rights to approve this PR?

I'm the maintainer of the repo, but as you can imagine, I cannot be an expert in all the technologies that we support, that's why we usually check with other team members, our community, and even vendors, about PRs touching certain modules. And kafka is a tough one 😅 Sorry if it's taking that long, I'm in the middle of a major refactor of the container APIs looking for a more idiomatic candidate for v1.0.0, so it's my fault if the repo is not properly attended. In any case, we'll focus on this one ASAP.

Thanks!

eddumelendez

Sorry for the delay, I've left some comments.

eddumelendez · 2024-07-02T17:31:00Z

modules/kafka/kafka.go

+			"KAFKA_LISTENERS":                                "EXTERNAL://0.0.0.0:9093,INTERNAL://0.0.0.0:9092,CONTROLLER://0.0.0.0:9094",
+			"KAFKA_REST_BOOTSTRAP_SERVERS":                   "EXTERNAL://0.0.0.0:9093,INTERNAL://0.0.0.0:9092,CONTROLLER://0.0.0.0:9094",
+			"KAFKA_LISTENER_SECURITY_PROTOCOL_MAP":           "INTERNAL:PLAINTEXT,EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT",
+			"KAFKA_INTER_BROKER_LISTENER_NAME":               "INTERNAL",


any reason why it should change?

More obvious way to show the purpose. It's also the way many people write listeners in articles

I'd not change this, as it could break existing client code using for reasons these env vars and the values

eddumelendez · 2024-07-02T17:32:23Z

modules/kafka/kafka.go

+
+type KafkaListener struct {
+	Name string
+	Ip   string


what about host?

Can u write an example? Cuz I think you can write it in IP

yes, you can use ip too. But, to be more generic my suggestion is to rename Ip to Host

eddumelendez · 2024-07-02T17:42:31Z

modules/kafka/kafka.go

+
+	envs := map[string]string{
+		"KAFKA_LISTENERS":                      "CONTROLLER://0.0.0.0:9094, EXTERNAL://0.0.0.0:9093",
+		"KAFKA_REST_BOOTSTRAP_SERVERS":         "CONTROLLER://0.0.0.0:9094, EXTERNAL://0.0.0.0:9093",


kafka local provides a rest-proxy service but I don't see any test for it. It is also a good opportunity to add a test.

will take a look

eddumelendez · 2024-07-02T18:04:35Z

modules/kafka/kafka_test.go

+func initKafkaTest(ctx context.Context, network string, brokers string, input string, output string) (testcontainers.Container, error) {
+	req := testcontainers.ContainerRequest{
+		FromDockerfile: testcontainers.FromDockerfile{
+			Context:       "./testdata",
+			Dockerfile:    "Dockerfile",
+			PrintBuildLog: true,
+			KeepImage:     true,
+		},
+		WaitingFor: wait.ForLog("start consuming events"),
+		Env: map[string]string{
+			"KAFKA_BROKERS":   brokers,
+			"KAFKA_TOPIC_IN":  input,
+			"KAFKA_TOPIC_OUT": output,
+		},
+		Networks: []string{network},
+	}
+
+	return testcontainers.GenericContainer(ctx, testcontainers.GenericContainerRequest{
+		ContainerRequest: req,
+		Started:          true,
+	})
+}


This can be reduced by using kcat. See https://github.com/testcontainers/testcontainers-java/blob/main/modules/kafka/src/test/java/org/testcontainers/containers/KafkaContainerTest.java#L188-L216

will take a look

mdelapenya · 2024-08-06T14:55:35Z

@catinapoke could you take a look at this PR? We'd like to start discussions for the new native Kafka image, but from a solid foundation. I'd appreciate if you could address Eddu's comments

Thanks for your time 🙇

catinapoke · 2024-08-09T17:50:03Z

@catinapoke could you take a look at this PR? We'd like to start discussions for the new native Kafka image, but from a solid foundation. I'd appreciate if you could address Eddu's comments

Thanks for your time you

If you mean to add all the stuff Eddu suggested than it will take a while because I don't have much time, also I think it could be approved at this point and finished in next stage.

In my opinion, your comments was enough to release this PR as it already fixed the problem and had several iterations. Honestly, I burned out a bit after month late response that says do one more fix, write one more test, redo this part of code. I expected little changes at most. btw you have access to my fork as I checked "Allow edits and access to secrets by maintainers "

so, yeah, I definitely love testcontainers and was willing to be contributor of it, but you can't polish things forever

fyi @mdelapenya

mdelapenya · 2024-08-12T09:46:05Z

Honestly, I burned out a bit after month late response that says do one more fix, write one more test, redo this part of code. I expected little changes at most. btw you have access to my fork as I checked "Allow edits and access to secrets by maintainers "

@catinapoke I'm sorry to hear that. Please consider that I'm not as expert in Kafka as you are, so please consider this me being cautious before merging the code. I'm really sorry if that caused frustration.

so, yeah, I definitely love testcontainers and was willing to be contributor of it, but you can't polish things forever

Eddu is maintainer in the Java project so he sometimes jumps in when I ping him, so sorry again if we were juggling with this PR too much. Kafka is a very complex project (to me) and there are some other ongoing issues that could be affected by the changes introduced here so I always want to make sure the PR tries to fit as many of them as possible (see https://github.com/testcontainers/testcontainers-go/issues?q=is%3Aissue+is%3Aopen+kafka)

If we add that travelling to conferences and summer vacations have played their role here, then the result is having the PR more time in the back burner.

In any case, I acknowledge it's taking more time than expected (you opened this PR back in April), so I want to offer my apologies for that. On the other hand, I also hope you understand the position of OSS maintainers and the stress it takes to maintain a popular project like this one. In this particular PR we tried to come with meaningful questions and I already told you about my lack of expertise in Kafka and the amount of work we had at that time.

In any case, I personally appreciate any contribution without exception, and I always try to put in the developer's feet whenever I review a PR. So I suffer in this situations too when a PR gets stuck for months, but I'd understand if you want to step back with your contributions.

mdelapenya

Hi @catinapoke sorry for the late response. Both @eddumelendez and I paired to review this, and the overall is that LGTM, although we left a few comments regarding:

not using 0.0.0.0 in the host of the listener
not changing the aliases to avoid eventual breaking changes in client code.
using kcat in the test

I think if we do this, the PR is ready to be merged, as the logic to add listeners seems correct.

Hope it's not too late, so we can make progress in this PR and merge it as soon the comments are addressed.

Cheers!

mdelapenya · 2024-08-27T15:27:04Z

modules/kafka/options.go

+	}
+}
+
+// WithListener adds a custom listener to the Redpanda containers. Listener


Suggested change

// WithListener adds a custom listener to the Redpanda containers. Listener

// WithListener adds a custom listener to the Kafka containers. Listener

mdelapenya · 2024-08-27T15:43:57Z

modules/kafka/kafka.go

+			"KAFKA_LISTENERS":                                "EXTERNAL://0.0.0.0:9093,INTERNAL://0.0.0.0:9092,CONTROLLER://0.0.0.0:9094",
+			"KAFKA_REST_BOOTSTRAP_SERVERS":                   "EXTERNAL://0.0.0.0:9093,INTERNAL://0.0.0.0:9092,CONTROLLER://0.0.0.0:9094",
+			"KAFKA_LISTENER_SECURITY_PROTOCOL_MAP":           "INTERNAL:PLAINTEXT,EXTERNAL:PLAINTEXT,CONTROLLER:PLAINTEXT",
+			"KAFKA_INTER_BROKER_LISTENER_NAME":               "INTERNAL",


I'd not change this, as it could break existing client code using for reasons these env vars and the values

mdelapenya · 2024-08-27T15:50:44Z

modules/kafka/kafka.go

+	// Trim
+	for i := 0; i < len(listeners); i++ {
+		listeners[i].Name = strings.ToUpper(strings.Trim(listeners[i].Name, " "))
+		listeners[i].Host = strings.Trim(listeners[i].Host, " ")


We should force that all listeners have the Host:Port pair. The reason is for the env vars to not use 0.0.0.0 but instead use the host.

mdelapenya · 2024-08-27T15:58:35Z

modules/kafka/kafka_test.go

+		Started:          true,
+	})
+
+	// TODO: use kcat


I'd encourage you to use the kcat image instead of the custom Go app that is built from testdata. It would be much simpler.

mdelapenya · 2024-08-28T10:54:37Z

@catinapoke I did not mention it, but if you're swamped or cannot attend this PR, I can step up and add commits on top of yours, if I may 🙏

catinapoke · 2024-09-01T17:48:39Z

@mdelapenya u can, I kinda started to work on PR again, but still not much time

* main: docs: refine heading badges in README (testcontainers#2770) feat(wait): for file (testcontainers#2731) feat(compose): select services via profiles (testcontainers#2758) chore(deps): bump mkdocs-markdownextradata-plugin from 0.2.5 to 0.2.6 (testcontainers#2761) fix: update template too (testcontainers#2763) chore(deps): bump actions/checkout from 4.1.1 to 4.1.7 (testcontainers#2762) chore(deps): bump mkdocs-include-markdown-plugin from 6.0.4 to 6.2.2 (testcontainers#2760) fix: check if the discovered docker socket responds (testcontainers#2741) Upgrade milvus-io/milvus-sdk-go to avoid checksum mismatch. (testcontainers#2753) Fix trailing slash on Image Prefix (testcontainers#2747) chore: use new testcontainers/ryuk:0.9.0 image (testcontainers#2750)

Fix kafka internal docker connection

fixed kafka internal docker connection

320e54d

catinapoke requested a review from a team as a code owner April 19, 2024 11:48

mdelapenya self-assigned this Apr 22, 2024

mdelapenya added the bug An issue with the library label Apr 22, 2024

added new option for kafka listeners

9b6537d

Merge remote-tracking branch 'original/main'

15f4ca3

mdelapenya reviewed May 12, 2024

View reviewed changes

modules/kafka/options.go Outdated Show resolved Hide resolved

fixed kafka docs with new options

c7552f4

mdelapenya reviewed May 13, 2024

View reviewed changes

catinapoke and others added 4 commits May 14, 2024 10:00

Update docs/modules/kafka.md

8904938

Co-authored-by: Manuel de la Peña <[email protected]>

Apply suggestions from code review for docs

7b38c28

Co-authored-by: Manuel de la Peña <[email protected]>

added new test, fixed docs from code review

98793d7

Merge branch 'main' of https://github.com/catinapoke/testcontainers-go

62a68b5

Merge remote-tracking branch 'Original/main'

a7c4a9f

mdelapenya reviewed May 16, 2024

View reviewed changes

modules/kafka/kafka_test.go Outdated Show resolved Hide resolved

renamed docker folder to testdata and fixed test name

bfa9191

mdelapenya requested a review from eddumelendez May 17, 2024 08:21

mdelapenya reviewed May 21, 2024

View reviewed changes

modules/kafka/kafka.go Outdated Show resolved Hide resolved

modules/kafka/kafka.go Outdated Show resolved Hide resolved

modules/kafka/kafka.go Show resolved Hide resolved

Merge remote-tracking branch 'Original/main'

eb71110

Merge branch 'main' into main

a4d91e2

eddumelendez reviewed Jul 2, 2024

View reviewed changes

catinapoke added 4 commits August 25, 2024 12:31

Merge remote-tracking branch 'upstream/main'

dd8fcec

go mod update

5c147d4

todo for PR

b31e1af

updated copyStarterScript for upstream

9f23a4d

mdelapenya reviewed Aug 27, 2024

View reviewed changes

mdelapenya added 12 commits September 2, 2024 16:56

fix: typo

e63a604

fix: use Run method

f9cb155

chore: use PLAINTEXT and BROKER

8394529

fix: handle error in tests

31950f4

chore: make lint

3aa2cbf

chore: use non deprecated APIs

d61004f

chore: handle errors in testss

df53491

fix: close sarama client

bf02852

fix: validation in test

152ef0c

chore: refactor test

efa0f7d

chore: add test using kcat

d669bfb

mdelapenya mentioned this pull request Sep 9, 2024

Fix kafka internal docker connection catinapoke/testcontainers-go#1

Merged

catinapoke added 2 commits September 17, 2024 18:45

Merge pull request #1 from mdelapenya/catinapoke/main

ba701e1

Fix kafka internal docker connection

fixed basic kafka test but network one is broken

b51be12

	// WithListener adds a custom listener to the Redpanda containers. Listener
	// WithListener adds a custom listener to the Kafka containers. Listener

Fix kafka internal docker connection #2490

Are you sure you want to change the base?

Fix kafka internal docker connection #2490

Conversation

catinapoke commented Apr 19, 2024

What does this PR do?

Why is it important?

Related issues

How to test this PR

netlify bot commented Apr 19, 2024 • edited Loading

✅ Deploy Preview for testcontainers-go ready!

catinapoke commented Apr 22, 2024

mdelapenya commented Apr 22, 2024

eddumelendez commented Apr 22, 2024

catinapoke commented Apr 22, 2024

mdelapenya commented May 8, 2024

mdelapenya commented May 8, 2024

catinapoke commented May 9, 2024

mdelapenya commented May 9, 2024

catinapoke commented May 10, 2024 • edited Loading

mdelapenya left a comment

Choose a reason for hiding this comment

catinapoke commented May 16, 2024

catinapoke commented May 17, 2024 • edited Loading

mdelapenya left a comment • edited Loading

Choose a reason for hiding this comment

catinapoke commented May 28, 2024

mdelapenya commented May 29, 2024

eddumelendez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catinapoke Jul 16, 2024 • 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

mdelapenya commented Aug 6, 2024

catinapoke commented Aug 9, 2024 • edited Loading

mdelapenya commented Aug 12, 2024 • edited Loading

mdelapenya 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

mdelapenya commented Aug 28, 2024

catinapoke commented Sep 1, 2024

netlify bot commented Apr 19, 2024 •

edited

Loading

catinapoke commented May 10, 2024 •

edited

Loading

catinapoke commented May 17, 2024 •

edited

Loading

mdelapenya left a comment •

edited

Loading

catinapoke Jul 16, 2024 •

edited

Loading

catinapoke commented Aug 9, 2024 •

edited

Loading

mdelapenya commented Aug 12, 2024 •

edited

Loading