-
Notifications
You must be signed in to change notification settings - Fork 2
Validate license before starting container #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,17 +4,20 @@ import ( | |||||
| "context" | ||||||
| "fmt" | ||||||
| "net/http" | ||||||
| "os" | ||||||
| stdruntime "runtime" | ||||||
| "time" | ||||||
|
|
||||||
| "github.com/containerd/errdefs" | ||||||
| "github.com/localstack/lstk/internal/api" | ||||||
| "github.com/localstack/lstk/internal/auth" | ||||||
| "github.com/localstack/lstk/internal/config" | ||||||
| "github.com/localstack/lstk/internal/output" | ||||||
| "github.com/localstack/lstk/internal/runtime" | ||||||
| ) | ||||||
|
|
||||||
| func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink) error { | ||||||
| a, err := auth.New(sink) | ||||||
| func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink, platformClient api.PlatformAPI) error { | ||||||
| a, err := auth.New(sink, platformClient) | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("failed to initialize auth: %w", err) | ||||||
| } | ||||||
|
|
@@ -50,6 +53,7 @@ func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink) error { | |||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Pull all images first | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
🤖 Prompt for AI Agents |
||||||
| for _, config := range containers { | ||||||
| // Remove any existing stopped container with the same name | ||||||
| if err := rt.Remove(ctx, config.Name); err != nil && !errdefs.IsNotFound(err) { | ||||||
|
|
@@ -66,7 +70,18 @@ func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink) error { | |||||
| if err := rt.PullImage(ctx, config.Image, progress); err != nil { | ||||||
| return fmt.Errorf("failed to pull image %s: %w", config.Image, err) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // TODO validate license for tag "latest" without resolving the actual image version, | ||||||
| // and avoid pulling all images first | ||||||
| for i, c := range cfg.Containers { | ||||||
| if err := validateLicense(ctx, rt, sink, platformClient, containers[i], &c, token); err != nil { | ||||||
| return err | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Start containers | ||||||
| for _, config := range containers { | ||||||
| output.EmitStatus(sink, "starting", config.Name, "") | ||||||
| containerID, err := rt.Start(ctx, config) | ||||||
| if err != nil { | ||||||
|
|
@@ -85,6 +100,45 @@ func Start(ctx context.Context, rt runtime.Runtime, sink output.Sink) error { | |||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| func validateLicense(ctx context.Context, rt runtime.Runtime, sink output.Sink, platformClient api.PlatformAPI, containerConfig runtime.ContainerConfig, cfgContainer *config.ContainerConfig, token string) error { | ||||||
| version := cfgContainer.Tag | ||||||
| if version == "" || version == "latest" { | ||||||
| actualVersion, err := rt.GetImageVersion(ctx, containerConfig.Image) | ||||||
| if err != nil { | ||||||
| return fmt.Errorf("could not resolve version from image %s: %w", containerConfig.Image, err) | ||||||
| } | ||||||
| version = actualVersion | ||||||
| } | ||||||
|
|
||||||
| productName, err := cfgContainer.ProductName() | ||||||
| if err != nil { | ||||||
| return err | ||||||
| } | ||||||
| output.EmitStatus(sink, "validating license", containerConfig.Name, version) | ||||||
|
|
||||||
| hostname, _ := os.Hostname() | ||||||
| licenseReq := &api.LicenseRequest{ | ||||||
|
Comment on lines
+119
to
+120
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||||||
| Product: api.ProductInfo{ | ||||||
| Name: productName, | ||||||
| Version: version, | ||||||
| }, | ||||||
| Credentials: api.CredentialsInfo{ | ||||||
| Token: token, | ||||||
| }, | ||||||
| Machine: api.MachineInfo{ | ||||||
| Hostname: hostname, | ||||||
| Platform: stdruntime.GOOS, | ||||||
| PlatformRelease: stdruntime.GOARCH, | ||||||
| }, | ||||||
carole-lavillonniere marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| } | ||||||
|
|
||||||
| if err := platformClient.GetLicense(ctx, licenseReq); err != nil { | ||||||
| return fmt.Errorf("license validation failed for %s:%s: %w", productName, version, err) | ||||||
| } | ||||||
|
|
||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| // awaitStartup polls until one of two outcomes: | ||||||
| // - Success: health endpoint returns 200 (license is valid, LocalStack is ready) | ||||||
| // - Failure: container stops running (e.g., license activation failed), returns error with container logs | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
snowflakeThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about we do this tiny refactoring later when we need to?