Skip to content

Commit

Permalink
Remove NetworkPolicy/SecurityPolicy Finalizer (vmware-tanzu#793)
Browse files Browse the repository at this point in the history 
This patch is to
1. Remove Finalizer for NetworkPolicy CR.
2. Remove Finalizer for SecurityPolicy in VPC network.
3. For T1 network work, the upgrade SecurityPolicy from V4.1.2 still has existing finalizer,
however, the new created SecurityPolicy has no finalizer any longer after upgrade.

After Finalizer is removed, the deletion process of NetworkPolicy/SecurityPolicy is modified
1. Add a new indexer function for tag nsx-op/namespace for SecurityPolicy store.

2. Once the K8s CR(NetworkPolicy/SecurityPolicy) is deleted, the CR will be deleted at once usually
if without finalizer. So, there no deletion timestamp could be found in the CR.
It's needed to handle K8s deletion even when k8s client finds that the CR not found.

3.Using nsx-op/namespace tag to fileter the corresponding NSX resources in the same namespace with
delete request, and then comparing CR name tag value, either SecurityPolicy CR or NetworkPolciy CR,
with delete request CR name to filter the NSX SecurityPolicy resources to be deleted.

4.If there are multiple NSX resources with the same CR anme tag value in the NSX store.
It's must check the the K8s CR exist to decide which CR was deleted
already, or the new created in the same namespace with the same name.
  • Loading branch information
@timdengyun
timdengyun authored Oct 11, 2024
1 parent 7223696 commit edb6207
Show file tree
Hide file tree
Showing 6 changed files with 213 additions and 92 deletions.
99 changes: 60 additions & 39 deletions pkg/controllers/networkpolicy/networkpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

"github.com/vmware-tanzu/nsx-operator/pkg/controllers/common"
"github.com/vmware-tanzu/nsx-operator/pkg/logger"
Expand Down Expand Up @@ -100,21 +99,21 @@ func (r *NetworkPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerSyncTotal, MetricResType)

if err := r.Client.Get(ctx, req.NamespacedName, networkPolicy); err != nil {
log.Error(err, "unable to fetch network policy", "req", req.NamespacedName)
return ResultNormal, client.IgnoreNotFound(err)
// IgnoreNotFound returns nil on NotFound errors.
if client.IgnoreNotFound(err) == nil {
if err := r.deleteNetworkPolicyByName(req.Namespace, req.Name); err != nil {
log.Error(err, "failed to delete NetworkPolicy", "networkpolicy", req.NamespacedName)
return ResultRequeue, err
}
return ResultNormal, nil
}
// In case that client is unable to check CR
log.Error(err, "client is unable to fetch NetworkPolicy CR", "req", req.NamespacedName)
return ResultRequeue, err
}

if networkPolicy.ObjectMeta.DeletionTimestamp.IsZero() {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerUpdateTotal, MetricResType)
if !controllerutil.ContainsFinalizer(networkPolicy, servicecommon.NetworkPolicyFinalizerName) {
controllerutil.AddFinalizer(networkPolicy, servicecommon.NetworkPolicyFinalizerName)
if err := r.Client.Update(ctx, networkPolicy); err != nil {
log.Error(err, "add finalizer", "networkpolicy", req.NamespacedName)
updateFail(r, ctx, networkPolicy, &err)
return ResultRequeue, err
}
log.V(1).Info("added finalizer on networkpolicy", "networkpolicy", req.NamespacedName)
}

if err := r.Service.CreateOrUpdateSecurityPolicy(networkPolicy); err != nil {
if errors.As(err, &nsxutil.RestrictionError{}) {
Expand All @@ -135,25 +134,14 @@ func (r *NetworkPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reques
updateSuccess(r, ctx, networkPolicy)
cleanNetworkPolicyErrorAnnotation(ctx, networkPolicy, r.Client)
} else {
if controllerutil.ContainsFinalizer(networkPolicy, servicecommon.NetworkPolicyFinalizerName) {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType)
if err := r.Service.DeleteSecurityPolicy(networkPolicy, false, false, servicecommon.ResourceTypeNetworkPolicy); err != nil {
log.Error(err, "deletion failed, would retry exponentially", "networkpolicy", req.NamespacedName)
deleteFail(r, ctx, networkPolicy, &err)
return ResultRequeue, err
}
controllerutil.RemoveFinalizer(networkPolicy, servicecommon.NetworkPolicyFinalizerName)
if err := r.Client.Update(ctx, networkPolicy); err != nil {
log.Error(err, "deletion failed, would retry exponentially", "networkpolicy", req.NamespacedName)
deleteFail(r, ctx, networkPolicy, &err)
return ResultRequeue, err
}
log.V(1).Info("removed finalizer", "networkpolicy", req.NamespacedName)
deleteSuccess(r, ctx, networkPolicy)
} else {
// only print a message because it's not a normal case
log.Info("finalizers cannot be recognized", "networkpolicy", req.NamespacedName)
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType)

if err := r.Service.DeleteSecurityPolicy(networkPolicy, false, false, servicecommon.ResourceTypeNetworkPolicy); err != nil {
log.Error(err, "deletion failed, would retry exponentially", "networkpolicy", req.NamespacedName)
deleteFail(r, ctx, networkPolicy, &err)
return ResultRequeue, err
}
deleteSuccess(r, ctx, networkPolicy)
}

return ResultNormal, nil
Expand Down Expand Up @@ -186,19 +174,12 @@ func (r *NetworkPolicyReconciler) CollectGarbage(ctx context.Context) {
if len(nsxPolicySet) == 0 {
return
}
policyList := &networkingv1.NetworkPolicyList{}
err := r.Client.List(ctx, policyList)

CRPolicySet, err := r.listNetworkPolciyCRIDs()
if err != nil {
log.Error(err, "failed to list NetworkPolicy")
return
}

CRPolicySet := sets.New[string]()
for _, policy := range policyList.Items {
CRPolicySet.Insert(r.Service.BuildNetworkPolicyAllowPolicyID(string(policy.UID)))
CRPolicySet.Insert(r.Service.BuildNetworkPolicyIsolationPolicyID(string(policy.UID)))
}

diffSet := nsxPolicySet.Difference(CRPolicySet)
for elem := range diffSet {
log.V(1).Info("GC collected NetworkPolicy", "ID", elem)
Expand All @@ -212,6 +193,46 @@ func (r *NetworkPolicyReconciler) CollectGarbage(ctx context.Context) {
}
}

func (r *NetworkPolicyReconciler) deleteNetworkPolicyByName(ns, name string) error {
nsxSecurityPolicies := r.Service.ListNetworkPolicyByName(ns, name)

CRPolicySet, err := r.listNetworkPolciyCRIDs()
if err != nil {
return err
}
for _, item := range nsxSecurityPolicies {
uid := nsxutil.FindTag(item.Tags, servicecommon.TagScopeNetworkPolicyUID)
if CRPolicySet.Has(uid) {
log.Info("skipping deletion, NetworkPolicy CR still exists in K8s", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
continue
}

log.Info("deleting NetworkPolicy", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
if err := r.Service.DeleteSecurityPolicy(types.UID(uid), false, false, servicecommon.ResourceTypeNetworkPolicy); err != nil {
log.Error(err, "failed to delete NetworkPolicy", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
return err
}
log.Info("successfully deleted NetworkPolicy", "networkPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
}
return nil
}

func (r *NetworkPolicyReconciler) listNetworkPolciyCRIDs() (sets.Set[string], error) {
networkPolicyList := &networkingv1.NetworkPolicyList{}
err := r.Client.List(context.Background(), networkPolicyList)
if err != nil {
log.Error(err, "failed to list NetworkPolicy CRs")
return nil, err
}

CRPolicySet := sets.New[string]()
for _, policy := range networkPolicyList.Items {
CRPolicySet.Insert(r.Service.BuildNetworkPolicyAllowPolicyID(string(policy.UID)))
CRPolicySet.Insert(r.Service.BuildNetworkPolicyIsolationPolicyID(string(policy.UID)))
}
return CRPolicySet, nil
}

func StartNetworkPolicyController(mgr ctrl.Manager, commonService servicecommon.Service, vpcService servicecommon.VPCServiceProvider) {
networkPolicyReconcile := NetworkPolicyReconciler{
Client: mgr.GetClient(),
Expand Down
105 changes: 68 additions & 37 deletions pkg/controllers/securitypolicy/securitypolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,12 +137,21 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
obj = &v1alpha1.SecurityPolicy{}
}

log.Info("reconciling securitypolicy CR", "securitypolicy", req.NamespacedName)
log.Info("reconciling SecurityPolicy CR", "securitypolicy", req.NamespacedName)
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerSyncTotal, MetricResType)

if err := r.Client.Get(ctx, req.NamespacedName, obj); err != nil {
log.Error(err, "unable to fetch security policy CR", "req", req.NamespacedName)
return ResultNormal, client.IgnoreNotFound(err)
// IgnoreNotFound returns nil on NotFound errors.
if client.IgnoreNotFound(err) == nil {
if err := r.deleteSecurityPolicyByName(req.Namespace, req.Name); err != nil {
log.Error(err, "failed to delete SecurityPolicy", "securitypolicy", req.NamespacedName)
return ResultRequeue, err
}
return ResultNormal, nil
}
// In case that client is unable to check CR
log.Error(err, "client is unable to fetch SecurityPolicy CR", "req", req.NamespacedName)
return ResultRequeue, err
}

isZero := false
Expand All @@ -152,7 +161,6 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
case *crdv1alpha1.SecurityPolicy:
o := obj.(*crdv1alpha1.SecurityPolicy)
isZero = o.ObjectMeta.DeletionTimestamp.IsZero()
finalizerName = servicecommon.SecurityPolicyFinalizerName
realObj = securitypolicy.VPCToT1(o)
case *v1alpha1.SecurityPolicy:
realObj = obj.(*v1alpha1.SecurityPolicy)
Expand All @@ -170,15 +178,6 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque

if isZero {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerUpdateTotal, MetricResType)
if !controllerutil.ContainsFinalizer(obj, finalizerName) {
controllerutil.AddFinalizer(obj, finalizerName)
if err := r.Client.Update(ctx, obj); err != nil {
log.Error(err, "add finalizer", "securitypolicy", req.NamespacedName)
updateFail(r, ctx, realObj, &err)
return ResultRequeue, err
}
log.V(1).Info("added finalizer on securitypolicy CR", "securitypolicy", req.NamespacedName)
}

if isCRInSysNs, err := util.IsSystemNamespace(r.Client, req.Namespace, nil); err != nil {
err = errors.New("fetch namespace associated with security policy CR failed")
Expand Down Expand Up @@ -213,25 +212,24 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
cleanSecurityPolicyErrorAnnotation(ctx, realObj, securitypolicy.IsVPCEnabled(r.Service), r.Client)
} else {
log.Info("reconciling CR to delete securitypolicy", "securitypolicy", req.NamespacedName)
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType)

// For T1 upgrade, the upgraded CRs still has finalizer
if controllerutil.ContainsFinalizer(obj, finalizerName) {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType)
if err := r.Service.DeleteSecurityPolicy(realObj.UID, false, false, servicecommon.ResourceTypeSecurityPolicy); err != nil {
log.Error(err, "deletion failed, would retry exponentially", "securitypolicy", req.NamespacedName)
deleteFail(r, ctx, realObj, &err)
return ResultRequeue, err
}
controllerutil.RemoveFinalizer(obj, finalizerName)
if err := r.Client.Update(ctx, obj); err != nil {
log.Error(err, "deletion failed, would retry exponentially", "securitypolicy", req.NamespacedName)
log.Error(err, "finalizer remove failed, would retry exponentially", "securitypolicy", req.NamespacedName)
deleteFail(r, ctx, realObj, &err)
return ResultRequeue, err
}
log.V(1).Info("removed finalizer", "securitypolicy", req.NamespacedName)
deleteSuccess(r, ctx, realObj)
} else {
// only print a message because it's not a normal case
log.Info("finalizers cannot be recognized", "securitypolicy", req.NamespacedName)
}
if err := r.Service.DeleteSecurityPolicy(realObj.UID, false, false, servicecommon.ResourceTypeSecurityPolicy); err != nil {
log.Error(err, "deletion failed, would retry exponentially", "securitypolicy", req.NamespacedName)
deleteFail(r, ctx, realObj, &err)
return ResultRequeue, err
}
deleteSuccess(r, ctx, realObj)
}

return ResultNormal, nil
Expand Down Expand Up @@ -361,16 +359,59 @@ func (r *SecurityPolicyReconciler) CollectGarbage(ctx context.Context) {
return
}

CRPolicySet, err := r.listSecurityPolciyCRIDs()
if err != nil {
return
}

diffSet := nsxPolicySet.Difference(CRPolicySet)
for elem := range diffSet {
log.V(1).Info("GC collected SecurityPolicy CR", "securityPolicyUID", elem)
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType)
err = r.Service.DeleteSecurityPolicy(types.UID(elem), true, false, servicecommon.ResourceTypeSecurityPolicy)
if err != nil {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteFailTotal, MetricResType)
} else {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteSuccessTotal, MetricResType)
}
}
}

func (r *SecurityPolicyReconciler) deleteSecurityPolicyByName(ns, name string) error {
nsxSecurityPolicies := r.Service.ListSecurityPolicyByName(ns, name)

CRPolicySet, err := r.listSecurityPolciyCRIDs()
if err != nil {
return err
}
for _, item := range nsxSecurityPolicies {
uid := nsxutil.FindTag(item.Tags, servicecommon.TagValueScopeSecurityPolicyUID)
if CRPolicySet.Has(uid) {
log.Info("skipping deletion, SecurityPolicy CR still exists in K8s", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
continue
}

log.Info("deleting SecurityPolicy", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
if err := r.Service.DeleteSecurityPolicy(types.UID(uid), false, false, servicecommon.ResourceTypeSecurityPolicy); err != nil {
log.Error(err, "failed to delete SecurityPolicy", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
return err
}
log.Info("successfully deleted SecurityPolicy", "securityPolicyUID", uid, "nsxSecurityPolicyId", *item.Id)
}
return nil
}

func (r *SecurityPolicyReconciler) listSecurityPolciyCRIDs() (sets.Set[string], error) {
var objectList client.ObjectList
if securitypolicy.IsVPCEnabled(r.Service) {
objectList = &crdv1alpha1.SecurityPolicyList{}
} else {
objectList = &v1alpha1.SecurityPolicyList{}
}
err := r.Client.List(ctx, objectList)
err := r.Client.List(context.Background(), objectList)
if err != nil {
log.Error(err, "failed to list SecurityPolicy CR")
return
return nil, err
}

CRPolicySet := sets.New[string]()
Expand All @@ -387,17 +428,7 @@ func (r *SecurityPolicyReconciler) CollectGarbage(ctx context.Context) {
}
}

diffSet := nsxPolicySet.Difference(CRPolicySet)
for elem := range diffSet {
log.V(1).Info("GC collected SecurityPolicy CR", "securityPolicyUID", elem)
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType)
err = r.Service.DeleteSecurityPolicy(types.UID(elem), true, false, servicecommon.ResourceTypeSecurityPolicy)
if err != nil {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteFailTotal, MetricResType)
} else {
metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteSuccessTotal, MetricResType)
}
}
return CRPolicySet, nil
}

// It is triggered by associated controller like pod, namespace, etc.
Expand Down
Loading

0 comments on commit edb6207

Please sign in to comment.