fix: avoid nil pointer dereference in CacheRuntime configmap builder - #6157
fix: avoid nil pointer dereference in CacheRuntime configmap builder#6157btxu-db wants to merge 3 commits into
Conversation
generateRuntimeConfigData dereferences runtime.Spec.Master/Worker/Client without checking whether the corresponding component is declared in the RuntimeClass topology. When a topology omits any of the three components, the controller panics and the Dataset stays in NotBound forever. Also return an explicit error when the topology declares no component at all, instead of silently producing an incomplete config. Fixes fluid-cloudnative#6147 Signed-off-by: btxu-db <btxu-db@outlook.com>
a350804 to
5b14366
Compare
There was a problem hiding this comment.
Pull request overview
Prevents CacheRuntime ConfigMap generation from panicking when topology components are absent.
Changes:
- Guards Master, Worker, and Client topology access.
- Rejects component-less runtime topologies.
- Adds regression tests, though some new branches remain uncovered.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
pkg/ddc/cache/engine/cm.go |
Adds topology validation and nil guards. |
pkg/ddc/cache/engine/cm_test.go |
Adds fixtures and regression tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func TestGenerateRuntimeConfigDataWithMissingClientTopology(t *testing.T) { | ||
| scheme := newCacheEngineTestScheme(t) | ||
| runtimeObj := newCacheRuntimeForConfigMapTest() | ||
| runtimeObj.Spec.Client.Disabled = false | ||
| runtimeClass := newCacheRuntimeClassForConfigMapTest() | ||
| runtimeClass.Topology = &datav1alpha1.RuntimeTopology{ | ||
| Master: &datav1alpha1.RuntimeComponentDefinition{}, | ||
| Worker: &datav1alpha1.RuntimeComponentDefinition{}, | ||
| } | ||
| dataset := newDatasetForConfigMapTest() | ||
| baseClient := fake.NewFakeClientWithScheme(scheme, runtimeObj, runtimeClass, dataset) | ||
| engine := &CacheEngine{Client: baseClient, name: "demo", namespace: "default"} | ||
|
|
||
| if _, err := engine.generateRuntimeConfigData(context.Background(), runtimeObj); err != nil { | ||
| t.Fatalf("expected no error when client topology is undefined, got %v", err) | ||
| } | ||
| } |
| func TestGenerateRuntimeConfigDataWithNilTopology(t *testing.T) { | ||
| scheme := newCacheEngineTestScheme(t) | ||
| runtimeObj := newCacheRuntimeForConfigMapTest() | ||
| runtimeClass := newCacheRuntimeClassForConfigMapTest() | ||
| runtimeClass.Topology = nil | ||
| dataset := newDatasetForConfigMapTest() | ||
| baseClient := fake.NewFakeClientWithScheme(scheme, runtimeObj, runtimeClass, dataset) | ||
| engine := &CacheEngine{Client: baseClient, name: "demo", namespace: "default"} | ||
|
|
||
| _, err := engine.generateRuntimeConfigData(context.Background(), runtimeObj) | ||
| if err == nil { | ||
| t.Fatal("expected error when topology is nil, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "at least one component should be defined") { | ||
| t.Fatalf("unexpected error message: %v", err) | ||
| } | ||
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6157 +/- ##
==========================================
+ Coverage 65.13% 65.17% +0.03%
==========================================
Files 485 485
Lines 34039 34045 +6
==========================================
+ Hits 22171 22188 +17
+ Misses 10127 10114 -13
- Partials 1741 1743 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cheyang
left a comment
cheyang
left a comment
There was a problem hiding this comment.
The fix itself looks right, and guarding each component is the correct shape. What I'd like resolved before merge is the scope claim, since it's what decides whether #6147 can be closed.
"cm.go is the only place" doesn't hold
dataload.go:97 and image.go:29 check the component but not Topology itself:
// pkg/ddc/cache/engine/dataload.go:97
if runtimeClass.Topology.Worker != nil {A nil Topology panics there the same way it did at cm.go:178, and the guard in transform.go doesn't help: genDataLoadValue is reached from the DataLoad controller (DataLoadReconciler -> OperationReconciler -> Operate -> generateDataLoadValueFile), and transform() is never called anywhere on that chain. Different entry point, different reconciler, so the cm.go fix doesn't cover it.
I checked this against your branch rather than guessing. With this PR applied, a CacheRuntimeClass that has dataOperationSpecs but no topology still crashes on the first DataLoad reconcile:
genDataLoadValue dataload.go:97
generateDataLoadValueFile dataload.go:61
The class only has to be accepted by the API server for this to happen, and it is: Topology is +optional, there's no validating webhook for CacheRuntimeClass, no CEL rule, no immutability marker, and CacheEngine.Validate is a no-op. A class that had topology and was edited later ends up in the same state.
sync.go:190 and sync.go:214 have the same shape. Those are latent today, because this PR's new error is raised earlier in Sync (via sync.go:55) and returns before syncRuntimeSpec runs. The deref is still wrong though.
The ask is small: either add runtimeClass.Topology != nil && at those sites, or drop the "only place" claim and open a follow-up so #6147 isn't closed as fully fixed while a reachable panic remains. A guard inside getRuntimeClass (runtime.go:77) would cover all five call sites at once, since it's the only loader.
The description points at the wrong field
It says generateRuntimeConfigData dereferences runtime.Spec.Master/Worker/Client and that these are "optional pointers in the API". They're value structs:
// api/v1alpha1/cacheruntime_types.go:145-153
Master CacheRuntimeMasterSpec `json:"master,omitempty"`
Worker CacheRuntimeWorkerSpec `json:"worker,omitempty"`
Client CacheRuntimeClientSpec `json:"client,omitempty"`runtime.Spec.Client.Disabled can't nil-panic. The real deref at the cm.go:178 you reported is runtimeClass.Topology.Client.Options, on the other object. Your guards are on the right field, so the code is fine; it's the explanation that misdirects.
Coverage
I also confirmed both of Copilot's points instead of taking them on faith, and left the details inline. Reverting the Master guard, the Worker guard, or the all-nil clause each leaves the tests green.
Reproduction harness and captured output: https://github.com/cheyang/fluid/tree/verify/cacheruntime-configmap-nil-pointer/docs/verification/cacheruntime-configmap-nil-pointer
| runtimeObj := newCacheRuntimeForConfigMapTest() | ||
| runtimeObj.Spec.Client.Disabled = false | ||
| runtimeClass := newCacheRuntimeClassForConfigMapTest() | ||
| runtimeClass.Topology = &datav1alpha1.RuntimeTopology{ |
There was a problem hiding this comment.
Agreeing with Copilot's comment here, and I verified it rather than taking it on faith: the Master and Worker guards have no regression coverage. Reverting either one individually leaves the cm.go tests green, so a future change could drop either guard and reintroduce the panic for those topologies without CI noticing.
drop Master component guard STILL GREEN <== no coverage
drop Worker component guard STILL GREEN <== no coverage
drop Client component guard fails (covered)
A table-driven case over the three components would close this and would be shorter than the current two tests.
One caveat if you check this yourself: don't use whole-package pass/fail as the signal. TestCacheEngine in this package has pre-existing spec failures unrelated to your PR, and they make every mutation look "caught". Scope it to -run TestGenerateRuntimeConfigData.
There was a problem hiding this comment.
Replaced both tests with one table-driven TestGenerateRuntimeConfigDataWithMissingComponentTopology
over the three components.
The fixture was the problem — all three components had Disabled: true, and the guard is
Topology.X != nil && !runtime.Spec.X.Disabled, so the && short-circuited on the second
operand and the branch never ran regardless of what Topology held. Only the Client test set
Client.Disabled = false, which is why it was the one component with real coverage. Each table
case now enables the component under test.
Re-ran the mutations scoped to -run TestGenerateRuntimeConfigData as you suggested. All three
fail now:
Master component guard dropped -> FAIL (covered)
Worker component guard dropped -> FAIL (covered)
Client component guard dropped -> FAIL (covered)
mutation script
cp pkg/ddc/cache/engine/cm.go /tmp/cm.go.bak
for C in Master Worker Client; do
cp /tmp/cm.go.bak pkg/ddc/cache/engine/cm.go
sed -i "s|runtimeClass.Topology.$C != nil && ||" pkg/ddc/cache/engine/cm.go
git diff --quiet pkg/ddc/cache/engine/cm.go && { echo "$C: sed did not match"; continue; }
if go test ./pkg/ddc/cache/engine/ -run TestGenerateRuntimeConfigData -count=1 >/dev
echo "$C component guard dropped -> still green (NOT covered)"
else
echo "$C component guard dropped -> FAIL (covered)"
fi
done
cp /tmp/cm.go.bak pkg/ddc/cache/engine/cm.go # restore
</details>| scheme := newCacheEngineTestScheme(t) | ||
| runtimeObj := newCacheRuntimeForConfigMapTest() | ||
| runtimeClass := newCacheRuntimeClassForConfigMapTest() | ||
| runtimeClass.Topology = nil |
There was a problem hiding this comment.
Also confirming Copilot's second point: the new error has two branches but only Topology == nil is tested. Removing the (Master == nil && Worker == nil && Client == nil) clause keeps the suite green, so the topology: {} representation is untested. Worth a second case setting Topology = &datav1alpha1.RuntimeTopology{}.
There was a problem hiding this comment.
Confirmed, added the second case.
TestGenerateRuntimeConfigDataWithNilTopology is now TestGenerateRuntimeConfigDataWithoutAnyComponent, table-driven with one case per branch — Topology = nil and Topology = &datav1alpha1.RuntimeTopology{}. The condition moved to validateRuntimeClassTopology in validate.go, called from getRuntimeClass.
Checked it both ways. Drop the (Master == nil && Worker == nil && Client == nil) clause and only the new case goes red:
--- FAIL: TestGenerateRuntimeConfigDataWithoutAnyComponent (0.00s)
--- FAIL: TestGenerateRuntimeConfigDataWithoutAnyComponent/topology_declares_no_component (0.00s)
cm_test.go:302: expected error, got nil
FAIL
FAIL github.com/fluid-cloudnative/fluid/pkg/ddc/cache/engine 0.019s
FAIL
ok github.com/fluid-cloudnative/fluid/pkg/ddc/cache/engine 0.024s
Put it back and the package is green again. So the two cases are hitting different branches rather than both riding on the nil check.
| return nil, err | ||
| } | ||
|
|
||
| if runtimeClass.Topology == nil || |
There was a problem hiding this comment.
nit: this is now the third copy of this condition. transform.go:52 and transform.go:86 already have it, error string included. A small validateTopology(runtimeClass) error helper would collapse all three and keep the message in one place.
There was a problem hiding this comment.
Collapsed into validateRuntimeClassTopology in validate.go. Removed the cm.go copy since the loader covers it; kept the two transform.go call sites because transform_test.go builds a CacheRuntimeClass directly and doesn't go through getRuntimeClass.
Move the topology check into getRuntimeClass so all five call sites are covered, including the DataLoad path that still panicked at dataload.go:97. Collapse the duplicated condition into validateRuntimeClassTopology. Signed-off-by: btxu-db <btxu-db@outlook.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: xliuqq The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Signed-off-by: btxu-db <btxu-db@outlook.com>
|
I. Describe what this PR does
CacheRuntimeClass.Topology and its Master/Worker/Client sub-fields are optional pointers, and the engine dereferences them in several places without a nil check. A class that omits a component — or omits topology altogether — panics the controller, and the Dataset never reaches Bound.
The check now lives in validateRuntimeClassTopology (validate.go), called from getRuntimeClass (runtime.go). That's the only place a CacheRuntimeClass enters the engine, so all five callers are covered: setup.go:39, cm.go:107, sync.go:50, ufs.go:97, dataload.go:56.
Guarding at the loader also picks up the dereferences at dataload.go:97, image.go:29, sync.go:190 and sync.go:214 — the first revision of this PR missed all four, and the DataLoad path still panicked. The two call sites in transform.go stay as they are, since transform_test.go builds a CacheRuntimeClass directly and never goes through the loader.
The fix covers all three components, not just Client as the issue title suggests — Master and Worker have the same unguarded dereference.
The loader also returns an explicit error for a topology that declares no component at all (topology: {}), rather than quietly producing an incomplete config.
II. Does this pull request fix one issue?
fixes #6147
III. List the added test cases
Three regression tests in pkg/ddc/cache/engine/cm_test.go:
TestGenerateRuntimeConfigDataWithMissingComponentTopology — table-driven, one case per component. Each case enables the component under test; otherwise && short-circuits and the guard never runs.
TestGenerateRuntimeConfigDataWithoutAnyComponent — table-driven, covering both topology absent and topology: {} with nothing declared.
TestGenerateDataLoadValueFileWithNilTopology — the DataLoad path, which the first revision didn't cover.
IV. Describe how to verify it
go test ./pkg/ddc/cache/engine/
-run 'TestGenerateRuntimeConfigData|TestGenerateDataLoadValueFile' -count=1
Before the fix the DataLoad path panics:
engine.(*CacheEngine).genDataLoadValue dataload.go:97
engine.(*CacheEngine).generateDataLoadValueFile dataload.go:61
After it, ok.
Every guard was checked by removing it and confirming something turns red (baseline green before each run):
mutation result
drop Master component guard FAIL
drop Worker component guard FAIL
drop Client component guard FAIL
drop the loader guard FAIL (both tests)
drop the topology: {} clause FAIL
One caveat: TestCacheEngine has 12 pre-existing spec failures in ufs_test.go / sync_test.go on master that have nothing to do with this change, so scope runs with -run.
Also reproduced on a kind cluster beforehand — the controller restarted 31 times and the Dataset stayed in NotBound, with the panic stack pointing at the Client dereference in the configmap builder.
V. Special notes for reviews
None.