Skip to content

Commit

Permalink
Fix leak in resourceimport_controller (#4251)
Browse files Browse the repository at this point in the history 
The ResourceImport reconciler only adds/updates installed ResourceImports to
the installedResImports cache, but never removes them when they are deleted
from the K8s API and all corresponding resources are deleted successfully.
This PR fixes the above issue.

Signed-off-by: Dyanngg <dingyang@vmware.com>
  • Loading branch information
@Dyanngg
Dyanngg committed Oct 17, 2022
1 parent 549e0fb commit cd4ccdd
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 44 deletions.
7 changes: 5 additions & 2 deletions multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,12 @@ func (r *ResourceImportReconciler) handleResImpDeleteForClusterNetworkPolicy(ctx
Name: acnpName,
},
}
if err := r.localClusterClient.Delete(ctx, acnp, &client.DeleteOptions{}); err != nil {
return ctrl.Result{}, client.IgnoreNotFound(err)
err := client.IgnoreNotFound(r.localClusterClient.Delete(ctx, acnp, &client.DeleteOptions{}))
if err != nil {
klog.ErrorS(err, "Failed to delete imported ACNP", "acnp", acnpName)
return ctrl.Result{}, err
}
r.installedResImports.Delete(*resImp)
return ctrl.Result{}, nil
}

Expand Down
3 changes: 3 additions & 0 deletions multicluster/controllers/multicluster/commonarea/acnp_resourceimport_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,9 @@ func TestResourceImportReconciler_handleCopySpanACNPDeleteEvent(t *testing.T) {
if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: common.AntreaMCSPrefix + acnpImportName}, acnp); !apierrors.IsNotFound(err) {
t.Errorf("ResourceImport Reconciler should delete ACNP successfully but got error = %v", err)
}
if _, exists, _ := r.installedResImports.Get(*acnpResImport); exists {
t.Errorf("Reconciler should delete ResImport from installedResImports after successful resource deletion")
}
}

func TestResourceImportReconciler_handleCopySpanACNPUpdateEvent(t *testing.T) {
Expand Down
9 changes: 7 additions & 2 deletions multicluster/controllers/multicluster/commonarea/clusterinfo_importer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,13 @@ func (r *ResourceImportReconciler) handleResImpUpdateForClusterInfo(ctx context.
func (r *ResourceImportReconciler) handleResImpDeleteForClusterInfo(ctx context.Context, req ctrl.Request, resImp *mcsv1alpha1.ResourceImport) (ctrl.Result, error) {
clusterInfoImport, clusterInfoImportName := newClusterInfoImport(req.Name, r.namespace)
klog.InfoS("Deleting ClusterInfoImport", "clusterinfoimport", clusterInfoImportName.String())
err := r.localClusterClient.Delete(ctx, clusterInfoImport, &client.DeleteOptions{})
return ctrl.Result{}, client.IgnoreNotFound(err)
err := client.IgnoreNotFound(r.localClusterClient.Delete(ctx, clusterInfoImport, &client.DeleteOptions{}))
if err != nil {
klog.ErrorS(err, "Failed to delete imported ClusterInfo", "clusterInfo", clusterInfoImportName)
return ctrl.Result{}, err
}
r.installedResImports.Delete(*resImp)
return ctrl.Result{}, nil
}

func newClusterInfoImport(name, namespace string) (*mcsv1alpha1.ClusterInfoImport, types.NamespacedName) {
Expand Down
77 changes: 42 additions & 35 deletions multicluster/controllers/multicluster/commonarea/clusterinfo_importer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"reflect"
"testing"

"github.com/stretchr/testify/assert"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -149,26 +150,30 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {
}

tests := []struct {
name string
existingCIResImport *mcsv1alpha1.ResourceImport
existingCIImport *mcsv1alpha1.ClusterInfoImport
expectedCIImport *mcsv1alpha1.ClusterInfoImport
isDelete bool
name string
existingCIResImport *mcsv1alpha1.ResourceImport
existingCIImport *mcsv1alpha1.ClusterInfoImport
expectedCIImport *mcsv1alpha1.ClusterInfoImport
isDelete bool
expectedInstalledResImpSize int
}{
{
name: "create ClusterInfoImport successfully",
existingCIResImport: ciResImportA,
expectedCIImport: &ciImportA,
name: "create ClusterInfoImport successfully",
existingCIResImport: ciResImportA,
expectedCIImport: &ciImportA,
expectedInstalledResImpSize: 1,
},
{
name: "skip import empty ResourceImport",
existingCIResImport: ciResImportEmptySpec,
expectedCIImport: nil,
name: "skip import empty ResourceImport",
existingCIResImport: ciResImportEmptySpec,
expectedCIImport: nil,
expectedInstalledResImpSize: 1,
},
{
name: "skip import ResourceImport from local",
existingCIResImport: ciResImportLocal,
expectedCIImport: nil,
name: "skip import ResourceImport from local",
existingCIResImport: ciResImportLocal,
expectedCIImport: nil,
expectedInstalledResImpSize: 1,
},
{
name: "update ClusterInfoImport successfully",
Expand All @@ -181,12 +186,14 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {
},
Spec: clusterBInfoNew,
},
expectedInstalledResImpSize: 1,
},
{
name: "delete ClusterInfoImport successfully",
existingCIResImport: ciResImportC,
existingCIImport: &ciImportC,
isDelete: true,
name: "delete ClusterInfoImport successfully",
existingCIResImport: ciResImportC,
existingCIImport: &ciImportC,
isDelete: true,
expectedInstalledResImpSize: 0,
},
}

Expand All @@ -202,8 +209,8 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {
}
remoteCluster := NewFakeRemoteCommonArea(fakeRemoteClient, "leader-cluster", "cluster-d", "default")
r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, "cluster-d", "default", remoteCluster)
if tt.isDelete {
r.installedResImports.Add(*ciResImportC)
if tt.existingCIResImport != nil {
r.installedResImports.Add(*tt.existingCIResImport)
}
ciResImportName := types.NamespacedName{
Namespace: tt.existingCIResImport.Namespace,
Expand All @@ -213,23 +220,23 @@ func TestResourceImportReconciler_handleClusterInfo(t *testing.T) {

if _, err := r.Reconcile(ctx, req); err != nil {
t.Errorf("ClusterInfo Importer should handle ResourceImport events successfully but got error = %v", err)
} else {
gotCIImp := &mcsv1alpha1.ClusterInfoImport{}
err := fakeClient.Get(ctx, ciResImportName, gotCIImp)
isNotFound := apierrors.IsNotFound(err)
if err != nil {
if tt.expectedCIImport == nil && !isNotFound {
t.Errorf("Expected to get not found error but got error = %v", err)
}
if tt.expectedCIImport != nil && isNotFound {
t.Errorf("Expected to get ClusterInfoImport %v but got not found error = %v", tt.expectedCIImport, err)
}
} else if tt.expectedCIImport != nil {
if !reflect.DeepEqual(tt.expectedCIImport.Spec, gotCIImp.Spec) {
t.Errorf("Expected ClusterInfoImport %v but got %v", tt.expectedCIImport.Spec, gotCIImp.Spec)
}
}
gotCIImp := &mcsv1alpha1.ClusterInfoImport{}
err := fakeClient.Get(ctx, ciResImportName, gotCIImp)
isNotFound := apierrors.IsNotFound(err)
if err != nil {
if tt.expectedCIImport == nil && !isNotFound {
t.Errorf("Expected to get not found error but got error = %v", err)
}
if tt.expectedCIImport != nil && isNotFound {
t.Errorf("Expected to get ClusterInfoImport %v but got not found error = %v", tt.expectedCIImport, err)
}
} else if tt.expectedCIImport != nil {
if !reflect.DeepEqual(tt.expectedCIImport.Spec, gotCIImp.Spec) {
t.Errorf("Expected ClusterInfoImport %v but got %v", tt.expectedCIImport.Spec, gotCIImp.Spec)
}
}
assert.Equal(t, tt.expectedInstalledResImpSize, len(r.installedResImports.List()), "Unexpected number of installed ResImports after reconciliation")
})
}

Expand Down
17 changes: 12 additions & 5 deletions multicluster/controllers/multicluster/commonarea/resourceimport_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,7 @@ func (r *ResourceImportReconciler) handleResImpDeleteForService(ctx context.Cont
}
err := r.localClusterClient.Delete(ctx, svc, &client.DeleteOptions{})
if err != nil && !apierrors.IsNotFound(err) {
klog.ErrorS(err, "Failed to delete imported Service", "service", svcName)
return ctrl.Result{}, err
}

Expand All @@ -245,8 +246,13 @@ func (r *ResourceImportReconciler) handleResImpDeleteForService(ctx context.Cont
Name: resImp.Spec.Name,
},
}
err = r.localClusterClient.Delete(ctx, svcImp, &client.DeleteOptions{})
return ctrl.Result{}, client.IgnoreNotFound(err)
err = client.IgnoreNotFound(r.localClusterClient.Delete(ctx, svcImp, &client.DeleteOptions{}))
if err != nil {
klog.ErrorS(err, "Failed to delete ServiceImport for ResourceImport", "serviceImport", svcImpName)
return ctrl.Result{}, err
}
r.installedResImports.Delete(*resImp)
return ctrl.Result{}, nil
}

