From 5bf6c9db6bc53af370109987de19e04a6fe29281 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Thu, 20 Oct 2022 17:58:39 +0800 Subject: [PATCH] Fix nil pointer error when collecting agent supportbundle fails (#4306) The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. https://github.com/antrea-io/antrea/pull/2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian --- .../registry/system/supportbundle/rest.go | 6 +- .../system/supportbundle/rest_test.go | 159 +++++++++++++++++- 2 files changed, 160 insertions(+), 5 deletions(-) diff --git a/pkg/apiserver/registry/system/supportbundle/rest.go b/pkg/apiserver/registry/system/supportbundle/rest.go index 55dbf4ed84f..3c928fb0e96 100644 --- a/pkg/apiserver/registry/system/supportbundle/rest.go +++ b/pkg/apiserver/registry/system/supportbundle/rest.go @@ -50,8 +50,10 @@ const ( ) var ( + // Declared as variables for testing. defaultFS = afero.NewOsFs() defaultExecutor = exec.New() + newAgentDumper = support.NewAgentDumper ) // NewControllerStorage creates a support bundle storage for working on antrea controller. @@ -164,7 +166,7 @@ func (r *supportBundleREST) Create(ctx context.Context, obj runtime.Object, _ re } }() - if err != nil { + if err == nil { r.clean(ctx, b.Filepath, bundleExpireDuration) } }(r.cache.Since) @@ -257,7 +259,7 @@ func (r *supportBundleREST) collect(ctx context.Context, dumpers ...func(string) } func (r *supportBundleREST) collectAgent(ctx context.Context, since string) (*systemv1beta1.SupportBundle, error) { - dumper := support.NewAgentDumper(defaultFS, defaultExecutor, r.ovsCtlClient, r.aq, r.npq, since, r.v4Enabled, r.v6Enabled) + dumper := newAgentDumper(defaultFS, defaultExecutor, r.ovsCtlClient, r.aq, r.npq, since, r.v4Enabled, r.v6Enabled) return r.collect( ctx, dumper.DumpLog, diff --git a/pkg/apiserver/registry/system/supportbundle/rest_test.go b/pkg/apiserver/registry/system/supportbundle/rest_test.go index 5a372ec21e2..704993dfa06 100644 --- a/pkg/apiserver/registry/system/supportbundle/rest_test.go +++ b/pkg/apiserver/registry/system/supportbundle/rest_test.go @@ -21,6 +21,7 @@ import ( "testing" "time" + "github.com/golang/mock/gomock" "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -29,7 +30,14 @@ import ( "k8s.io/utils/exec" exectesting "k8s.io/utils/exec/testing" + agentquerier "antrea.io/antrea/pkg/agent/querier" + agentqueriertest "antrea.io/antrea/pkg/agent/querier/testing" system "antrea.io/antrea/pkg/apis/system/v1beta1" + "antrea.io/antrea/pkg/ovs/ovsctl" + ovsctltest "antrea.io/antrea/pkg/ovs/ovsctl/testing" + "antrea.io/antrea/pkg/querier" + queriertest "antrea.io/antrea/pkg/querier/testing" + "antrea.io/antrea/pkg/support" ) type testExec struct { @@ -149,11 +157,12 @@ func TestControllerStorage(t *testing.T) { collectedBundle = object.(*system.SupportBundle) return collectedBundle.Status == system.SupportBundleStatusCollected }, time.Second*2, time.Millisecond*100) - require.NotEmpty(t, collectedBundle.Filepath) - defer defaultFS.Remove(collectedBundle.Filepath) + filePath := collectedBundle.Filepath + require.NotEmpty(t, filePath) + defer defaultFS.Remove(filePath) assert.NotEmpty(t, collectedBundle.Sum) assert.Greater(t, collectedBundle.Size, uint32(0)) - exist, err := afero.Exists(defaultFS, collectedBundle.Filepath) + exist, err := afero.Exists(defaultFS, filePath) require.NoError(t, err) require.True(t, exist) @@ -173,4 +182,148 @@ func TestControllerStorage(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: modeController}, Status: system.SupportBundleStatusNone, }, object) + assert.Eventuallyf(t, func() bool { + exist, err := afero.Exists(defaultFS, filePath) + require.NoError(t, err) + return !exist + }, time.Second*2, time.Millisecond*100, "Supportbundle file %s was not deleted after deleting the Supportbundle object", filePath) +} + +type fakeAgentDumper struct { + returnErr error +} + +func (f *fakeAgentDumper) DumpFlows(basedir string) error { + return f.returnErr +} + +func (f *fakeAgentDumper) DumpHostNetworkInfo(basedir string) error { + return f.returnErr +} + +func (f *fakeAgentDumper) DumpLog(basedir string) error { + return f.returnErr +} + +func (f *fakeAgentDumper) DumpAgentInfo(basedir string) error { + return f.returnErr +} + +func (f *fakeAgentDumper) DumpNetworkPolicyResources(basedir string) error { + return f.returnErr +} + +func (f *fakeAgentDumper) DumpHeapPprof(basedir string) error { + return f.returnErr +} + +func (f *fakeAgentDumper) DumpOVSPorts(basedir string) error { + return f.returnErr +} + +func TestAgentStorage(t *testing.T) { + defaultFS = afero.NewMemMapFs() + defaultExecutor = new(testExec) + newAgentDumper = func(fs afero.Fs, executor exec.Interface, ovsCtlClient ovsctl.OVSCtlClient, aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolicyInfoQuerier, since string, v4Enabled, v6Enabled bool) support.AgentDumper { + return &fakeAgentDumper{} + } + defer func() { + defaultFS = afero.NewOsFs() + defaultExecutor = exec.New() + newAgentDumper = support.NewAgentDumper + }() + + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + fakeOVSCtl := ovsctltest.NewMockOVSCtlClient(ctrl) + fakeAgentQuerier := agentqueriertest.NewMockAgentQuerier(ctrl) + fakeNetworkPolicyQuerier := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl) + storage := NewAgentStorage(fakeOVSCtl, fakeAgentQuerier, fakeNetworkPolicyQuerier, true, true) + _, err := storage.SupportBundle.Create(ctx, &system.SupportBundle{ + ObjectMeta: metav1.ObjectMeta{ + Name: modeAgent, + }, + Status: system.SupportBundleStatusNone, + Since: "-1h", + }, nil, nil) + require.NoError(t, err) + + var collectedBundle *system.SupportBundle + assert.Eventually(t, func() bool { + object, err := storage.SupportBundle.Get(ctx, modeAgent, nil) + require.NoError(t, err) + collectedBundle = object.(*system.SupportBundle) + return collectedBundle.Status == system.SupportBundleStatusCollected + }, time.Second*2, time.Millisecond*100) + require.NotEmpty(t, collectedBundle.Filepath) + filePath := collectedBundle.Filepath + defer defaultFS.Remove(filePath) + assert.NotEmpty(t, collectedBundle.Sum) + assert.Greater(t, collectedBundle.Size, uint32(0)) + exist, err := afero.Exists(defaultFS, filePath) + require.NoError(t, err) + require.True(t, exist) + + _, deleted, err := storage.SupportBundle.Delete(ctx, modeAgent, nil, nil) + assert.NoError(t, err) + assert.True(t, deleted) + object, err := storage.SupportBundle.Get(ctx, modeAgent, nil) + assert.NoError(t, err) + assert.Equal(t, &system.SupportBundle{ + ObjectMeta: metav1.ObjectMeta{Name: modeAgent}, + Status: system.SupportBundleStatusNone, + }, object) + assert.Eventuallyf(t, func() bool { + exist, err := afero.Exists(defaultFS, filePath) + require.NoError(t, err) + return !exist + }, time.Second*2, time.Millisecond*100, "Supportbundle file %s was not deleted after deleting the Supportbundle object", filePath) +} + +func TestAgentStorageFailure(t *testing.T) { + defaultFS = afero.NewMemMapFs() + defaultExecutor = new(testExec) + newAgentDumper = func(fs afero.Fs, executor exec.Interface, ovsCtlClient ovsctl.OVSCtlClient, aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolicyInfoQuerier, since string, v4Enabled, v6Enabled bool) support.AgentDumper { + return &fakeAgentDumper{returnErr: fmt.Errorf("iptables not found")} + } + defer func() { + defaultFS = afero.NewOsFs() + defaultExecutor = exec.New() + newAgentDumper = support.NewAgentDumper + }() + + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + fakeOVSCtl := ovsctltest.NewMockOVSCtlClient(ctrl) + fakeAgentQuerier := agentqueriertest.NewMockAgentQuerier(ctrl) + fakeNetworkPolicyQuerier := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl) + + storage := NewAgentStorage(fakeOVSCtl, fakeAgentQuerier, fakeNetworkPolicyQuerier, true, true) + _, err := storage.SupportBundle.Create(ctx, &system.SupportBundle{ + ObjectMeta: metav1.ObjectMeta{ + Name: modeAgent, + }, + Status: system.SupportBundleStatusNone, + Since: "-1h", + }, nil, nil) + require.NoError(t, err) + + var collectedBundle *system.SupportBundle + assert.Eventually(t, func() bool { + object, err := storage.SupportBundle.Get(ctx, modeAgent, nil) + require.NoError(t, err) + collectedBundle = object.(*system.SupportBundle) + return collectedBundle.Status == system.SupportBundleStatusNone + }, time.Second*2, time.Millisecond*100) + assert.Empty(t, collectedBundle.Filepath) + assert.Empty(t, collectedBundle.Sum) + assert.Empty(t, collectedBundle.Size) + + _, err = storage.SupportBundle.Get(ctx, modeController, nil) + assert.Equal(t, errors.NewNotFound(system.Resource("supportBundle"), modeController), err) + _, deleted, err := storage.SupportBundle.Delete(ctx, modeController, nil, nil) + assert.Equal(t, errors.NewNotFound(system.Resource("supportBundle"), modeController), err) + assert.False(t, deleted) }