Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions internal/container/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,13 @@ func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink, opts Start

tel := opts.Telemetry

var hostEnv []string
for _, e := range os.Environ() {
if strings.HasPrefix(e, "CI=") || (strings.HasPrefix(e, "LOCALSTACK_") && !strings.HasPrefix(e, "LOCALSTACK_AUTH_TOKEN=")) {
hostEnv = append(hostEnv, e)
}
}

containers := make([]runtime.ContainerConfig, len(opts.Containers))
for i, c := range opts.Containers {
image, err := c.Image()
Expand Down Expand Up @@ -128,6 +135,8 @@ func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink, opts Start
"MAIN_CONTAINER_NAME="+containerName,
)

env = append(env, hostEnv...)

var binds []runtime.BindMount
if socketPath := rt.SocketPath(); socketPath != "" {
binds = append(binds, runtime.BindMount{HostPath: socketPath, ContainerPath: "/var/run/docker.sock"})
Expand Down
24 changes: 24 additions & 0 deletions internal/container/start_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"errors"
"io"
"strings"
"testing"

"github.com/localstack/lstk/internal/log"
Expand Down Expand Up @@ -64,3 +65,26 @@ func TestServicePortRange_ReturnsExpectedPorts(t *testing.T) {
assert.Equal(t, "4559", ports[50].ContainerPort)
assert.Equal(t, "4559", ports[50].HostPort)
}

func TestFilterHostEnv(t *testing.T) {
tests := []struct {
name string
env string
expected bool
}{
{"CI is included", "CI=true", true},
{"LOCALSTACK_DISABLE_EVENTS is included", "LOCALSTACK_DISABLE_EVENTS=1", true},
{"LOCALSTACK_HOST is included", "LOCALSTACK_HOST=0.0.0.0", true},
{"LOCALSTACK_AUTH_TOKEN is excluded", "LOCALSTACK_AUTH_TOKEN=secret", false},
{"PATH is excluded", "PATH=/usr/bin", false},
{"HOME is excluded", "HOME=/root", false},
{"LOCALSTACK_BUILD_VERSION is included", "LOCALSTACK_BUILD_VERSION=3.0.0", true},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := strings.HasPrefix(tt.env, "CI=") || (strings.HasPrefix(tt.env, "LOCALSTACK_") && !strings.HasPrefix(tt.env, "LOCALSTACK_AUTH_TOKEN="))
assert.Equal(t, tt.expected, got)
})
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: I don't see the point of these unit tests. They are duplicating the logic of the code under test. I think we can delete and rely solely on the new integration tests.

92 changes: 23 additions & 69 deletions test/integration/start_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,13 @@ package integration_test

import (
"context"
"crypto/tls"
"fmt"
"net"
"net/http"
"os"
"path/filepath"
"strconv"
"strings"
"testing"

"github.com/docker/docker/api/types/container"
"github.com/docker/go-connections/nat"
"github.com/localstack/lstk/test/integration/env"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -164,66 +159,36 @@ func TestStartCommandSetsUpContainerCorrectly(t *testing.T) {
assert.Equal(t, containerName, envVars["MAIN_CONTAINER_NAME"])
assert.NotEmpty(t, envVars["LOCALSTACK_AUTH_TOKEN"])
})
}

