fix: handle null Docker runtimes - #2109
Conversation
A JSON null runtimes entry decodes to a nil interface. Adding, removing, or looking up a runtime currently asserts that value to a map and panics. Treat a null runtimes entry like an absent entry before the assertion. Cover all three operations using a configuration loaded from a file and verify unrelated settings survive saving the updated configuration. Signed-off-by: xinjun.jiang <xinjun.jiang@daocloud.io>
| // Read the existing runtimes | ||
| runtimes := make(map[string]any) | ||
| if _, exists := config["runtimes"]; exists { | ||
| if config["runtimes"] != nil { |
There was a problem hiding this comment.
| if config["runtimes"] != nil { | |
| rt, ok := config["runtimes"].(map[string]any); ok { |
There was a problem hiding this comment.
Applied in 8884e35: AddRuntime now uses the checked rt, ok := config["runtimes"].(map[string]any) assertion to reuse the map. The null-runtimes add regression passes.
| } | ||
|
|
||
| if _, exists := config["runtimes"]; exists { | ||
| if config["runtimes"] != nil { |
There was a problem hiding this comment.
Applied in 8884e35: RemoveRuntime now uses the checked runtime-map assertion directly. The null-runtimes removal regression passes.
|
|
||
| var runtimes map[string]any | ||
| if _, ok := cfg["runtimes"]; ok { | ||
| if cfg["runtimes"] != nil { |
There was a problem hiding this comment.
Applied in 8884e35: GetRuntimeConfig now uses a checked runtime-map assertion and keeps the map scoped to that branch. The null-runtimes lookup regression passes.
There was a problem hiding this comment.
Fixed in 8884e35: RemoveRuntime now checks the default-runtime string assertion. Added a file-based regression for removal with "default-runtime": null; it panics on the previous PR head and passes with this change.
There was a problem hiding this comment.
Fixed in 8884e35: the unset path in UpdateDefaultRuntime now checks the default-runtime string assertion. Added a file-based null-default regression; it panics on the previous PR head and passes with this change.
| if cfg["runtimes"] != nil { | ||
| runtimes = cfg["runtimes"].(map[string]any) | ||
| if r, ok := runtimes[name]; ok { | ||
| dr := dockerRuntime(r.(map[string]any)) |
There was a problem hiding this comment.
Fixed in 8884e35: individual runtime definitions now use a checked map assertion, so a null definition returns an empty runtime configuration. Added a file-based "runtimes": {"nvidia": null} lookup regression; it panics on the previous PR head and passes with this change.
Use checked type assertions for runtime maps, default runtime names, and individual runtime definitions. Null default-runtime values and runtime entries should not panic during removal, unsetting, or lookup. Add file-based regressions for each remaining null configuration path. Signed-off-by: xinjun.jiang <xinjun.jiang@daocloud.io>
|
/ok to test 8884e35 |
Coverage Report for CI Build 36838559285Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.2%) to 44.266%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
3 participants
Description
nvidia-ctk runtime configure --runtime=docker --config=daemon.jsonpanics when the file contains{"runtimes":null}. Docker configuration methods also panic when removing or unsetting a nulldefault-runtime, or looking up a null runtime definition.Use checked type assertions for the runtime map, default runtime, and individual runtime definitions. Null entries are handled like missing entries. File-based regressions cover these paths and verify the saved configuration preserves unrelated settings.
Checklist
make test) — the same four packages and nine subtests fail on unchanged main; see belowmake lint)Testing
default-runtimeand null runtime definitions each panic on the previous PR head and pass with the checked assertions. All Docker configuration package tests pass.{"runtimes":null,"log-driver":"json-file"}. The updated CLI exits successfully, preserveslog-driver, and produces the same configuration on a second run.make build,make fmt, andmake lintcompleted; lint reported 0 issues.go test -race ./pkg/config/... ./cmd/nvidia-ctk/runtime/... -count=1passed.make testwas rerun on this change and an unchanged checkout of main (84e2c2c182bfa0b2edab4fdca27e5197faba0ca7). Both fail the same nine subtests incmd/nvidia-ctk-installer,cmd/nvidia-ctk-installer/toolkit,internal/modifier, andpkg/nvcdi, involving driver-library discovery in test fixtures and CSV hook expectations. All other packages pass.