feat(Cubemaster): opt-in envd injection for image-based templates#626
feat(Cubemaster): opt-in envd injection for image-based templates#6260717dn6hyc8-byte wants to merge 3 commits into
Conversation
0717dn6hyc8-byte
requested review from
chenhengqi,
fslongjin,
ls-ggg,
tinklone and
up2wing
as code owners
Review Summary: PR #626 — Envd Sidecar InjectionThis PR adds support for injecting an envd sidecar binary into template rootfs images. Overall the code is well-structured with good separation of concerns and decent test coverage. Below are the noteworthy findings. ♠ 📦 Medium: Unsynchronized envd binary path between injection and wrapper** envd_inject.go (lines 25-26)** hardcodes the destination path as If an operator sets Recommendation: Use a single source of truth. Move 🔟 Medium: Silently swallowed envd SHA error in fingerprintfingerprint.go (lines 49-53): if shouldInjectEnvdIntoTemplate(req) {
if sha, err := envdBinarySHA256(); err == nil {
envdSHA = sha
}
}When Recommendation: Log a warning when the SHA computation fails. 🔟 Low: Misleading log message in envd_sidecar.go (line 52)The log message says "baked into template rootfs", but 🚢 Info: Redundant envd binary reads during fingerprint computation
🚢 Info: Test coverage gaps
Reviews contributed by: code-quality, security, test-coverage, performance, and documentation-accuracy reviewers. |
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/u/20101770?s=60&v=4){kind=small}
| envdHostDirDefault = "/usr/local/share/cubesandbox-envd" | ||
| envdHostDirEnv = "CUBE_MASTER_ENVD_HOST_DIR" | ||
| envdBinaryName = "envd" | ||
| envdInImageDir = "/usr/local/bin" |
There was a problem hiding this comment.
Path desynchronization risk: This hardcodes the in-image destination as /usr/local/bin/envd. However, envd_sidecar.go:20-21 defines a separate envdInImagePathDefault + envdInImagePathEnv (CUBE_MASTER_ENVD_IN_IMAGE_PATH) for where the wrapper expects the binary. If an operator overrides the env var, the wrapper references a path where no binary was injected.
Consider having both packages read from the same configurable function (e.g., a shared helper in constants) to prevent silent runtime failures.
| CubeEgressCAFingerprint string `json:"cube_egress_ca_fingerprint,omitempty"` | ||
| EnvdBinarySHA256 string `json:"envd_binary_sha256,omitempty"` | ||
| } | ||
| envdSHA := "" |
There was a problem hiding this comment.
Silently discarded error: When envdBinarySHA256() fails, the error is discarded and envdSHA stays empty. The fingerprint is computed without the envd hash, so a subsequent envd binary change won't invalidate the artifact cache. The later injectEnvdIntoRootfs call will fail if the binary is missing, but only after the fingerprint has already been used for artifact deduplication.
Consider logging a warning at minimum so operators can detect this inconsistency.
| } | ||
| wrapEntrypointForEnvd(mainContainer) | ||
| out.ExposedPorts = append(out.ExposedPorts, envdPort) | ||
| log.G(ctx).Infof("envd: wrapped entrypoint of container %q (envd path=%s, baked into template rootfs)", mainContainer.Name, envdInImagePath()) |
There was a problem hiding this comment.
Misleading log message: This function (injectEnvdSidecar) runs at sandbox-creation time and only wraps the container entrypoint — the binary was already baked into the rootfs during template build (in envd_inject.go). The phrase "baked into template rootfs" would mislead operators debugging issues into thinking this function mutates the rootfs. Consider rewording to "pre-baked at build time" or simply dropping the "baked" qualifier.
| envdHostDirDefault = "/usr/local/share/cubesandbox-envd" | ||
| envdHostDirEnv = "CUBE_MASTER_ENVD_HOST_DIR" | ||
| envdBinaryName = "envd" | ||
| envdInImageDir = "/usr/local/bin" |
There was a problem hiding this comment.
Path desynchronization risk: This hardcodes the in-image destination as /usr/local/bin/envd. However, envd_sidecar.go:20-21 defines a separate envdInImagePathDefault + envdInImagePathEnv (CUBE_MASTER_ENVD_IN_IMAGE_PATH) for where the wrapper expects the binary. If an operator overrides the env var, the wrapper references a path where no binary was injected.
Consider having both packages read from the same configurable function (e.g., a shared helper in constants) to prevent silent runtime failures.
| CubeEgressCAFingerprint string `json:"cube_egress_ca_fingerprint,omitempty"` | ||
| EnvdBinarySHA256 string `json:"envd_binary_sha256,omitempty"` | ||
| } | ||
| envdSHA := "" |
There was a problem hiding this comment.
Silently discarded error: When envdBinarySHA256() fails, the error is discarded and envdSHA stays empty. The fingerprint is computed without the envd hash, so a subsequent envd binary change won't invalidate the artifact cache. The later injectEnvdIntoRootfs call will fail if the binary is missing, but only after the fingerprint has already been used for artifact deduplication.
Consider logging a warning at minimum so operators can detect this inconsistency.
| } | ||
| wrapEntrypointForEnvd(mainContainer) | ||
| out.ExposedPorts = append(out.ExposedPorts, envdPort) | ||
| log.G(ctx).Infof("envd: wrapped entrypoint of container %q (envd path=%s, baked into template rootfs)", mainContainer.Name, envdInImagePath()) |
There was a problem hiding this comment.
Misleading log message: This function (injectEnvdSidecar) runs at sandbox-creation time and only wraps the container entrypoint — the binary was already baked into the rootfs during template build (in envd_inject.go). The phrase "baked into template rootfs" would mislead operators debugging issues into thinking this function mutates the rootfs. Consider rewording to "pre-baked at build time" or simply dropping the "baked" qualifier.
| envdHostDirEnv = "CUBE_MASTER_ENVD_HOST_DIR" | ||
| envdBinaryName = "envd" | ||
| envdInImageDir = "/usr/local/bin" | ||
| envdInImagePath = "/usr/local/bin/envd" |
There was a problem hiding this comment.
The destination path /usr/local/bin/envd is defined here as envdInImagePath and also duplicated in envd_sidecar.go:20 as envdInImagePathDefault. There's also envdInImagePath() in envd_sidecar.go which supports an env-var override (CUBE_MASTER_ENVD_IN_IMAGE_PATH), but the injection side (envd_inject.go) doesn't read that same env var — it hardcodes the path.
If an operator sets CUBE_MASTER_ENVD_IN_IMAGE_PATH=/opt/envd/bin/envd, the sidecar wrapper will reference /opt/envd/bin/envd, but the binary was copied to /usr/local/bin/envd (the hardcoded path). The envd sidecar will silently fail to start at runtime.
Suggestion: Move the path resolution to a shared package or define envdInImagePath() in a common location that both packages import, so the injection and sidecar both use the same configurable default + env-var override logic.
| envdSHA := "" | ||
| if shouldInjectEnvdIntoTemplate(req) { | ||
| if sha, err := envdBinarySHA256(req); err == nil { | ||
| envdSHA = sha | ||
| } |
There was a problem hiding this comment.
When envdBinarySHA256(req) fails (e.g. binary missing or permission error), the error is silently discarded and envdSHA stays empty. The fingerprint is then computed without the envd hash component, so the artifact cache could be incorrectly reused when the envd binary changes. This silently produces a cache hit for a build that should produce different output.
At minimum, log a warning when this fails. If the envd binary is required for this build (i.e., shouldInjectEnvdIntoTemplate is true), consider failing the build instead.
| func envdBinarySHA256(req *types.CreateTemplateFromImageReq) (string, error) { | ||
| src, err := os.Open(envdHostPath(req)) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| defer src.Close() | ||
| hasher := sha256.New() | ||
| if _, err := io.Copy(hasher, src); err != nil { | ||
| return "", err | ||
| } | ||
| return hex.EncodeToString(hasher.Sum(nil)), nil |
There was a problem hiding this comment.
This function reads the envd binary from disk again to compute its SHA256. The binary is already read (and hashed via io.TeeReader) during injectEnvdIntoRootfs. That hash is returned but only used in the log message (artifact_build.go:171 ignores it with _).
This means the envd binary is read from disk 3 times per template build: once via buildTemplateSpecFingerprintWithCA -> envdBinarySHA256, again in ensureRootfsArtifact path, and a third time during the actual copy in injectEnvdIntoRootfs.
Consider threading the SHA256 hash from injectEnvdIntoRootfs back to the fingerprint computation, or computing it once at the top of runTemplateImageJob and passing it through.
| return envdInImagePathDefault | ||
| } | ||
|
|
||
| func injectEnvdSidecar(ctx context.Context, req *types.CreateCubeSandboxReq, out *cubebox.RunCubeSandboxRequest) error { |
There was a problem hiding this comment.
The function signature declares error as a return value, but every return path returns nil. The call site in util.go:159 treats it as fallible, wrapping the error. Consider removing the error return to match the implementation, or make it fallible if there's a planned future use.
| srcPath := envdHostPath(req) | ||
| src, err := os.Open(srcPath) | ||
| if err != nil { | ||
| return "", fmt.Errorf("envd-inject: open %q (set %s to override): %w", srcPath, envdHostDirEnv, err) |
There was a problem hiding this comment.
The error message says "(set CUBE_MASTER_ENVD_HOST_DIR to override)" but the primary user-facing override is --envd-path CLI flag, which flows through req.EnvdPath and is checked before the env var in envdHostPath(). If a user passes --envd-path pointing to a missing binary, this error suggests setting an environment variable instead.
Consider detecting which source was used and mentioning it in the error, or expanding the message to mention both override mechanisms.
0717dn6hyc8-byte
changed the title
0717dn6hyc8-byte
changed the title
0717dn6hyc8-byte
changed the title
|
I want to understand the motivation behind this PR and under what scenarios it is necessary. I feel that this goes against common usage—since it's built from a container image, why would we need to additionally inject an envd? This will also affect the container's startup commands and such. I find the design of this PR very strange. |
For example, in development-environment / code-sandbox scenarios, we want to quickly build interactive development sandboxes from existing base images. Therefore, this PR injects |
Indeed, you make a good point. From that perspective, automatically injecting envd is indeed a great idea! Thank you for your PR. I'll study the code in your PR over the next couple of days. |
2 participants
Summary
This PR introduces an optional envd injection path for templates created from OCI images. When enabled, CubeMaster embeds a host-side envd binary into the template root filesystem before sealing the ext4 artifact. It also records the binary's SHA256 in the template fingerprint and marks the generated template request, so restored sandboxes can start the embedded envd using an entrypoint wrapper.
By default, this injection feature is disabled to maintain the existing behavior for the community.
Motivation
Some CubeSandbox users need an envd process to be available automatically in restored sandboxes, so developer tooling can attach without requiring image authors to modify every base image manually.
What's changed
1. CLI opt-in controls
cubemastercli template create-from-image--enable-inject-envdto explicitly enableenvdinjection.cubemastercli template create-from-image--envd-pathto select the host-sideenvdbinary.If
--envd-pathis omitted, CubeMaster falls back to$CUBE_MASTER_ENVD_HOST_DIR/envd, then/usr/local/share/cubesandbox-envd/envd.2. Template rootfs injection
templatecenterenvdinto/usr/local/bin/envdinside the exported template rootfs before sealing the ext4 artifact.templatecentercube.master.inject_envd=trueis present in template container overrides.templatecenterenvdbinary fails the template build instead of producing an incomplete artifact.3. Artifact fingerprinting
templatecenterenvdbinary SHA256 in the template spec fingbled.templatecenterenvdinjection.This prevents reusing artifacts baked with an older
envdbinary after the selectedenvdversion changes.4. Runtime entrypoint wrapping
CubeMaster sandbox servicecube.master.inject_envd=true, wraps the main container command .CubeMaster sandbox service/usr/local/bin/envdin the background and thenexecs the original command.CubeMaster sandbox serviceenvdexposed port.CubeMaster sandbox serviceAssisted-by: Claude Code:ppio/pa/gpt-5.5