Validate license before starting container#22
Validate license before starting container#22carole-lavillonniere wants to merge 2 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughCommands now construct and pass a PlatformClient into auth and container startup. Container.Start inspects image versions, validates licenses via PlatformAPI.GetLicense before starting containers. New API types and license request flow added; tests and env.example updated for LOCALSTACK_API_ENDPOINT and license checks. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI/Command
participant Container as Container.Start()
participant Platform as PlatformClient
participant API as License API
CLI->>Container: Start(ctx, rt, platformClient, onProgress)
Container->>Container: Pull images
Container->>Container: Resolve image versions (GetImageVersion)
Container->>Platform: GetLicense(ctx, LicenseRequest)
Platform->>API: POST /v1/license/request
API-->>Platform: 200 / 400 / 403
alt 200 OK
Platform-->>Container: nil
Container->>Container: Start containers
Container-->>CLI: Initialization complete
else 400/403
Platform-->>Container: error
Container-->>CLI: Initialization failed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
carole-lavillonniere
force-pushed
the
carole/drg-444
branch
from
dfd7fcf to
b26a81f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/integration/license_test.go`:
- Around line 40-45: The HTTP handler uses assert calls and type assertions that
can panic in a separate goroutine; create a validationErrors channel (e.g.,
validationErrors := make(chan error, 1)) and inside the handler replace assert.*
calls and direct type assertions on req with safe checks (use the comma-ok form
or type switches for req["product"] and req["credentials"], compare values
against authToken, and on any mismatch send a descriptive error to
validationErrors instead of calling assert). After the request completes in the
test, perform a non-blocking select on validationErrors (e.g., select { case err
:= <-validationErrors: t.Fatalf("request validation failed: %v", err) default:
}) to fail the test if the handler reported an error.
In `@test/integration/login_device_flow_test.go`:
- Line 146: The test currently removes LOCALSTACK_API_ENDPOINT causing
NewPlatformClient() to hit the real https://api.localstack.cloud; instead create
a local mock HTTP server (e.g., httptest.Server) that returns {"confirmed":
false} for the platform check, then set cmd.Env to include
LOCALSTACK_API_ENDPOINT pointing to that mock server URL (rather than removing
it) so NewPlatformClient() uses the local endpoint; update the test to
start/close the mock server around the test and ensure the mock handler returns
the deterministic failure response referenced by NewPlatformClient().
🧹 Nitpick comments (5)
env.example (1)
2-3: Consider adding comments to document these variables.The new environment variables lack documentation explaining their purpose and when they're needed. Adding brief comments would improve clarity for users setting up the environment.
📝 Suggested documentation enhancement
export LOCALSTACK_AUTH_TOKEN=ls-... +# Platform API endpoint for license validation export LOCALSTACK_API_ENDPOINT=https://api.staging.aws.localstack.cloud +# Web application URL for the LocalStack platform export LOCALSTACK_WEB_APP_URL=https://app.staging.aws.localstack.cloudinternal/config/config.go (1)
67-73: Consider reusing ProductName() inside Image() to avoid duplicate lookups.♻️ Suggested refactor
func (c *ContainerConfig) Image() (string, error) { - productName, ok := emulatorImages[c.Type] - if !ok { - return "", fmt.Errorf("%s emulator not supported yet by lstk", c.Type) - } + productName, err := c.ProductName() + if err != nil { + return "", err + } tag := c.Tag if tag == "" { tag = "latest" } return fmt.Sprintf("localstack/%s:%s", productName, tag), nil }internal/auth/auth.go (1)
18-26: Guard against nil platformClient to avoid runtime panics.🛡️ Suggested guard
func New(platformClient api.PlatformAPI) (*Auth, error) { + if platformClient == nil { + return nil, errors.New("platform client is required") + } kr, err := newSystemKeyring() if err != nil { return nil, err }test/integration/login_browser_flow_test.go (1)
63-64: Hardcoded sleep is fragile for synchronization.Using
time.Sleep(1 * time.Second)to wait for the callback server may cause flaky tests if startup is delayed. Consider polling the endpoint until it responds or using a ready signal.♻️ Suggested improvement using polling
- // Wait for callback server to be ready - time.Sleep(1 * time.Second) + // Wait for callback server to be ready by polling + for i := 0; i < 10; i++ { + conn, err := net.Dial("tcp", "127.0.0.1:45678") + if err == nil { + conn.Close() + break + } + time.Sleep(100 * time.Millisecond) + }internal/api/client.go (1)
221-230: Consider including server error details in failure messages.The hardcoded error messages for 400/403 don't include any details from the response body. If the server provides diagnostic information, it would be helpful for debugging.
♻️ Optional: Read error response body
switch resp.StatusCode { case http.StatusOK: return nil case http.StatusBadRequest: + body, _ := io.ReadAll(resp.Body) + if len(body) > 0 { + return fmt.Errorf("license validation failed: %s", string(body)) + } return fmt.Errorf("license validation failed: invalid token format, missing license assignment, or missing subscription") case http.StatusForbidden: + body, _ := io.ReadAll(resp.Body) + if len(body) > 0 { + return fmt.Errorf("license validation failed: %s", string(body)) + } return fmt.Errorf("license validation failed: invalid, inactive, or expired authentication token or subscription") default: return fmt.Errorf("license request failed with status %d", resp.StatusCode) }
carole-lavillonniere
force-pushed
the
carole/drg-444
branch
from
b26a81f to
8fa996c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/integration/login_browser_flow_test.go (1)
73-76:⚠️ Potential issue | 🟠 MajorTest design conflates login flow with container startup, causing CI failure.
The pipeline failure indicates the test fails because
localstack/localstack-pro:latestcannot be pulled in the CI environment. The test expects both "Login successful" and "License activation failed" messages, but the actual failure point is during image pulling, not license validation.If the goal is to test the browser login flow and token storage, consider stopping after verifying the token is stored, rather than letting the flow proceed to container startup. Alternatively, mock the runtime/container layer to avoid real image pulls.
🛠️ Suggested approach: Focus the test on login flow only
Consider restructuring the test to either:
- Mock the container runtime to avoid image pulls, or
- End the test after login succeeds and token is stored, before container startup begins
- // Login should succeed, but container will fail with invalid token - assert.Contains(t, string(out), "Login successful") - assert.Contains(t, string(out), "License activation failed") + // Verify login succeeded (container startup may fail in CI without images) + assert.Contains(t, string(out), "Login successful")test/integration/login_device_flow_test.go (1)
121-129:⚠️ Potential issue | 🟠 MajorAdd retry loop before asserting container is running. The container creation is asynchronous; the CLI command completes before the container is fully started, causing the "No such container: localstack-aws" failure. The context already has a 2-minute timeout available, so a short retry with 30-second deadline and 500ms polling is appropriate.
🔧 Suggested retry to reduce flakiness
- inspect, err := dockerClient.ContainerInspect(ctx, containerName) - require.NoError(t, err) - assert.True(t, inspect.State.Running, "container should be running") + deadline := time.Now().Add(30 * time.Second) + for { + inspect, err := dockerClient.ContainerInspect(ctx, containerName) + if err == nil && inspect.State.Running { + break + } + if time.Now().After(deadline) { + t.Fatalf("container should be running; last error: %v", err) + } + time.Sleep(500 * time.Millisecond) + }
🤖 Fix all issues with AI agents
In `@test/integration/license_test.go`:
- Around line 21-84: Tests fail because images are pulled in the startup path
before license validation; move or gate the image-pull logic so license
validation occurs first. In the startup flow in start.go, change the sequence
where images are pulled (the pullImages / ensureImagesPulled call inside the
Start/StartContainers flow) to first call the license validation routine
(validateLicenseForTag or the existing license validation code path) for each
relevant image tag (including resolving 'latest' to a check without attempting a
full pull), or add a runtime flag/mocker hook that skips real pulls during
tests; update the start sequence to only pull images after licenses are
validated or when a skip-pull/test-mode flag is not set so tests can run without
network image pulls.
🧹 Nitpick comments (4)
internal/config/config.go (1)
67-73: Consider extracting the common lookup logic.
ProductName()andImage()both perform identical map lookups with the same error handling. This is minor duplication but could be consolidated.♻️ Optional: Extract common lookup
+func (c *ContainerConfig) productName() (string, error) { + name, ok := emulatorImages[c.Type] + if !ok { + return "", fmt.Errorf("%s emulator not supported yet by lstk", c.Type) + } + return name, nil +} + func (c *ContainerConfig) Image() (string, error) { - productName, ok := emulatorImages[c.Type] - if !ok { - return "", fmt.Errorf("%s emulator not supported yet by lstk", c.Type) + productName, err := c.productName() + if err != nil { + return "", err } tag := c.Tag if tag == "" { tag = "latest" } return fmt.Sprintf("localstack/%s:%s", productName, tag), nil } func (c *ContainerConfig) ProductName() (string, error) { - productName, ok := emulatorImages[c.Type] - if !ok { - return "", fmt.Errorf("%s emulator not supported yet by lstk", c.Type) - } - return productName, nil + return c.productName() }internal/container/start.go (1)
122-122: Silently ignoringos.Hostname()error.The error from
os.Hostname()is discarded, resulting in an empty hostname if it fails. This may be intentional for simplicity, but consider logging the error for debugging purposes.♻️ Optional: Log hostname retrieval failure
- hostname, _ := os.Hostname() + hostname, err := os.Hostname() + if err != nil { + log.Printf("Could not retrieve hostname: %v", err) + }test/integration/login_browser_flow_test.go (1)
25-39: Mock server is incomplete for the browser login flow.The mock server returns an
exchange_tokenin the auth request response, but doesn't handle the token exchange endpoint (/v1/auth/request/{id}/exchange). The browser callback stores the token directly via the callback URL, bypassing this, but having a more complete mock would improve test reliability and documentation of the expected flow.Additionally, the license endpoint returns
StatusOK, which contradicts the test's expectation of "License activation failed" at line 75.cmd/root.go (1)
22-38: Code duplication betweenrootCmd.RunandstartCmd.Run.The
Runfunction here is nearly identical tostartCmd.Runincmd/start.go(lines 17-33). Both create a Docker runtime, platform client, progress callback, and callcontainer.Start. Consider extracting this shared logic into a helper function to reduce duplication and ensure both entry points stay in sync.♻️ Suggested refactor to extract shared startup logic
// In a shared location, e.g., cmd/helpers.go func runStart(ctx context.Context) error { rt, err := runtime.NewDockerRuntime() if err != nil { return err } platformClient := api.NewPlatformClient() onProgress := func(msg string) { fmt.Println(msg) } return container.Start(ctx, rt, platformClient, onProgress) }Then both commands can call:
if err := runStart(cmd.Context()); err != nil { fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) }
carole-lavillonniere
force-pushed
the
carole/drg-444
branch
3 times, most recently
from
42b7cf1 to
8adb3bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@internal/auth/auth.go`:
- Around line 18-26: The constructor New should guard against a nil
platformClient to fail fast and avoid panics when browserLogin is later used;
update New to check if platformClient == nil and return a descriptive error
(instead of creating browserLogin) so callers get an immediate error,
referencing the New function, the Auth struct, browserLogin field, and
newBrowserLogin helper when implementing the check.
In `@internal/container/start.go`:
- Around line 122-135: The license request currently sets
Machine.PlatformRelease to stdruntime.GOARCH (CPU arch) instead of the OS
release; change it to call a helper that returns the OS release string (e.g.,
getOSRelease()) and assign that to api.MachineInfo.PlatformRelease when building
licenseReq. Implement getOSRelease() to detect runtime.GOOS and return the OS
release appropriately (for Unix-like systems run "uname -r" or use syscall/unix
uname, for Windows use the appropriate Windows API/registry call), and ensure
any errors fall back to an empty string or a safe default; update the
construction in start.go to use getOSRelease() instead of stdruntime.GOARCH.
In `@test/integration/login_browser_flow_test.go`:
- Around line 43-46: The child process may receive a duplicate
LOCALSTACK_API_ENDPOINT if the parent env already contains it; update the
envWithout call used to build cmd.Env so it removes both keys before appending
the single override. Replace envWithout("LOCALSTACK_AUTH_TOKEN") with
envWithout("LOCALSTACK_AUTH_TOKEN", "LOCALSTACK_API_ENDPOINT") so cmd.Env =
append(envWithout(...), "LOCALSTACK_API_ENDPOINT="+mockServer.URL) to ensure
only the intended endpoint is present.
8adb3bc to
cc9889b
Compare
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
Looks good, just some minor nits :)
internal/config/config.go
Outdated
| tag = "latest" | ||
| } | ||
| return fmt.Sprintf("%s:%s", baseImage, tag), nil | ||
| return fmt.Sprintf("localstack/%s:%s", productName, tag), nil |
There was a problem hiding this comment.
nit: maybe lets put the "localstack" somewhere as a const named vendor or just localstack to avoid typos
| return path, nil | ||
| } | ||
|
|
||
| func (c *ContainerConfig) ProductName() (string, error) { |
There was a problem hiding this comment.
q: did we check that the other products are actually binding their product name to the image name? I think for snowflake it might not be just snowflake
There was a problem hiding this comment.
How about we do this tiny refactoring later when we need to?
internal/container/start.go
Outdated
| if err != nil { | ||
| log.Printf("Could not resolve version from image %s: %v", containerConfig.Image, err) | ||
| if version == "" { | ||
| version = "latest" |
There was a problem hiding this comment.
q: does this now work after all or is this just a placeholder until it does?
There was a problem hiding this comment.
No that's just me committing old code 🙈 9e84b5d
| authToken := os.Getenv("LOCALSTACK_AUTH_TOKEN") | ||
| require.NotEmpty(t, authToken, "LOCALSTACK_AUTH_TOKEN must be set to run this test") |
There was a problem hiding this comment.
nit: would swap those two lines to have all the preconditions together at the start :D
There was a problem hiding this comment.
They cannot be swapped since line 24 relies on line 23, or did you mean something else?
There was a problem hiding this comment.
🤦🏼 sorry, misread it
Anyways, we could use os.LookupEnv here instead because it doesn't swallow away the result of the syscall. Would be more accurate I guess. I'll leave it up to you
// Getenv retrieves the value of the environment variable named by the key.
// It returns the value, which will be empty if the variable is not present.
// To distinguish between an empty value and an unset value, use [LookupEnv].
func Getenv(key string) string {
testlog.Getenv(key)
v, _ := syscall.Getenv(key)
return v
}
// LookupEnv retrieves the value of the environment variable named
// by the key. If the variable is present in the environment the
// value (which may be empty) is returned and the boolean is true.
// Otherwise the returned value will be empty and the boolean will
// be false.
func LookupEnv(key string) (string, bool) {
testlog.Getenv(key)
return syscall.Getenv(key)
}
There was a problem hiding this comment.
What would you suggest we do with the bool returned?
There was a problem hiding this comment.
I think with the bool we know specifically if it's not set, with getting it and checking it could either be not set or just set to empty. probably irrelevant
| validationErrors := make(chan error, 1) | ||
|
|
||
| // Mock platform API that returns success | ||
| mockServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { |
| } | ||
|
|
||
| func envWithoutAuthToken() []string { | ||
| func envWithout(keys ...string) []string { |
carole-lavillonniere
force-pushed
the
carole/drg-444
branch
from
cc9889b to
d6c4f3e
Compare
silv-io
left a comment
silv-io
left a comment
There was a problem hiding this comment.
LGTM! Just need to address the merge conflicts now 😬
2 participants
The CLI now validates licenses upfront via the platform API, providing clear error messages if validation fails, rather than waiting for the container to fail during startup.
This should be further improved in the future by resolving the version for the
latesttag without having to pull the images (DRG-504, DRG-508).