func (r *ResourceImportReconciler) handleResImpUpdateForEndpoints(ctx context.Context, resImp *multiclusterv1alpha1.ResourceImport) (ctrl.Result, error) {
Expand Down Expand Up @@ -316,11 +322,12 @@ func (r *ResourceImportReconciler) handleResImpDeleteForEndpoints(ctx context.Co
Namespace: resImp.Spec.Namespace,
},
}
err := r.localClusterClient.Delete(ctx, ep, &client.DeleteOptions{})
err := client.IgnoreNotFound(r.localClusterClient.Delete(ctx, ep, &client.DeleteOptions{}))
if err != nil {
klog.InfoS("Failed to delete imported Endpoints", "endpoints", epNamespacedName, "err", err)
return ctrl.Result{}, client.IgnoreNotFound(err)
klog.ErrorS(err, "Failed to delete imported Endpoints", "endpoints", epNamespacedName)
return ctrl.Result{}, err
}
r.installedResImports.Delete(*resImp)
return ctrl.Result{}, nil
}

Expand Down
6 changes: 6 additions & 0 deletions multicluster/controllers/multicluster/commonarea/resourceimport_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,11 +229,17 @@ func TestResourceImportReconciler_handleDeleteEvent(t *testing.T) {
if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "default", Name: "nginx"}, svcImp); !apierrors.IsNotFound(err) {
t.Errorf("ResourceImport Reconciler should delete a ServiceImport successfully but got error = %v", err)
}
if _, exists, _ := r.installedResImports.Get(*svcResImport); exists {
t.Errorf("Reconciler should delete ResImport from installedResImports after successful resource deletion")
}
case "Endpoints":
ep := &corev1.Endpoints{}
if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "default", Name: "antrea-mc-nginx"}, ep); !apierrors.IsNotFound(err) {
t.Errorf("ResourceImport Reconciler should delete an Endpoint successfully but got error = %v", err)
}
if _, exists, _ := r.installedResImports.Get(*epResImport); exists {
t.Errorf("Reconciler should delete ResImport from installedResImports after successful resource deletion")
}
}
}
})
Expand Down

0 comments on commit cd4ccdd

Please sign in to comment.