Skip to content

Commit

Permalink
Remove NetworkPolicy/SecurityPolicy Finalizer
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 the new tag: nsx-op/network_policy_namespaced_name and tag:nsx-op/security_policy_namespaced_name
for NetworkPolicy and SecurityPolicy, respectively.
The tag nsx-op/XX_namespaced_name value: crNamespace/crName will be used for new indexer function
in order to retrieve securitypolicy NSX 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 crNamespace/crName to get the corresponding NSX resource and delete it.

4.If there are multiple NSX resources with the same crNamespace/crName tag value in the NSX store.
It's must check the the K8s CR exist to decide which CR is deleted
or the new created in the same namespace with the same name.
  • Loading branch information
@timdengyun
timdengyun committed Oct 6, 2024
1 parent 5c97290 commit f4d44d7
Show file tree
Hide file tree
Showing 10 changed files with 378 additions and 212 deletions.
100 changes: 61 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.Name, req.Namespace); 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,47 @@ func (r *NetworkPolicyReconciler) CollectGarbage(ctx context.Context) {
}
}

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

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
106 changes: 69 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.Name, req.Namespace); 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,60 @@ 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(name, ns string) error {
nsxSecurityPolicies := r.Service.ListSecurityPolicyByNamespacedName(name, ns)

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 +429,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 f4d44d7

Please sign in to comment.