Skip to content

Commit

Permalink
fix: enhance host configuration port binding
Browse files Browse the repository at this point in the history
This commit refactors the mergePortBinding function to accommodate exposed ports with their respective protocols.
  • Loading branch information
Willian S. de Souza committed Apr 24, 2024
1 parent ab60437 commit b42ed50
Show file tree
Hide file tree
Showing 2 changed files with 36 additions and 28 deletions.
10 changes: 3 additions & 7 deletions lifecycle.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@ import (
"errors"
"fmt"
"io"
"strings"

"github.com/docker/docker/api/types/container"
"github.com/docker/docker/api/types/network"
"github.com/docker/go-connections/nat"
"golang.org/x/exp/slices"
)

// ContainerRequestHook is a hook that will be called before a container is created.
Expand Down Expand Up @@ -458,7 +456,7 @@ func (p *DockerProvider) preCreateContainerHook(ctx context.Context, req Contain
if len(exposedPorts) == 0 && !hostConfig.NetworkMode.IsContainer() {
hostConfig.PortBindings = exposedPortMap
} else {
hostConfig.PortBindings = mergePortBindings(hostConfig.PortBindings, exposedPortMap, req.ExposedPorts)
hostConfig.PortBindings = mergePortBindings(hostConfig.PortBindings, exposedPortMap)
}

return nil
Expand Down Expand Up @@ -525,15 +523,13 @@ func combineContainerHooks(defaultHooks, userDefinedHooks []ContainerLifecycleHo
}
}

func mergePortBindings(configPortMap, exposedPortMap nat.PortMap, exposedPorts []string) nat.PortMap {
func mergePortBindings(configPortMap, exposedPortMap nat.PortMap) nat.PortMap {
if exposedPortMap == nil {
exposedPortMap = make(map[nat.Port][]nat.PortBinding)
}

for k, v := range configPortMap {
if slices.Contains(exposedPorts, strings.Split(string(k), "/")[0]) {
exposedPortMap[k] = v
}
exposedPortMap[k] = v
}
return exposedPortMap
}
Expand Down
54 changes: 33 additions & 21 deletions lifecycle_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ func TestPreCreateModifierHook(t *testing.T) {
)
})

t.Run("Request contains exposed port modifiers", func(t *testing.T) {
t.Run("Request contains exposed port modifiers without protocol", func(t *testing.T) {
req := ContainerRequest{
Image: nginxAlpineImage, // alpine image does expose port 80
HostConfigModifier: func(hostConfig *container.HostConfig) {
Expand Down Expand Up @@ -334,6 +334,37 @@ func TestPreCreateModifierHook(t *testing.T) {
assert.Equal(t, "localhost", inputHostConfig.PortBindings["80/tcp"][0].HostIP)
assert.Equal(t, "8080", inputHostConfig.PortBindings["80/tcp"][0].HostPort)
})

t.Run("Request contains exposed port modifiers with protocol", func(t *testing.T) {
req := ContainerRequest{
Image: nginxAlpineImage, // alpine image does expose port 80
HostConfigModifier: func(hostConfig *container.HostConfig) {
hostConfig.PortBindings = nat.PortMap{
"80/tcp": []nat.PortBinding{
{
HostIP: "localhost",
HostPort: "8080",
},
},
}
},
ExposedPorts: []string{"80/tcp"},
}

// define empty inputs to be overwritten by the pre create hook
inputConfig := &container.Config{
Image: req.Image,
}
inputHostConfig := &container.HostConfig{}
inputNetworkingConfig := &network.NetworkingConfig{}

err = provider.preCreateContainerHook(ctx, req, inputConfig, inputHostConfig, inputNetworkingConfig)
require.NoError(t, err)

// assertions
assert.Equal(t, "localhost", inputHostConfig.PortBindings["80/tcp"][0].HostIP)
assert.Equal(t, "8080", inputHostConfig.PortBindings["80/tcp"][0].HostPort)
})
}

func TestMergePortBindings(t *testing.T) {
Expand All @@ -352,7 +383,6 @@ func TestMergePortBindings(t *testing.T) {
arg: arg{
configPortMap: nil,
parsedPortMap: nil,
exposedPorts: nil,
},
expected: map[nat.Port][]nat.PortBinding{},
},
Expand All @@ -363,7 +393,6 @@ func TestMergePortBindings(t *testing.T) {
"80/tcp": {{HostIP: "1", HostPort: "2"}},
},
parsedPortMap: nil,
exposedPorts: nil,
},
expected: map[nat.Port][]nat.PortBinding{},
},
Expand All @@ -374,22 +403,6 @@ func TestMergePortBindings(t *testing.T) {
parsedPortMap: map[nat.Port][]nat.PortBinding{
"80/tcp": {{HostIP: "", HostPort: ""}},
},
exposedPorts: nil,
},
expected: map[nat.Port][]nat.PortBinding{
"80/tcp": {{HostIP: "", HostPort: ""}},
},
},
{
name: "parsed and configured but not exposed",
arg: arg{
configPortMap: map[nat.Port][]nat.PortBinding{
"80/tcp": {{HostIP: "1", HostPort: "2"}},
},
parsedPortMap: map[nat.Port][]nat.PortBinding{
"80/tcp": {{HostIP: "", HostPort: ""}},
},
exposedPorts: nil,
},
expected: map[nat.Port][]nat.PortBinding{
"80/tcp": {{HostIP: "", HostPort: ""}},
Expand All @@ -407,7 +420,6 @@ func TestMergePortBindings(t *testing.T) {
"80/tcp": {{HostIP: "", HostPort: ""}},
"90/tcp": {{HostIP: "", HostPort: ""}},
},
exposedPorts: []string{"70", "80"},
},
expected: map[nat.Port][]nat.PortBinding{
"70/tcp": {{HostIP: "1", HostPort: "2"}},
Expand All @@ -419,7 +431,7 @@ func TestMergePortBindings(t *testing.T) {

for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
res := mergePortBindings(c.arg.configPortMap, c.arg.parsedPortMap, c.arg.exposedPorts)
res := mergePortBindings(c.arg.configPortMap, c.arg.parsedPortMap)
assert.Equal(t, c.expected, res)
})
}
Expand Down

0 comments on commit b42ed50

Please sign in to comment.