Skip to content

fix(sandbox): return volume mounts in sandbox info and list APIs#696

Open
xiaojunxiang2023 wants to merge 1 commit into
TencentCloud:masterfrom
xiaojunxiang2023:fix/host-mount-display
Open

fix(sandbox): return volume mounts in sandbox info and list APIs#696
xiaojunxiang2023 wants to merge 1 commit into
TencentCloud:masterfrom
xiaojunxiang2023:fix/host-mount-display

Conversation

@xiaojunxiang2023

@xiaojunxiang2023 xiaojunxiang2023 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Expose volume_mounts on Cubelet list Container responses and thread them through CubeMaster sandbox info/list into CubeAPI GET /sandboxes and list payloads.
  • Populate hostPath when injecting host-dir mounts so clients can see bind-mount source paths after create.
  • Extend SandboxVolumeMount with optional hostPath and readOnly; regenerate openapi.yml and web TypeScript schema; update SDK models and cubemastercli info output.

Problem

Host mounts are applied at create time via metadata["host-mount"], but sandbox detail/list APIs always returned volumeMounts: null. Web, CLI, and SDK callers could not inspect mounts on a running sandbox.

Test plan

  • go test ./services/cubebox -run TestToGRPCContainerIncludesVolumeMounts (Cubelet)
  • go test ./pkg/service/sandbox -run 'TestInjectHostDirMounts|TestCollectVolumeMounts' (CubeMaster)
  • go test ./cmd/cubemastercli/commands/cubebox -run TestPrintSandboxInfoBlockIncludesVolumeMounts (CLI)
  • cargo test map_volume_mounts (CubeAPI)

@cubesandboxbot

cubesandboxbot Bot commented Jul 1, 2026

Copy link
Copy Markdown

Review: PR #696 -- Expose volume mounts in sandbox info/list APIs

Overall a well-structured, multi-layer change that correctly propagates volume mount data from Cubelet up through CubeMaster and CubeAPI. The test coverage for the new functions is solid, and the code follows established patterns. Below are the findings I consider most noteworthy.


High Severity

1. hostPath disclosure to all API consumers (Security -- CWE-200)

The hostPath field of host-directory mounts is exposed to every authenticated caller of GET /sandboxes and GET /sandboxes/{id}, revealing host filesystem layout to tenants that did not create the mount. In a multi-tenant environment this leaks server-side paths.

Recommendation: Either redact hostPath from API responses (it is an implementation detail the sandbox consumer does not need), or add authorization so only the sandbox owner can see it.

2. ContainerInfo.VolumeMounts defined but never populated (Code quality)

ContainerInfo at types/types.go:513 has a VolumeMounts []*VolumeMountInfo field, but neither doget() (bcakofffice : sandbox_info.go) nor doOneList() (scandbox_list.go) populates it -- volume mounts are only set at the SandboxData level.

Recommendation: Either populate containerInfo.VolumeMounts in the loop at sandbox_info.go:158, or remove the field from ContainerInfo.

3. Rust map_volume_mounts() -- per-call heap allocations scale with list size (Performance)

map_volume_mounts() is called once per sandbox on every list/info API call. For each mount, every String field is cloned from an already-deserialized CubeVolumeMount into a SandboxVolumeMount.

Recommendation: Deserialize directly into SandboxVolumeMount using serde(alias), eliminating the intermediate type.


Medium Severity

4. Path traversal not blocked in injectHostDirMounts (Security -- CWE-22)
hostdir_mount.go validates only strings.HasPrefix(o.HostPath, "/") -- a hostPath of /tmp/../../../etc/passwd passes the check.

Recommendation: Use filepath.Clean() before the HasPrefix check and consider an allowlist of permitted parent directories.

5. Volume mount metadata visible across tenants in list API (Security -- CWE-862)
The list API returns volume mounts for all sandboxes on a node regardless of ownership.

Recommendation: Apply label/annotation-based access filtering to volume mount exposure, similar to matchFilter() in sandbox_list.go.

6. Go double iteration over mounts with intermediate allocation (Performance)
collectVolumeMountsFromContainers() and volumeMountsToContainerInfo() at volume_mounts.go:19-62 are always called as a pair, creating an intermediate slice.

Recommendation: Merge into a single function that filters and converts in one pass.

7. Shared pointer aliasing in injectHostDirMounts (Code quality)
The vm slice at hostdir_mount.go:90-97 is appended to every container -- all containers share the same proto struct pointers.

Recommendation: Use proto.Clone() or construct fresh structs per container.

8. No backward-compatibility test for responses without volumeMounts (Test coverage)
No SDK test asserts that a pre-feature JSON response (without volumeMounts) decodes without error.

Recommendation: Add one test per SDK layer serving the old response format.


