feat: expose Redpanda's listener in the docker network #1994
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
sago2k8
force-pushed
the
feat/expose-kafka-listener-in-docker-net
branch
2 times, most recently
from
7d4e8d6
to
d37251b
Compare
sago2k8
force-pushed
the
feat/expose-kafka-listener-in-docker-net
branch
5 times, most recently
from
7c11390
to
dc92721
Compare
sago2k8
force-pushed
the
feat/expose-kafka-listener-in-docker-net
branch
2 times, most recently
from
6d23f44
to
19e5e9c
Compare
There was a problem hiding this comment.
Hi @sago2k8, thanks for submitting this feature. It LGTM, although I've added a few comments regarding format (errors and its evaluation) and comments (not blockers).
Regarding documentation, I'd like to see this new functional option added to the docs here: docs/modules/redpanda.md. Please take a look at the codeinclude blocks in order to embed existing code snippets into the docs.
Once this is added, I think we can merge.
Cheers!
sago2k8
force-pushed
the
feat/expose-kafka-listener-in-docker-net
branch
2 times, most recently
from
a3ff346
to
7baad87
Compare
Thanks for the review @mdelapenya, I think I addressed all your comments and added the documentation. could you give it another look please :) |
New function WithListener lets you add a new listener to the redpanda container that can be used within any of the docker networks of the container Signed-off-by: Santiago Jimenez Giraldo <sago2k8@gmail.com>
sago2k8
force-pushed
the
feat/expose-kafka-listener-in-docker-net
branch
from
41d7932
to
5064d3b
Compare
|
I think I addressed all your comments @mdelapenya, thanks for the review !! |
sago2k8
force-pushed
the
feat/expose-kafka-listener-in-docker-net
branch
from
02bfd01
to
bfa5b8c
Compare
|
Just noticed there was a merge conflict in one of my commits, addressed now :) |
Add test for new WithListener function, validate connectivity and couple of asserts in the construction of the listener Signed-off-by: Santiago Jimenez Giraldo <sago2k8@gmail.com>
Add documentation for the additional listener option (WithListener) for the Redpanda module Signed-off-by: Santiago Jimenez Giraldo <sago2k8@gmail.com>
sago2k8
force-pushed
the
feat/expose-kafka-listener-in-docker-net
branch
from
bfa5b8c
to
9b9fd50
Compare
There was a problem hiding this comment.
This LGTM, thanks for your time!
@eddumelendez if you see anything weird, please let's do it in a follow up PR 🙏
| for _, network := range req.Networks { | ||
| req.NetworkAliases[network] = append(req.NetworkAliases[network], listener.Address) | ||
| } |
There was a problem hiding this comment.
one issue with this approach that Java implementation still have is that when adding Toxiproxy upfront Kafka container and using 0.0.0.0:<random-toxiproxy-port> as a listener the registration will fail because will try to add 0.0.0.0 as an alias. I forgot to mention that when suggesting the approach initially.
There was a problem hiding this comment.
@sago2k8 @eddumelendez would you like to collaborate in a follow-up PR for this?
There was a problem hiding this comment.
Hey, thanks for your comments, yes I could work on it, do you happen to have a suggestion of a expected behaviour for that specific case
Add a new
WithListenerfunction to allow adding new listeners to the advertised listeners so connection within the docker networks is possibleWhat does this PR do?
Why is it important?
Currently there is no way to access the Kafka Api from another container in the same docker network, at least not an obvious one, so as Redpanda docs suggests adding an extra advertise listener allows access to the Kafka endpoint from within the docker network so it can be consume from another test-container.
Related issues