#1152 execute HostConfigModifier at last

[xmh19936688]

#1152 execute HostConfigModifier at last, or some values will be overwrited

What does this PR do?

execute HostConfigModifier at last

Why is it important?

Currently some values such as PortBindings will be overwrited.

Related issues

• Closes [Bug]: config portBindings in HostConfigModifier do not work #1152

[netlify]

✅ Deploy Preview for testcontainers-go ready!

Name	Link
🔨 Latest commit	959635c
🔍 Latest deploy log	https://app.netlify.com/sites/testcontainers-go/deploys/6465cefd981521000833ac37
😎 Deploy Preview	https://deploy-preview-1153--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 settings.

[mdelapenya]

@xmh19936688 could you extend the existing unit tests to demonstrate the bug? 🙏

[mdelapenya]

golangci-lint is complaining that there is a file not go-fmt'ed. Could you check it?

[mdelapenya]

Could you double check that the existing tests are passing? I see TestPreCreateModifierHook failing on CI too

[xmh19936688]

At line 110, it says Host config's port bindings should be overwritten by the modifier. But I noticed that HostIP and HostPort has been modified at line 48 and 49. Why the expectd value is empty string at line 105 and 106? What's the correct behavior?

[mdelapenya]

Why the expectd value is empty string at line 105 and 106?

I think the tests in this case are driven by the implementation/execution of nat.ParsePortSpecs, and after debugging that function, the returned binding comes empty for the passed exposed port.

What's the correct behavior?

What is your idea or use case for it? I remember implementing the lifecycle code as a refactor of the at-that-time existing code, so I tried to replicate it adding unit tests for it.

[xmh19936688]

I'd like to specify a host port instead of the random one. So the prerequisite of specifing hostIP and hostPort is it has been exposed, right?

[mdelapenya]

I'd like to specify a host port instead of the random one. So the prerequisite of specifing hostIP and hostPort is it has been exposed, right?

I see your point: because the host config modifier allows to define e.g. custom PortBindings, then they are not applied because the ParsePortSpecs call takes place last.

Then I'm OK with changing the order. Do you think we can update the tests to represent the changes in the behaviours?

[xmh19936688]

Do you think we can update the tests to represent the changes in the behaviours?

Sounds good! I think we need some cases to represent how ExposedPorts and HostConfigModifier effacts.
For example: ExposedPorts contains 2379 and 2380, and HostConfigModifier binds 2380 to 32380 and 8080 to 38080. What will the result be? (Like this: 2379 to a random hostport, 2380 to 32380, no 8080 ?)

The port in image.ContainerConfig.ExposedPorts may also have affect, do they exposed implicitly?

[mdelapenya]

The port in image.ContainerConfig.ExposedPorts may also have affect, do they exposed implicitly?

We are only exposing those ports automatically if the container request exposes zero ports and the the container does not run in a container network:

if len(exposedPorts) == 0 && !hostConfig.NetworkMode.IsContainer() {

[xmh19936688]

if len(exposedPorts) == 0 && !hostConfig.NetworkMode.IsContainer() {

Apologize for not getting you. Should I do with that if like this?

if len(exposedPorts) == 0 && !hostConfig.NetworkMode.IsContainer() {
    hostConfig.PortBindings = exposedPortMap
} else {
    hostConfig.PortBindings = mergePortBindings(hostConfig.PortBindings, exposedPortMap, req.ExposedPorts)
}

[mdelapenya]

if len(exposedPorts) == 0 && !hostConfig.NetworkMode.IsContainer() {
    hostConfig.PortBindings = exposedPortMap
} else {
    hostConfig.PortBindings = mergePortBindings(hostConfig.PortBindings, exposedPortMap, req.ExposedPorts)
}

I think that will do it: if there are no exposed ports, then keep the behavior of nat.ParsePortSpecs(exposedPorts), else merge with the fix in this PR. We could probably add a comment before the if/else block explaining why we need and not need to merge the port bindings.

wdyt?

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
14.1% Duplication

[mdelapenya]

LGTM, I'm waiting for the rest of the GH actions to pass to update this branch including all dependabot updates. Once finished, I'll merge this one.

Thanks for your patience!! 🙏