Low Severity

  • OpenAPI SandboxVolumeMount properties lack descriptions (openapi.yml lines 938-952).
  • Rust struct and proto fields lack doc comments.
  • map_volume_mounts always wraps read_only in Some(bool).
  • Missing test cases: injectHostDirMounts with multiple mount options; error path for non-absolute paths; nil mount inside a valid container.

Positive aspects

  • Helper functions are clean, single-responsibility, with sensible nil/empty handling.
  • Proto field number 10 (next available) avoids wire compatibility issues.
  • The sandbox pod container is correctly skipped, preventing internal infrastructure mounts from leaking.
  • Go SDK and Python SDK tests validate deserialization end-to-end.
  • Rust serde aliases handle Go-side protobuf JSON field naming conventions.

Expose container volume_mounts from Cubelet list through CubeMaster to
CubeAPI GET /sandboxes responses. Host-dir mounts now populate hostPath
on the VolumeMount spec so web, CLI, and SDK clients can inspect bind
mounts after create.

Signed-off-by: xiaojunxiang <xiaojunxiang@kingsoft.com>
@xiaojunxiang2023 xiaojunxiang2023 force-pushed the fix/host-mount-display branch from bd58b0b to 1e47471 Compare July 1, 2026 08:38
@xiaojunxiang2023

xiaojunxiang2023 commented Jul 1, 2026

Copy link
Copy Markdown
Contributor Author

Review: PR #696 — Expose volume mounts in sandbox info/list APIs

Overall: Well-structured change that consistently threads volume mount data through the entire stack (Cubelet to CubeMaster to CubeAPI to SDKs to CLI to Web). Tests cover the happy path well. A few items below.

1. openapi.yml copyright header unintentionally removed

File: openapi.yml (lines 1-6, deleted)

The diff removes the copyright header block. This was likely an accidental deletion. Please restore or confirm dliberate.

2. Exported Rust struct CubeVolumeMount missing doc comment

File: CubeAPI/src/cubemaster/mod.rs:9

A brief doc comment would help distinguish this deserialization target from the serialization-only VolumeMount.

3. Exported Go types/functions missing godoc comments

Files:

  • CubeMaster/pkg/service/sandbox/types/types.go:529 - VolumeMountInfo
  • CubeMaster/pkg/service/sandbox/volume_mounts.go:567 - collectVolumeMountsFromContainers
  • CubeMaster/pkg/service/sandbox/volume_mounts.go:589 - volumeMountsToContainerInfo

All exported but undocumented.

4. CLI tabwriter alignment may produce ragged output

File: CubeMaster/cmd/cubemastercli/commands/cubebox/info.go:127-137

The 4-column VOLUME_MOUNTS header is aligned by the same tabwriter as surrounding 2-column rows, likely producing uneven columns.

5. Test coverage gaps: edge cases in mapping functions

  • map_volume_mounts with empty input
  • map_volume_mounts where all entries filter to empty
  • map_volume_mounts with mixed valid + invalid
  • collectVolumeMountsFromContainers with nil container in the slice
  • volumeMountsToContainerInfo with nil mount or empty slice

6. volumeMountsToContainerInfo defensive nil check (optional)

File: CubeMaster/pkg/service/sandbox/volume_mounts.go:595

Nil guard is redundant since its caller already filters nils.

7. map_volume_mounts filter could use a comment

File: CubeAPI/src/services/sandboxes.rs:736

Filter intentionally preserves partial entries; an inline comment would make this explicit.

Summary: No security concerns - data-propagation-only change. All existing auth/access controls apply

Fixed

  • ** 1 ** Restored the openapi.yml copyright header removed by the OpenAPI export.
  • ** 2 ** Added a doc comment on CubeVolumeMount (CubeMaster deserialization shape vs request VolumeMount).
  • ** 3 ** Added godoc on VolumeMountInfo, collectVolumeMountsFromContainers, and volumeMountsToContainerInfo.
  • ** 5 ** Added tests for empty map_volume_mounts input, all-empty entries filtered out, nil container in the slice, and nil mount in volumeMountsToContainerInfo.
  • ** 7 ** Documented why the map_volume_mounts filter keeps partial name/path entries.

No code change (by design)

  • ** 4 ** cubemastercli info reuses the same tabwriter as the surrounding 2-column rows; 4-column VOLUME_MOUNTS may look uneven but remains readable for ops/debug. Happy to polish formatting in a follow-up if maintainers prefer.
  • ** 6 ** Keeping the nil guard in volumeMountsToContainerInfo — protobuf slices can contain nil entries and callers may grow over time.

@kinwin-ustc

Copy link
Copy Markdown
Collaborator

@ls-ggg Does this conflict with our volume work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants