-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
feat(ryuk): make listen address of exposed port configurable #2809
feat(ryuk): make listen address of exposed port configurable #2809
Conversation
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
c59809c
to
d7e0d34
Compare
d7e0d34
to
3cb18ac
Compare
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.
Nice addition, thanks for the PR.
I've made some initial suggestions but we should wait for #2728 to be merged before completing as there's going to be some conflicts and it will be easier to fix here.
We should also look to test that the reaper works as expected using a custom port so a test for that should be added, best to wait for the above PR to land first as that changes quite a bit in that area.
3cb18ac
to
c5bec9c
Compare
Thanks for your comments! |
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.
Thanks for the quick changes, second round...
internal/config/config.go
Outdated
ryukAddress := os.Getenv("TESTCONTAINERS_RYUK_ADDRESS") | ||
if ryukAddress != "" { |
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.
nit: we could fold this to be a little more idiomatic
ryukAddress := os.Getenv("TESTCONTAINERS_RYUK_ADDRESS") | |
if ryukAddress != "" { | |
if ryukAddress := os.Getenv("TESTCONTAINERS_RYUK_ADDRESS"); ryukAddress != "" { |
internal/config/config_test.go
Outdated
@@ -140,6 +142,15 @@ func TestReadTCConfig(t *testing.T) { | |||
assert.Equal(t, expected, config) | |||
}) | |||
|
|||
t.Run("Invalid IP address", func(t *testing.T) { |
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: to my previous set of comments, were working towards simple names with no spaces:
t.Run("Invalid IP address", func(t *testing.T) { | |
t.Run("invalid-address", func(t *testing.T) { |
internal/config/config_test.go
Outdated
t.Cleanup(func() { | ||
os.Unsetenv("TESTCONTAINERS_RYUK_ADDRESS") | ||
}) |
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.
bug: this isn't needed t.Setenv already adds a clean up function.
t.Cleanup(func() { | |
os.Unsetenv("TESTCONTAINERS_RYUK_ADDRESS") | |
}) |
reaper.go
Outdated
exposedPort := string(listeningPort) | ||
if tcConfig.RyukAddress != "" { | ||
exposedPort = net.JoinHostPort(tcConfig.RyukAddress, "") + ":" + exposedPort |
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.
bug: pretty sure this would result in a double colon, I would expect the following, did I miss something?
exposedPort := string(listeningPort) | |
if tcConfig.RyukAddress != "" { | |
exposedPort = net.JoinHostPort(tcConfig.RyukAddress, "") + ":" + exposedPort | |
exposedPort := config.ReaperDefaultPort | |
if tcConfig.RyukAddress != "" { | |
exposedPort = net.JoinHostPort(tcConfig.RyukAddress, exposedPort) |
Also note that the other PR has port configurable using the env var RYUK_PORT
, not sure if that will land, but just making you aware.
reaper_test.go
Outdated
@@ -408,6 +408,22 @@ func Test_NewReaper(t *testing.T) { | |||
"TESTCONTAINERS_HUB_IMAGE_NAME_PREFIX": "registry.mycompany.com/mirror", | |||
}, | |||
}, | |||
{ | |||
name: "Reaper with a custom listen address", |
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: simple test names
name: "Reaper with a custom listen address", | |
name: "custom-address", |
internal/config/config.go
Outdated
func parseIP(input string) string { | ||
ip := net.ParseIP(input) | ||
if ip == nil { | ||
panic("invalid IP address: " + input) |
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: quote so users can see issues like trailing white space
panic("invalid IP address: " + input) | |
panic(fmt.Sprintf("invalid IP address: %q", input)) |
@romainlaurent not sure you wanted to delete this branch? |
Indeed, a branch rename had more impact than expected. Unfortunately, this approach hasn't fixed my issue. The problem I have is that my tests are running in GitLab CI with the Docker executor. I will propose something different where I override the Reaper.Endpoint field here. We can override the What do you think about it? |
@romainlaurent You probably read this about Gitlab CI, right? https://golang.testcontainers.org/system_requirements/ci/gitlab_ci/ Isn't it working for you? |
@mdelapenya thanks for your answer! |
c5bec9c
to
5cf6c3f
Compare
5cf6c3f
to
937f3b5
Compare
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.
I don't think this is a workable fix I'm afraid as if ForListeningPort
doesn't work then the container wont be accessible.
reaper.go
Outdated
@@ -249,7 +249,7 @@ func newReaper(ctx context.Context, sessionID string, provider ReaperProvider) ( | |||
ExposedPorts: []string{string(listeningPort)}, | |||
Labels: core.DefaultLabels(sessionID), | |||
Privileged: tcConfig.RyukPrivileged, | |||
WaitingFor: wait.ForListeningPort(listeningPort), | |||
WaitingFor: wait.ForLog("Started"), |
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.
bug: If the listening for check doesn't work then the reaper wont be accessible so this is not a good fix.
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.
It could work by adding TESTCONTAINERS_RYUK_ENDPOINT_OVERRIDE_BY_NAME=true, allowing us to contact the container directly through its name.
I'm not sure it's the most elegant solution, so I'm open to any alternatives you think would be better.
reaper.go
Outdated
reaperContainer.ID[:8], port.Proto(), port.Port(), err) | ||
} | ||
} | ||
err = wait.ForLog("Started").WaitUntilReady(ctx, reaperContainer) |
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.
bug: If the listening for doesn't work the container isn't accessible so wont work.
b7ecbee
to
61e3e59
Compare
61e3e59
to
755f116
Compare
After a lot of try, I couldn't find a quick and elegant way to fix my problem. As it's specific at our gitlab runner configuration, I'll close this PR. |
@romainlaurent would you mind creating a issue to track this as I think its something we should look to fix. |
What does this PR do?
Configurable listen address for the ExposedPorts on Ryuk container.
Why is it important?
We run our containers from a gitlab runner where there is a network policy which forbid containers exposing port to listen on 0.0.0.0 .
Related issues
The unit tests should cover the use cases