Fix runner crash: align WASI_CONFIG wire format with controller#14
Merged
Fix runner crash: align WASI_CONFIG wire format with controller#14
Conversation
env: map[string]string (not []EnvVar), dirs: hostPath/guestPath (not volumeMounts). Add golden-file contract tests for both Go and Rust sides. Fixes #13
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughController now emits WASI JSON with Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(60,120,60,0.5)
Controller->>Runner: Set WASI_CONFIG JSON (env: map, dirs[], args, network)
end
rect rgba(60,60,120,0.5)
Runner->>ConfigParser: Parse WASI_CONFIG into WasiConfig
ConfigParser->>EnvSetup: For each (key,val) set env
ConfigParser->>DirsSetup: For each dir -> preopen guest_path from host_path (respect read_only)
DirsSetup->>Filesystem: Check host_path exists
end
rect rgba(120,60,60,0.5)
Runner->>Network: Apply network constraints
Runner->>WasmProcess: Launch WASM with env, preopened dirs, args
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/reconciler/wasmmodule/wasmmodule_test.go (1)
32-33: Considert.Parallel()for this isolated unit test.This test has no shared mutable state and can run in parallel with the rest of the package tests.
💡 Proposed fix
func TestBuildRunnerConfigMatchesGolden(t *testing.T) { + t.Parallel() + trueVal := true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reconciler/wasmmodule/wasmmodule_test.go` around lines 32 - 33, The test TestBuildRunnerConfigMatchesGolden can be run in parallel; add t.Parallel() at the start of the test to enable parallel execution. Open the TestBuildRunnerConfigMatchesGolden function and insert a call to t.Parallel() (e.g., immediately after func TestBuildRunnerConfigMatchesGolden(t *testing.T) {) so the isolated test runs concurrently with other tests; ensure this change does not alter the existing trueVal or other local setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reconciler/wasmmodule/wasmmodule_test.go`:
- Around line 79-80: The test currently discards errors from json.MarshalIndent
when creating gotJSON and wantJSON; update the test in wasmmodule_test.go to
check and handle those errors instead of ignoring them by capturing the error
return from json.MarshalIndent for both gotMap and wantMap (the variables
gotJSON and wantJSON) and calling t.Fatalf or t.Errorf with a descriptive
message on error; ensure the error messages include the marshal error so test
diagnostics remain helpful.
---
Nitpick comments:
In `@pkg/reconciler/wasmmodule/wasmmodule_test.go`:
- Around line 32-33: The test TestBuildRunnerConfigMatchesGolden can be run in
parallel; add t.Parallel() at the start of the test to enable parallel
execution. Open the TestBuildRunnerConfigMatchesGolden function and insert a
call to t.Parallel() (e.g., immediately after func
TestBuildRunnerConfigMatchesGolden(t *testing.T) {) so the isolated test runs
concurrently with other tests; ensure this change does not alter the existing
trueVal or other local setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a36d770-7156-4dd3-a894-dc927aebfd67
📒 Files selected for processing (5)
pkg/reconciler/wasmmodule/testdata/wasi_config.golden.jsonpkg/reconciler/wasmmodule/wasmmodule_test.gorunner/src/config.rsrunner/src/main.rsrunner/src/server.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/reconciler/wasmmodule/wasmmodule_test.go`:
- Line 83: Add a blank line between the preceding error-checking block and the
second assignment to wantJSON to satisfy the wsl_v5 linter; specifically, in
wasmmodule_test.go insert an empty line before the statement that initializes
wantJSON, err := json.MarshalIndent(wantMap, "", " ") so the wantJSON
assignment is visually separated from the prior if err != nil { ... } block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 839979d6-d314-4512-9a1c-35102053f21f
📒 Files selected for processing (1)
pkg/reconciler/wasmmodule/wasmmodule_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
========================================
+ Coverage 1.71% 6.56% +4.84%
========================================
Files 22 22
Lines 640 640
========================================
+ Hits 11 42 +31
+ Misses 629 593 -36
- Partials 0 5 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@runner/src/config.rs`:
- Around line 140-169: The test test_parse_golden_wasi_config uses a fragile
relative path when calling std::fs::read_to_string; change it to build the
absolute crate-root anchored path using CARGO_MANIFEST_DIR (e.g., use
concat!(env!("CARGO_MANIFEST_DIR"),
"/../pkg/reconciler/wasmmodule/testdata/wasi_config.golden.json") as the
argument to read_to_string) so the golden file is resolved correctly when the
test binary runs outside Cargo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9efa216-c974-4836-a744-d4ad2555e248
📒 Files selected for processing (4)
pkg/reconciler/wasmmodule/wasmmodule.gopkg/reconciler/wasmmodule/wasmmodule_test.gorunner/src/config.rsrunner/src/server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- runner/src/server.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/reconciler/wasmmodule/wasmmodule.go (1)
213-216:⚠️ Potential issue | 🟠 Major
allowIpNameLookup=falseis lost on the wire due toomitemptyon a boolean field.At line 215,
AllowIPNameLookupusesjson:"allowIpNameLookup,omitempty". In Go's JSON marshalling,falsevalues are omitted whenomitemptyis set. On the Rust runner side (line 90), the field has#[serde(default = "default_true")], so a missing field defaults totrue. This makes it impossible to transmit an explicitfalsevalue.💡 Proposed fix
type RunnerNetworkConfig struct { Inherit bool `json:"inherit,omitempty"` - AllowIPNameLookup bool `json:"allowIpNameLookup,omitempty"` + AllowIPNameLookup bool `json:"allowIpNameLookup"` TCPBind []string `json:"tcpBind,omitempty"` TCPConnect []string `json:"tcpConnect,omitempty"` UDPBind []string `json:"udpBind,omitempty"` UDPConnect []string `json:"udpConnect,omitempty"` UDPOutgoing []string `json:"udpOutgoing,omitempty"` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reconciler/wasmmodule/wasmmodule.go` around lines 213 - 216, The boolean field AllowIPNameLookup on struct RunnerNetworkConfig is tagged with `omitempty`, so false values are dropped during JSON marshaling and the Rust runner's default (true) is used; remove `omitempty` from the `json` tag on AllowIPNameLookup (i.e., change `json:"allowIpNameLookup,omitempty"` to `json:"allowIpNameLookup"`) so explicit false is preserved across the wire and the Rust side receives the intended value. Ensure you update any related marshaling tests or usages that assumed omission behavior.
🧹 Nitpick comments (1)
pkg/reconciler/wasmmodule/wasmmodule_test.go (1)
54-57: Add aAllowIPNameLookup=falseregression case.The golden case only validates
true. Add a second case withfalseto lock wire-format semantics for explicit DNS disable and prevent silent regressions.🧪 Example follow-up test snippet
func TestBuildRunnerConfigMatchesGolden(t *testing.T) { t.Parallel() @@ assertJSONEqual(t, got, string(goldenBytes)) + + t.Run("preserves allowIpNameLookup=false", func(t *testing.T) { + falseVal := false + module.Spec.Network.AllowIPNameLookup = &falseVal + + got, err := wasmmodule.BuildRunnerConfig(module) + if err != nil { + t.Fatalf("BuildRunnerConfig() error: %v", err) + } + + var gotMap map[string]any + if err := json.Unmarshal([]byte(got), &gotMap); err != nil { + t.Fatalf("unmarshal got: %v", err) + } + network, ok := gotMap["network"].(map[string]any) + if !ok { + t.Fatalf("network object missing in output JSON") + } + if v, ok := network["allowIpNameLookup"]; !ok || v != false { + t.Fatalf("expected allowIpNameLookup=false, got %v (present=%v)", v, ok) + } + }) }Also applies to: 74-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/reconciler/wasmmodule/wasmmodule_test.go` around lines 54 - 57, Add a regression test case that sets NetworkSpec.AllowIPNameLookup to false (e.g., introduce a falseVal boolean and a second test entry alongside the existing trueVal case) so the golden output validates both true and false wire-format semantics; update the test table in wasmmodule_test.go to include this new case using the same structure as the existing case (Network: &api.NetworkSpec{Inherit:false, AllowIPNameLookup:&falseVal, TCP:...}) and add/commit the corresponding golden/expected output for the false case to lock the explicit-DNS-disable behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/reconciler/wasmmodule/wasmmodule.go`:
- Around line 213-216: The boolean field AllowIPNameLookup on struct
RunnerNetworkConfig is tagged with `omitempty`, so false values are dropped
during JSON marshaling and the Rust runner's default (true) is used; remove
`omitempty` from the `json` tag on AllowIPNameLookup (i.e., change
`json:"allowIpNameLookup,omitempty"` to `json:"allowIpNameLookup"`) so explicit
false is preserved across the wire and the Rust side receives the intended
value. Ensure you update any related marshaling tests or usages that assumed
omission behavior.
---
Nitpick comments:
In `@pkg/reconciler/wasmmodule/wasmmodule_test.go`:
- Around line 54-57: Add a regression test case that sets
NetworkSpec.AllowIPNameLookup to false (e.g., introduce a falseVal boolean and a
second test entry alongside the existing trueVal case) so the golden output
validates both true and false wire-format semantics; update the test table in
wasmmodule_test.go to include this new case using the same structure as the
existing case (Network: &api.NetworkSpec{Inherit:false,
AllowIPNameLookup:&falseVal, TCP:...}) and add/commit the corresponding
golden/expected output for the false case to lock the explicit-DNS-disable
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecfdd408-e067-490f-897f-998fce34a7fb
📒 Files selected for processing (4)
pkg/reconciler/wasmmodule/wasmmodule.gopkg/reconciler/wasmmodule/wasmmodule_test.gorunner/src/config.rsrunner/src/server.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- runner/src/server.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Fixes #13
Runner crashed immediately on startup with:
All pods ended up in CrashLoopBackOff.
Root Cause
Two type mismatches between the JSON produced by the controller and what the runner expected:
map[string]string(e.g.{"KEY":"VALUE"}), runner expectedVec<EnvVar>(an array of{name, value}objects) — direct crash cause"dirs"withhostPath/guestPathfields, runner expected"volumeMounts"withname/mountPath— silently droppedFix
runner/src/config.rs: changedenvtoHashMap<String, String>, replacedvolume_mounts: Vec<VolumeMount>withdirs: Vec<DirConfig>usinghostPath/guestPathrunner/src/server.rs: updatedbuild_wasi_ctx()to iterate the map for env anddirsfor mountsrunner/src/main.rs: updated diagnostic log lineTests
Added a golden-file contract test that locks both sides to the same JSON schema:
pkg/reconciler/wasmmodule/testdata/wasi_config.golden.json— single source of truth for the wire formatpkg/reconciler/wasmmodule/wasmmodule_test.go— Go test: assertsbuildRunnerConfig()output matches the golden filerunner/src/config.rs(test module) — Rust test: asserts the golden file parses intoWasiConfigcorrectlyAssisted-by: 🤖 Claude Sonnet 4.6
Summary by cubic
Fixes runner crash on startup by aligning the WASI_CONFIG JSON with the controller; env is now a map and directory mounts use hostPath/guestPath. Pods start cleanly and dir mounts are applied.
Written for commit 1667d10. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests