fix(sandbox): return volume mounts in sandbox info and list APIs#696
fix(sandbox): return volume mounts in sandbox info and list APIs#696xiaojunxiang2023 wants to merge 1 commit into
Conversation
112314a to
bd58b0b
Compare
Review: PR #696 -- Expose volume mounts in sandbox info/list APIsOverall 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 Severity1. 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 Severity4. Path traversal not blocked in injectHostDirMounts (Security -- CWE-22) 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) 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) Recommendation: Merge into a single function that filters and converts in one pass. 7. Shared pointer aliasing in injectHostDirMounts (Code quality) Recommendation: Use proto.Clone() or construct fresh structs per container. 8. No backward-compatibility test for responses without volumeMounts (Test coverage) Recommendation: Add one test per SDK layer serving the old response format. Low Severity
Positive aspects
|
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>
bd58b0b to
1e47471
Compare
Fixed
No code change (by design)
|
|
@ls-ggg Does this conflict with our volume work? |
Summary
volume_mountson Cubelet listContainerresponses and thread them through CubeMaster sandbox info/list into CubeAPIGET /sandboxesand list payloads.hostPathwhen injecting host-dir mounts so clients can see bind-mount source paths after create.SandboxVolumeMountwith optionalhostPathandreadOnly; regenerateopenapi.ymland web TypeScript schema; update SDK models andcubemastercli infooutput.Problem
Host mounts are applied at create time via
metadata["host-mount"], but sandbox detail/list APIs always returnedvolumeMounts: 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)