-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
fix(kafka): Fix internal docker connection #2894
Open
mdelapenya
wants to merge
59
commits into
testcontainers:main
Choose a base branch
from
mdelapenya:kafka-listeners
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
59 commits
Select commit
Hold shift + click to select a range
320e54d
fixed kafka internal docker connection
catinapoke 9b6537d
added new option for kafka listeners
catinapoke 15f4ca3
Merge remote-tracking branch 'original/main'
catinapoke c7552f4
fixed kafka docs with new options
catinapoke 8904938
Update docs/modules/kafka.md
catinapoke 7b38c28
Apply suggestions from code review for docs
catinapoke 98793d7
added new test, fixed docs from code review
catinapoke 62a68b5
Merge branch 'main' of https://github.com/catinapoke/testcontainers-go
catinapoke a7c4a9f
Merge remote-tracking branch 'Original/main'
catinapoke bfa9191
renamed docker folder to testdata and fixed test name
catinapoke f1b0d48
Merge remote-tracking branch 'Original/main'
catinapoke 128d31a
added test for input listeners validation and refactor from code review
catinapoke ead068a
added unit test for trimValidateListeners
catinapoke eb71110
Merge remote-tracking branch 'Original/main'
catinapoke a4d91e2
Merge branch 'main' into main
mdelapenya dd8fcec
Merge remote-tracking branch 'upstream/main'
catinapoke 5c147d4
go mod update
catinapoke b31e1af
todo for PR
catinapoke 9f23a4d
updated copyStarterScript for upstream
catinapoke e63a604
fix: typo
mdelapenya f9cb155
fix: use Run method
mdelapenya 8394529
chore: use PLAINTEXT and BROKER
mdelapenya 31950f4
fix: handle error in tests
mdelapenya 3aa2cbf
chore: make lint
mdelapenya d61004f
chore: use non deprecated APIs
mdelapenya df53491
chore: handle errors in testss
mdelapenya bf02852
fix: close sarama client
mdelapenya 152ef0c
fix: validation in test
mdelapenya efa0f7d
chore: refactor test
mdelapenya d669bfb
chore: add test using kcat
mdelapenya 4c44b97
Merge branch 'main' into catinapoke/main
mdelapenya ba701e1
Merge pull request #1 from mdelapenya/catinapoke/main
catinapoke b51be12
fixed basic kafka test but network one is broken
catinapoke c8c47f4
not working test with kcat
catinapoke cc60b86
Merge branch 'main' into catinapoke/main
mdelapenya 84cd919
fix: use PLAINTEXT in test
mdelapenya 8a65a7b
chor: rename container variable
mdelapenya fdbabd2
chore: refactor unit tests for the new test function pattern
mdelapenya 3429d0b
chore: use kcat container function
mdelapenya 01ba4e9
chore: use require and new CleanupContainer functions
mdelapenya 63ee6cf
chore: use test function pattern even more
mdelapenya 8660157
chore: exclude lint error
mdelapenya 3c1e739
chore: remove unused code
mdelapenya 4f152f2
chore: rename to Listener
mdelapenya 76cf56d
chore: trim listeners in the option
mdelapenya ec67726
chore: use empty struct to not consume memory
mdelapenya da0deb8
chore: format errors
mdelapenya f2aaa60
chore: simplify advertised slice
mdelapenya f82cfc9
chore: simplify envs for listeners
mdelapenya 2b00774
chore: make WithListeners self-contained
mdelapenya 50f9bcc
fix: proper advertised slice
mdelapenya 7772f4f
chore: simplify test helpers
mdelapenya 45f6660
chore: use require
mdelapenya aa341f8
chore: extract inline helper to a function
mdelapenya 278ec35
chore: refactor options to support writing to the container request
mdelapenya fc81847
docs: refinement
mdelapenya 4ab2546
docs: document withClusterID function
mdelapenya 9adbd97
chore: remove unused
mdelapenya 67c3480
WIP
mdelapenya File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -54,3 +54,61 @@ func (k *TestKafkaConsumer) ConsumeClaim(session sarama.ConsumerGroupSession, cl | |||||
} | ||||||
} | ||||||
} | ||||||
|
||||||
// Consumer represents a Sarama consumer group consumer | ||||||
type TestConsumer struct { | ||||||
t *testing.T | ||||||
ready chan bool | ||||||
messages []*sarama.ConsumerMessage | ||||||
} | ||||||
|
||||||
func NewTestConsumer(t *testing.T) TestConsumer { | ||||||
t.Helper() | ||||||
|
||||||
return TestConsumer{ | ||||||
t: t, | ||||||
ready: make(chan bool), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
} | ||||||
|
||||||
// Setup is run at the beginning of a new session, before ConsumeClaim | ||||||
func (c *TestConsumer) Setup(sarama.ConsumerGroupSession) error { | ||||||
// Mark the consumer as ready | ||||||
close(c.ready) | ||||||
return nil | ||||||
} | ||||||
|
||||||
// Cleanup is run at the end of a session, once all ConsumeClaim goroutines have exited | ||||||
func (consumer *TestConsumer) Cleanup(sarama.ConsumerGroupSession) error { | ||||||
return nil | ||||||
} | ||||||
|
||||||
// ConsumeClaim must start a consumer loop of ConsumerGroupClaim's Messages(). | ||||||
// Once the Messages() channel is closed, the Handler must finish its processing | ||||||
// loop and exit. | ||||||
func (c *TestConsumer) ConsumeClaim(session sarama.ConsumerGroupSession, claim sarama.ConsumerGroupClaim) error { | ||||||
// NOTE: | ||||||
// Do not move the code below to a goroutine. | ||||||
// The `ConsumeClaim` itself is called within a goroutine, see: | ||||||
// https://github.com/IBM/sarama/blob/main/consumer_group.go#L27-L29 | ||||||
for { | ||||||
select { | ||||||
case message, ok := <-claim.Messages(): | ||||||
if !ok { | ||||||
c.t.Log("message channel was closed") | ||||||
return nil | ||||||
} | ||||||
c.t.Logf("Message claimed: value = %s, timestamp = %v, topic = %s", string(message.Value), message.Timestamp, message.Topic) | ||||||
session.MarkMessage(message, "") | ||||||
|
||||||
// Store the message to be consumed later | ||||||
c.messages = append(c.messages, message) | ||||||
|
||||||
// Should return when `session.Context()` is done. | ||||||
// If not, will raise `ErrRebalanceInProgress` or `read tcp <ip>:<port>: i/o timeout` when kafka rebalance. see: | ||||||
// https://github.com/IBM/sarama/issues/1192 | ||||||
case <-session.Context().Done(): | ||||||
return nil | ||||||
} | ||||||
} | ||||||
} |
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,7 +22,7 @@ const ( | |
// starterScript { | ||
starterScriptContent = `#!/bin/bash | ||
source /etc/confluent/docker/bash-config | ||
export KAFKA_ADVERTISED_LISTENERS=PLAINTEXT://%s:%d,BROKER://%s:9092 | ||
export KAFKA_ADVERTISED_LISTENERS=%s | ||
echo Starting Kafka KRaft mode | ||
sed -i '/KAFKA_ZOOKEEPER_CONNECT/d' /etc/confluent/docker/configure | ||
echo 'kafka-storage format --ignore-formatted -t "$(kafka-storage random-uuid)" -c /etc/kafka/kafka.properties' >> /etc/confluent/docker/configure | ||
|
@@ -38,6 +38,12 @@ type KafkaContainer struct { | |
ClusterID string | ||
} | ||
|
||
type Listener struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: Add doc comment. |
||
Name string | ||
Host string | ||
Port string | ||
} | ||
|
||
// Deprecated: use Run instead | ||
// RunContainer creates an instance of the Kafka container type | ||
func RunContainer(ctx context.Context, opts ...testcontainers.ContainerCustomizer) (*KafkaContainer, error) { | ||
|
@@ -70,37 +76,44 @@ func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustom | |
Entrypoint: []string{"sh"}, | ||
// this CMD will wait for the starter script to be copied into the container and then execute it | ||
Cmd: []string{"-c", "while [ ! -f " + starterScript + " ]; do sleep 0.1; done; bash " + starterScript}, | ||
LifecycleHooks: []testcontainers.ContainerLifecycleHooks{ | ||
{ | ||
PostStarts: []testcontainers.ContainerHook{ | ||
// Use a single hook to copy the starter script and wait for | ||
// the Kafka server to be ready. This prevents the wait running | ||
// if the starter script fails to copy. | ||
func(ctx context.Context, c testcontainers.Container) error { | ||
// 1. copy the starter script into the container | ||
if err := copyStarterScript(ctx, c); err != nil { | ||
return fmt.Errorf("copy starter script: %w", err) | ||
} | ||
|
||
// 2. wait for the Kafka server to be ready | ||
return wait.ForLog(".*Transitioning from RECOVERY to RUNNING.*").AsRegexp().WaitUntilReady(ctx, c) | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
genericContainerReq := testcontainers.GenericContainerRequest{ | ||
ContainerRequest: req, | ||
Started: true, | ||
} | ||
|
||
settings := defaultOptions(&genericContainerReq) | ||
for _, opt := range opts { | ||
if apply, ok := opt.(Option); ok { | ||
if err := apply(&settings); err != nil { | ||
return nil, err | ||
} | ||
} | ||
if err := opt.Customize(&genericContainerReq); err != nil { | ||
return nil, err | ||
} | ||
} | ||
|
||
genericContainerReq.ContainerRequest.LifecycleHooks = []testcontainers.ContainerLifecycleHooks{ | ||
{ | ||
PostStarts: []testcontainers.ContainerHook{ | ||
// Use a single hook to copy the starter script and wait for | ||
// the Kafka server to be ready. This prevents the wait running | ||
// if the starter script fails to copy. | ||
func(ctx context.Context, c testcontainers.Container) error { | ||
// 1. copy the starter script into the container | ||
if err := copyStarterScript(ctx, c, &settings); err != nil { | ||
return fmt.Errorf("copy starter script: %w", err) | ||
} | ||
|
||
// 2. wait for the Kafka server to be ready | ||
return wait.ForLog(".*Transitioning from RECOVERY to RUNNING.*").AsRegexp().WaitUntilReady(ctx, c) | ||
}, | ||
}, | ||
}, | ||
} | ||
|
||
err := validateKRaftVersion(genericContainerReq.Image) | ||
if err != nil { | ||
return nil, err | ||
|
@@ -122,31 +135,34 @@ func Run(ctx context.Context, img string, opts ...testcontainers.ContainerCustom | |
} | ||
|
||
// copyStarterScript copies the starter script into the container. | ||
func copyStarterScript(ctx context.Context, c testcontainers.Container) error { | ||
func copyStarterScript(ctx context.Context, c testcontainers.Container, settings *options) error { | ||
if err := wait.ForListeningPort(publicPort). | ||
SkipInternalCheck(). | ||
WaitUntilReady(ctx, c); err != nil { | ||
return fmt.Errorf("wait for exposed port: %w", err) | ||
} | ||
|
||
host, err := c.Host(ctx) | ||
if err != nil { | ||
return fmt.Errorf("host: %w", err) | ||
if len(settings.Listeners) == 0 { | ||
defaultInternal, err := brokerListener(ctx, c) | ||
if err != nil { | ||
return fmt.Errorf("default internal listener: %w", err) | ||
} | ||
settings.Listeners = append(settings.Listeners, defaultInternal) | ||
} | ||
|
||
inspect, err := c.Inspect(ctx) | ||
defaultExternal, err := plainTextListener(ctx, c) | ||
if err != nil { | ||
return fmt.Errorf("inspect: %w", err) | ||
return fmt.Errorf("default external listener: %w", err) | ||
} | ||
|
||
hostname := inspect.Config.Hostname | ||
settings.Listeners = append(settings.Listeners, defaultExternal) | ||
|
||
port, err := c.MappedPort(ctx, publicPort) | ||
if err != nil { | ||
return fmt.Errorf("mapped port: %w", err) | ||
advertised := make([]string, len(settings.Listeners)) | ||
for i, item := range settings.Listeners { | ||
advertised[i] = item.Name + "://" + item.Host + ":" + item.Port | ||
} | ||
|
||
scriptContent := fmt.Sprintf(starterScriptContent, host, port.Int(), hostname) | ||
scriptContent := fmt.Sprintf(starterScriptContent, strings.Join(advertised, ",")) | ||
|
||
if err := c.CopyToContainer(ctx, []byte(scriptContent), starterScript, 0o755); err != nil { | ||
return fmt.Errorf("copy to container: %w", err) | ||
|
@@ -155,14 +171,6 @@ func copyStarterScript(ctx context.Context, c testcontainers.Container) error { | |
return nil | ||
} | ||
|
||
func WithClusterID(clusterID string) testcontainers.CustomizeRequestOption { | ||
return func(req *testcontainers.GenericContainerRequest) error { | ||
req.Env["CLUSTER_ID"] = clusterID | ||
|
||
return nil | ||
} | ||
} | ||
|
||
// Brokers retrieves the broker connection strings from Kafka with only one entry, | ||
// defined by the exposed public port. | ||
func (kc *KafkaContainer) Brokers(ctx context.Context) ([]string, error) { | ||
|
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: its idiomatic to use a
struct{}
for this pattern