From 6b75a573e987ef3061e574cd24e4c15ef91e8ff8 Mon Sep 17 00:00:00 2001 From: Anand Kumar Date: Fri, 4 Aug 2023 04:29:12 -0700 Subject: [PATCH] Fix an issue in tracker module. Consider a scenario where the following sequence of events are received. An ANP with Ipaddress block and ATGroup is realized -> Secret delete -> Sync with cloud finds diff and updates member -> AT Modify -> AT Modofy -> NP delete -> ProcessTrackers -> AT delete. This results in a scenario where a tracker has one appliedToSgs, there are no nps in npIndexers, prevAppliedToSgs is nil, computing NP status will result in an empty appliedToToNpMap and there by an error. Fix is to add an additional check to compute Npstatus from appliedToToNpMap and appliedToSgs. Signed-off-by: Anand Kumar --- .../registry/virtualmachinepolicy/rest.go | 12 +++++-- pkg/controllers/networkpolicy/tracker.go | 31 ++++++++++++++++++- 2 files changed, 40 insertions(+), 3 deletions(-) diff --git a/pkg/apiserver/registry/virtualmachinepolicy/rest.go b/pkg/apiserver/registry/virtualmachinepolicy/rest.go index 6e6de941..812db77b 100644 --- a/pkg/apiserver/registry/virtualmachinepolicy/rest.go +++ b/pkg/apiserver/registry/virtualmachinepolicy/rest.go @@ -83,7 +83,10 @@ func (r *REST) Get(ctx context.Context, name string, _ *metav1.GetOptions) (runt return nil, errors.NewNotFound(runtimev1alpha1.Resource("virtualmachinepolicy"), name) } // For a given vm namespaced name there should be only one matching tracker. - return r.convertToVmp(objs[0]), nil + if vmp := r.convertToVmp(objs[0]); vmp != nil { + return vmp, nil + } + return nil, nil } func (r *REST) List(ctx context.Context, _ *internalversion.ListOptions) (runtime.Object, error) { @@ -97,7 +100,9 @@ func (r *REST) List(ctx context.Context, _ *internalversion.ListOptions) (runtim vmpList := &runtimev1alpha1.VirtualMachinePolicyList{} for _, obj := range objs { vmp := r.convertToVmp(obj) - vmpList.Items = append(vmpList.Items, *vmp) + if vmp != nil { + vmpList.Items = append(vmpList.Items, *vmp) + } } return vmpList, nil } @@ -135,6 +140,9 @@ func (r *REST) ConvertToTable(_ context.Context, obj runtime.Object, _ runtime.O func (r *REST) convertToVmp(obj interface{}) *runtimev1alpha1.VirtualMachinePolicy { failed, inProgress := false, false tracker := obj.(*networkpolicy.CloudResourceNpTracker) + if len(tracker.NpStatus) == 0 { + return nil + } for _, status := range tracker.NpStatus { if status.Realization == runtimev1alpha1.Failed { failed = true diff --git a/pkg/controllers/networkpolicy/tracker.go b/pkg/controllers/networkpolicy/tracker.go index 34803db3..69af1673 100644 --- a/pkg/controllers/networkpolicy/tracker.go +++ b/pkg/controllers/networkpolicy/tracker.go @@ -61,6 +61,7 @@ func (r *NetworkPolicyReconciler) newCloudResourceNpTracker(rsc *cloudresource.C } // A vm is uniquely identified by its cloud assigned id and vpc id. + // TODO: Same vm imported in multiple Namespaces is not supported. vm := vmItems[0].(*runtimev1alpha1.VirtualMachine) tracker := &CloudResourceNpTracker{ appliedToSGs: make(map[string]*appliedToSecurityGroup), @@ -109,7 +110,8 @@ func (r *NetworkPolicyReconciler) processCloudResourceNpTrackers() { npStatus, ok := status[tracker.NamespacedName.Namespace] if !ok { // Should not occur. - log.Error(fmt.Errorf("failed to get NetworkPolicy status for tracker"), "", "tracker", tracker) + log.Error(fmt.Errorf("failed to get NetworkPolicy status for tracker"), "", + "tracker", tracker, "appliedToSGs", tracker.appliedToSGs, "appliedToToNpMap", tracker.appliedToToNpMap) tracker.unmarkDirty() continue } @@ -161,6 +163,7 @@ func (c *CloudResourceNpTracker) computeNpStatus(r *NetworkPolicyReconciler) map // retrieve all network policies related to cloud resource's applied groups npMap := make(map[interface{}]string) + changed := false appliedToToNpMap := make(map[string][]types.NamespacedName) for key, asg := range c.appliedToSGs { nps, err := r.networkPolicyIndexer.ByIndex(networkPolicyIndexerByAppliedToGrp, asg.id.Name) @@ -172,6 +175,7 @@ func (c *CloudResourceNpTracker) computeNpStatus(r *NetworkPolicyReconciler) map // Not considering cloud resources belongs to multiple AppliedToGroups of same NetworkPolicy. for _, i := range nps { namespacedName := types.NamespacedName{Namespace: i.(*networkPolicy).Namespace, Name: i.(*networkPolicy).Name} + changed = true appliedToToNpMap[asg.id.Name] = append(appliedToToNpMap[asg.id.Name], namespacedName) npMap[i] = key } @@ -231,6 +235,7 @@ func (c *CloudResourceNpTracker) computeNpStatus(r *NetworkPolicyReconciler) map } for _, asg := range newPrevSgs { + changed = true if asg.status == nil { delete(c.appliedToToNpMap, asg.id.Name) delete(newPrevSgs, asg.id.CloudResourceID.String()) @@ -284,6 +289,30 @@ func (c *CloudResourceNpTracker) computeNpStatus(r *NetworkPolicyReconciler) map } } + // When a NP is deleted, but the appliedToGroup delete is not received yet, then NP Status is computed based on + // previous appliedToSgs. Suppose prevAppliedToSGs is nil, then NP status will be empty. + // An additional check required to compute the NP status, based on the appliedToToNpMap and appliedToSGs. + // If there are any entries in appliedToToNpMap with an appliedToSG, set the np status as in progress, since + // appliedToGroup is in a transient state. + if !changed && len(c.appliedToToNpMap) > 0 { + for _, asg := range c.appliedToSGs { + if namespacedNames, ok := c.appliedToToNpMap[asg.id.Name]; ok { + for _, namespacedName := range namespacedNames { + npList, ok := ret[namespacedName.Namespace] + if !ok { + npList = make(map[string]*runtimev1alpha1.NetworkPolicyStatus) + ret[namespacedName.Namespace] = npList + } + npList[namespacedName.Name] = &runtimev1alpha1.NetworkPolicyStatus{ + Reason: NoneStr, + Realization: runtimev1alpha1.InProgress, + } + appliedToToNpMap[asg.id.Name] = append(appliedToToNpMap[asg.id.Name], namespacedName) + } + } + } + } + // Update the map with the latest data. c.appliedToToNpMap = appliedToToNpMap // Update the previous appliedToSgs.