Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ func (c *collector) Pull(_ context.Context) error {

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

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

c.dcaClient = client
return c.dcaClient
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -438,6 +438,46 @@ func TestPullAppNameWithDCA(t *testing.T) {
assert.Contains(t, container.CollectorTags, "container_name:active-container-app")
}

// TestPullDCAConnectionFailureDoesNotPanic is a regression test for a nil
// pointer dereference that happened when the cluster agent connection failed.
//
// getDCAClient() calls clusteragent.GetClusterAgentClient(), which returns
// (*DCAClient, error). On failure that nil *DCAClient must not be stored into
// the c.dcaClient interface field: doing so produces a non-nil "typed nil"
// interface that passes the `c.dcaClient != nil` cache check on the next Pull
// and panics when GetCFAppsMetadataForNode is called on the nil receiver.
//
// dcaClient is intentionally left unset so getDCAClient() goes through the
// GetClusterAgentClient() path, which errors here since no cluster agent
// endpoint is configured.
func TestPullDCAConnectionFailureDoesNotPanic(t *testing.T) {
containers := []garden.Container{
&activeContainerWithoutProperties,
}
fakeGardenUtil := FakeGardenUtil{
containers: containers,
}
workloadmetaStore := fxutil.Test[workloadmetamock.Mock](t, fx.Options(
core.MockBundle(),
workloadmetafxmock.MockModule(workloadmeta.NewParams()),
))

c := collector{
gardenUtil: &fakeGardenUtil,
store: workloadmetaStore,
seen: make(map[workloadmeta.EntityID]struct{}),
dcaEnabled: true, // enabled, but the connection will fail
}

// The first Pull triggers a failed cluster agent connection. A second Pull
// must not panic: before the fix, the failed connection poisoned the cached
// dcaClient field with a typed nil.
require.NoError(t, c.Pull(context.TODO()))
require.NotPanics(t, func() {
require.NoError(t, c.Pull(context.TODO()))
})
}

func TestPullNoAppNameWithoutDCA(t *testing.T) {
containers := []garden.Container{
&activeContainerWithoutProperties,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# Each section from every release note are combined when the
# CHANGELOG.rst is rendered. So the text needs to be worded so that
# it does not depend on any information only available in another
# section. This may mean repeating some details, but each section
# must be readable independently of the other.
#
# Each section note must be formatted as reStructuredText.
---
fixes:
- |
On Cloud Foundry, fixed a crash (nil pointer dereference) in the
``cloudfoundry-vm`` workloadmeta collector that occurred when the
connection to the Cluster Agent failed. A failed connection no longer
poisons the cached client, so subsequent metadata pulls recover instead
of panicking.
Loading