From dbe193cc64f737691be797a49034febb6b0200ba Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 12 Sep 2024 19:58:14 +0530 Subject: [PATCH 1/7] fix: handle 127 error code for podman compatibility --- wait/host_port.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/wait/host_port.go b/wait/host_port.go index a3e9137006..e44f50ae62 100644 --- a/wait/host_port.go +++ b/wait/host_port.go @@ -209,7 +209,7 @@ func internalCheck(ctx context.Context, internalPort nat.Port, target StrategyTa if exitCode == 0 { break - } else if exitCode == 126 { + } else if exitCode == 126 || exitCode == 127 { return errShellNotExecutable } } From c0801bfbe1173aa5dede73f08e92d419b07f408d Mon Sep 17 00:00:00 2001 From: Vivek Date: Thu, 12 Sep 2024 20:28:45 +0530 Subject: [PATCH 2/7] fix: remove magic numbers; handle 126 and 127 error code separately --- wait/host_port.go | 16 +++++++++++++++- wait/host_port_test.go | 2 +- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/wait/host_port.go b/wait/host_port.go index e44f50ae62..c0f686e27d 100644 --- a/wait/host_port.go +++ b/wait/host_port.go @@ -13,6 +13,13 @@ import ( "github.com/docker/go-connections/nat" ) +type exitStatus = int + +const ( + exitEaccess exitStatus = 126 // container cmd can't be invoked (permission denied) + exitCmdNotFound exitStatus = 127 // container cmd not found/does not exist or invalid bind-mount +) + // Implement interface var ( _ Strategy = (*HostPortStrategy)(nil) @@ -20,6 +27,7 @@ var ( ) var errShellNotExecutable = errors.New("/bin/sh command not executable") +var errShellNotFound = errors.New("/bin/sh command not found") type HostPortStrategy struct { // Port is a string containing port number and protocol in the format "80/tcp" @@ -207,10 +215,16 @@ func internalCheck(ctx context.Context, internalPort nat.Port, target StrategyTa return fmt.Errorf("%w, host port waiting failed", err) } + /* + Docker maps exit code 127 to 126, while Podman treats them separately. + Handle both to ensure compatibility with Docker and Podman. + */ if exitCode == 0 { break - } else if exitCode == 126 || exitCode == 127 { + } else if exitCode == exitEaccess { return errShellNotExecutable + } else if exitCode == exitCmdNotFound { + return errShellNotFound } } return nil diff --git a/wait/host_port_test.go b/wait/host_port_test.go index bf349f05a7..3ef1476260 100644 --- a/wait/host_port_test.go +++ b/wait/host_port_test.go @@ -497,7 +497,7 @@ func TestHostPortStrategySucceedsGivenShellIsNotInstalled(t *testing.T) { }, ExecImpl: func(_ context.Context, _ []string, _ ...exec.ProcessOption) (int, io.Reader, error) { // This is the error that would be returned if the shell is not installed. - return 126, nil, nil + return exitEaccess, nil, nil }, } From 3f5bf9245f56a91e480753df933a0f95af850d31 Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 13 Sep 2024 07:40:40 +0530 Subject: [PATCH 3/7] chore: handle comments for removing exitStatus type; handle returned error and switch case instead of if-else-if --- wait/host_port.go | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/wait/host_port.go b/wait/host_port.go index c0f686e27d..b3a8fd2950 100644 --- a/wait/host_port.go +++ b/wait/host_port.go @@ -13,11 +13,9 @@ import ( "github.com/docker/go-connections/nat" ) -type exitStatus = int - const ( - exitEaccess exitStatus = 126 // container cmd can't be invoked (permission denied) - exitCmdNotFound exitStatus = 127 // container cmd not found/does not exist or invalid bind-mount + exitEaccess = 126 // container cmd can't be invoked (permission denied) + exitCmdNotFound = 127 // container cmd not found/does not exist or invalid bind-mount ) // Implement interface @@ -159,13 +157,17 @@ func (hp *HostPortStrategy) WaitUntilReady(ctx context.Context, target StrategyT } if err = internalCheck(ctx, internalPort, target); err != nil { - if errors.Is(errShellNotExecutable, err) { + switch { + case errors.Is(err, errShellNotExecutable): log.Println("Shell not executable in container, only external port validated") return nil + case errors.Is(err, errShellNotFound): + log.Println("Shell not found in container") + return nil + default: + return fmt.Errorf("internal check: %w", err) } - - return fmt.Errorf("internal check: %w", err) - } + } return nil } @@ -215,19 +217,18 @@ func internalCheck(ctx context.Context, internalPort nat.Port, target StrategyTa return fmt.Errorf("%w, host port waiting failed", err) } - /* - Docker maps exit code 127 to 126, while Podman treats them separately. - Handle both to ensure compatibility with Docker and Podman. - */ - if exitCode == 0 { - break - } else if exitCode == exitEaccess { + // Docker has a issue which override exit code 127 to 126 due to: + // https://github.com/moby/moby/issues/45795 + // Handle both to ensure compatibility with Docker and Podman for now. + switch exitCode { + case 0: + return nil + case exitEaccess: return errShellNotExecutable - } else if exitCode == exitCmdNotFound { + case exitCmdNotFound: return errShellNotFound } } - return nil } func buildInternalCheckCommand(internalPort int) string { From 409e6f38ced68274540c943a52a6cdc55033d7ed Mon Sep 17 00:00:00 2001 From: Vivek Date: Fri, 13 Sep 2024 12:31:07 +0530 Subject: [PATCH 4/7] feat: add unit test for exit code 127 --- wait/host_port_test.go | 55 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/wait/host_port_test.go b/wait/host_port_test.go index 3ef1476260..dbb479cf2a 100644 --- a/wait/host_port_test.go +++ b/wait/host_port_test.go @@ -508,3 +508,58 @@ func TestHostPortStrategySucceedsGivenShellIsNotInstalled(t *testing.T) { err = wg.WaitUntilReady(context.Background(), target) require.NoError(t, err) } + +func TestHostPortStrategySucceedsGivenShellIsNotFound(t *testing.T) { + listener, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatal(err) + } + defer listener.Close() + + rawPort := listener.Addr().(*net.TCPAddr).Port + port, err := nat.NewPort("tcp", strconv.Itoa(rawPort)) + if err != nil { + t.Fatal(err) + } + + target := &MockStrategyTarget{ + HostImpl: func(_ context.Context) (string, error) { + return "localhost", nil + }, + InspectImpl: func(_ context.Context) (*types.ContainerJSON, error) { + return &types.ContainerJSON{ + NetworkSettings: &types.NetworkSettings{ + NetworkSettingsBase: types.NetworkSettingsBase{ + Ports: nat.PortMap{ + "80": []nat.PortBinding{ + { + HostIP: "0.0.0.0", + HostPort: port.Port(), + }, + }, + }, + }, + }, + }, nil + }, + MappedPortImpl: func(_ context.Context, _ nat.Port) (nat.Port, error) { + return port, nil + }, + StateImpl: func(_ context.Context) (*types.ContainerState, error) { + return &types.ContainerState{ + Running: true, + }, nil + }, + ExecImpl: func(_ context.Context, _ []string, _ ...exec.ProcessOption) (int, io.Reader, error) { + // This is the error that would be returned if the shell is not found. + return exitCmdNotFound, nil, nil + }, + } + + wg := NewHostPortStrategy("80"). + WithStartupTimeout(5 * time.Second). + WithPollInterval(100 * time.Millisecond) + + err = wg.WaitUntilReady(context.Background(), target) + require.NoError(t, err) +} \ No newline at end of file From 9f7c2987352bd5f8a3fd5fd8b2e2a1c86f6a5e7a Mon Sep 17 00:00:00 2001 From: Vivek Date: Sat, 14 Sep 2024 11:56:24 +0530 Subject: [PATCH 5/7] chore: add internalCheck() tests for exit code = 126/127 --- wait/host_port_test.go | 104 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 103 insertions(+), 1 deletion(-) diff --git a/wait/host_port_test.go b/wait/host_port_test.go index dbb479cf2a..1186ba7d43 100644 --- a/wait/host_port_test.go +++ b/wait/host_port_test.go @@ -562,4 +562,106 @@ func TestHostPortStrategySucceedsGivenShellIsNotFound(t *testing.T) { err = wg.WaitUntilReady(context.Background(), target) require.NoError(t, err) -} \ No newline at end of file +} + +func TestInternalCheckFailsGivenShellIsNotInstalled(t *testing.T) { + listener, err := net.Listen("tcp", "localhost:0") + require.NoError(t, err) + defer listener.Close() + + rawPort := listener.Addr().(*net.TCPAddr).Port + port, err := nat.NewPort("tcp", strconv.Itoa(rawPort)) + require.NoError(t, err) + + target := &MockStrategyTarget{ + HostImpl: func(_ context.Context) (string, error) { + return "localhost", nil + }, + InspectImpl: func(_ context.Context) (*types.ContainerJSON, error) { + return &types.ContainerJSON{ + NetworkSettings: &types.NetworkSettings{ + NetworkSettingsBase: types.NetworkSettingsBase{ + Ports: nat.PortMap{ + "80": []nat.PortBinding{ + { + HostIP: "0.0.0.0", + HostPort: port.Port(), + }, + }, + }, + }, + }, + }, nil + }, + MappedPortImpl: func(_ context.Context, _ nat.Port) (nat.Port, error) { + return port, nil + }, + StateImpl: func(_ context.Context) (*types.ContainerState, error) { + return &types.ContainerState{ + Running: true, + }, nil + }, + ExecImpl: func(_ context.Context, _ []string, _ ...exec.ProcessOption) (int, io.Reader, error) { + // This is the error that would be returned if the shell is not installed. + return exitEaccess, nil, nil + }, + } + + { + err := internalCheck(context.Background(), "80", target) + require.Error(t, err) + + require.Contains(t, err.Error(), errShellNotExecutable.Error()) + } +} + +func TestInternalCheckFailsGivenShellIsNotFound(t *testing.T) { + listener, err := net.Listen("tcp", "localhost:0") + require.NoError(t, err) + defer listener.Close() + + rawPort := listener.Addr().(*net.TCPAddr).Port + port, err := nat.NewPort("tcp", strconv.Itoa(rawPort)) + require.NoError(t, err) + + target := &MockStrategyTarget{ + HostImpl: func(_ context.Context) (string, error) { + return "localhost", nil + }, + InspectImpl: func(_ context.Context) (*types.ContainerJSON, error) { + return &types.ContainerJSON{ + NetworkSettings: &types.NetworkSettings{ + NetworkSettingsBase: types.NetworkSettingsBase{ + Ports: nat.PortMap{ + "80": []nat.PortBinding{ + { + HostIP: "0.0.0.0", + HostPort: port.Port(), + }, + }, + }, + }, + }, + }, nil + }, + MappedPortImpl: func(_ context.Context, _ nat.Port) (nat.Port, error) { + return port, nil + }, + StateImpl: func(_ context.Context) (*types.ContainerState, error) { + return &types.ContainerState{ + Running: true, + }, nil + }, + ExecImpl: func(_ context.Context, _ []string, _ ...exec.ProcessOption) (int, io.Reader, error) { + // This is the error that would be returned if the shell is not found. + return exitCmdNotFound, nil, nil + }, + } + + { + err := internalCheck(context.Background(), "80", target) + require.Error(t, err) + + require.Contains(t, err.Error(), errShellNotFound.Error()) + } +} From 79b4a8535616e6f51988dca56a7feea583b62459 Mon Sep 17 00:00:00 2001 From: Vivek Date: Sun, 15 Sep 2024 09:49:14 +0530 Subject: [PATCH 6/7] chore: replace t.fatal() with require.NoError() --- wait/host_port_test.go | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/wait/host_port_test.go b/wait/host_port_test.go index 1186ba7d43..125e76fb00 100644 --- a/wait/host_port_test.go +++ b/wait/host_port_test.go @@ -456,16 +456,12 @@ func TestHostPortStrategyFailsWhileInternalCheckingDueToUnexpectedContainerStatu func TestHostPortStrategySucceedsGivenShellIsNotInstalled(t *testing.T) { listener, err := net.Listen("tcp", "localhost:0") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer listener.Close() rawPort := listener.Addr().(*net.TCPAddr).Port port, err := nat.NewPort("tcp", strconv.Itoa(rawPort)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) target := &MockStrategyTarget{ HostImpl: func(_ context.Context) (string, error) { @@ -511,16 +507,12 @@ func TestHostPortStrategySucceedsGivenShellIsNotInstalled(t *testing.T) { func TestHostPortStrategySucceedsGivenShellIsNotFound(t *testing.T) { listener, err := net.Listen("tcp", "localhost:0") - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) defer listener.Close() rawPort := listener.Addr().(*net.TCPAddr).Port port, err := nat.NewPort("tcp", strconv.Itoa(rawPort)) - if err != nil { - t.Fatal(err) - } + require.NoError(t, err) target := &MockStrategyTarget{ HostImpl: func(_ context.Context) (string, error) { From d76ecc10f996d2e3ecc9236784983d32534e6a66 Mon Sep 17 00:00:00 2001 From: Vivek Date: Wed, 18 Sep 2024 20:55:22 +0530 Subject: [PATCH 7/7] chore: merge internal check tests with existing tests; revert mockStrategyTarget to MockStrategyTarget to avoid export errors; run make lint --- wait/host_port.go | 8 +-- wait/host_port_test.go | 120 +++++++---------------------------------- 2 files changed, 24 insertions(+), 104 deletions(-) diff --git a/wait/host_port.go b/wait/host_port.go index b3a8fd2950..9360517a04 100644 --- a/wait/host_port.go +++ b/wait/host_port.go @@ -24,8 +24,10 @@ var ( _ StrategyTimeout = (*HostPortStrategy)(nil) ) -var errShellNotExecutable = errors.New("/bin/sh command not executable") -var errShellNotFound = errors.New("/bin/sh command not found") +var ( + errShellNotExecutable = errors.New("/bin/sh command not executable") + errShellNotFound = errors.New("/bin/sh command not found") +) type HostPortStrategy struct { // Port is a string containing port number and protocol in the format "80/tcp" @@ -167,7 +169,7 @@ func (hp *HostPortStrategy) WaitUntilReady(ctx context.Context, target StrategyT default: return fmt.Errorf("internal check: %w", err) } - } + } return nil } diff --git a/wait/host_port_test.go b/wait/host_port_test.go index 125e76fb00..18a15aed82 100644 --- a/wait/host_port_test.go +++ b/wait/host_port_test.go @@ -1,8 +1,10 @@ package wait import ( + "bytes" "context" "io" + "log" "net" "strconv" "testing" @@ -501,8 +503,17 @@ func TestHostPortStrategySucceedsGivenShellIsNotInstalled(t *testing.T) { WithStartupTimeout(5 * time.Second). WithPollInterval(100 * time.Millisecond) + oldWriter := log.Default().Writer() + var buf bytes.Buffer + log.Default().SetOutput(&buf) + t.Cleanup(func() { + log.Default().SetOutput(oldWriter) + }) + err = wg.WaitUntilReady(context.Background(), target) require.NoError(t, err) + + require.Contains(t, buf.String(), "Shell not executable in container, only external port validated") } func TestHostPortStrategySucceedsGivenShellIsNotFound(t *testing.T) { @@ -552,108 +563,15 @@ func TestHostPortStrategySucceedsGivenShellIsNotFound(t *testing.T) { WithStartupTimeout(5 * time.Second). WithPollInterval(100 * time.Millisecond) - err = wg.WaitUntilReady(context.Background(), target) - require.NoError(t, err) -} - -func TestInternalCheckFailsGivenShellIsNotInstalled(t *testing.T) { - listener, err := net.Listen("tcp", "localhost:0") - require.NoError(t, err) - defer listener.Close() - - rawPort := listener.Addr().(*net.TCPAddr).Port - port, err := nat.NewPort("tcp", strconv.Itoa(rawPort)) - require.NoError(t, err) - - target := &MockStrategyTarget{ - HostImpl: func(_ context.Context) (string, error) { - return "localhost", nil - }, - InspectImpl: func(_ context.Context) (*types.ContainerJSON, error) { - return &types.ContainerJSON{ - NetworkSettings: &types.NetworkSettings{ - NetworkSettingsBase: types.NetworkSettingsBase{ - Ports: nat.PortMap{ - "80": []nat.PortBinding{ - { - HostIP: "0.0.0.0", - HostPort: port.Port(), - }, - }, - }, - }, - }, - }, nil - }, - MappedPortImpl: func(_ context.Context, _ nat.Port) (nat.Port, error) { - return port, nil - }, - StateImpl: func(_ context.Context) (*types.ContainerState, error) { - return &types.ContainerState{ - Running: true, - }, nil - }, - ExecImpl: func(_ context.Context, _ []string, _ ...exec.ProcessOption) (int, io.Reader, error) { - // This is the error that would be returned if the shell is not installed. - return exitEaccess, nil, nil - }, - } - - { - err := internalCheck(context.Background(), "80", target) - require.Error(t, err) - - require.Contains(t, err.Error(), errShellNotExecutable.Error()) - } -} + oldWriter := log.Default().Writer() + var buf bytes.Buffer + log.Default().SetOutput(&buf) + t.Cleanup(func() { + log.Default().SetOutput(oldWriter) + }) -func TestInternalCheckFailsGivenShellIsNotFound(t *testing.T) { - listener, err := net.Listen("tcp", "localhost:0") - require.NoError(t, err) - defer listener.Close() - - rawPort := listener.Addr().(*net.TCPAddr).Port - port, err := nat.NewPort("tcp", strconv.Itoa(rawPort)) + err = wg.WaitUntilReady(context.Background(), target) require.NoError(t, err) - target := &MockStrategyTarget{ - HostImpl: func(_ context.Context) (string, error) { - return "localhost", nil - }, - InspectImpl: func(_ context.Context) (*types.ContainerJSON, error) { - return &types.ContainerJSON{ - NetworkSettings: &types.NetworkSettings{ - NetworkSettingsBase: types.NetworkSettingsBase{ - Ports: nat.PortMap{ - "80": []nat.PortBinding{ - { - HostIP: "0.0.0.0", - HostPort: port.Port(), - }, - }, - }, - }, - }, - }, nil - }, - MappedPortImpl: func(_ context.Context, _ nat.Port) (nat.Port, error) { - return port, nil - }, - StateImpl: func(_ context.Context) (*types.ContainerState, error) { - return &types.ContainerState{ - Running: true, - }, nil - }, - ExecImpl: func(_ context.Context, _ []string, _ ...exec.ProcessOption) (int, io.Reader, error) { - // This is the error that would be returned if the shell is not found. - return exitCmdNotFound, nil, nil - }, - } - - { - err := internalCheck(context.Background(), "80", target) - require.Error(t, err) - - require.Contains(t, err.Error(), errShellNotFound.Error()) - } + require.Contains(t, buf.String(), "Shell not found in container") }