fix: avoid nil pointer dereference in CacheRuntime configmap builder by btxu-db · Pull Request #6157 · fluid-cloudnative/fluid

btxu-db · 2026-08-09T03:26:24Z

I. Describe what this PR does

generateRuntimeConfigData dereferences runtime.Spec.Master/Worker/Client
without checking whether the corresponding component is declared in the
RuntimeClass topology. These three fields are optional pointers in the API,
so when a topology omits any of them the controller panics and the Dataset
never reaches Bound.

cm.go is the only place in the repo where this nil check is missing;
transform.go already guards the same fields. This PR aligns cm.go with
that existing pattern.

Note that the fix covers all three components, not only Client as
mentioned in the issue title — the same unguarded dereference exists for
Master and Worker.

This PR also returns an explicit error when the topology declares no
component at all, instead of silently producing an incomplete config.

II. Does this pull request fix one issue?

fixes #6147

III. List the added test cases

Two regression tests in pkg/ddc/cache/engine/cm_test.go:

TestGenerateRuntimeConfigDataWithMissingClientTopology — covers the
panic path: without this fix it panics at cm.go:178.
TestGenerateRuntimeConfigDataWithNilTopology — covers the silent path:
without this fix no error is returned at all, and an incomplete config is
produced.

IV. Describe how to verify it

Both tests were verified in both directions (run separately, since a panic
aborts the whole test binary):

test	without the fix	with the fix
`...WithMissingClientTopology`	panic at `cm.go:178`	PASS
`...WithNilTopology`	FAIL: expected error, got nil	PASS

Reproduced on a kind cluster before the fix: the controller restarted 31
times and the Dataset stayed in NotBound, with the panic stack pointing at
cm.go:178.

V. Special notes for reviews

None.

xliuqq

/lgtm

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>

sonarqubecloud · 2026-08-09T03:48:41Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Copilot

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 · 2026-08-09T05:25:30Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.13%. Comparing base (05f0665) to head (5b14366).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6157   +/-   ##
=======================================
  Coverage   65.13%   65.13%           
=======================================
  Files         485      485           
  Lines       34039    34043    +4     
=======================================
+ Hits        22171    22175    +4     
  Misses      10127    10127           
  Partials     1741     1741

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

cheyang

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

cheyang · 2026-08-09T15:19:08Z

+	runtimeObj := newCacheRuntimeForConfigMapTest()
+	runtimeObj.Spec.Client.Disabled = false
+	runtimeClass := newCacheRuntimeClassForConfigMapTest()
+	runtimeClass.Topology = &datav1alpha1.RuntimeTopology{


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.

cheyang · 2026-08-09T15:19:08Z

+	scheme := newCacheEngineTestScheme(t)
+	runtimeObj := newCacheRuntimeForConfigMapTest()
+	runtimeClass := newCacheRuntimeClassForConfigMapTest()
+	runtimeClass.Topology = nil


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{}.

cheyang · 2026-08-09T15:19:08Z

 		return nil, err
 	}
-
+	if runtimeClass.Topology == nil ||


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.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

xliuqq approved these changes Aug 9, 2026

View reviewed changes

btxu-db force-pushed the fix/cacheruntime-configmap-nil-pointer branch from a350804 to 5b14366 Compare August 9, 2026 03:47

btxu-db requested a review from xliuqq August 9, 2026 03:49

cheyang requested a balanced review from Copilot August 9, 2026 05:18

Copilot started reviewing on behalf of cheyang August 9, 2026 05:18 View session

Copilot AI reviewed Aug 9, 2026

View reviewed changes

cheyang requested changes Aug 9, 2026

View reviewed changes

cheyang requested a balanced review from Copilot August 10, 2026 05:44

Copilot started reviewing on behalf of cheyang August 10, 2026 05:45 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

Navigation Menu

Uh oh!

fix: avoid nil pointer dereference in CacheRuntime configmap builder - #6157

fix: avoid nil pointer dereference in CacheRuntime configmap builder#6157
btxu-db wants to merge 1 commit into
fluid-cloudnative:masterfrom
btxu-db:fix/cacheruntime-configmap-nil-pointer

btxu-db commented Aug 9, 2026

Uh oh!

xliuqq left a comment

Uh oh!

sonarqubecloud Bot commented Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

codecov Bot commented Aug 9, 2026

Uh oh!

cheyang left a comment

Uh oh!

cheyang Aug 9, 2026

Uh oh!

cheyang Aug 9, 2026

Uh oh!

cheyang Aug 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

btxu-db commented Aug 9, 2026

I. Describe what this PR does

II. Does this pull request fix one issue?

III. List the added test cases

IV. Describe how to verify it

V. Special notes for reviews

Uh oh!

xliuqq left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented Aug 9, 2026

Quality Gate passed

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov Bot commented Aug 9, 2026

Codecov Report

Uh oh!

cheyang left a comment

Choose a reason for hiding this comment

"cm.go is the only place" doesn't hold

The description points at the wrong field

Coverage

Uh oh!

cheyang Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

cheyang Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

cheyang Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants