Fix nil pointer error when collecting agent supportbundle fails

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.

#2598 tried to fix it but did
the opposite.

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

pkg/agent/apiserver/apiserver.go

@@ -42,6 +42,7 @@ import (
 	"antrea.io/antrea/pkg/agent/apiserver/handlers/podinterface"
 	"antrea.io/antrea/pkg/agent/apiserver/handlers/serviceexternalip"
 	agentquerier "antrea.io/antrea/pkg/agent/querier"
+	"antrea.io/antrea/pkg/agent/util/iptables"
 	systeminstall "antrea.io/antrea/pkg/apis/system/install"
 	systemv1beta1 "antrea.io/antrea/pkg/apis/system/v1beta1"
 	"antrea.io/antrea/pkg/apiserver/handlers/loglevel"
@@ -90,7 +91,11 @@ func installHandlers(aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolic
 func installAPIGroup(s *genericapiserver.GenericAPIServer, aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolicyInfoQuerier, v4Enabled, v6Enabled bool) error {
 	systemGroup := genericapiserver.NewDefaultAPIGroupInfo(systemv1beta1.GroupName, scheme, metav1.ParameterCodec, codecs)
 	systemStorage := map[string]rest.Storage{}
-	supportBundleStorage := supportbundle.NewAgentStorage(ovsctl.NewClient(aq.GetNodeConfig().OVSBridge), aq, npq, v4Enabled, v6Enabled)
+	iptables, err := iptables.New(v4Enabled, v6Enabled)
+	if err != nil {
+		return fmt.Errorf("failed to new iptables client: %v", err)
+	}
+	supportBundleStorage := supportbundle.NewAgentStorage(ovsctl.NewClient(aq.GetNodeConfig().OVSBridge), aq, npq, iptables)
 	systemStorage["supportbundles"] = supportBundleStorage.SupportBundle
 	systemStorage["supportbundles/download"] = supportBundleStorage.Download
 	systemGroup.VersionedResourcesStorageMap["v1beta1"] = systemStorage

pkg/apiserver/registry/system/supportbundle/rest.go

@@ -37,6 +37,7 @@ import (
 	"k8s.io/utils/exec"
 
 	agentquerier "antrea.io/antrea/pkg/agent/querier"
+	"antrea.io/antrea/pkg/agent/util/iptables"
 	systemv1beta1 "antrea.io/antrea/pkg/apis/system/v1beta1"
 	"antrea.io/antrea/pkg/ovs/ovsctl"
 	"antrea.io/antrea/pkg/querier"
@@ -71,7 +72,7 @@ func NewControllerStorage() Storage {
 }
 
 // NewAgentStorage creates a support bundle storage for working on antrea agent.
-func NewAgentStorage(client ovsctl.OVSCtlClient, aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolicyInfoQuerier, v4Enabled, v6Enabled bool) Storage {
+func NewAgentStorage(client ovsctl.OVSCtlClient, aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolicyInfoQuerier, iptables iptables.Interface) Storage {
 	bundle := &supportBundleREST{
 		mode:         modeAgent,
 		ovsCtlClient: client,
@@ -81,8 +82,7 @@ func NewAgentStorage(client ovsctl.OVSCtlClient, aq agentquerier.AgentQuerier, n
 			ObjectMeta: metav1.ObjectMeta{Name: modeAgent},
 			Status:     systemv1beta1.SupportBundleStatusNone,
 		},
-		v4Enabled: v4Enabled,
-		v6Enabled: v6Enabled,
+		iptables: iptables,
 	}
 	return Storage{
 		Mode:          modeAgent,
@@ -116,8 +116,7 @@ type supportBundleREST struct {
 	ovsCtlClient ovsctl.OVSCtlClient
 	aq           agentquerier.AgentQuerier
 	npq          querier.AgentNetworkPolicyInfoQuerier
-	v4Enabled    bool
-	v6Enabled    bool
+	iptables     iptables.Interface
 }
 
 // Create triggers a bundle generation. It only allows resource creation when
@@ -164,7 +163,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 +256,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 := support.NewAgentDumper(defaultFS, defaultExecutor, r.ovsCtlClient, r.aq, r.npq, r.iptables, since)
 	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,13 @@ import (
 	"k8s.io/utils/exec"
 	exectesting "k8s.io/utils/exec/testing"
 
+	agentqueriertest "antrea.io/antrea/pkg/agent/querier/testing"
+	iptablestest "antrea.io/antrea/pkg/agent/util/iptables/testing"
+	clusterinformationv1beta1 "antrea.io/antrea/pkg/apis/crd/v1beta1"
 	system "antrea.io/antrea/pkg/apis/system/v1beta1"
+	ovsctltest "antrea.io/antrea/pkg/ovs/ovsctl/testing"
+	"antrea.io/antrea/pkg/querier"
+	queriertest "antrea.io/antrea/pkg/querier/testing"
 )
 
 type testExec struct {
@@ -174,3 +181,110 @@ func TestControllerStorage(t *testing.T) {
 		Status:     system.SupportBundleStatusNone,
 	}, object)
 }
+
+func TestNewAgentStorage(t *testing.T) {
+	defaultFS = afero.NewMemMapFs()
+	defaultExecutor = new(testExec)
+	defer func() {
+		defaultFS = afero.NewOsFs()
+		defaultExecutor = exec.New()
+	}()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	fakeOVSCtl := ovsctltest.NewMockOVSCtlClient(ctrl)
+	fakeAgentQuerier := agentqueriertest.NewMockAgentQuerier(ctrl)
+	fakeNetworkPolicyQuerier := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl)
+	fakeIPTables := iptablestest.NewMockInterface(ctrl)
+	fakeOVSCtl.EXPECT().DumpFlows()
+	fakeOVSCtl.EXPECT().DumpPortsDesc()
+	fakeNetworkPolicyQuerier.EXPECT().GetAddressGroups()
+	fakeNetworkPolicyQuerier.EXPECT().GetAppliedToGroups()
+	fakeNetworkPolicyQuerier.EXPECT().GetNetworkPolicies(&querier.NetworkPolicyQueryFilter{})
+	fakeAgentQuerier.EXPECT().GetAgentInfo(&clusterinformationv1beta1.AntreaAgentInfo{}, false)
+	fakeIPTables.EXPECT().Save()
+	storage := NewAgentStorage(fakeOVSCtl, fakeAgentQuerier, fakeNetworkPolicyQuerier, fakeIPTables)
+	_, err := storage.SupportBundle.Create(context.TODO(), &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(context.TODO(), 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(collectedBundle.Filepath)
+	assert.NotEmpty(t, collectedBundle.Sum)
+	assert.Greater(t, collectedBundle.Size, uint32(0))
+	exist, err := afero.Exists(defaultFS, collectedBundle.Filepath)
+	require.NoError(t, err)
+	require.True(t, exist)
+
+	_, deleted, err := storage.SupportBundle.Delete(context.TODO(), modeAgent, nil, nil)
+	assert.NoError(t, err)
+	assert.True(t, deleted)
+	object, err := storage.SupportBundle.Get(context.TODO(), 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 TestNewAgentStorageFailure(t *testing.T) {
+	defaultFS = afero.NewMemMapFs()
+	defaultExecutor = new(testExec)
+	defer func() {
+		defaultFS = afero.NewOsFs()
+		defaultExecutor = exec.New()
+	}()
+
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+	fakeOVSCtl := ovsctltest.NewMockOVSCtlClient(ctrl)
+	fakeAgentQuerier := agentqueriertest.NewMockAgentQuerier(ctrl)
+	fakeNetworkPolicyQuerier := queriertest.NewMockAgentNetworkPolicyInfoQuerier(ctrl)
+	fakeIPTables := iptablestest.NewMockInterface(ctrl)
+	fakeIPTables.EXPECT().Save().Return(nil, fmt.Errorf("iptables not found"))
+
+	storage := NewAgentStorage(fakeOVSCtl, fakeAgentQuerier, fakeNetworkPolicyQuerier, fakeIPTables)
+	_, err := storage.SupportBundle.Create(context.TODO(), &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(context.TODO(), 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(context.TODO(), modeController, nil)
+	assert.Equal(t, errors.NewNotFound(system.Resource("supportBundle"), modeController), err)
+	_, deleted, err := storage.SupportBundle.Delete(context.TODO(), modeController, nil, nil)
+	assert.Equal(t, errors.NewNotFound(system.Resource("supportBundle"), modeController), err)
+	assert.False(t, deleted)
+}

pkg/support/dump.go

@@ -31,6 +31,7 @@ import (
 	"k8s.io/utils/exec"
 
 	agentquerier "antrea.io/antrea/pkg/agent/querier"
+	"antrea.io/antrea/pkg/agent/util/iptables"
 	clusterinformationv1beta1 "antrea.io/antrea/pkg/apis/crd/v1beta1"
 	controllerquerier "antrea.io/antrea/pkg/controller/querier"
 	"antrea.io/antrea/pkg/ovs/ovsctl"
@@ -281,6 +282,7 @@ type agentDumper struct {
 	aq           agentquerier.AgentQuerier
 	npq          querier.AgentNetworkPolicyInfoQuerier
 	since        string
+	iptables     iptables.Interface
 	v4Enabled    bool
 	v6Enabled    bool
 }
@@ -328,15 +330,14 @@ func (d *agentDumper) DumpOVSPorts(basedir string) error {
 	return writeFile(d.fs, filepath.Join(basedir, "ovsports"), "ports", []byte(strings.Join(portData, "\n")))
 }
 
-func NewAgentDumper(fs afero.Fs, executor exec.Interface, ovsCtlClient ovsctl.OVSCtlClient, aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolicyInfoQuerier, since string, v4Enabled, v6Enabled bool) AgentDumper {
+func NewAgentDumper(fs afero.Fs, executor exec.Interface, ovsCtlClient ovsctl.OVSCtlClient, aq agentquerier.AgentQuerier, npq querier.AgentNetworkPolicyInfoQuerier, iptables iptables.Interface, since string) AgentDumper {
 	return &agentDumper{
 		fs:           fs,
 		executor:     executor,
 		ovsCtlClient: ovsCtlClient,
 		aq:           aq,
 		npq:          npq,
+		iptables:     iptables,
 		since:        since,
-		v4Enabled:    v4Enabled,
-		v6Enabled:    v6Enabled,
 	}
 }

pkg/support/dump_others.go

@@ -22,7 +22,6 @@ import (
 	"path"
 	"path/filepath"
 
-	"antrea.io/antrea/pkg/agent/util/iptables"
 	"antrea.io/antrea/pkg/util/logdir"
 )
 
@@ -47,11 +46,7 @@ func (d *agentDumper) DumpHostNetworkInfo(basedir string) error {
 }
 
 func (d *agentDumper) dumpIPTables(basedir string) error {
-	c, err := iptables.New(d.v4Enabled, d.v6Enabled)
-	if err != nil {
-		return err
-	}
-	data, err := c.Save()
+	data, err := d.iptables.Save()
 	if err != nil {
 		return err
 	}