From 2c34d57e7b9c9baddcb96617ced88b4cc40e3a09 Mon Sep 17 00:00:00 2001 From: Yang Ding Date: Mon, 28 Feb 2022 13:08:46 -0800 Subject: [PATCH] Rebase and resolve comments Signed-off-by: Yang Ding --- .../v1alpha1/resourceexport_webhook.go | 2 +- .../commonarea/acnpimport_controller.go | 158 +++++++ .../commonarea/acnpimport_controller_test.go | 418 ++++++++++++++++++ .../commonarea/resourceimport_controller.go | 131 +----- .../resourceimport_controller_test.go | 378 ---------------- .../multicluster/resourceexport_controller.go | 30 +- 6 files changed, 595 insertions(+), 522 deletions(-) create mode 100644 multicluster/controllers/multicluster/commonarea/acnpimport_controller.go create mode 100644 multicluster/controllers/multicluster/commonarea/acnpimport_controller_test.go diff --git a/multicluster/apis/multicluster/v1alpha1/resourceexport_webhook.go b/multicluster/apis/multicluster/v1alpha1/resourceexport_webhook.go index 4429f182860..96770b999bf 100644 --- a/multicluster/apis/multicluster/v1alpha1/resourceexport_webhook.go +++ b/multicluster/apis/multicluster/v1alpha1/resourceexport_webhook.go @@ -58,7 +58,7 @@ func (r *ResourceExport) Default() { r.Labels[common.SourceKind] = common.AntreaClusterNetworkPolicyKind } if r.DeletionTimestamp.IsZero() && !common.StringExistsInSlice(r.Finalizers, common.ResourceExportFinalizer) { - r.Finalizers = []string{common.ResourceExportFinalizer} + r.Finalizers = append(r.Finalizers, common.ResourceExportFinalizer) } } diff --git a/multicluster/controllers/multicluster/commonarea/acnpimport_controller.go b/multicluster/controllers/multicluster/commonarea/acnpimport_controller.go new file mode 100644 index 00000000000..4fc4abfec9b --- /dev/null +++ b/multicluster/controllers/multicluster/commonarea/acnpimport_controller.go @@ -0,0 +1,158 @@ +/* +Copyright 2022 Antrea Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package commonarea + +import ( + "context" + "errors" + + apiequality "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "k8s.io/klog/v2" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + multiclusterv1alpha1 "antrea.io/antrea/multicluster/apis/multicluster/v1alpha1" + "antrea.io/antrea/multicluster/controllers/multicluster/common" + "antrea.io/antrea/pkg/apis/crd/v1alpha1" +) + +func (r *ResourceImportReconciler) handleResImpUpdateForClusterNetworkPolicy(ctx context.Context, resImp *multiclusterv1alpha1.ResourceImport) (ctrl.Result, error) { + acnpImpName := types.NamespacedName{ + Namespace: "", + Name: resImp.Spec.Name, + } + acnpName := types.NamespacedName{ + Namespace: "", + Name: common.AntreaMCSPrefix + resImp.Spec.Name, + } + klog.InfoS("Updating ACNP and ACNPImport corresponding to ResourceImport", + "acnp", acnpName.String(), "resourceimport", klog.KObj(resImp)) + + acnp := &v1alpha1.ClusterNetworkPolicy{} + err := r.localClusterClient.Get(ctx, acnpName, acnp) + acnpNotFound := apierrors.IsNotFound(err) + if err != nil && !acnpNotFound { + return ctrl.Result{}, err + } + if !acnpNotFound { + if _, ok := acnp.Annotations[common.AntreaMCACNPAnnotation]; !ok { + err := errors.New("unable to import Antrea ClusterNetworkPolicy which conflicts with existing one") + klog.ErrorS(err, "", "acnp", klog.KObj(acnp)) + return ctrl.Result{}, err + } + } + acnpObj := getMCAntreaClusterPolicy(resImp) + tierKind, tierName := &v1alpha1.Tier{}, acnpObj.Spec.Tier + err = r.localClusterClient.Get(ctx, types.NamespacedName{Namespace: "", Name: tierName}, tierKind) + tierNotFound := apierrors.IsNotFound(err) + if !tierNotFound { + if acnpNotFound { + if err = r.localClusterClient.Create(ctx, acnpObj, &client.CreateOptions{}); err != nil { + klog.ErrorS(err, "failed to create imported Antrea ClusterNetworkPolicy", "acnp", klog.KObj(acnpObj)) + return ctrl.Result{}, err + } + } else if !apiequality.Semantic.DeepEqual(acnp.Spec, acnpObj.Spec) { + acnp.Spec = acnpObj.Spec + if err = r.localClusterClient.Update(ctx, acnp, &client.UpdateOptions{}); err != nil { + klog.ErrorS(err, "failed to update imported Antrea ClusterNetworkPolicy", "acnp", klog.KObj(acnpObj)) + return ctrl.Result{}, err + } + } + } else if tierNotFound && !acnpNotFound { + if err = r.localClusterClient.Delete(ctx, acnpObj, &client.DeleteOptions{}); err != nil { + klog.ErrorS(err, "failed to delete imported Antrea ClusterNetworkPolicy that no longer has a valid Tier for the current cluster", "acnp", klog.KObj(acnpObj)) + return ctrl.Result{}, err + } + } + acnpImp := &multiclusterv1alpha1.ACNPImport{} + err = r.localClusterClient.Get(ctx, acnpImpName, acnpImp) + acnpImpNotFound := apierrors.IsNotFound(err) + if err != nil && !acnpImpNotFound { + klog.ErrorS(err, "failed to get existing ACNPImports") + return ctrl.Result{}, err + } + acnpImpObj := getACNPImport(resImp, tierNotFound) + if acnpImpNotFound { + err := r.localClusterClient.Create(ctx, acnpImpObj, &client.CreateOptions{}) + if err != nil { + klog.ErrorS(err, "failed to create ACNPImport", "acnpimport", klog.KObj(acnpImpObj)) + return ctrl.Result{}, err + } + r.installedResImports.Add(*resImp) + } + patchACNPImportStatus := false + if len(acnpImp.Status.Conditions) == 0 { + acnpImp.Status = acnpImpObj.Status + patchACNPImportStatus = true + } else { + if acnpImp.Status.Conditions[0].Status != acnpImpObj.Status.Conditions[0].Status { + acnpImp.Status = acnpImpObj.Status + patchACNPImportStatus = true + } + } + if patchACNPImportStatus { + if err := r.localClusterClient.Status().Update(ctx, acnpImp); err != nil { + klog.ErrorS(err, "failed to update acnpImport status", "acnpImport", klog.KObj(acnpImp)) + } + } + return ctrl.Result{}, nil +} + +func (r *ResourceImportReconciler) handleResImpDeleteForClusterNetworkPolicy(ctx context.Context, resImp *multiclusterv1alpha1.ResourceImport) (ctrl.Result, error) { + acnpImpName := types.NamespacedName{ + Namespace: "", + Name: resImp.Spec.Name, + } + acnpName := types.NamespacedName{ + Namespace: "", + Name: common.AntreaMCSPrefix + resImp.Spec.Name, + } + klog.InfoS("Deleting ACNP and ACNPImport corresponding to ResourceImport", + "acnp", acnpName.String(), "resourceimport", klog.KObj(resImp)) + + var err error + cleanupACNPImport := func() (ctrl.Result, error) { + acnpImp := &multiclusterv1alpha1.ACNPImport{} + err = r.localClusterClient.Get(ctx, acnpImpName, acnpImp) + if err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + err = r.localClusterClient.Delete(ctx, acnpImp, &client.DeleteOptions{}) + if err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + return ctrl.Result{}, nil + } + + acnp := &v1alpha1.ClusterNetworkPolicy{} + err = r.localClusterClient.Get(ctx, acnpName, acnp) + if err != nil { + if apierrors.IsNotFound(err) { + klog.V(2).InfoS("ACNP corresponding to ResourceImport has already been deleted", + "acnp", acnpName.String(), "resourceimport", klog.KObj(resImp)) + return cleanupACNPImport() + } + return ctrl.Result{}, err + } + err = r.localClusterClient.Delete(ctx, acnp, &client.DeleteOptions{}) + if err != nil { + return ctrl.Result{}, err + } + return cleanupACNPImport() +} \ No newline at end of file diff --git a/multicluster/controllers/multicluster/commonarea/acnpimport_controller_test.go b/multicluster/controllers/multicluster/commonarea/acnpimport_controller_test.go new file mode 100644 index 00000000000..f396cf35a6b --- /dev/null +++ b/multicluster/controllers/multicluster/commonarea/acnpimport_controller_test.go @@ -0,0 +1,418 @@ +/* +Copyright 2022 Antrea Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package commonarea + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + mcsv1alpha1 "antrea.io/antrea/multicluster/apis/multicluster/v1alpha1" + "antrea.io/antrea/multicluster/controllers/multicluster/common" + "antrea.io/antrea/pkg/apis/crd/v1alpha1" +) + +var ( + acnpImportName = "acnp-for-isolation" + acnpResImportName = leaderNamespace + "-" + acnpImportName + + acnpImpReq = ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: leaderNamespace, + Name: acnpResImportName, + }} + acnpImpNoMatchingTierReq = ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: leaderNamespace, + Name: "default-acnp-no-matching-tier", + }} + + allowAction = v1alpha1.RuleActionAllow + dropAction = v1alpha1.RuleActionDrop + securityOpsTier = &v1alpha1.Tier{ + ObjectMeta: metav1.ObjectMeta{ + Name: "securityops", + }, + Spec: v1alpha1.TierSpec{ + Priority: int32(100), + Description: "[READ-ONLY]: System generated SecurityOps Tier", + }, + } + acnpResImport = &mcsv1alpha1.ResourceImport{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: leaderNamespace, + Name: acnpResImportName, + }, + Spec: mcsv1alpha1.ResourceImportSpec{ + Name: acnpImportName, + Kind: common.AntreaClusterNetworkPolicyKind, + ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ + Tier: "securityops", + Priority: 1.0, + AppliedTo: []v1alpha1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{}}, + }, + Ingress: []v1alpha1.Rule{ + { + Action: &dropAction, + From: []v1alpha1.NetworkPolicyPeer{ + { + Namespaces: &v1alpha1.PeerNamespaces{ + Match: v1alpha1.NamespaceMatchSelf, + }, + }, + }, + }, + }, + }, + }, + } + acnpResImportNoMatchingTier = &mcsv1alpha1.ResourceImport{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: leaderNamespace, + Name: "default-acnp-no-matching-tier", + }, + Spec: mcsv1alpha1.ResourceImportSpec{ + Name: "acnp-no-matching-tier", + Kind: common.AntreaClusterNetworkPolicyKind, + ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ + Tier: "somerandomtier", + Priority: 1.0, + AppliedTo: []v1alpha1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{}}, + }, + }, + }, + } +) + +func TestResourceImportReconciler_handleCopySpanACNPCreateEvent(t *testing.T) { + remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) + go remoteMgr.Start() + defer remoteMgr.Stop() + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(securityOpsTier).Build() + fakeRemoteClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(acnpResImport, acnpResImportNoMatchingTier).Build() + remoteCluster := NewFakeRemoteCommonArea(scheme, &remoteMgr, fakeRemoteClient, "leader-cluster", "default") + + tests := []struct { + name string + acnpImportName string + req ctrl.Request + expectedSuccess bool + }{ + { + name: "import ACNP of pre-defined tiers", + acnpImportName: acnpImportName, + req: acnpImpReq, + expectedSuccess: true, + }, + { + name: "import ACNP of non-existing tier", + acnpImportName: "acnp-no-matching-tier", + req: acnpImpNoMatchingTierReq, + expectedSuccess: false, + }, + } + r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, localClusterID, remoteCluster) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if _, err := r.Reconcile(ctx, tt.req); err != nil { + if err != nil { + t.Errorf("ResourceImport Reconciler should handle ACNP create event successfully but got error = %v", err) + } + } else { + acnp := &v1alpha1.ClusterNetworkPolicy{} + err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: common.AntreaMCSPrefix + tt.acnpImportName}, acnp) + if tt.expectedSuccess && err != nil { + t.Errorf("ResourceImport Reconciler should import an ACNP successfully but got error = %v", err) + } else if !tt.expectedSuccess && (err == nil || !apierrors.IsNotFound(err)) { + t.Errorf("ResourceImport Reconciler should not import an ACNP whose Tier does not exist in current cluster. Expected NotFound error. Actual err = %v", err) + } + acnpImport := &mcsv1alpha1.ACNPImport{} + if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: tt.acnpImportName}, acnpImport); err != nil { + t.Errorf("ResourceImport Reconciler should create ACNPImport for ACNP type resouc") + } + status := acnpImport.Status.Conditions + if len(status) > 0 && status[0].Type == mcsv1alpha1.ACNPImportRealizable { + if tt.expectedSuccess && status[0].Status != corev1.ConditionTrue { + t.Errorf("ACNPImport %v realizable status should be True but is %v instead", acnpImportName, status[0].Status) + } else if !tt.expectedSuccess && status[0].Status != corev1.ConditionFalse { + t.Errorf("ACNPImport %v realizable status should be False but is %v instead", acnpImportName, status[0].Status) + } + } else { + t.Errorf("No realizable status provided for ACNPImport %v", acnpImportName) + } + } + }) + } +} + +func TestResourceImportReconciler_handleCopySpanACNPDeleteEvent(t *testing.T) { + remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) + go remoteMgr.Start() + defer remoteMgr.Stop() + + existingACNP := &v1alpha1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.AntreaMCSPrefix + acnpImportName, + }, + } + existingACNPImport := &mcsv1alpha1.ACNPImport{ + ObjectMeta: metav1.ObjectMeta{ + Name: acnpImportName, + }, + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existingACNP, existingACNPImport).Build() + fakeRemoteClient := fake.NewClientBuilder().WithScheme(scheme).Build() + remoteCluster := NewFakeRemoteCommonArea(scheme, &remoteMgr, fakeRemoteClient, "leader-cluster", "default") + + r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, localClusterID, remoteCluster) + r.installedResImports.Add(*acnpResImport) + + if _, err := r.Reconcile(ctx, acnpImpReq); err != nil { + t.Errorf("ResourceImport Reconciler should handle ACNP ResourceImport delete event successfully but got error = %v", err) + } + acnp := &v1alpha1.ClusterNetworkPolicy{} + 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) + } + acnpImport := &mcsv1alpha1.ACNPImport{} + if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: acnpImportName}, acnpImport); !apierrors.IsNotFound(err) { + t.Errorf("ResourceImport Reconciler should delete ACNPImport successfully but got error = %v", err) + } +} + + +func TestResourceImportReconciler_handleCopySpanACNPUpdateEvent(t *testing.T) { + remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) + go remoteMgr.Start() + defer remoteMgr.Stop() + + existingACNP1 := &v1alpha1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.AntreaMCSPrefix + acnpImportName, + Annotations: map[string]string{common.AntreaMCACNPAnnotation: "true"}, + }, + Spec: v1alpha1.ClusterNetworkPolicySpec{ + Tier: "securityops", + Priority: 1.0, + AppliedTo: []v1alpha1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{}}, + }, + Ingress: []v1alpha1.Rule{ + { + Action: &allowAction, + From: []v1alpha1.NetworkPolicyPeer{ + { + Namespaces: &v1alpha1.PeerNamespaces{ + Match: v1alpha1.NamespaceMatchSelf, + }, + }, + }, + }, + }, + }, + } + existingACNPImport1 := &mcsv1alpha1.ACNPImport{ + ObjectMeta: metav1.ObjectMeta{ + Name: acnpImportName, + }, + Status: mcsv1alpha1.ACNPImportStatus{ + Conditions: []mcsv1alpha1.ACNPImportCondition{ + { + Type: mcsv1alpha1.ACNPImportRealizable, + Status: corev1.ConditionTrue, + }, + }, + }, + } + existingACNPImport2 := &mcsv1alpha1.ACNPImport{ + ObjectMeta: metav1.ObjectMeta{ + Name: "acnp-no-matching-tier", + }, + Status: mcsv1alpha1.ACNPImportStatus{ + Conditions: []mcsv1alpha1.ACNPImportCondition{ + { + Type: mcsv1alpha1.ACNPImportRealizable, + Status: corev1.ConditionFalse, + }, + }, + }, + } + updatedResImport2 := &mcsv1alpha1.ResourceImport{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: leaderNamespace, + Name: "default-acnp-no-matching-tier", + }, + Spec: mcsv1alpha1.ResourceImportSpec{ + Name: "acnp-no-matching-tier", + Kind: common.AntreaClusterNetworkPolicyKind, + ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ + Tier: "securityops", + Priority: 1.0, + AppliedTo: []v1alpha1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{}}, + }, + }, + }, + } + existingACNP3 := &v1alpha1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.AntreaMCSPrefix + "valid-updated-to-no-valid", + Annotations: map[string]string{common.AntreaMCACNPAnnotation: "true"}, + }, + Spec: v1alpha1.ClusterNetworkPolicySpec{ + Tier: "securityops", + Priority: 1.0, + AppliedTo: []v1alpha1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{}}, + }, + }, + } + existingACNPImport3 := &mcsv1alpha1.ACNPImport{ + ObjectMeta: metav1.ObjectMeta{ + Name: "valid-updated-to-no-valid", + }, + Status: mcsv1alpha1.ACNPImportStatus{ + Conditions: []mcsv1alpha1.ACNPImportCondition{ + { + Type: mcsv1alpha1.ACNPImportRealizable, + Status: corev1.ConditionTrue, + }, + }, + }, + } + updatedResImport3 := &mcsv1alpha1.ResourceImport{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: leaderNamespace, + Name: "default-valid-updated-to-no-valid", + }, + Spec: mcsv1alpha1.ResourceImportSpec{ + Name: acnpImportName, + Kind: common.AntreaClusterNetworkPolicyKind, + ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ + Tier: "somerandomtier", + Priority: 1.0, + AppliedTo: []v1alpha1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{}}, + }, + }, + }, + } + acnpImp3Req := ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: leaderNamespace, + Name: "default-valid-updated-to-no-valid", + }} + acnpImp4Req := ctrl.Request{NamespacedName: types.NamespacedName{ + Namespace: leaderNamespace, + Name: "default-name-conflict", + }} + existingACNP4 := &v1alpha1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: common.AntreaMCSPrefix + "name-conflict", + }, + Spec: v1alpha1.ClusterNetworkPolicySpec{ + Tier: "securityops", + Priority: 1.0, + AppliedTo: []v1alpha1.NetworkPolicyPeer{ + {NamespaceSelector: &metav1.LabelSelector{}}, + }, + }, + } + + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existingACNP1, existingACNPImport1, existingACNPImport2, + existingACNP3, existingACNPImport3, existingACNP4, securityOpsTier).Build() + fakeRemoteClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(acnpResImport, updatedResImport2, updatedResImport3).Build() + remoteCluster := NewFakeRemoteCommonArea(scheme, &remoteMgr, fakeRemoteClient, "leader-cluster", "default") + + r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, localClusterID, remoteCluster) + r.installedResImports.Add(*acnpResImport) + r.installedResImports.Add(*acnpResImportNoMatchingTier) + r.installedResImports.Add(*updatedResImport3) + + tests := []struct { + name string + acnpImportName string + req ctrl.Request + expectErr bool + expectImportSuccess bool + expectedUpdatedACNPSpec *v1alpha1.ClusterNetworkPolicySpec + }{ + { + name: "update acnp spec", + acnpImportName: acnpImportName, + req: acnpImpReq, + expectErr: false, + expectImportSuccess: true, + expectedUpdatedACNPSpec: acnpResImport.Spec.ClusterNetworkPolicy, + }, + { + name: "imported acnp missing tier update to valid tier", + acnpImportName: "acnp-no-matching-tier", + req: acnpImpNoMatchingTierReq, + expectErr: false, + expectImportSuccess: true, + expectedUpdatedACNPSpec: updatedResImport2.Spec.ClusterNetworkPolicy, + }, + { + name: "valid imported acnp update to missing tier", + req: acnpImp3Req, + acnpImportName: "valid-updated-to-no-valid", + expectErr: false, + expectImportSuccess: false, + expectedUpdatedACNPSpec: nil, + }, + { + name: "name conflict with existing acnp", + req: acnpImp4Req, + acnpImportName: "name-conflict", + expectErr: true, + expectImportSuccess: false, + expectedUpdatedACNPSpec: nil, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if _, err := r.Reconcile(ctx, tt.req); err != nil { + if tt.expectErr { + assert.Contains(t, err.Error(), "conflicts with existing one") + } else { + t.Errorf("ResourceImport Reconciler should handle update event successfully but got error = %v", err) + } + } else { + if tt.expectedUpdatedACNPSpec != nil { + acnp := &v1alpha1.ClusterNetworkPolicy{} + err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: common.AntreaMCSPrefix + tt.acnpImportName}, acnp) + if tt.expectImportSuccess && err != nil { + t.Errorf("ResourceImport Reconciler should import an ACNP successfully but got error = %v", err) + } else if !tt.expectImportSuccess && (err == nil || !apierrors.IsNotFound(err)) { + t.Errorf("ResourceImport Reconciler should not import an ACNP whose Tier does not exist in current cluster. Expected NotFound error. Actual err = %v", err) + } else if !reflect.DeepEqual(acnp.Spec, *tt.expectedUpdatedACNPSpec) { + t.Errorf("ACNP spec was not updated successfully") + } + } + } + }) + } +} \ No newline at end of file diff --git a/multicluster/controllers/multicluster/commonarea/resourceimport_controller.go b/multicluster/controllers/multicluster/commonarea/resourceimport_controller.go index 2485aa91e3d..e2ba32d3525 100644 --- a/multicluster/controllers/multicluster/commonarea/resourceimport_controller.go +++ b/multicluster/controllers/multicluster/commonarea/resourceimport_controller.go @@ -93,7 +93,7 @@ func NewResourceImportReconciler(client client.Client, scheme *runtime.Scheme, l // Reconcile will attempt to ensure that the imported Resource is installed in local cluster as per the // ResourceImport object. func (r *ResourceImportReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - klog.V(2).InfoS("reconciling ResourceImport", "resourceimport", req.NamespacedName) + klog.V(2).InfoS("Reconciling ResourceImport", "resourceimport", req.NamespacedName) // TODO: Must check whether this ResourceImport must be reconciled by this member cluster. Check `spec.clusters` field. if r.localClusterClient == nil { return ctrl.Result{}, errors.New("localClusterClient has not been initialized properly, no local cluster client") @@ -109,7 +109,7 @@ func (r *ResourceImportReconciler) Reconcile(ctx context.Context, req ctrl.Reque if err != nil { isDeleted = apierrors.IsNotFound(err) if !isDeleted { - klog.InfoS("unable to fetch ResourceImport", "resourceimport", req.NamespacedName.String(), "err", err) + klog.InfoS("Unable to fetch ResourceImport", "resourceimport", req.NamespacedName.String(), "err", err) return ctrl.Result{}, err } else { resImpObj, exist, err := r.installedResImports.GetByKey(req.NamespacedName.String()) @@ -351,133 +351,8 @@ func (r *ResourceImportReconciler) handleResImpDeleteForEndpoints(ctx context.Co return ctrl.Result{}, nil } -func (r *ResourceImportReconciler) handleResImpUpdateForClusterNetworkPolicy(ctx context.Context, resImp *multiclusterv1alpha1.ResourceImport) (ctrl.Result, error) { - acnpImpName := types.NamespacedName{ - Namespace: "", - Name: resImp.Spec.Name, - } - acnpName := types.NamespacedName{ - Namespace: "", - Name: common.AntreaMCSPrefix + resImp.Spec.Name, - } - klog.InfoS("Updating ACNP and ACNPImport corresponding to ResourceImport", - "acnp", acnpName.String(), "resourceimport", klog.KObj(resImp)) - - acnp := &v1alpha1.ClusterNetworkPolicy{} - err := r.localClusterClient.Get(ctx, acnpName, acnp) - acnpNotFound := apierrors.IsNotFound(err) - if err != nil && !acnpNotFound { - return ctrl.Result{}, err - } - if !acnpNotFound { - if _, ok := acnp.Annotations[common.AntreaMCACNPAnnotation]; !ok { - err := errors.New("unable to import Antrea ClusterNetworkPolicy which conflicts with existing one") - klog.ErrorS(err, "", "acnp", klog.KObj(acnp)) - return ctrl.Result{}, err - } - } - acnpObj := getMCAntreaClusterPolicy(resImp) - tierKind, tierName := &v1alpha1.Tier{}, acnpObj.Spec.Tier - err = r.localClusterClient.Get(ctx, types.NamespacedName{Namespace: "", Name: tierName}, tierKind) - tierNotFound := apierrors.IsNotFound(err) - if !tierNotFound { - if acnpNotFound { - if err = r.localClusterClient.Create(ctx, acnpObj, &client.CreateOptions{}); err != nil { - klog.ErrorS(err, "failed to create imported Antrea ClusterNetworkPolicy", "acnp", klog.KObj(acnpObj)) - return ctrl.Result{}, err - } - } else if !apiequality.Semantic.DeepEqual(acnp.Spec, acnpObj.Spec) { - acnp.Spec = acnpObj.Spec - if err = r.localClusterClient.Update(ctx, acnp, &client.UpdateOptions{}); err != nil { - klog.ErrorS(err, "failed to update imported Antrea ClusterNetworkPolicy", "acnp", klog.KObj(acnpObj)) - return ctrl.Result{}, err - } - } - } else if tierNotFound && !acnpNotFound { - if err = r.localClusterClient.Delete(ctx, acnpObj, &client.DeleteOptions{}); err != nil { - klog.ErrorS(err, "failed to delete imported Antrea ClusterNetworkPolicy that no longer have a valid Tier for the current cluster", "acnp", klog.KObj(acnpObj)) - return ctrl.Result{}, err - } - } - acnpImp := &multiclusterv1alpha1.ACNPImport{} - err = r.localClusterClient.Get(ctx, acnpImpName, acnpImp) - acnpImpNotFound := apierrors.IsNotFound(err) - if err != nil && !acnpImpNotFound { - klog.ErrorS(err, "failed to get existing ACNPImports") - return ctrl.Result{}, err - } - acnpImpObj := getACNPImport(resImp, tierNotFound) - if acnpImpNotFound { - err := r.localClusterClient.Create(ctx, acnpImpObj, &client.CreateOptions{}) - if err != nil { - klog.ErrorS(err, "failed to create ACNPImport", "acnpimport", klog.KObj(acnpImpObj)) - return ctrl.Result{}, err - } - r.installedResImports.Add(*resImp) - } - patchACNPImportStatus := false - if len(acnpImp.Status.Conditions) == 0 { - acnpImp.Status = acnpImpObj.Status - patchACNPImportStatus = true - } else { - if acnpImp.Status.Conditions[0].Status != acnpImpObj.Status.Conditions[0].Status { - acnpImp.Status = acnpImpObj.Status - patchACNPImportStatus = true - } - } - if patchACNPImportStatus { - if err := r.localClusterClient.Status().Update(ctx, acnpImp); err != nil { - klog.ErrorS(err, "failed to update acnpImport status", "acnpImport", klog.KObj(acnpImp)) - } - } - return ctrl.Result{}, nil -} - -func (r *ResourceImportReconciler) handleResImpDeleteForClusterNetworkPolicy(ctx context.Context, resImp *multiclusterv1alpha1.ResourceImport) (ctrl.Result, error) { - acnpImpName := types.NamespacedName{ - Namespace: "", - Name: resImp.Spec.Name, - } - acnpName := types.NamespacedName{ - Namespace: "", - Name: common.AntreaMCSPrefix + resImp.Spec.Name, - } - klog.InfoS("Deleting ACNP and ACNPImport corresponding to ResourceImport", - "acnp", acnpName.String(), "resourceimport", klog.KObj(resImp)) - - var err error - cleanupACNPImport := func() (ctrl.Result, error) { - acnpImp := &multiclusterv1alpha1.ACNPImport{} - err = r.localClusterClient.Get(ctx, acnpImpName, acnpImp) - if err != nil { - return ctrl.Result{}, client.IgnoreNotFound(err) - } - err = r.localClusterClient.Delete(ctx, acnpImp, &client.DeleteOptions{}) - if err != nil { - return ctrl.Result{}, client.IgnoreNotFound(err) - } - return ctrl.Result{}, nil - } - - acnp := &v1alpha1.ClusterNetworkPolicy{} - err = r.localClusterClient.Get(ctx, acnpName, acnp) - if err != nil { - if apierrors.IsNotFound(err) { - klog.V(2).InfoS("ACNP corresponding to ResourceImport has already been deleted", - "acnp", acnpName.String(), "resourceimport", klog.KObj(resImp)) - return cleanupACNPImport() - } - return ctrl.Result{}, err - } - err = r.localClusterClient.Delete(ctx, acnp, &client.DeleteOptions{}) - if err != nil { - return ctrl.Result{}, err - } - return cleanupACNPImport() -} - func getMCService(resImp *multiclusterv1alpha1.ResourceImport) *corev1.Service { - mcsPorts := []corev1.ServicePort{} + var mcsPorts []corev1.ServicePort for _, p := range resImp.Spec.ServiceImport.Spec.Ports { mcsPorts = append(mcsPorts, corev1.ServicePort{ Name: p.Name, diff --git a/multicluster/controllers/multicluster/commonarea/resourceimport_controller_test.go b/multicluster/controllers/multicluster/commonarea/resourceimport_controller_test.go index 140a9988e6f..8bf0166b6bd 100644 --- a/multicluster/controllers/multicluster/commonarea/resourceimport_controller_test.go +++ b/multicluster/controllers/multicluster/commonarea/resourceimport_controller_test.go @@ -44,8 +44,6 @@ var ( leaderNamespace = "default" svcResImportName = leaderNamespace + "-" + "nginx-service" epResImportName = leaderNamespace + "-" + "nginx-endpoints" - acnpImportName = "acnp-for-isolation" - acnpResImportName = leaderNamespace + "-" + acnpImportName svcImportReq = ctrl.Request{NamespacedName: types.NamespacedName{ Namespace: leaderNamespace, @@ -55,14 +53,6 @@ var ( Namespace: leaderNamespace, Name: epResImportName, }} - acnpImpReq = ctrl.Request{NamespacedName: types.NamespacedName{ - Namespace: leaderNamespace, - Name: acnpResImportName, - }} - acnpImpNoMatchingTierReq = ctrl.Request{NamespacedName: types.NamespacedName{ - Namespace: leaderNamespace, - Name: "default-acnp-no-matching-tier", - }} ctx = context.Background() scheme = runtime.NewScheme() @@ -119,63 +109,6 @@ var ( }, }, } - allowAction = v1alpha1.RuleActionAllow - dropAction = v1alpha1.RuleActionDrop - securityOpsTier = &v1alpha1.Tier{ - ObjectMeta: metav1.ObjectMeta{ - Name: "securityops", - }, - Spec: v1alpha1.TierSpec{ - Priority: int32(100), - Description: "[READ-ONLY]: System generated SecurityOps Tier", - }, - } - acnpResImport = &mcsv1alpha1.ResourceImport{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: leaderNamespace, - Name: acnpResImportName, - }, - Spec: mcsv1alpha1.ResourceImportSpec{ - Name: acnpImportName, - Kind: common.AntreaClusterNetworkPolicyKind, - ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ - Tier: "securityops", - Priority: 1.0, - AppliedTo: []v1alpha1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{}}, - }, - Ingress: []v1alpha1.Rule{ - { - Action: &dropAction, - From: []v1alpha1.NetworkPolicyPeer{ - { - Namespaces: &v1alpha1.PeerNamespaces{ - Match: v1alpha1.NamespaceMatchSelf, - }, - }, - }, - }, - }, - }, - }, - } - acnpResImportNoMatchingTier = &mcsv1alpha1.ResourceImport{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: leaderNamespace, - Name: "default-acnp-no-matching-tier", - }, - Spec: mcsv1alpha1.ResourceImportSpec{ - Name: "acnp-no-matching-tier", - Kind: common.AntreaClusterNetworkPolicyKind, - ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ - Tier: "somerandomtier", - Priority: 1.0, - AppliedTo: []v1alpha1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{}}, - }, - }, - }, - } ) func init() { @@ -240,68 +173,6 @@ func TestResourceImportReconciler_handleCreateEvent(t *testing.T) { } } -func TestResourceImportReconciler_handleCopySpanACNPCreateEvent(t *testing.T) { - remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) - go remoteMgr.Start() - defer remoteMgr.Stop() - - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(securityOpsTier).Build() - fakeRemoteClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(acnpResImport, acnpResImportNoMatchingTier).Build() - remoteCluster := NewFakeRemoteCommonArea(scheme, &remoteMgr, fakeRemoteClient, "leader-cluster", "default") - - tests := []struct { - name string - acnpImportName string - req ctrl.Request - expectedSuccess bool - }{ - { - name: "import ACNP of pre-defined tiers", - acnpImportName: acnpImportName, - req: acnpImpReq, - expectedSuccess: true, - }, - { - name: "import ACNP of non-existing tier", - acnpImportName: "acnp-no-matching-tier", - req: acnpImpNoMatchingTierReq, - expectedSuccess: false, - }, - } - r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, localClusterID, remoteCluster) - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if _, err := r.Reconcile(ctx, tt.req); err != nil { - if err != nil { - t.Errorf("ResourceImport Reconciler should handle ACNP create event successfully but got error = %v", err) - } - } else { - acnp := &v1alpha1.ClusterNetworkPolicy{} - err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: common.AntreaMCSPrefix + tt.acnpImportName}, acnp) - if tt.expectedSuccess && err != nil { - t.Errorf("ResourceImport Reconciler should import an ACNP successfully but got error = %v", err) - } else if !tt.expectedSuccess && (err == nil || !apierrors.IsNotFound(err)) { - t.Errorf("ResourceImport Reconciler should not import an ACNP whose Tier does not exist in current cluster. Expected NotFound error. Actual err = %v", err) - } - acnpImport := &mcsv1alpha1.ACNPImport{} - if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: tt.acnpImportName}, acnpImport); err != nil { - t.Errorf("ResourceImport Reconciler should create ACNPImport for ACNP type resouc") - } - status := acnpImport.Status.Conditions - if len(status) > 0 && status[0].Type == mcsv1alpha1.ACNPImportRealizable { - if tt.expectedSuccess && status[0].Status != corev1.ConditionTrue { - t.Errorf("ACNPImport %v realizable status should be True but is %v instead", acnpImportName, status[0].Status) - } else if !tt.expectedSuccess && status[0].Status != corev1.ConditionFalse { - t.Errorf("ACNPImport %v realizable status should be False but is %v instead", acnpImportName, status[0].Status) - } - } else { - t.Errorf("No realizable status provided for ACNPImport %v", acnpImportName) - } - } - }) - } -} - func TestResourceImportReconciler_handleDeleteEvent(t *testing.T) { remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) go remoteMgr.Start() @@ -376,42 +247,6 @@ func TestResourceImportReconciler_handleDeleteEvent(t *testing.T) { } } -func TestResourceImportReconciler_handleCopySpanACNPDeleteEvent(t *testing.T) { - remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) - go remoteMgr.Start() - defer remoteMgr.Stop() - - existingACNP := &v1alpha1.ClusterNetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: common.AntreaMCSPrefix + acnpImportName, - }, - } - existingACNPImport := &mcsv1alpha1.ACNPImport{ - ObjectMeta: metav1.ObjectMeta{ - Name: acnpImportName, - }, - } - - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existingACNP, existingACNPImport).Build() - fakeRemoteClient := fake.NewClientBuilder().WithScheme(scheme).Build() - remoteCluster := NewFakeRemoteCommonArea(scheme, &remoteMgr, fakeRemoteClient, "leader-cluster", "default") - - r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, localClusterID, remoteCluster) - r.installedResImports.Add(*acnpResImport) - - if _, err := r.Reconcile(ctx, acnpImpReq); err != nil { - t.Errorf("ResourceImport Reconciler should handle ACNP ResourceImport delete event successfully but got error = %v", err) - } - acnp := &v1alpha1.ClusterNetworkPolicy{} - 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) - } - acnpImport := &mcsv1alpha1.ACNPImport{} - if err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: acnpImportName}, acnpImport); !apierrors.IsNotFound(err) { - t.Errorf("ResourceImport Reconciler should delete ACNPImport successfully but got error = %v", err) - } -} - func TestResourceImportReconciler_handleUpdateEvent(t *testing.T) { remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) go remoteMgr.Start() @@ -662,219 +497,6 @@ func TestResourceImportReconciler_handleUpdateEvent(t *testing.T) { } } -func TestResourceImportReconciler_handleCopySpanACNPUpdateEvent(t *testing.T) { - remoteMgr := NewRemoteCommonAreaManager("test-clusterset", common.ClusterID(localClusterID)) - go remoteMgr.Start() - defer remoteMgr.Stop() - - existingACNP1 := &v1alpha1.ClusterNetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: common.AntreaMCSPrefix + acnpImportName, - Annotations: map[string]string{common.AntreaMCACNPAnnotation: "true"}, - }, - Spec: v1alpha1.ClusterNetworkPolicySpec{ - Tier: "securityops", - Priority: 1.0, - AppliedTo: []v1alpha1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{}}, - }, - Ingress: []v1alpha1.Rule{ - { - Action: &allowAction, - From: []v1alpha1.NetworkPolicyPeer{ - { - Namespaces: &v1alpha1.PeerNamespaces{ - Match: v1alpha1.NamespaceMatchSelf, - }, - }, - }, - }, - }, - }, - } - existingACNPImport1 := &mcsv1alpha1.ACNPImport{ - ObjectMeta: metav1.ObjectMeta{ - Name: acnpImportName, - }, - Status: mcsv1alpha1.ACNPImportStatus{ - Conditions: []mcsv1alpha1.ACNPImportCondition{ - { - Type: mcsv1alpha1.ACNPImportRealizable, - Status: corev1.ConditionTrue, - }, - }, - }, - } - existingACNPImport2 := &mcsv1alpha1.ACNPImport{ - ObjectMeta: metav1.ObjectMeta{ - Name: "acnp-no-matching-tier", - }, - Status: mcsv1alpha1.ACNPImportStatus{ - Conditions: []mcsv1alpha1.ACNPImportCondition{ - { - Type: mcsv1alpha1.ACNPImportRealizable, - Status: corev1.ConditionFalse, - }, - }, - }, - } - updatedResImport2 := &mcsv1alpha1.ResourceImport{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: leaderNamespace, - Name: "default-acnp-no-matching-tier", - }, - Spec: mcsv1alpha1.ResourceImportSpec{ - Name: "acnp-no-matching-tier", - Kind: common.AntreaClusterNetworkPolicyKind, - ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ - Tier: "securityops", - Priority: 1.0, - AppliedTo: []v1alpha1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{}}, - }, - }, - }, - } - existingACNP3 := &v1alpha1.ClusterNetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: common.AntreaMCSPrefix + "valid-updated-to-no-valid", - Annotations: map[string]string{common.AntreaMCACNPAnnotation: "true"}, - }, - Spec: v1alpha1.ClusterNetworkPolicySpec{ - Tier: "securityops", - Priority: 1.0, - AppliedTo: []v1alpha1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{}}, - }, - }, - } - existingACNPImport3 := &mcsv1alpha1.ACNPImport{ - ObjectMeta: metav1.ObjectMeta{ - Name: "valid-updated-to-no-valid", - }, - Status: mcsv1alpha1.ACNPImportStatus{ - Conditions: []mcsv1alpha1.ACNPImportCondition{ - { - Type: mcsv1alpha1.ACNPImportRealizable, - Status: corev1.ConditionTrue, - }, - }, - }, - } - updatedResImport3 := &mcsv1alpha1.ResourceImport{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: leaderNamespace, - Name: "default-valid-updated-to-no-valid", - }, - Spec: mcsv1alpha1.ResourceImportSpec{ - Name: acnpImportName, - Kind: common.AntreaClusterNetworkPolicyKind, - ClusterNetworkPolicy: &v1alpha1.ClusterNetworkPolicySpec{ - Tier: "somerandomtier", - Priority: 1.0, - AppliedTo: []v1alpha1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{}}, - }, - }, - }, - } - acnpImp3Req := ctrl.Request{NamespacedName: types.NamespacedName{ - Namespace: leaderNamespace, - Name: "default-valid-updated-to-no-valid", - }} - acnpImp4Req := ctrl.Request{NamespacedName: types.NamespacedName{ - Namespace: leaderNamespace, - Name: "default-name-conflict", - }} - existingACNP4 := &v1alpha1.ClusterNetworkPolicy{ - ObjectMeta: metav1.ObjectMeta{ - Name: common.AntreaMCSPrefix + "name-conflict", - }, - Spec: v1alpha1.ClusterNetworkPolicySpec{ - Tier: "securityops", - Priority: 1.0, - AppliedTo: []v1alpha1.NetworkPolicyPeer{ - {NamespaceSelector: &metav1.LabelSelector{}}, - }, - }, - } - - fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(existingACNP1, existingACNPImport1, existingACNPImport2, - existingACNP3, existingACNPImport3, existingACNP4, securityOpsTier).Build() - fakeRemoteClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(acnpResImport, updatedResImport2, updatedResImport3).Build() - remoteCluster := NewFakeRemoteCommonArea(scheme, &remoteMgr, fakeRemoteClient, "leader-cluster", "default") - - r := NewResourceImportReconciler(fakeClient, scheme, fakeClient, localClusterID, remoteCluster) - r.installedResImports.Add(*acnpResImport) - r.installedResImports.Add(*acnpResImportNoMatchingTier) - r.installedResImports.Add(*updatedResImport3) - - tests := []struct { - name string - acnpImportName string - req ctrl.Request - expectErr bool - expectImportSuccess bool - expectedUpdatedACNPSpec *v1alpha1.ClusterNetworkPolicySpec - }{ - { - name: "update acnp spec", - acnpImportName: acnpImportName, - req: acnpImpReq, - expectErr: false, - expectImportSuccess: true, - expectedUpdatedACNPSpec: acnpResImport.Spec.ClusterNetworkPolicy, - }, - { - name: "imported acnp missing tier update to valid tier", - acnpImportName: "acnp-no-matching-tier", - req: acnpImpNoMatchingTierReq, - expectErr: false, - expectImportSuccess: true, - expectedUpdatedACNPSpec: updatedResImport2.Spec.ClusterNetworkPolicy, - }, - { - name: "valid imported acnp update to missing tier", - req: acnpImp3Req, - acnpImportName: "valid-updated-to-no-valid", - expectErr: false, - expectImportSuccess: false, - expectedUpdatedACNPSpec: nil, - }, - { - name: "name conflict with existing acnp", - req: acnpImp4Req, - acnpImportName: "name-conflict", - expectErr: true, - expectImportSuccess: false, - expectedUpdatedACNPSpec: nil, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - if _, err := r.Reconcile(ctx, tt.req); err != nil { - if tt.expectErr { - assert.Contains(t, err.Error(), "conflicts with existing one") - } else { - t.Errorf("ResourceImport Reconciler should handle update event successfully but got error = %v", err) - } - } else { - if tt.expectedUpdatedACNPSpec != nil { - acnp := &v1alpha1.ClusterNetworkPolicy{} - err := fakeClient.Get(ctx, types.NamespacedName{Namespace: "", Name: common.AntreaMCSPrefix + tt.acnpImportName}, acnp) - if tt.expectImportSuccess && err != nil { - t.Errorf("ResourceImport Reconciler should import an ACNP successfully but got error = %v", err) - } else if !tt.expectImportSuccess && (err == nil || !apierrors.IsNotFound(err)) { - t.Errorf("ResourceImport Reconciler should not import an ACNP whose Tier does not exist in current cluster. Expected NotFound error. Actual err = %v", err) - } else if !reflect.DeepEqual(acnp.Spec, *tt.expectedUpdatedACNPSpec) { - t.Errorf("ACNP spec was not updated successfully") - } - } - } - }) - } -} - func checkAnnotation(t *testing.T, svcImport *k8smcsapi.ServiceImport) { id, ok := svcImport.Annotations[common.AntreaMCClusterIDAnnotation] if id != localClusterID || !ok { diff --git a/multicluster/controllers/multicluster/resourceexport_controller.go b/multicluster/controllers/multicluster/resourceexport_controller.go index ad03e96848e..73bbcd438b4 100644 --- a/multicluster/controllers/multicluster/resourceexport_controller.go +++ b/multicluster/controllers/multicluster/resourceexport_controller.go @@ -74,7 +74,7 @@ func NewResourceExportReconciler( // Reconcile will process all kinds of ResourceExport. Service and Endpoint kinds of ResourceExport // will be handled in this file, and all other kinds will have their own handler files, eg: newkind_handler.go func (r *ResourceExportReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - klog.V(2).InfoS("reconciling ResourceExport", "resourceexport", req.NamespacedName) + klog.V(2).InfoS("Reconciling ResourceExport", "resourceexport", req.NamespacedName) var resExport mcsv1alpha1.ResourceExport if err := r.Client.Get(ctx, req.NamespacedName, &resExport); err != nil { return ctrl.Result{}, client.IgnoreNotFound(err) @@ -132,13 +132,13 @@ func (r *ResourceExportReconciler) Reconcile(ctx context.Context, req ctrl.Reque if createResImport { if err = r.Client.Create(ctx, resImport, &client.CreateOptions{}); err != nil { - klog.ErrorS(err, "failed to create ResourceImport", "resourceimport", resImportName.String()) + klog.ErrorS(err, "Failed to create ResourceImport", "resourceimport", resImportName.String()) return ctrl.Result{}, err } r.updateResourceExportStatus(&resExport, succeed) - klog.InfoS("create ResourceImport successfully", "resourceimport", resImportName.String()) + klog.InfoS("Create ResourceImport successfully", "resourceimport", resImportName.String()) } else if changed { - klog.InfoS("update ResourceImport for ResoureExport", "resourceimport", resImportName.String(), "resourceexport", req.NamespacedName) + klog.InfoS("Update ResourceImport for ResoureExport", "resourceimport", resImportName.String(), "resourceexport", req.NamespacedName) if err = r.handleUpdateEvent(ctx, resImport, &resExport); err != nil { return ctrl.Result{}, err } @@ -161,13 +161,13 @@ func (r *ResourceExportReconciler) handleUpdateEvent(ctx context.Context, var err error if err = r.Client.Update(ctx, resImport, &client.UpdateOptions{}); err != nil { - klog.ErrorS(err, "failed to update ResourceImport", "resourceimport", resImpName.String()) + klog.ErrorS(err, "Failed to update ResourceImport", "resourceimport", resImpName.String()) return err } latestResImport := &mcsv1alpha1.ResourceImport{} err = r.Client.Get(ctx, resImpName, latestResImport) if err != nil { - klog.ErrorS(err, "failed to get latest ResourceImport", "resourceimport", resImpName.String()) + klog.ErrorS(err, "Failed to get latest ResourceImport", "resourceimport", resImpName.String()) return err } @@ -192,7 +192,7 @@ func (r *ResourceExportReconciler) handleUpdateEvent(ctx context.Context, latestResImport.Status.ClusterStatuses = append(latestResImport.Status.ClusterStatuses, newStatus) } if err := r.Client.Status().Update(ctx, latestResImport, &client.UpdateOptions{}); err != nil { - klog.ErrorS(err, "failed to update ResourceImport Status", "resourceimport", resImpName.String()) + klog.ErrorS(err, "Failed to update ResourceImport Status", "resourceimport", resImpName.String()) return err } return nil @@ -208,7 +208,7 @@ func (r *ResourceExportReconciler) handleDeleteEvent(ctx context.Context, resExp return err } resImportName := GetResourceImportName(resExport) - klog.Infof("There is resImport to delete named %s", resImportName) + klog.V(2).InfoS("Deleting ResourceImport created by ResourceExport", "resourceimport", resImportName.String(), "resourceexport", resExport.Name) undeleteItems := RemoveDeletedResourceExports(reList.Items) if len(undeleteItems) == 0 { @@ -227,7 +227,7 @@ func (r *ResourceExportReconciler) handleDeleteEvent(ctx context.Context, resExp func (r *ResourceExportReconciler) cleanUpResourceImport(ctx context.Context, resImp types.NamespacedName, re interface{}) error { - klog.InfoS("cleanup ResourceImport", "resourceimport", resImp.String()) + klog.InfoS("Cleaning up ResourceImport", "resourceimport", resImp.String()) resImport := &mcsv1alpha1.ResourceImport{ObjectMeta: metav1.ObjectMeta{ Name: resImp.Name, Namespace: resImp.Namespace, @@ -241,7 +241,7 @@ func (r *ResourceExportReconciler) updateEndpointResourceImport(ctx context.Cont resImport := &mcsv1alpha1.ResourceImport{} err := r.Client.Get(ctx, resImpName, resImport) if err != nil { - klog.ErrorS(err, "failed to get ResourceImport", "resourceimport", resImpName) + klog.ErrorS(err, "Failed to get ResourceImport", "resourceimport", resImpName) return client.IgnoreNotFound(err) } newResImport, changed, err := r.refreshEndpointsResourceImport(existRe, resImport, false) @@ -267,7 +267,7 @@ func (r *ResourceExportReconciler) getExistingResImport(ctx context.Context, err := r.Client.Get(ctx, resImportName, existResImport) if err != nil { if !apierrors.IsNotFound(err) { - klog.ErrorS(err, "failed to get ResourceImport", "resourceimport", resImportName.String()) + klog.ErrorS(err, "Failed to get ResourceImport", "resourceimport", resImportName.String()) return createResImport, nil, err } existResImport = &mcsv1alpha1.ResourceImport{ @@ -310,7 +310,7 @@ func (r *ResourceExportReconciler) refreshServiceResourceImport( if !apiequality.Semantic.DeepEqual(newResImport.Spec.ServiceImport.Spec.Ports, convertedPorts) { undeletedItems, err := r.getNotDeletedResourceExports(resExport) if err != nil { - klog.ErrorS(err, "failed to list ResourceExports, retry later") + klog.ErrorS(err, "Failed to list ResourceExports, retry later") return newResImport, false, err } // When there is only one Service ResourceExport, ResourceImport should reflect the change @@ -369,7 +369,7 @@ func (r *ResourceExportReconciler) refreshEndpointsResourceImport( return newResImport, true, nil } // check all matched Endpoints ResourceExport and generate a new EndpointSubset - newSubsets := []corev1.EndpointSubset{} + var newSubsets []corev1.EndpointSubset undeleteItems, err := r.getNotDeletedResourceExports(resExport) if err != nil { klog.ErrorS(err, "failed to list ResourceExports, retry later") @@ -423,7 +423,7 @@ func (r *ResourceExportReconciler) getNotDeletedResourceExports(resExport *mcsv1 } func (r *ResourceExportReconciler) updateResourceExportStatus(resExport *mcsv1alpha1.ResourceExport, res resReason) { - newConditions := []mcsv1alpha1.ResourceExportCondition{} + var newConditions []mcsv1alpha1.ResourceExportCondition switch res { case succeed: newConditions = []mcsv1alpha1.ResourceExportCondition{ @@ -519,7 +519,7 @@ func GetResourceImportName(resExport *mcsv1alpha1.ResourceExport) types.Namespac // RemoveDeletedResourceExports remove any ResourceExports with non-zero DeletionTimestamp // which is actually deleted object. func RemoveDeletedResourceExports(items []mcsv1alpha1.ResourceExport) []mcsv1alpha1.ResourceExport { - undeleteItems := []mcsv1alpha1.ResourceExport{} + var undeleteItems []mcsv1alpha1.ResourceExport for _, i := range items { if i.DeletionTimestamp.IsZero() { undeleteItems = append(undeleteItems, i)