t.Run("docker socket mount", func(t *testing.T) {
if !strings.HasPrefix(dockerClient.DaemonHost(), "unix://") {
t.Skip("Docker daemon is not reachable via unix socket")
}

assert.True(t, hasBindTarget(inspect.HostConfig.Binds, "/var/run/docker.sock"),
"expected Docker socket bind mount to /var/run/docker.sock, got: %v", inspect.HostConfig.Binds)

envVars := containerEnvToMap(inspect.Config.Env)
assert.Equal(t, "unix:///var/run/docker.sock", envVars["DOCKER_HOST"])
})
func TestStartCommandPassesCIAndLocalStackEnvVars(t *testing.T) {
requireDocker(t)
_ = env.Require(t, env.AuthToken)

t.Run("service port range", func(t *testing.T) {
for p := 4510; p <= 4559; p++ {
port := nat.Port(fmt.Sprintf("%d/tcp", p))
bindings := inspect.HostConfig.PortBindings[port]
if assert.NotEmpty(t, bindings, "port %d/tcp should be bound", p) {
assert.Equal(t, strconv.Itoa(p), bindings[0].HostPort)
}
}
})
cleanup()
t.Cleanup(cleanup)

t.Run("main port", func(t *testing.T) {
mainBindings := inspect.HostConfig.PortBindings[nat.Port("4566/tcp")]
require.NotEmpty(t, mainBindings, "port 4566/tcp should be bound")
assert.Equal(t, "4566", mainBindings[0].HostPort)
})
t.Setenv("CI", "true")
t.Setenv("LOCALSTACK_DISABLE_EVENTS", "1")
t.Setenv("LOCALSTACK_AUTH_TOKEN", "host-token")
Comment on lines +171 to +173
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: we should not be setting env vars like this, instead when running the lstk command:

  _, stderr, err := runLstk(t, ctx, "",
      env.With(env.APIEndpoint, mockServer.URL).
          With(env.CI, "true").
          With(env.DisableEvents, "1").
          With(env.AuthToken, "host-token"),
      "start")

With env.With we inject the env vars explicitly into the subprocess env without mutating the test process env, it is cleaner and consistent with how other tests work.


t.Run("https port", func(t *testing.T) {
httpsBindings := inspect.HostConfig.PortBindings[nat.Port("443/tcp")]
require.NotEmpty(t, httpsBindings, "port 443/tcp should be bound")
assert.Equal(t, "443", httpsBindings[0].HostPort)
})
mockServer := createMockLicenseServer(true)
defer mockServer.Close()

t.Run("volume mount", func(t *testing.T) {
assert.True(t, hasBindTarget(inspect.HostConfig.Binds, "/var/lib/localstack"),
"expected volume bind mount to /var/lib/localstack, got: %v", inspect.HostConfig.Binds)
})
ctx := testContext(t)
_, stderr, err := runLstk(t, ctx, "", env.With(env.APIEndpoint, mockServer.URL), "start")
require.NoError(t, err, "lstk start failed: %s", stderr)
requireExitCode(t, 0, err)

t.Run("http health endpoint", func(t *testing.T) {
resp, err := http.Get("http://localhost.localstack.cloud:4566/_localstack/health")
require.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
})
inspect, err := dockerClient.ContainerInspect(ctx, containerName)
require.NoError(t, err, "failed to inspect container")
require.True(t, inspect.State.Running)

t.Run("https health endpoint", func(t *testing.T) {
// LS certificate is not in system trust store
// But cert validity is out of scope here: use InsecureSkipVerify
client := &http.Client{
Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
},
}
resp, err := client.Get("https://localhost.localstack.cloud/_localstack/health")
require.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, http.StatusOK, resp.StatusCode)
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: why are we removing all these subtests?

  • docker socket mount
  • service port range
  • main port
  • https port
  • volume mount
  • http health endpoint
  • https health endpoint

envVars := containerEnvToMap(inspect.Config.Env)
assert.Equal(t, "true", envVars["CI"])
assert.Equal(t, "1", envVars["LOCALSTACK_DISABLE_EVENTS"])
assert.NotEmpty(t, envVars["LOCALSTACK_AUTH_TOKEN"])
assert.NotEqual(t, "host-token", envVars["LOCALSTACK_AUTH_TOKEN"], "host LOCALSTACK_AUTH_TOKEN should not be passed through")
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: can we cover the new functionality with an integration test? It could set CI and another LOCALSTACK_* env var, and make sure it's passed to the container. In case we change the value of CI, let's reset it to its original value after the test is done.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made an attempt to resolve in 2d8e0ec.


// containerEnvToMap converts a Docker container's []string env to a map.
Expand All @@ -236,17 +201,6 @@ func containerEnvToMap(envList []string) map[string]string {
return m
}

// hasBindTarget checks if any bind mount targets the given container path.
func hasBindTarget(binds []string, containerPath string) bool {
for _, b := range binds {
parts := strings.Split(b, ":")
if len(parts) >= 2 && parts[1] == containerPath {
return true
}
}
return false
}

func cleanup() {
ctx := context.Background()
_ = dockerClient.ContainerStop(ctx, containerName, container.StopOptions{})
Expand Down
Loading