From 193ab0683acd1372a0d9e071c847b21c3b4ce299 Mon Sep 17 00:00:00 2001 From: Robin Vanderstraeten Date: Tue, 16 Jul 2024 18:55:45 +0200 Subject: [PATCH 1/5] Fix incorrect parsing of exposedPorts in readiness check --- lifecycle.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/lifecycle.go b/lifecycle.go index a40ebc0fac..94550dcf7e 100644 --- a/lifecycle.go +++ b/lifecycle.go @@ -223,16 +223,19 @@ var defaultReadinessHook = func() ContainerLifecycleHooks { } exposedAndMappedPorts := jsonRaw.NetworkSettings.Ports + portMap, _, err := nat.ParsePortSpecs(dockerContainer.exposedPorts) + if err != nil { + return err + } - for _, exposedPort := range dockerContainer.exposedPorts { - portMap := nat.Port(exposedPort) + for exposedPort, _ := range portMap { // having entries in exposedAndMappedPorts, where the key is the exposed port, // and the value is the mapped port, means that the port has been already mapped. - if _, ok := exposedAndMappedPorts[portMap]; !ok { + if _, ok := exposedAndMappedPorts[exposedPort]; !ok { // check if the port is mapped with the protocol (default is TCP) - if !strings.Contains(exposedPort, "/") { - portMap = nat.Port(fmt.Sprintf("%s/tcp", exposedPort)) - if _, ok := exposedAndMappedPorts[portMap]; !ok { + if !strings.Contains(string(exposedPort), "/") { + exposedPort = nat.Port(fmt.Sprintf("%s/tcp", string(exposedPort))) + if _, ok := exposedAndMappedPorts[exposedPort]; !ok { return fmt.Errorf("port %s is not mapped yet", exposedPort) } } else { From 0b7232abddd98d31ab9292ca0971a8a68b308b2e Mon Sep 17 00:00:00 2001 From: Robin Vanderstraeten Date: Fri, 26 Jul 2024 15:14:30 +0200 Subject: [PATCH 2/5] Add tests for port mapping check in lifecycle.go --- lifecycle.go | 48 +++++++++++++++-------------- lifecycle_test.go | 77 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 22 deletions(-) diff --git a/lifecycle.go b/lifecycle.go index 94550dcf7e..1ce274b4ee 100644 --- a/lifecycle.go +++ b/lifecycle.go @@ -198,6 +198,31 @@ var defaultLogConsumersHook = func(cfg *LogConsumerConfig) ContainerLifecycleHoo } } +func checkPortsMapped(exposedAndMappedPorts nat.PortMap, exposedPorts []string) error { + portMap, _, err := nat.ParsePortSpecs(exposedPorts) + if err != nil { + return err + } + + for exposedPort, _ := range portMap { + // having entries in exposedAndMappedPorts, where the key is the exposed port, + // and the value is the mapped port, means that the port has been already mapped. + if _, ok := exposedAndMappedPorts[exposedPort]; !ok { + // check if the port is mapped with the protocol (default is TCP) + if !strings.Contains(string(exposedPort), "/") { + exposedPort = nat.Port(fmt.Sprintf("%s/tcp", string(exposedPort))) + if _, ok := exposedAndMappedPorts[exposedPort]; !ok { + return fmt.Errorf("port %s is not mapped yet", exposedPort) + } + } else { + return fmt.Errorf("port %s is not mapped yet", exposedPort) + } + } + } + + return nil +} + // defaultReadinessHook is a hook that will wait for the container to be ready var defaultReadinessHook = func() ContainerLifecycleHooks { return ContainerLifecycleHooks{ @@ -223,28 +248,7 @@ var defaultReadinessHook = func() ContainerLifecycleHooks { } exposedAndMappedPorts := jsonRaw.NetworkSettings.Ports - portMap, _, err := nat.ParsePortSpecs(dockerContainer.exposedPorts) - if err != nil { - return err - } - - for exposedPort, _ := range portMap { - // having entries in exposedAndMappedPorts, where the key is the exposed port, - // and the value is the mapped port, means that the port has been already mapped. - if _, ok := exposedAndMappedPorts[exposedPort]; !ok { - // check if the port is mapped with the protocol (default is TCP) - if !strings.Contains(string(exposedPort), "/") { - exposedPort = nat.Port(fmt.Sprintf("%s/tcp", string(exposedPort))) - if _, ok := exposedAndMappedPorts[exposedPort]; !ok { - return fmt.Errorf("port %s is not mapped yet", exposedPort) - } - } else { - return fmt.Errorf("port %s is not mapped yet", exposedPort) - } - } - } - - return nil + return checkPortsMapped(exposedAndMappedPorts, dockerContainer.exposedPorts) }, b, func(err error, duration time.Duration) { diff --git a/lifecycle_test.go b/lifecycle_test.go index 95ee4c5f5a..ba466f8e30 100644 --- a/lifecycle_test.go +++ b/lifecycle_test.go @@ -456,6 +456,83 @@ func TestMergePortBindings(t *testing.T) { } } +func TestPortMappingCheck(t *testing.T) { + makePortMap := func(ports []string) nat.PortMap { + out := make(nat.PortMap) + for _, port := range ports { + // We don't care about the actual binding in this test + out[nat.Port(port)] = make([]nat.PortBinding, 0) + } + return out + } + + tests := []struct { + name string + exposedAndMappedPorts nat.PortMap + exposedPorts []string + expectError bool + }{ + { + name: "No protocol given", + exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + exposedPorts: []string{"1024"}, + expectError: false, + }, + { + name: "Protocol given", + exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + exposedPorts: []string{"1024/tcp"}, + expectError: false, + }, + { + name: "Protocol and target port given", + exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + exposedPorts: []string{"1024:1024/tcp"}, + expectError: false, + }, + { + name: "Only target port given, no protocol", + exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + exposedPorts: []string{"1024:1024"}, + expectError: false, + }, + { + name: "Multiple ports with different variations", + exposedAndMappedPorts: makePortMap([]string{"1024/tcp", "1025/tcp", "1026/tcp"}), + exposedPorts: []string{"1024", "25:1025/tcp", "1026:1026"}, + expectError: false, + }, + { + name: "No ports mapped yet", + exposedAndMappedPorts: makePortMap([]string{}), + exposedPorts: []string{"1024"}, + expectError: true, + }, + { + name: "The wrong port has been mapped", + exposedAndMappedPorts: makePortMap([]string{"1023/tcp"}), + exposedPorts: []string{"1024"}, + expectError: true, + }, + { + name: "A subset of ports have been mapped", + exposedAndMappedPorts: makePortMap([]string{"1024/tcp", "1025/tcp"}), + exposedPorts: []string{"1024", "1025", "1026"}, + expectError: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkPortsMapped(tt.exposedAndMappedPorts, tt.exposedPorts) + if tt.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + func TestLifecycleHooks(t *testing.T) { tests := []struct { name string From 5fe2f77b1c6101b2c550f182ec4021d828ff5bd0 Mon Sep 17 00:00:00 2001 From: Robin Vanderstraeten Date: Fri, 26 Jul 2024 18:00:53 +0200 Subject: [PATCH 3/5] Apply suggestions from code review --- lifecycle.go | 20 +++++++-------- lifecycle_test.go | 62 ++++++++++++++++++----------------------------- 2 files changed, 34 insertions(+), 48 deletions(-) diff --git a/lifecycle.go b/lifecycle.go index 1ce274b4ee..b61ca4fd78 100644 --- a/lifecycle.go +++ b/lifecycle.go @@ -201,20 +201,21 @@ var defaultLogConsumersHook = func(cfg *LogConsumerConfig) ContainerLifecycleHoo func checkPortsMapped(exposedAndMappedPorts nat.PortMap, exposedPorts []string) error { portMap, _, err := nat.ParsePortSpecs(exposedPorts) if err != nil { - return err + return fmt.Errorf("could not check mapped ports: %w", err) } - for exposedPort, _ := range portMap { + for exposedPort := range portMap { // having entries in exposedAndMappedPorts, where the key is the exposed port, // and the value is the mapped port, means that the port has been already mapped. if _, ok := exposedAndMappedPorts[exposedPort]; !ok { // check if the port is mapped with the protocol (default is TCP) - if !strings.Contains(string(exposedPort), "/") { - exposedPort = nat.Port(fmt.Sprintf("%s/tcp", string(exposedPort))) - if _, ok := exposedAndMappedPorts[exposedPort]; !ok { - return fmt.Errorf("port %s is not mapped yet", exposedPort) - } - } else { + if strings.Contains(string(exposedPort), "/") { + return fmt.Errorf("port %s is not mapped yet", exposedPort) + } + + // Port didn't have a type, default to tcp and retry. + exposedPort = nat.Port(fmt.Sprintf("%s/tcp", string(exposedPort))) + if _, ok := exposedAndMappedPorts[exposedPort]; !ok { return fmt.Errorf("port %s is not mapped yet", exposedPort) } } @@ -247,8 +248,7 @@ var defaultReadinessHook = func() ContainerLifecycleHooks { return err } - exposedAndMappedPorts := jsonRaw.NetworkSettings.Ports - return checkPortsMapped(exposedAndMappedPorts, dockerContainer.exposedPorts) + return checkPortsMapped(jsonRaw.NetworkSettings.Ports, dockerContainer.exposedPorts) }, b, func(err error, duration time.Duration) { diff --git a/lifecycle_test.go b/lifecycle_test.go index ba466f8e30..19cace94f4 100644 --- a/lifecycle_test.go +++ b/lifecycle_test.go @@ -457,78 +457,64 @@ func TestMergePortBindings(t *testing.T) { } func TestPortMappingCheck(t *testing.T) { - makePortMap := func(ports []string) nat.PortMap { + makePortMap := func(ports ...string) nat.PortMap { out := make(nat.PortMap) for _, port := range ports { // We don't care about the actual binding in this test - out[nat.Port(port)] = make([]nat.PortBinding, 0) + out[nat.Port(port)] = nil } return out } - tests := []struct { - name string + tests := map[string]struct { exposedAndMappedPorts nat.PortMap exposedPorts []string expectError bool }{ - { - name: "No protocol given", - exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + "no-protocol": { + exposedAndMappedPorts: makePortMap("1024/tcp"), exposedPorts: []string{"1024"}, - expectError: false, }, - { - name: "Protocol given", - exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + "protocol": { + exposedAndMappedPorts: makePortMap("1024/tcp"), exposedPorts: []string{"1024/tcp"}, - expectError: false, }, - { - name: "Protocol and target port given", - exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + "protocol-target-port": { + exposedAndMappedPorts: makePortMap("1024/tcp"), exposedPorts: []string{"1024:1024/tcp"}, - expectError: false, }, - { - name: "Only target port given, no protocol", - exposedAndMappedPorts: makePortMap([]string{"1024/tcp"}), + "target-port": { + exposedAndMappedPorts: makePortMap("1024/tcp"), exposedPorts: []string{"1024:1024"}, - expectError: false, }, - { - name: "Multiple ports with different variations", - exposedAndMappedPorts: makePortMap([]string{"1024/tcp", "1025/tcp", "1026/tcp"}), + "multiple-ports": { + exposedAndMappedPorts: makePortMap("1024/tcp", "1025/tcp", "1026/tcp"), exposedPorts: []string{"1024", "25:1025/tcp", "1026:1026"}, - expectError: false, }, - { - name: "No ports mapped yet", - exposedAndMappedPorts: makePortMap([]string{}), + "no-mapped-ports": { + exposedAndMappedPorts: makePortMap(), exposedPorts: []string{"1024"}, expectError: true, }, - { - name: "The wrong port has been mapped", - exposedAndMappedPorts: makePortMap([]string{"1023/tcp"}), + "wrong-mapped-port": { + exposedAndMappedPorts: makePortMap("1023/tcp"), exposedPorts: []string{"1024"}, expectError: true, }, - { - name: "A subset of ports have been mapped", - exposedAndMappedPorts: makePortMap([]string{"1024/tcp", "1025/tcp"}), + "subset-mapped-ports": { + exposedAndMappedPorts: makePortMap("1024/tcp", "1025/tcp"), exposedPorts: []string{"1024", "1025", "1026"}, expectError: true, }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { + for name, tt := range tests { + t.Run(name, func(t *testing.T) { err := checkPortsMapped(tt.exposedAndMappedPorts, tt.exposedPorts) if tt.expectError { - assert.Error(t, err) - } else { - assert.NoError(t, err) + require.Error(t, err) + return } + require.NoError(t, err) }) } } From 42344f6f5abb2fd6218156a402c97a2e2ed68bae Mon Sep 17 00:00:00 2001 From: Robin Vanderstraeten Date: Mon, 29 Jul 2024 10:43:00 +0200 Subject: [PATCH 4/5] Apply review comments --- lifecycle.go | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/lifecycle.go b/lifecycle.go index b61ca4fd78..a0c6b99b88 100644 --- a/lifecycle.go +++ b/lifecycle.go @@ -201,23 +201,25 @@ var defaultLogConsumersHook = func(cfg *LogConsumerConfig) ContainerLifecycleHoo func checkPortsMapped(exposedAndMappedPorts nat.PortMap, exposedPorts []string) error { portMap, _, err := nat.ParsePortSpecs(exposedPorts) if err != nil { - return fmt.Errorf("could not check mapped ports: %w", err) + return fmt.Errorf("parse exposed ports: %w", err) } for exposedPort := range portMap { // having entries in exposedAndMappedPorts, where the key is the exposed port, // and the value is the mapped port, means that the port has been already mapped. - if _, ok := exposedAndMappedPorts[exposedPort]; !ok { - // check if the port is mapped with the protocol (default is TCP) - if strings.Contains(string(exposedPort), "/") { - return fmt.Errorf("port %s is not mapped yet", exposedPort) - } + if _, ok := exposedAndMappedPorts[exposedPort]; ok { + continue + } - // Port didn't have a type, default to tcp and retry. - exposedPort = nat.Port(fmt.Sprintf("%s/tcp", string(exposedPort))) - if _, ok := exposedAndMappedPorts[exposedPort]; !ok { - return fmt.Errorf("port %s is not mapped yet", exposedPort) - } + // check if the port is mapped with the protocol (default is TCP) + if strings.Contains(string(exposedPort), "/") { + return fmt.Errorf("port %s is not mapped yet", exposedPort) + } + + // Port didn't have a type, default to tcp and retry. + exposedPort += "/tcp" + if _, ok := exposedAndMappedPorts[exposedPort]; !ok { + return fmt.Errorf("port %s is not mapped yet", exposedPort) } } From 352e5ac49e9907a3ed43f34be916778d0ebea68c Mon Sep 17 00:00:00 2001 From: Robin Vanderstraeten Date: Tue, 30 Jul 2024 11:18:57 +0200 Subject: [PATCH 5/5] Add testcase with explicit ipv4 binding --- lifecycle_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lifecycle_test.go b/lifecycle_test.go index 19cace94f4..f4b0a2ae37 100644 --- a/lifecycle_test.go +++ b/lifecycle_test.go @@ -491,6 +491,10 @@ func TestPortMappingCheck(t *testing.T) { exposedAndMappedPorts: makePortMap("1024/tcp", "1025/tcp", "1026/tcp"), exposedPorts: []string{"1024", "25:1025/tcp", "1026:1026"}, }, + "only-ipv4": { + exposedAndMappedPorts: makePortMap("1024/tcp"), + exposedPorts: []string{"0.0.0.0::1024/tcp"}, + }, "no-mapped-ports": { exposedAndMappedPorts: makePortMap(), exposedPorts: []string{"1024"},