Fix nil pointer error when collecting agent supportbundle fails (antr…

…ea-io#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.

antrea-io#2598 tried to fix it but did
the opposite.

Signed-off-by: Quan Tian <qtian@vmware.com>

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,

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)
 }