Validate license before starting container#22
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces license validation into the LocalStack CLI startup and authentication flows. It adds platformClient dependency injection across cmd and auth packages, extends the platform API with license request functionality, and integrates license validation checks into the container startup process before containers begin running. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (cmd/root.go)
participant Container as Container.Start()
participant Docker as DockerRuntime
participant PlatformAPI as PlatformAPI Client
CLI->>Container: Start(ctx, rt, sink, platformClient)
Container->>Docker: Pull all images
Docker-->>Container: Images ready
Container->>Docker: GetImageVersion(imageName)
Docker-->>Container: version (from LOCALSTACK_BUILD_VERSION)
Container->>Container: validateLicense(ctx, rt, sink, platformClient, ...)
Container->>PlatformAPI: GetLicense(ctx, LicenseRequest{product, credentials, machine})
alt License Valid
PlatformAPI-->>Container: 200 OK, license accepted
Container->>Docker: Start containers
Docker-->>Container: Containers running
else License Invalid/Forbidden
PlatformAPI-->>Container: 400/403 error
Container-->>CLI: Error (license validation failed)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 😬
carole-lavillonniere
force-pushed
the
carole/drg-444
branch
from
9e84b5d to
abcc682
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/container/start.go`:
- Line 56: Replace the terse comment "Pull all images first" with a why-focused
comment explaining that images are pulled up-front to resolve image tags/digests
and ensure the exact images are available before creating containers (preventing
container creation failures or non-deterministic runs due to late tag resolution
or network issues); update the comment located at the "Pull all images first"
line in internal/container/start.go so it explicitly mentions resolving
tags/digests and avoiding create-time failures.
- Around line 119-120: The call to os.Hostname() currently ignores its error;
update the code around the hostname variable so you capture the error (e.g.,
hostname, err := os.Hostname()), check err, and handle it: either return/surface
the error from the current function or log a warning (using the existing logger)
and set a safe fallback like "unknown-host" before building licenseReq; ensure
the log includes the error value and keep using hostname when constructing the
LicenseRequest.
🧹 Nitpick comments (2)
test/integration/start_test.go (1)
93-96: Consider excludingLOCALSTACK_API_ENDPOINTfrom the base environment.If the parent environment already contains
LOCALSTACK_API_ENDPOINT, the duplicate entry may cause the wrong endpoint to be used. UseenvWithoutto ensure only the intended endpoint is present.Suggested fix
cmd.Env = append(os.Environ(), +cmd.Env = append( + envWithout("LOCALSTACK_API_ENDPOINT"), "LOCALSTACK_AUTH_TOKEN=invalid-token", "LOCALSTACK_API_ENDPOINT="+mockServer.URL, )test/integration/license_test.go (1)
40-53: Also assert the product version in the mock request.Right now the handler only checks the product name; an empty or missing version would still pass, which weakens coverage for the “latest” resolution path.
🔧 Suggested test tightening
- product, ok := req["product"].(map[string]interface{}) - if !ok || product["name"] != "localstack-pro" { + product, ok := req["product"].(map[string]interface{}) + version, vok := product["version"].(string) + if !ok || product["name"] != "localstack-pro" || !vok || version == "" { validationErrors <- fmt.Errorf("invalid product field") http.Error(w, "invalid product", http.StatusBadRequest) return }
| } | ||
| } | ||
|
|
||
| // Pull all images first |
There was a problem hiding this comment.
Make the “Pull all images first” comment explain the why.
The current comment restates the code; consider adding the rationale (e.g., needed to resolve tags) so it’s not just descriptive.
✍️ Example rewording
- // Pull all images first
+ // Pull images first so tag resolution (e.g., "latest") can read local image metadata before license validation.As per coding guidelines, "Avoid adding comments for self-explanatory code; only comment when the 'why' isn't obvious from the code itself".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Pull all images first | |
| // Pull images first so tag resolution (e.g., "latest") can read local image metadata before license validation. |
🤖 Prompt for AI Agents
In `@internal/container/start.go` at line 56, Replace the terse comment "Pull all
images first" with a why-focused comment explaining that images are pulled
up-front to resolve image tags/digests and ensure the exact images are available
before creating containers (preventing container creation failures or
non-deterministic runs due to late tag resolution or network issues); update the
comment located at the "Pull all images first" line in
internal/container/start.go so it explicitly mentions resolving tags/digests and
avoiding create-time failures.
| hostname, _ := os.Hostname() | ||
| licenseReq := &api.LicenseRequest{ |
There was a problem hiding this comment.
Handle os.Hostname() errors instead of discarding them.
This function returns an error; it should be checked and either surfaced or downgraded to a warning with a safe fallback.
✅ Suggested handling
- hostname, _ := os.Hostname()
+ hostname, err := os.Hostname()
+ if err != nil {
+ output.EmitWarning(sink, fmt.Sprintf("failed to read hostname: %v", err))
+ hostname = ""
+ }As per coding guidelines, "Errors returned by functions should always be checked unless in test files".
🤖 Prompt for AI Agents
In `@internal/container/start.go` around lines 119 - 120, The call to
os.Hostname() currently ignores its error; update the code around the hostname
variable so you capture the error (e.g., hostname, err := os.Hostname()), check
err, and handle it: either return/surface the error from the current function or
log a warning (using the existing logger) and set a safe fallback like
"unknown-host" before building licenseReq; ensure the log includes the error
value and keep using hostname when constructing the LicenseRequest.
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).