Skip to content

Commit 5836295

Browse files
NouemanKHALclaude
andcommitted
fix(cloudfoundry): avoid nil panic on failed DCA connection
getDCAClient stored the result of GetClusterAgentClient (which returns (*DCAClient, error)) directly into the c.dcaClient interface field before checking the error. On a failed connection the returned nil *DCAClient was boxed into the interface, producing a non-nil "typed nil". The next Pull passed the `c.dcaClient != nil` cache check and panicked when calling GetCFAppsMetadataForNode on the nil receiver. Assign to a local *DCAClient and only store it on success. Also call the local client returned by getDCAClient in Pull instead of the field. This regressed in 7.81 when GetClusterAgentClient's return type changed from DCAClientInterface to *DCAClient (#50814); previously the error path returned a true nil interface and was harmless. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 98c5a2c commit 5836295

3 files changed

Lines changed: 63 additions & 3 deletions

File tree

comp/core/workloadmeta/collectors/internal/cloudfoundry/vm/cf_vm.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ func (c *collector) Pull(_ context.Context) error {
108108

109109
var allContainersTags map[string][]string
110110
if dcaClient := c.getDCAClient(); dcaClient != nil {
111-
allContainersTags, err = c.dcaClient.GetCFAppsMetadataForNode(c.nodeName)
111+
allContainersTags, err = dcaClient.GetCFAppsMetadataForNode(c.nodeName)
112112
if err != nil {
113113
log.Debugf("Unable to fetch CF tags from cluster agent, CF tags will be missing, err: %v", err)
114114
}
@@ -231,13 +231,18 @@ func (c *collector) getDCAClient() clusteragent.DCAClientInterface {
231231
return c.dcaClient
232232
}
233233

234-
var err error
235-
c.dcaClient, err = clusteragent.GetClusterAgentClient()
234+
// Assign to a local *DCAClient first and only store it into the interface
235+
// field on success. GetClusterAgentClient returns (*DCAClient, error); on
236+
// failure that nil pointer would otherwise be boxed into the c.dcaClient
237+
// interface field, producing a non-nil "typed nil" that passes the
238+
// `c.dcaClient != nil` check above and panics when a method is called on it.
239+
client, err := clusteragent.GetClusterAgentClient()
236240
if err != nil {
237241
log.Debugf("Could not initialise the communication with the cluster agent, PCF tags may be missing, err: %v", err)
238242
return nil
239243
}
240244

245+
c.dcaClient = client
241246
return c.dcaClient
242247
}
243248

comp/core/workloadmeta/collectors/internal/cloudfoundry/vm/cf_vm_test.go

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,46 @@ func TestPullAppNameWithDCA(t *testing.T) {
438438
assert.Contains(t, container.CollectorTags, "container_name:active-container-app")
439439
}
440440

441+
// TestPullDCAConnectionFailureDoesNotPanic is a regression test for a nil
442+
// pointer dereference that happened when the cluster agent connection failed.
443+
//
444+
// getDCAClient() calls clusteragent.GetClusterAgentClient(), which returns
445+
// (*DCAClient, error). On failure that nil *DCAClient must not be stored into
446+
// the c.dcaClient interface field: doing so produces a non-nil "typed nil"
447+
// interface that passes the `c.dcaClient != nil` cache check on the next Pull
448+
// and panics when GetCFAppsMetadataForNode is called on the nil receiver.
449+
//
450+
// dcaClient is intentionally left unset so getDCAClient() goes through the
451+
// GetClusterAgentClient() path, which errors here since no cluster agent
452+
// endpoint is configured.
453+
func TestPullDCAConnectionFailureDoesNotPanic(t *testing.T) {
454+
containers := []garden.Container{
455+
&activeContainerWithoutProperties,
456+
}
457+
fakeGardenUtil := FakeGardenUtil{
458+
containers: containers,
459+
}
460+
workloadmetaStore := fxutil.Test[workloadmetamock.Mock](t, fx.Options(
461+
core.MockBundle(),
462+
workloadmetafxmock.MockModule(workloadmeta.NewParams()),
463+
))
464+
465+
c := collector{
466+
gardenUtil: &fakeGardenUtil,
467+
store: workloadmetaStore,
468+
seen: make(map[workloadmeta.EntityID]struct{}),
469+
dcaEnabled: true, // enabled, but the connection will fail
470+
}
471+
472+
// The first Pull triggers a failed cluster agent connection. A second Pull
473+
// must not panic: before the fix, the failed connection poisoned the cached
474+
// dcaClient field with a typed nil.
475+
require.NoError(t, c.Pull(context.TODO()))
476+
require.NotPanics(t, func() {
477+
require.NoError(t, c.Pull(context.TODO()))
478+
})
479+
}
480+
441481
func TestPullNoAppNameWithoutDCA(t *testing.T) {
442482
containers := []garden.Container{
443483
&activeContainerWithoutProperties,
Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
# Each section from every release note are combined when the
2+
# CHANGELOG.rst is rendered. So the text needs to be worded so that
3+
# it does not depend on any information only available in another
4+
# section. This may mean repeating some details, but each section
5+
# must be readable independently of the other.
6+
#
7+
# Each section note must be formatted as reStructuredText.
8+
---
9+
fixes:
10+
- |
11+
On Cloud Foundry, fixed a crash (nil pointer dereference) in the
12+
``cloudfoundry-vm`` workloadmeta collector that occurred when the
13+
connection to the Cluster Agent failed. A failed connection no longer
14+
poisons the cached client, so subsequent metadata pulls recover instead
15+
of panicking.

0 commit comments

Comments
 (0)