From c255e443193248f17456d5d0f217ab6119ad8932 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 24 Mar 2021 19:44:42 -0700 Subject: [PATCH 01/17] Upgrading Namespace to use NSController --- npm/nameSpaceController.go | 519 +++++++++++++++++++++++++++++++++++++ npm/namespace.go | 284 -------------------- npm/npm.go | 108 ++++---- npm/util/util.go | 11 + 4 files changed, 587 insertions(+), 335 deletions(-) create mode 100644 npm/nameSpaceController.go delete mode 100644 npm/namespace.go diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go new file mode 100644 index 0000000000..5ccbba556c --- /dev/null +++ b/npm/nameSpaceController.go @@ -0,0 +1,519 @@ +// Copyright 2018 Microsoft. All rights reserved. +// MIT License +package npm + +import ( + "fmt" + "reflect" + "time" + + "github.com/Azure/azure-container-networking/log" + "github.com/Azure/azure-container-networking/npm/ipsm" + "github.com/Azure/azure-container-networking/npm/iptm" + "github.com/Azure/azure-container-networking/npm/metrics" + "github.com/Azure/azure-container-networking/npm/util" + + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + "k8s.io/apimachinery/pkg/util/wait" + coreinformer "k8s.io/client-go/informers/core/v1" + "k8s.io/client-go/kubernetes" + corelisters "k8s.io/client-go/listers/core/v1" + "k8s.io/client-go/tools/cache" + "k8s.io/client-go/util/workqueue" +) + +type Namespace struct { + name string + LabelsMap map[string]string // NameSpace labels + SetMap map[string]string + IpsMgr *ipsm.IpsetManager + iptMgr *iptm.IptablesManager + resourceVersion uint64 // NameSpace ResourceVersion +} + +// newNS constructs a new namespace object. +func newNs(name string) (*Namespace, error) { + ns := &Namespace{ + name: name, + LabelsMap: make(map[string]string), + SetMap: make(map[string]string), + IpsMgr: ipsm.NewIpsetManager(), + iptMgr: iptm.NewIptablesManager(), + // resource version is converted to uint64 + // so make sure it is initialized to "0" + resourceVersion: 0, + } + + return ns, nil +} + +func getNamespaceObjFromNsObj(nsObj *Namespace) *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: nsObj.name, + Labels: nsObj.LabelsMap, + }, + } +} + +// setResourceVersion setter func for RV +func setResourceVersion(nsObj *Namespace, rv string) { + nsObj.resourceVersion = util.ParseResourceVersion(rv) +} + +func isSystemNs(nsObj *corev1.Namespace) bool { + return nsObj.ObjectMeta.Name == util.KubeSystemFlag +} + +func isInvalidNamespaceUpdate(oldNsObj, newNsObj *corev1.Namespace) (isInvalidUpdate bool) { + isInvalidUpdate = oldNsObj.ObjectMeta.Name == newNsObj.ObjectMeta.Name && + newNsObj.ObjectMeta.DeletionTimestamp == nil && + newNsObj.ObjectMeta.DeletionGracePeriodSeconds == nil + isInvalidUpdate = isInvalidUpdate && reflect.DeepEqual(oldNsObj.ObjectMeta.Labels, newNsObj.ObjectMeta.Labels) + + return +} + +type nameSpaceController struct { + clientset *kubernetes.Clientset + nameSpaceLister corelisters.NamespaceLister + nameSpaceListerSynced cache.InformerSynced + workqueue workqueue.RateLimitingInterface + // TODO does not need to have whole NetworkPolicyManager pointer. Need to improve it + npMgr *NetworkPolicyManager +} + +func NewNameSpaceController(nameSpaceInformer coreinformer.NamespaceInformer, clientset *kubernetes.Clientset, npMgr *NetworkPolicyManager) *nameSpaceController { + nameSpaceController := &nameSpaceController{ + clientset: clientset, + nameSpaceLister: nameSpaceInformer.Lister(), + nameSpaceListerSynced: nameSpaceInformer.Informer().HasSynced, + workqueue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "Namespaces"), + npMgr: npMgr, + } + + nameSpaceInformer.Informer().AddEventHandler( + cache.ResourceEventHandlerFuncs{ + AddFunc: nameSpaceController.addNamespace, + UpdateFunc: nameSpaceController.updateNamespace, + DeleteFunc: nameSpaceController.deleteNamespace, + }, + ) + return nameSpaceController +} + +// filter this event if we do not need to handle this event +func (nsc *nameSpaceController) needSync(obj interface{}, event string) (string, bool) { + needSync := false + var key string + + nsObj, ok := obj.(*corev1.Namespace) + if !ok { + metrics.SendErrorLogAndMetric(util.NSID, "%s Pod: Received unexpected object type: %v", event, obj) + return key, needSync + } + + var err error + if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil { + utilruntime.HandleError(err) + err = fmt.Errorf("[%sNameSpace] Error: nameSpaceKey is empty for %s namespace", event, util.GetNSNameWithPrefix(nsObj.Name)) + metrics.SendErrorLogAndMetric(util.NSID, err.Error()) + return key, needSync + } + + log.Logf("[NAMESPACE %s EVENT] for namespace [%s]", event, key) + + needSync = true + return key, needSync +} + +func (nsc *nameSpaceController) addNamespace(obj interface{}) { + key, needSync := nsc.needSync(obj, "ADD") + if !needSync { + log.Logf("[NAMESPACE ADD EVENT] No need to sync this namespace [%s]", key) + return + } + nsc.workqueue.Add(key) +} + +func (nsc *nameSpaceController) updateNamespace(old, new interface{}) { + key, needSync := nsc.needSync(new, "UPDATE") + if !needSync { + log.Logf("[NAMESPACE UPDATE EVENT] No need to sync this namespace [%s]", key) + return + } + + nsObj, _ := new.(*corev1.Namespace) + + if nsObj.ObjectMeta.DeletionTimestamp != nil || + nsObj.ObjectMeta.DeletionGracePeriodSeconds != nil { + nsc.deleteNamespace(new) + return + } + + oldNsObj, ok := old.(*corev1.Namespace) + if ok { + if oldNsObj.ResourceVersion == nsObj.ResourceVersion { + log.Logf("[NAMESPACE UPDATE EVENT] Resourceversion is same for this namespace [%s]", key) + return + } + } + + nsKey := util.GetNSNameWithPrefix(key) + nsc.npMgr.Lock() + cachedNsObj, nsExists := nsc.npMgr.NsMap[nsKey] + if !nsExists { + nsc.workqueue.Add(key) + return + } + + if reflect.DeepEqual(cachedNsObj.LabelsMap, nsObj.ObjectMeta.Labels) { + log.Logf("[NAMESPACE UPDATE EVENT] Namespace [%s] labels did not change", key) + return + } + nsc.npMgr.Unlock() + + nsc.workqueue.Add(key) +} + +func (nsc *nameSpaceController) deleteNamespace(obj interface{}) { + nsObj, ok := obj.(*corev1.Namespace) + // DeleteFunc gets the final state of the resource (if it is known). + // Otherwise, it gets an object of type DeletedFinalStateUnknown. + // This can happen if the watch is closed and misses the delete event and + // the controller doesn't notice the deletion until the subsequent re-list + if !ok { + tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + if !ok { + metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE DELETE EVENT]: Received unexpected object type: %v", obj) + return + } + + if nsObj, ok = tombstone.Obj.(*corev1.Namespace); !ok { + metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE DELETE EVENT]: Received unexpected object type (error decoding object tombstone, invalid type): %v", obj) + return + } + } + + var err error + var key string + if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil { + utilruntime.HandleError(err) + err = fmt.Errorf("[NAMESPACE DELETE EVENT] Error: nameSpaceKey is empty for %s namespace", util.GetNSNameWithPrefix(nsObj.Name)) + metrics.SendErrorLogAndMetric(util.NSID, err.Error()) + return + } + + nsc.npMgr.Lock() + defer nsc.npMgr.Unlock() + + nsKey := util.GetNSNameWithPrefix(key) + _, nsExists := nsc.npMgr.NsMap[nsKey] + if !nsExists { + log.Logf("[NAMESPACE Delete EVENT] Namespace [%s] does not exist in case, so returning", key) + return + } + + nsc.workqueue.Add(key) +} + +func (nsc *nameSpaceController) Run(threadiness int, stopCh <-chan struct{}) error { + defer utilruntime.HandleCrash() + defer nsc.workqueue.ShutDown() + + // Start the informer factories to begin populating the informer caches + log.Logf("Starting Namespace controller\n") + + log.Logf("Starting workers") + // Launch two workers to process namespace resources + for i := 0; i < threadiness; i++ { + go wait.Until(nsc.runWorker, time.Second, stopCh) + } + + log.Logf("Started workers") + <-stopCh + log.Logf("Shutting down workers") + + return nil +} + +func (nsc *nameSpaceController) runWorker() { + for nsc.processNextWorkItem() { + } +} + +func (nsc *nameSpaceController) processNextWorkItem() bool { + obj, shutdown := nsc.workqueue.Get() + + if shutdown { + return false + } + + err := func(obj interface{}) error { + defer nsc.workqueue.Done(obj) + var key string + var ok bool + if key, ok = obj.(string); !ok { + // As the item in the workqueue is actually invalid, we call + // Forget here else we'd go into a loop of attempting to + // process a work item that is invalid. + nsc.workqueue.Forget(obj) + utilruntime.HandleError(fmt.Errorf("expected string in workqueue but got %#v", obj)) + return nil + } + // Run the syncHandler, passing it the namespace string of the + // resource to be synced. + // TODO : may consider using "c.queue.AddAfter(key, *requeueAfter)" according to error type later + if err := nsc.syncNameSpace(key); err != nil { + // Put the item back on the workqueue to handle any transient errors. + nsc.workqueue.AddRateLimited(key) + return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) + } + // Finally, if no error occurs we Forget this item so it does not + // get queued again until another change happens. + nsc.workqueue.Forget(obj) + log.Logf("Successfully synced '%s'", key) + return nil + }(obj) + + if err != nil { + utilruntime.HandleError(err) + return true + } + + return true +} + +// syncNameSpace compares the actual state with the desired, and attempts to converge the two. +func (nsc *nameSpaceController) syncNameSpace(key string) error { + // Get the NameSpace resource with this key + nsObj, err := nsc.nameSpaceLister.Get(key) + // lock to complete events + // TODO: Reduce scope of lock later + nsc.npMgr.Lock() + defer nsc.npMgr.Unlock() + if err != nil { + if errors.IsNotFound(err) { + utilruntime.HandleError(fmt.Errorf("NameSpace '%s' in work queue no longer exists", key)) + // find the namespace object from a local cache and start cleaning up process (calling cleanUpDeletedPod function) + nsKey := util.GetNSNameWithPrefix(key) + cachedNs, found := nsc.npMgr.NsMap[nsKey] + if found { + // TODO: better to use cachedPod when calling cleanUpDeletedPod? + err = nsc.cleanDeletedNamespace(getNamespaceObjFromNsObj(cachedNs)) + if err != nil { + // cleaning process was failed, need to requeue and retry later. + return fmt.Errorf("Cannot delete ipset due to %s\n", err.Error()) + } + } + // for other transient apiserver error requeue with exponential backoff + return err + } + } + + if nsObj.DeletionTimestamp != nil || nsObj.DeletionGracePeriodSeconds != nil { + return nsc.cleanDeletedNamespace(nsObj) + + } + + err = nsc.syncUpdateNameSpace(nsObj) + // 1. deal with error code and retry this + if err != nil { + return fmt.Errorf("Failed to sync namespace due to %s\n", err.Error()) + } + + return nil +} + +// InitAllNsList syncs all-namespace ipset list. +func (npMgr *NetworkPolicyManager) InitAllNsList() error { + allNs := npMgr.NsMap[util.KubeAllNamespacesFlag] + for ns := range npMgr.NsMap { + if ns == util.KubeAllNamespacesFlag { + continue + } + + if err := allNs.IpsMgr.AddToList(util.KubeAllNamespacesFlag, ns); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[InitAllNsList] Error: failed to add namespace set %s to ipset list %s with err: %v", ns, util.KubeAllNamespacesFlag, err) + return err + } + } + + return nil +} + +// UninitAllNsList cleans all-namespace ipset list. +func (npMgr *NetworkPolicyManager) UninitAllNsList() error { + allNs := npMgr.NsMap[util.KubeAllNamespacesFlag] + for ns := range npMgr.NsMap { + if ns == util.KubeAllNamespacesFlag { + continue + } + + if err := allNs.IpsMgr.DeleteFromList(util.KubeAllNamespacesFlag, ns); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[UninitAllNsList] Error: failed to delete namespace set %s from list %s with err: %v", ns, util.KubeAllNamespacesFlag, err) + return err + } + } + + return nil +} + +// syncAddNameSpace handles adding namespace to ipset. +func (nsc *nameSpaceController) syncAddNameSpace(nsObj *corev1.Namespace) error { + var err error + + nsName, nsLabel := util.GetNSNameWithPrefix(nsObj.ObjectMeta.Name), nsObj.ObjectMeta.Labels + log.Logf("NAMESPACE CREATING: [%s/%v]", nsName, nsLabel) + + ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr + // Create ipset for the namespace. + if err = ipsMgr.CreateSet(nsName, append([]string{util.IpsetNetHashFlag})); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to create ipset for namespace %s with err: %v", nsName, err) + return err + } + + if err = ipsMgr.AddToList(util.KubeAllNamespacesFlag, nsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to add %s to all-namespace ipset list with err: %v", nsName, err) + return err + } + + // Add the namespace to its label's ipset list. + nsLabels := util.GetSetsFromLabels(nsObj.ObjectMeta.Labels) + for _, nsLabel := range nsLabels { + labelKey := util.GetNSNameWithPrefix(nsLabel) + log.Logf("Adding namespace %s to ipset list %s", nsName, labelKey) + if err = ipsMgr.AddToList(labelKey, nsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", nsName, labelKey, err) + return err + } + } + + ns, err := newNs(nsName) + if err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to create namespace %s with err: %v", nsName, err) + } + setResourceVersion(ns, nsObj.GetObjectMeta().GetResourceVersion()) + + // Append all labels to the cache NS obj + ns.LabelsMap = util.AppendMap(ns.LabelsMap, nsLabel) + nsc.npMgr.NsMap[nsName] = ns + + return nil +} + +// syncUpdateNameSpace handles updating namespace in ipset. +func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) error { + var err error + newNsNs, newNsLabel := util.GetNSNameWithPrefix(newNsObj.ObjectMeta.Name), newNsObj.ObjectMeta.Labels + log.Logf( + "NAMESPACE UPDATING:\n namespace: [%s/%v]", + newNsNs, newNsLabel, + ) + + // If orignal AddNamespace failed for some reason, then NS will not be found + // in nsMap, resulting in retry of ADD. + curNsObj, exists := nsc.npMgr.NsMap[newNsNs] + if !exists { + if newNsObj.ObjectMeta.DeletionTimestamp == nil && newNsObj.ObjectMeta.DeletionGracePeriodSeconds == nil { + if err = nsc.syncAddNameSpace(newNsObj); err != nil { + return err + } + } + + return nil + } + + newRv := util.ParseResourceVersion(newNsObj.ObjectMeta.ResourceVersion) + if !util.CompareUintResourceVersions(curNsObj.resourceVersion, newRv) { + log.Logf("Cached NameSpace has larger ResourceVersion number than new Obj. NameSpace: %s Cached RV: %d New RV:\n", + newNsNs, + curNsObj.resourceVersion, + newRv, + ) + return nil + } + + //If the Namespace is not deleted, delete removed labels and create new labels + addToIPSets, deleteFromIPSets := util.GetIPSetListCompareLabels(curNsObj.LabelsMap, newNsLabel) + + // Delete the namespace from its label's ipset list. + ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr + for _, nsLabelVal := range deleteFromIPSets { + labelKey := util.GetNSNameWithPrefix(nsLabelVal) + log.Logf("Deleting namespace %s from ipset list %s", newNsNs, labelKey) + if err = ipsMgr.DeleteFromList(labelKey, newNsNs); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", newNsNs, labelKey, err) + return err + } + } + + // Add the namespace to its label's ipset list. + for _, nsLabelVal := range addToIPSets { + labelKey := util.GetNSNameWithPrefix(nsLabelVal) + log.Logf("Adding namespace %s to ipset list %s", newNsNs, labelKey) + if err = ipsMgr.AddToList(labelKey, newNsNs); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", newNsNs, labelKey, err) + return err + } + } + + // Append all labels to the cache NS obj + curNsObj.LabelsMap = util.ClearAndAppendMap(curNsObj.LabelsMap, newNsLabel) + setResourceVersion(curNsObj, newNsObj.GetObjectMeta().GetResourceVersion()) + nsc.npMgr.NsMap[newNsNs] = curNsObj + + return nil +} + +// cleanDeletedNamespace handles deleting namespace from ipset. +func (nsc *nameSpaceController) cleanDeletedNamespace(nsObj *corev1.Namespace) error { + var err error + + nsName, nsLabel := util.GetNSNameWithPrefix(nsObj.ObjectMeta.Name), nsObj.ObjectMeta.Labels + log.Logf("NAMESPACE DELETING: [%s/%v]", nsName, nsLabel) + + cachedNsObj, exists := nsc.npMgr.NsMap[nsName] + if !exists { + return nil + } + + log.Logf("NAMESPACE DELETING cached labels: [%s/%v]", nsName, cachedNsObj.LabelsMap) + // Delete the namespace from its label's ipset list. + ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr + nsLabels := cachedNsObj.LabelsMap + for nsLabelKey, nsLabelVal := range nsLabels { + labelKey := util.GetNSNameWithPrefix(nsLabelKey) + log.Logf("Deleting namespace %s from ipset list %s", nsName, labelKey) + if err = ipsMgr.DeleteFromList(labelKey, nsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, labelKey, err) + return err + } + + label := util.GetNSNameWithPrefix(nsLabelKey + ":" + nsLabelVal) + log.Logf("Deleting namespace %s from ipset list %s", nsName, label) + if err = ipsMgr.DeleteFromList(label, nsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, label, err) + return err + } + } + + // Delete the namespace from all-namespace ipset list. + if err = ipsMgr.DeleteFromList(util.KubeAllNamespacesFlag, nsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, util.KubeAllNamespacesFlag, err) + return err + } + + // Delete ipset for the namespace. + if err = ipsMgr.DeleteSet(nsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete ipset for namespace %s with err: %v", nsName, err) + return err + } + + delete(nsc.npMgr.NsMap, nsName) + + return nil +} diff --git a/npm/namespace.go b/npm/namespace.go deleted file mode 100644 index bd724bbf9e..0000000000 --- a/npm/namespace.go +++ /dev/null @@ -1,284 +0,0 @@ -// Copyright 2018 Microsoft. All rights reserved. -// MIT License -package npm - -import ( - "reflect" - - "github.com/Azure/azure-container-networking/log" - "github.com/Azure/azure-container-networking/npm/ipsm" - "github.com/Azure/azure-container-networking/npm/iptm" - "github.com/Azure/azure-container-networking/npm/metrics" - "github.com/Azure/azure-container-networking/npm/util" - - corev1 "k8s.io/api/core/v1" -) - -type Namespace struct { - name string - LabelsMap map[string]string // NameSpace labels - SetMap map[string]string - IpsMgr *ipsm.IpsetManager - iptMgr *iptm.IptablesManager - resourceVersion uint64 // NameSpace ResourceVersion -} - -// newNS constructs a new namespace object. -func newNs(name string) (*Namespace, error) { - ns := &Namespace{ - name: name, - LabelsMap: make(map[string]string), - SetMap: make(map[string]string), - IpsMgr: ipsm.NewIpsetManager(), - iptMgr: iptm.NewIptablesManager(), - // resource version is converted to uint64 - // so make sure it is initialized to "0" - resourceVersion: 0, - } - - return ns, nil -} - -// setResourceVersion setter func for RV -func setResourceVersion(nsObj *Namespace, rv string) { - nsObj.resourceVersion = util.ParseResourceVersion(rv) -} - -func isSystemNs(nsObj *corev1.Namespace) bool { - return nsObj.ObjectMeta.Name == util.KubeSystemFlag -} - -func isInvalidNamespaceUpdate(oldNsObj, newNsObj *corev1.Namespace) (isInvalidUpdate bool) { - isInvalidUpdate = oldNsObj.ObjectMeta.Name == newNsObj.ObjectMeta.Name && - newNsObj.ObjectMeta.DeletionTimestamp == nil && - newNsObj.ObjectMeta.DeletionGracePeriodSeconds == nil - isInvalidUpdate = isInvalidUpdate && reflect.DeepEqual(oldNsObj.ObjectMeta.Labels, newNsObj.ObjectMeta.Labels) - - return -} - -// InitAllNsList syncs all-namespace ipset list. -func (npMgr *NetworkPolicyManager) InitAllNsList() error { - allNs := npMgr.NsMap[util.KubeAllNamespacesFlag] - for ns := range npMgr.NsMap { - if ns == util.KubeAllNamespacesFlag { - continue - } - - if err := allNs.IpsMgr.AddToList(util.KubeAllNamespacesFlag, ns); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[InitAllNsList] Error: failed to add namespace set %s to ipset list %s with err: %v", ns, util.KubeAllNamespacesFlag, err) - return err - } - } - - return nil -} - -// UninitAllNsList cleans all-namespace ipset list. -func (npMgr *NetworkPolicyManager) UninitAllNsList() error { - allNs := npMgr.NsMap[util.KubeAllNamespacesFlag] - for ns := range npMgr.NsMap { - if ns == util.KubeAllNamespacesFlag { - continue - } - - if err := allNs.IpsMgr.DeleteFromList(util.KubeAllNamespacesFlag, ns); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[UninitAllNsList] Error: failed to delete namespace set %s from list %s with err: %v", ns, util.KubeAllNamespacesFlag, err) - return err - } - } - - return nil -} - -// AddNamespace handles adding namespace to ipset. -func (npMgr *NetworkPolicyManager) AddNamespace(nsObj *corev1.Namespace) error { - var err error - - nsName, nsLabel := util.GetNSNameWithPrefix(nsObj.ObjectMeta.Name), nsObj.ObjectMeta.Labels - log.Logf("NAMESPACE CREATING: [%s/%v]", nsName, nsLabel) - - ipsMgr := npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr - // Create ipset for the namespace. - if err = ipsMgr.CreateSet(nsName, append([]string{util.IpsetNetHashFlag})); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to create ipset for namespace %s with err: %v", nsName, err) - return err - } - - if err = ipsMgr.AddToList(util.KubeAllNamespacesFlag, nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to add %s to all-namespace ipset list with err: %v", nsName, err) - return err - } - - // Add the namespace to its label's ipset list. - nsLabels := nsObj.ObjectMeta.Labels - for nsLabelKey, nsLabelVal := range nsLabels { - labelKey := util.GetNSNameWithPrefix(nsLabelKey) - log.Logf("Adding namespace %s to ipset list %s", nsName, labelKey) - if err = ipsMgr.AddToList(labelKey, nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", nsName, labelKey, err) - return err - } - - label := util.GetNSNameWithPrefix(nsLabelKey + ":" + nsLabelVal) - log.Logf("Adding namespace %s to ipset list %s", nsName, label) - if err = ipsMgr.AddToList(label, nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", nsName, label, err) - return err - } - } - - ns, err := newNs(nsName) - if err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to create namespace %s with err: %v", nsName, err) - return err - } - setResourceVersion(ns, nsObj.GetObjectMeta().GetResourceVersion()) - - // Append all labels to the cache NS obj - ns.LabelsMap = util.AppendMap(ns.LabelsMap, nsLabel) - npMgr.NsMap[nsName] = ns - - return nil -} - -// UpdateNamespace handles updating namespace in ipset. -func (npMgr *NetworkPolicyManager) UpdateNamespace(oldNsObj *corev1.Namespace, newNsObj *corev1.Namespace) error { - if isInvalidNamespaceUpdate(oldNsObj, newNsObj) { - return nil - } - - var err error - oldNsNs, oldNsLabel := util.GetNSNameWithPrefix(oldNsObj.ObjectMeta.Name), oldNsObj.ObjectMeta.Labels - newNsNs, newNsLabel := util.GetNSNameWithPrefix(newNsObj.ObjectMeta.Name), newNsObj.ObjectMeta.Labels - log.Logf( - "NAMESPACE UPDATING:\n old namespace: [%s/%v]\n new namespace: [%s/%v]", - oldNsNs, oldNsLabel, newNsNs, newNsLabel, - ) - - if oldNsNs != newNsNs { - if err = npMgr.DeleteNamespace(oldNsObj); err != nil { - return err - } - - if newNsObj.ObjectMeta.DeletionTimestamp == nil && newNsObj.ObjectMeta.DeletionGracePeriodSeconds == nil { - if err = npMgr.AddNamespace(newNsObj); err != nil { - return err - } - } - - return nil - } - - // If orignal AddNamespace failed for some reason, then NS will not be found - // in nsMap, resulting in retry of ADD. - curNsObj, exists := npMgr.NsMap[newNsNs] - if !exists { - if newNsObj.ObjectMeta.DeletionTimestamp == nil && newNsObj.ObjectMeta.DeletionGracePeriodSeconds == nil { - if err = npMgr.AddNamespace(newNsObj); err != nil { - return err - } - } - - return nil - } - - newRv := util.ParseResourceVersion(newNsObj.ObjectMeta.ResourceVersion) - if !util.CompareUintResourceVersions(curNsObj.resourceVersion, newRv) { - log.Logf("Cached NameSpace has larger ResourceVersion number than new Obj. NameSpace: %s Cached RV: %d New RV:\n", - oldNsNs, - curNsObj.resourceVersion, - newRv, - ) - return nil - } - - //if no change in labels then return - if reflect.DeepEqual(curNsObj.LabelsMap, newNsLabel) { - log.Logf( - "NAMESPACE UPDATING: nothing to delete or add. namespace: [%s/%v]", - newNsNs, newNsLabel, - ) - return nil - } - - //If the Namespace is not deleted, delete removed labels and create new labels - addToIPSets, deleteFromIPSets := util.GetIPSetListCompareLabels(curNsObj.LabelsMap, newNsLabel) - - // Delete the namespace from its label's ipset list. - ipsMgr := npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr - for _, nsLabelVal := range deleteFromIPSets { - labelKey := util.GetNSNameWithPrefix(nsLabelVal) - log.Logf("Deleting namespace %s from ipset list %s", oldNsNs, labelKey) - if err = ipsMgr.DeleteFromList(labelKey, oldNsNs); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", oldNsNs, labelKey, err) - return err - } - } - - // Add the namespace to its label's ipset list. - for _, nsLabelVal := range addToIPSets { - labelKey := util.GetNSNameWithPrefix(nsLabelVal) - log.Logf("Adding namespace %s to ipset list %s", oldNsNs, labelKey) - if err = ipsMgr.AddToList(labelKey, oldNsNs); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", oldNsNs, labelKey, err) - return err - } - } - - // Append all labels to the cache NS obj - curNsObj.LabelsMap = util.ClearAndAppendMap(curNsObj.LabelsMap, newNsLabel) - setResourceVersion(curNsObj, newNsObj.GetObjectMeta().GetResourceVersion()) - npMgr.NsMap[newNsNs] = curNsObj - - return nil -} - -// DeleteNamespace handles deleting namespace from ipset. -func (npMgr *NetworkPolicyManager) DeleteNamespace(nsObj *corev1.Namespace) error { - var err error - - nsName, nsLabel := util.GetNSNameWithPrefix(nsObj.ObjectMeta.Name), nsObj.ObjectMeta.Labels - log.Logf("NAMESPACE DELETING: [%s/%v]", nsName, nsLabel) - - cachedNsObj, exists := npMgr.NsMap[nsName] - if !exists { - return nil - } - - log.Logf("NAMESPACE DELETING cached labels: [%s/%v]", nsName, cachedNsObj.LabelsMap) - // Delete the namespace from its label's ipset list. - ipsMgr := npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr - nsLabels := cachedNsObj.LabelsMap - for nsLabelKey, nsLabelVal := range nsLabels { - labelKey := util.GetNSNameWithPrefix(nsLabelKey) - log.Logf("Deleting namespace %s from ipset list %s", nsName, labelKey) - if err = ipsMgr.DeleteFromList(labelKey, nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, labelKey, err) - return err - } - - label := util.GetNSNameWithPrefix(nsLabelKey + ":" + nsLabelVal) - log.Logf("Deleting namespace %s from ipset list %s", nsName, label) - if err = ipsMgr.DeleteFromList(label, nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, label, err) - return err - } - } - - // Delete the namespace from all-namespace ipset list. - if err = ipsMgr.DeleteFromList(util.KubeAllNamespacesFlag, nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, util.KubeAllNamespacesFlag, err) - return err - } - - // Delete ipset for the namespace. - if err = ipsMgr.DeleteSet(nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete ipset for namespace %s with err: %v", nsName, err) - return err - } - - delete(npMgr.NsMap, nsName) - - return nil -} diff --git a/npm/npm.go b/npm/npm.go index b83ef51dc2..58d8a0cbf5 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -43,10 +43,13 @@ type NetworkPolicyManager struct { sync.Mutex clientset *kubernetes.Clientset - informerFactory informers.SharedInformerFactory - podInformer coreinformers.PodInformer - nsInformer coreinformers.NamespaceInformer - npInformer networkinginformers.NetworkPolicyInformer + informerFactory informers.SharedInformerFactory + podInformer coreinformers.PodInformer + nsInformer coreinformers.NamespaceInformer + npInformer networkinginformers.NetworkPolicyInformer + nameSpaceController *nameSpaceController + + npInformer networkinginformers.NetworkPolicyInformer NodeName string NsMap map[string]*Namespace @@ -304,53 +307,56 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in }, ) - nsInformer.Informer().AddEventHandler( - // Namespace event handlers - cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { - nameSpaceObj, ok := obj.(*corev1.Namespace) - if !ok { - metrics.SendErrorLogAndMetric(util.NpmID, "ADD NameSpace: Received unexpected object type: %v", obj) - return - } - npMgr.Lock() - npMgr.AddNamespace(nameSpaceObj) - npMgr.Unlock() - }, - UpdateFunc: func(old, new interface{}) { - oldNameSpaceObj, ok := old.(*corev1.Namespace) - if !ok { - metrics.SendErrorLogAndMetric(util.NpmID, "UPDATE NameSpace: Received unexpected old object type: %v", oldNameSpaceObj) - return - } - newNameSpaceObj, ok := new.(*corev1.Namespace) - if !ok { - metrics.SendErrorLogAndMetric(util.NpmID, "UPDATE NameSpace: Received unexpected new object type: %v", newNameSpaceObj) - return - } - npMgr.Lock() - npMgr.UpdateNamespace(oldNameSpaceObj, newNameSpaceObj) - npMgr.Unlock() - }, - DeleteFunc: func(obj interface{}) { - nameSpaceObj, ok := obj.(*corev1.Namespace) - if !ok { - tombstone, ok := obj.(cache.DeletedFinalStateUnknown) - if !ok { - metrics.SendErrorLogAndMetric(util.NpmID, "DELETE NameSpace: Received unexpected object type: %v", obj) - return - } - if nameSpaceObj, ok = tombstone.Obj.(*corev1.Namespace); !ok { - metrics.SendErrorLogAndMetric(util.NpmID, "DELETE NameSpace: Received unexpected object type: %v", obj) - return - } - } - npMgr.Lock() - npMgr.DeleteNamespace(nameSpaceObj) - npMgr.Unlock() - }, - }, - ) + // create NameSpace controller + npMgr.nameSpaceController = NewNameSpaceController(nsInformer, clientset, npMgr) + + // nsInformer.Informer().AddEventHandler( + // // Namespace event handlers + // cache.ResourceEventHandlerFuncs{ + // AddFunc: func(obj interface{}) { + // nameSpaceObj, ok := obj.(*corev1.Namespace) + // if !ok { + // metrics.SendErrorLogAndMetric(util.NpmID, "ADD NameSpace: Received unexpected object type: %v", obj) + // return + // } + // npMgr.Lock() + // npMgr.AddNamespace(nameSpaceObj) + // npMgr.Unlock() + // }, + // UpdateFunc: func(old, new interface{}) { + // oldNameSpaceObj, ok := old.(*corev1.Namespace) + // if !ok { + // metrics.SendErrorLogAndMetric(util.NpmID, "UPDATE NameSpace: Received unexpected old object type: %v", oldNameSpaceObj) + // return + // } + // newNameSpaceObj, ok := new.(*corev1.Namespace) + // if !ok { + // metrics.SendErrorLogAndMetric(util.NpmID, "UPDATE NameSpace: Received unexpected new object type: %v", newNameSpaceObj) + // return + // } + // npMgr.Lock() + // npMgr.UpdateNamespace(oldNameSpaceObj, newNameSpaceObj) + // npMgr.Unlock() + // }, + // DeleteFunc: func(obj interface{}) { + // nameSpaceObj, ok := obj.(*corev1.Namespace) + // if !ok { + // tombstone, ok := obj.(cache.DeletedFinalStateUnknown) + // if !ok { + // metrics.SendErrorLogAndMetric(util.NpmID, "DELETE NameSpace: Received unexpected object type: %v", obj) + // return + // } + // if nameSpaceObj, ok = tombstone.Obj.(*corev1.Namespace); !ok { + // metrics.SendErrorLogAndMetric(util.NpmID, "DELETE NameSpace: Received unexpected object type: %v", obj) + // return + // } + // } + // npMgr.Lock() + // npMgr.DeleteNamespace(nameSpaceObj) + // npMgr.Unlock() + // }, + // }, + // ) npInformer.Informer().AddEventHandler( // Network policy event handlers diff --git a/npm/util/util.go b/npm/util/util.go index 9f7d455cf5..c631c52579 100644 --- a/npm/util/util.go +++ b/npm/util/util.go @@ -297,3 +297,14 @@ func ParseResourceVersion(rv string) uint64 { func GetObjKeyFunc(obj interface{}) (string, error) { return cache.MetaNamespaceKeyFunc(obj) } + +// GetSetsFromLabels for a given map of labels will return ipset names +func GetSetsFromLabels(labels map[string]string) []string { + l := []string{} + + for k, v := range labels { + l = append(l, k, fmt.Sprintf("%s:%s", k, v)) + } + + return l +} From 55b6118a39dcf791f1bf3d4857210eadbf4d75f4 Mon Sep 17 00:00:00 2001 From: vakr Date: Mon, 29 Mar 2021 17:36:37 -0700 Subject: [PATCH 02/17] Unit tests for namespace controller --- npm/nameSpaceController.go | 8 +- npm/nameSpaceController_test.go | 402 ++++++++++++++++++++++++++++++++ npm/namespace_test.go | 338 --------------------------- npm/npm.go | 50 ---- npm/npm_test.go | 27 +++ npm/nwpolicy_test.go | 45 ---- 6 files changed, 433 insertions(+), 437 deletions(-) create mode 100644 npm/nameSpaceController_test.go delete mode 100644 npm/namespace_test.go create mode 100644 npm/npm_test.go diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index 5ccbba556c..1744036d29 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -78,7 +78,7 @@ func isInvalidNamespaceUpdate(oldNsObj, newNsObj *corev1.Namespace) (isInvalidUp } type nameSpaceController struct { - clientset *kubernetes.Clientset + clientset kubernetes.Interface nameSpaceLister corelisters.NamespaceLister nameSpaceListerSynced cache.InformerSynced workqueue workqueue.RateLimitingInterface @@ -86,7 +86,7 @@ type nameSpaceController struct { npMgr *NetworkPolicyManager } -func NewNameSpaceController(nameSpaceInformer coreinformer.NamespaceInformer, clientset *kubernetes.Clientset, npMgr *NetworkPolicyManager) *nameSpaceController { +func NewNameSpaceController(nameSpaceInformer coreinformer.NamespaceInformer, clientset kubernetes.Interface, npMgr *NetworkPolicyManager) *nameSpaceController { nameSpaceController := &nameSpaceController{ clientset: clientset, nameSpaceLister: nameSpaceInformer.Lister(), @@ -306,7 +306,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { err = nsc.cleanDeletedNamespace(getNamespaceObjFromNsObj(cachedNs)) if err != nil { // cleaning process was failed, need to requeue and retry later. - return fmt.Errorf("Cannot delete ipset due to %s\n", err.Error()) + return fmt.Errorf("cannot delete ipset due to %s\n", err.Error()) } } // for other transient apiserver error requeue with exponential backoff @@ -322,7 +322,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { err = nsc.syncUpdateNameSpace(nsObj) // 1. deal with error code and retry this if err != nil { - return fmt.Errorf("Failed to sync namespace due to %s\n", err.Error()) + return fmt.Errorf("failed to sync namespace due to %s\n", err.Error()) } return nil diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go new file mode 100644 index 0000000000..a8ed94c182 --- /dev/null +++ b/npm/nameSpaceController_test.go @@ -0,0 +1,402 @@ +// Copyright 2018 Microsoft. All rights reserved. +// MIT License +package npm + +import ( + "reflect" + "testing" + "time" + + "github.com/Azure/azure-container-networking/npm/ipsm" + "github.com/Azure/azure-container-networking/npm/util" + corev1 "k8s.io/api/core/v1" + networkingv1 "k8s.io/api/networking/v1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + kubeinformers "k8s.io/client-go/informers" + k8sfake "k8s.io/client-go/kubernetes/fake" + core "k8s.io/client-go/testing" +) + +var ( + alwaysReady = func() bool { return true } + noResyncPeriodFunc = func() time.Duration { return 0 } +) + +type expectedNsValues struct { + expectedLenOfPodMap int + expectedLenOfNsMap int + expectedLenOfWorkQueue int +} + +type nameSpaceFixture struct { + t *testing.T + + kubeclient *k8sfake.Clientset + // Objects to put in the store. + nsLister []*corev1.Namespace + // Actions expected to happen on the client. + kubeactions []core.Action + // Objects from here preloaded into NewSimpleFake. + kubeobjects []runtime.Object + + // (TODO) will remove npMgr if possible + npMgr *NetworkPolicyManager + ipsMgr *ipsm.IpsetManager + nsController *nameSpaceController + kubeInformer kubeinformers.SharedInformerFactory +} + +func newNsFixture(t *testing.T) *nameSpaceFixture { + f := &nameSpaceFixture{ + t: t, + nsLister: []*corev1.Namespace{}, + kubeobjects: []runtime.Object{}, + npMgr: newNPMgr(t), + ipsMgr: ipsm.NewIpsetManager(), + } + return f +} + +func (f *nameSpaceFixture) newNsController() { + f.kubeclient = k8sfake.NewSimpleClientset(f.kubeobjects...) + f.kubeInformer = kubeinformers.NewSharedInformerFactory(f.kubeclient, noResyncPeriodFunc()) + + f.nsController = NewNameSpaceController(f.kubeInformer.Core().V1().Namespaces(), f.kubeclient, f.npMgr) + f.nsController.nameSpaceListerSynced = alwaysReady + + for _, ns := range f.nsLister { + f.kubeInformer.Core().V1().Namespaces().Informer().GetIndexer().Add(ns) + } +} + +func (f *nameSpaceFixture) ipSetSave(ipsetConfigFile string) { + // call /sbin/ipset save -file /var/log/ipset-test.conf + f.t.Logf("Start storing ipset to %s", ipsetConfigFile) + if err := f.ipsMgr.Save(ipsetConfigFile); err != nil { + f.t.Errorf("TestAddPod failed @ ipsMgr.Save") + } +} + +func (f *nameSpaceFixture) ipSetRestore(ipsetConfigFile string) { + // call /sbin/ipset restore -file /var/log/ipset-test.conf + f.t.Logf("Start re-storing ipset to %s", ipsetConfigFile) + if err := f.ipsMgr.Restore(ipsetConfigFile); err != nil { + f.t.Errorf("TestAddPod failed @ ipsMgr.Restore") + } +} +func newNPMgr(t *testing.T) *NetworkPolicyManager { + npMgr := &NetworkPolicyManager{ + NsMap: make(map[string]*Namespace), + PodMap: make(map[string]*NpmPod), + RawNpMap: make(map[string]*networkingv1.NetworkPolicy), + ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), + TelemetryEnabled: false, + } + + // This initialization important as without this NPM will panic + allNs, _ := newNs(util.KubeAllNamespacesFlag) + npMgr.NsMap[util.KubeAllNamespacesFlag] = allNs + return npMgr +} + +func newNameSpace(name, rv string, labels map[string]string) *corev1.Namespace { + return &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Labels: labels, + ResourceVersion: rv, + }, + } +} + +func addNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) { + f.nsLister = append(f.nsLister, nsObj) + f.kubeobjects = append(f.kubeobjects, nsObj) + + f.newNsController() + stopCh := make(chan struct{}) + defer close(stopCh) + f.kubeInformer.Start(stopCh) + + t.Logf("Calling add namespace event") + f.nsController.addNamespace(nsObj) + f.nsController.processNextWorkItem() +} + +func updateNamespace(t *testing.T, f *nameSpaceFixture, oldNsObj, newNsObj *corev1.Namespace) { + addNamespace(t, f, oldNsObj) + t.Logf("Complete add namespace event") + + t.Logf("Updating kubeinformer namespace object") + f.kubeInformer.Core().V1().Namespaces().Informer().GetIndexer().Update(newNsObj) + + t.Logf("Calling update namespace event") + f.nsController.updateNamespace(oldNsObj, newNsObj) + f.nsController.processNextWorkItem() +} + +func deleteNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) { + addNamespace(t, f, nsObj) + t.Logf("Complete add namespace event") + + t.Logf("Updating kubeinformer namespace object") + f.kubeInformer.Core().V1().Namespaces().Informer().GetIndexer().Delete(nsObj) + + t.Logf("Calling delete namespace event") + f.nsController.deleteNamespace(nsObj) + f.nsController.processNextWorkItem() +} + +func TestNewNs(t *testing.T) { + if _, err := newNs("test"); err != nil { + t.Errorf("TestnewNs failed @ newNs") + } +} + +func TestAllNsList(t *testing.T) { + npMgr := &NetworkPolicyManager{} + + ipsMgr := ipsm.NewIpsetManager() + if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { + t.Errorf("TestAllNsList failed @ ipsMgr.Save") + } + + defer func() { + if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { + t.Errorf("TestAllNsList failed @ ipsMgr.Restore") + } + }() + + if err := npMgr.InitAllNsList(); err != nil { + t.Errorf("TestAllNsList failed @ InitAllNsList") + } + + if err := npMgr.UninitAllNsList(); err != nil { + t.Errorf("TestAllNsList failed @ UninitAllNsList") + } +} + +func TestAddNamespace(t *testing.T) { + f := newNsFixture(t) + f.ipSetSave(util.IpsetTestConfigFile) + defer f.ipSetRestore(util.IpsetTestConfigFile) + + nsObj := newNameSpace( + "test-namespace", + "0", + map[string]string{ + "app": "test-namespace", + }, + ) + + addNamespace(t, f, nsObj) + + testCases := []expectedNsValues{ + {0, 2, 0}, + } + checkNsTestResult("TestAddNamespace", f, testCases) + + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(nsObj.Name)]; exists { + t.Errorf("TestAddNamespace failed @ npMgr.nsMap check") + } +} + +func TestUpdateNamespace(t *testing.T) { + f := newNsFixture(t) + f.ipSetSave(util.IpsetTestConfigFile) + defer f.ipSetRestore(util.IpsetTestConfigFile) + + oldNsObj := newNameSpace( + "test-namespace", + "0", + map[string]string{ + "app": "test-namespace", + }, + ) + + newNsObj := newNameSpace( + "test-namespace", + "1", + map[string]string{ + "app": "new-test-namespace", + }, + ) + updateNamespace(t, f, oldNsObj, newNsObj) + + testCases := []expectedNsValues{ + {0, 2, 0}, + } + checkNsTestResult("TestUpdateNamespace", f, testCases) + + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + t.Errorf("TestUpdateNamespace failed @ npMgr.nsMap check") + } + + if !reflect.DeepEqual( + newNsObj.Labels, + f.npMgr.NsMap[util.GetNSNameWithPrefix(oldNsObj.Name)].LabelsMap, + ) { + t.Fatalf("TestUpdateNamespace failed @ npMgr.nsMap labelMap check") + } +} + +func TestAddNamespaceLabel(t *testing.T) { + f := newNsFixture(t) + f.ipSetSave(util.IpsetTestConfigFile) + defer f.ipSetRestore(util.IpsetTestConfigFile) + + oldNsObj := newNameSpace( + "test-namespace", + "0", + map[string]string{ + "app": "test-namespace", + }, + ) + newNsObj := newNameSpace( + "test-namespace", + "1", + map[string]string{ + "app": "new-test-namespace", + "update": "true", + }, + ) + updateNamespace(t, f, oldNsObj, newNsObj) + + testCases := []expectedNsValues{ + {0, 2, 0}, + } + checkNsTestResult("TestAddNamespaceLabel", f, testCases) + + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + t.Errorf("TestAddNamespaceLabel failed @ npMgr.nsMap check") + } + + if !reflect.DeepEqual( + newNsObj.Labels, + f.npMgr.NsMap[util.GetNSNameWithPrefix(oldNsObj.Name)].LabelsMap, + ) { + t.Fatalf("TestAddNamespaceLabel failed @ npMgr.nsMap labelMap check") + } +} + +func TestAddNamespaceLabelSameRv(t *testing.T) { + f := newNsFixture(t) + f.ipSetSave(util.IpsetTestConfigFile) + defer f.ipSetRestore(util.IpsetTestConfigFile) + + oldNsObj := newNameSpace( + "test-namespace", + "0", + map[string]string{ + "app": "test-namespace", + }, + ) + + newNsObj := newNameSpace( + "test-namespace", + "0", + map[string]string{ + "app": "new-test-namespace", + "update": "true", + }, + ) + updateNamespace(t, f, oldNsObj, newNsObj) + + testCases := []expectedNsValues{ + {0, 2, 0}, + } + checkNsTestResult("TestAddNamespaceLabelSameRv", f, testCases) + + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + t.Errorf("TestAddNamespaceLabelSameRv failed @ npMgr.nsMap check") + } + + if !reflect.DeepEqual( + oldNsObj.Labels, + f.npMgr.NsMap[util.GetNSNameWithPrefix(oldNsObj.Name)].LabelsMap, + ) { + t.Fatalf("TestAddNamespaceLabelSameRv failed @ npMgr.nsMap labelMap check") + } +} + +func TestDeleteandUpdateNamespaceLabel(t *testing.T) { + f := newNsFixture(t) + f.ipSetSave(util.IpsetTestConfigFile) + defer f.ipSetRestore(util.IpsetTestConfigFile) + + oldNsObj := newNameSpace( + "test-namespace", + "0", + map[string]string{ + "app": "old-test-namespace", + "update": "true", + "group": "test", + }, + ) + + newNsObj := newNameSpace( + "test-namespace", + "1", + map[string]string{ + "app": "old-test-namespace", + "update": "false", + }, + ) + updateNamespace(t, f, oldNsObj, newNsObj) + + testCases := []expectedNsValues{ + {0, 2, 0}, + } + checkNsTestResult("TestDeleteandUpdateNamespaceLabel", f, testCases) + + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.nsMap check") + } + + if !reflect.DeepEqual( + oldNsObj.Labels, + f.npMgr.NsMap[util.GetNSNameWithPrefix(oldNsObj.Name)].LabelsMap, + ) { + t.Fatalf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.nsMap labelMap check") + } +} + +func TestDeleteNamespace(t *testing.T) { + f := newNsFixture(t) + f.ipSetSave(util.IpsetTestConfigFile) + defer f.ipSetRestore(util.IpsetTestConfigFile) + + nsObj := newNameSpace( + "test-namespace", + "0", + map[string]string{ + "app": "test-namespace", + }, + ) + deleteNamespace(t, f, nsObj) + + testCases := []expectedNsValues{ + {0, 1, 0}, + } + checkNsTestResult("TestDeleteNamespace", f, testCases) + + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(nsObj.Name)]; exists { + t.Errorf("TestDeleteNamespace failed @ npMgr.nsMap check") + } +} + +func checkNsTestResult(testName string, f *nameSpaceFixture, testCases []expectedNsValues) { + for _, test := range testCases { + if got := len(f.npMgr.PodMap); got != test.expectedLenOfPodMap { + f.t.Errorf("PodMap length = %d, want %d", got, test.expectedLenOfPodMap) + } + if got := len(f.npMgr.NsMap); got != test.expectedLenOfNsMap { + f.t.Errorf("npMgr length = %d, want %d", got, test.expectedLenOfNsMap) + } + if got := f.nsController.workqueue.Len(); got != test.expectedLenOfWorkQueue { + f.t.Errorf("Workqueue length = %d, want %d", got, test.expectedLenOfWorkQueue) + } + } +} diff --git a/npm/namespace_test.go b/npm/namespace_test.go deleted file mode 100644 index 8e8abdd104..0000000000 --- a/npm/namespace_test.go +++ /dev/null @@ -1,338 +0,0 @@ -// Copyright 2018 Microsoft. All rights reserved. -// MIT License -package npm - -import ( - "os" - "reflect" - "testing" - - "github.com/Azure/azure-container-networking/npm/iptm" - "github.com/Azure/azure-container-networking/npm/metrics" - - "github.com/Azure/azure-container-networking/npm/ipsm" - "github.com/Azure/azure-container-networking/npm/util" - corev1 "k8s.io/api/core/v1" - networkingv1 "k8s.io/api/networking/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func TestNewNs(t *testing.T) { - if _, err := newNs("test"); err != nil { - t.Errorf("TestnewNs failed @ newNs") - } -} - -func TestAllNsList(t *testing.T) { - npMgr := &NetworkPolicyManager{} - - ipsMgr := ipsm.NewIpsetManager() - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAllNsList failed @ ipsMgr.Save") - } - - defer func() { - if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAllNsList failed @ ipsMgr.Restore") - } - }() - - if err := npMgr.InitAllNsList(); err != nil { - t.Errorf("TestAllNsList failed @ InitAllNsList") - } - - if err := npMgr.UninitAllNsList(); err != nil { - t.Errorf("TestAllNsList failed @ UninitAllNsList") - } -} - -func TestAddNamespace(t *testing.T) { - npMgr := &NetworkPolicyManager{ - NsMap: make(map[string]*Namespace), - PodMap: make(map[string]*NpmPod), - RawNpMap: make(map[string]*networkingv1.NetworkPolicy), - ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), - TelemetryEnabled: false, - } - - allNs, err := newNs(util.KubeAllNamespacesFlag) - if err != nil { - panic(err.Error) - } - npMgr.NsMap[util.KubeAllNamespacesFlag] = allNs - - ipsMgr := ipsm.NewIpsetManager() - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAddNamespace failed @ ipsMgr.Save") - } - - defer func() { - if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAddNamespace failed @ ipsMgr.Restore") - } - }() - - nsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-namespace", - Labels: map[string]string{ - "app": "test-namespace", - }, - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(nsObj); err != nil { - t.Errorf("TestAddNamespace @ npMgr.AddNamespace") - } - npMgr.Unlock() -} - -func TestUpdateNamespace(t *testing.T) { - npMgr := &NetworkPolicyManager{ - NsMap: make(map[string]*Namespace), - PodMap: make(map[string]*NpmPod), - RawNpMap: make(map[string]*networkingv1.NetworkPolicy), - ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), - TelemetryEnabled: false, - } - - allNs, err := newNs(util.KubeAllNamespacesFlag) - if err != nil { - panic(err.Error) - } - npMgr.NsMap[util.KubeAllNamespacesFlag] = allNs - - ipsMgr := ipsm.NewIpsetManager() - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestUpdateNamespace failed @ ipsMgr.Save") - } - - defer func() { - if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestUpdateNamespace failed @ ipsMgr.Restore") - } - }() - - oldNsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "old-test-namespace", - Labels: map[string]string{ - "app": "old-test-namespace", - }, - }, - } - - newNsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "new-test-namespace", - Labels: map[string]string{ - "app": "new-test-namespace", - }, - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(oldNsObj); err != nil { - t.Errorf("TestUpdateNamespace failed @ npMgr.AddNamespace") - } - - if err := npMgr.UpdateNamespace(oldNsObj, newNsObj); err != nil { - t.Errorf("TestUpdateNamespace failed @ npMgr.UpdateNamespace") - } - npMgr.Unlock() -} - -func TestAddNamespaceLabel(t *testing.T) { - npMgr := &NetworkPolicyManager{ - NsMap: make(map[string]*Namespace), - PodMap: make(map[string]*NpmPod), - RawNpMap: make(map[string]*networkingv1.NetworkPolicy), - ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), - TelemetryEnabled: false, - } - - allNs, err := newNs(util.KubeAllNamespacesFlag) - if err != nil { - t.Fatal(err.Error()) - } - npMgr.NsMap[util.KubeAllNamespacesFlag] = allNs - - ipsMgr := ipsm.NewIpsetManager() - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAddNamespaceLabel failed @ ipsMgr.Save") - } - - defer func() { - if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAddNamespaceLabel failed @ ipsMgr.Restore") - } - }() - - oldNsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "old-test-namespace", - Labels: map[string]string{ - "app": "old-test-namespace", - }, - ResourceVersion: "0", - }, - } - - newNsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "old-test-namespace", - Labels: map[string]string{ - "app": "old-test-namespace", - "update": "true", - }, - - ResourceVersion: "1", - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(oldNsObj); err != nil { - t.Fatalf("TestAddNamespaceLabel failed @ npMgr.AddNamespace with err %v", err) - } - - if err := npMgr.UpdateNamespace(oldNsObj, newNsObj); err != nil { - t.Fatalf("TestAddNamespaceLabel failed @ npMgr.UpdateNamespace with err %v", err) - } - - if !reflect.DeepEqual(npMgr.NsMap["ns-"+newNsObj.Name].LabelsMap, newNsObj.ObjectMeta.Labels) { - t.Fatalf("TestAddNamespaceLabel failed @ npMgr.nsMap labelMap check") - } - - npMgr.Unlock() -} - -func TestDeleteandUpdateNamespaceLabel(t *testing.T) { - npMgr := &NetworkPolicyManager{ - NsMap: make(map[string]*Namespace), - PodMap: make(map[string]*NpmPod), - RawNpMap: make(map[string]*networkingv1.NetworkPolicy), - ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), - TelemetryEnabled: false, - } - - allNs, err := newNs(util.KubeAllNamespacesFlag) - if err != nil { - t.Fatal(err.Error()) - } - npMgr.NsMap[util.KubeAllNamespacesFlag] = allNs - - ipsMgr := ipsm.NewIpsetManager() - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ ipsMgr.Save") - } - - defer func() { - if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ ipsMgr.Restore") - } - }() - - oldNsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "old-test-namespace", - Labels: map[string]string{ - "app": "old-test-namespace", - "update": "true", - "group": "test", - }, - ResourceVersion: "0", - }, - } - - newNsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "old-test-namespace", - Labels: map[string]string{ - "app": "old-test-namespace", - "update": "false", - }, - ResourceVersion: "1", - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(oldNsObj); err != nil { - t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.AddNamespace") - } - - if err := npMgr.UpdateNamespace(oldNsObj, newNsObj); err != nil { - t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.UpdateNamespace") - } - - if !reflect.DeepEqual(npMgr.NsMap["ns-"+newNsObj.Name].LabelsMap, newNsObj.ObjectMeta.Labels) { - t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.nsMap labelMap check") - } - npMgr.Unlock() -} - -func TestDeleteNamespace(t *testing.T) { - npMgr := &NetworkPolicyManager{ - NsMap: make(map[string]*Namespace), - PodMap: make(map[string]*NpmPod), - RawNpMap: make(map[string]*networkingv1.NetworkPolicy), - ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), - TelemetryEnabled: false, - } - - allNs, err := newNs(util.KubeAllNamespacesFlag) - if err != nil { - panic(err.Error) - } - npMgr.NsMap[util.KubeAllNamespacesFlag] = allNs - - ipsMgr := ipsm.NewIpsetManager() - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestDeleteNamespace failed @ ipsMgr.Save") - } - - defer func() { - if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestDeleteNamespace failed @ ipsMgr.Restore") - } - }() - - nsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-namespace", - Labels: map[string]string{ - "app": "test-namespace", - }, - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(nsObj); err != nil { - t.Errorf("TestDeleteNamespace @ npMgr.AddNamespace") - } - - if err := npMgr.DeleteNamespace(nsObj); err != nil { - t.Errorf("TestDeleteNamespace @ npMgr.DeleteNamespace") - } - - if _, exists := npMgr.NsMap["ns-"+nsObj.Name]; exists { - t.Errorf("TestDeleteNamespace failed @ npMgr.nsMap check") - } - npMgr.Unlock() -} - -func TestMain(m *testing.M) { - metrics.InitializeAll() - iptMgr := iptm.NewIptablesManager() - iptMgr.Save(util.IptablesConfigFile) - - ipsMgr := ipsm.NewIpsetManager() - ipsMgr.Save(util.IpsetConfigFile) - - exitCode := m.Run() - - iptMgr.Restore(util.IptablesConfigFile) - ipsMgr.Restore(util.IpsetConfigFile) - - os.Exit(exitCode) -} diff --git a/npm/npm.go b/npm/npm.go index 58d8a0cbf5..00f22f86b5 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -49,8 +49,6 @@ type NetworkPolicyManager struct { npInformer networkinginformers.NetworkPolicyInformer nameSpaceController *nameSpaceController - npInformer networkinginformers.NetworkPolicyInformer - NodeName string NsMap map[string]*Namespace PodMap map[string]*NpmPod // Key is ns-// @@ -310,54 +308,6 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in // create NameSpace controller npMgr.nameSpaceController = NewNameSpaceController(nsInformer, clientset, npMgr) - // nsInformer.Informer().AddEventHandler( - // // Namespace event handlers - // cache.ResourceEventHandlerFuncs{ - // AddFunc: func(obj interface{}) { - // nameSpaceObj, ok := obj.(*corev1.Namespace) - // if !ok { - // metrics.SendErrorLogAndMetric(util.NpmID, "ADD NameSpace: Received unexpected object type: %v", obj) - // return - // } - // npMgr.Lock() - // npMgr.AddNamespace(nameSpaceObj) - // npMgr.Unlock() - // }, - // UpdateFunc: func(old, new interface{}) { - // oldNameSpaceObj, ok := old.(*corev1.Namespace) - // if !ok { - // metrics.SendErrorLogAndMetric(util.NpmID, "UPDATE NameSpace: Received unexpected old object type: %v", oldNameSpaceObj) - // return - // } - // newNameSpaceObj, ok := new.(*corev1.Namespace) - // if !ok { - // metrics.SendErrorLogAndMetric(util.NpmID, "UPDATE NameSpace: Received unexpected new object type: %v", newNameSpaceObj) - // return - // } - // npMgr.Lock() - // npMgr.UpdateNamespace(oldNameSpaceObj, newNameSpaceObj) - // npMgr.Unlock() - // }, - // DeleteFunc: func(obj interface{}) { - // nameSpaceObj, ok := obj.(*corev1.Namespace) - // if !ok { - // tombstone, ok := obj.(cache.DeletedFinalStateUnknown) - // if !ok { - // metrics.SendErrorLogAndMetric(util.NpmID, "DELETE NameSpace: Received unexpected object type: %v", obj) - // return - // } - // if nameSpaceObj, ok = tombstone.Obj.(*corev1.Namespace); !ok { - // metrics.SendErrorLogAndMetric(util.NpmID, "DELETE NameSpace: Received unexpected object type: %v", obj) - // return - // } - // } - // npMgr.Lock() - // npMgr.DeleteNamespace(nameSpaceObj) - // npMgr.Unlock() - // }, - // }, - // ) - npInformer.Informer().AddEventHandler( // Network policy event handlers cache.ResourceEventHandlerFuncs{ diff --git a/npm/npm_test.go b/npm/npm_test.go new file mode 100644 index 0000000000..75430f9ac2 --- /dev/null +++ b/npm/npm_test.go @@ -0,0 +1,27 @@ +package npm + +import ( + "os" + "testing" + + "github.com/Azure/azure-container-networking/npm/ipsm" + "github.com/Azure/azure-container-networking/npm/iptm" + "github.com/Azure/azure-container-networking/npm/metrics" + "github.com/Azure/azure-container-networking/npm/util" +) + +func TestMain(m *testing.M) { + metrics.InitializeAll() + iptMgr := iptm.NewIptablesManager() + iptMgr.Save(util.IptablesConfigFile) + + ipsMgr := ipsm.NewIpsetManager() + ipsMgr.Save(util.IpsetConfigFile) + + exitCode := m.Run() + + iptMgr.Restore(util.IptablesConfigFile) + ipsMgr.Restore(util.IpsetConfigFile) + + os.Exit(exitCode) +} diff --git a/npm/nwpolicy_test.go b/npm/nwpolicy_test.go index 87a229d663..256d0556f8 100644 --- a/npm/nwpolicy_test.go +++ b/npm/nwpolicy_test.go @@ -57,21 +57,6 @@ func TestAddNetworkPolicy(t *testing.T) { } }() - nsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-nwpolicy", - Labels: map[string]string{ - "app": "test-namespace", - }, - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(nsObj); err != nil { - t.Errorf("TestAddNetworkPolicy @ npMgr.AddNamespace") - } - npMgr.Unlock() - tcp := corev1.ProtocolTCP port8000 := intstr.FromInt(8000) allowIngress := &networkingv1.NetworkPolicy{ @@ -204,21 +189,6 @@ func TestUpdateNetworkPolicy(t *testing.T) { t.Errorf("TestUpdateNetworkPolicy failed @ ipsMgr.CreateSet, adding kube-system set%+v", err) } - nsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-nwpolicy", - Labels: map[string]string{ - "app": "test-namespace", - }, - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(nsObj); err != nil { - t.Errorf("TestUpdateNetworkPolicy @ npMgr.AddNamespace") - } - npMgr.Unlock() - tcp, udp := corev1.ProtocolTCP, corev1.ProtocolUDP allowIngress := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -319,21 +289,6 @@ func TestDeleteNetworkPolicy(t *testing.T) { t.Errorf("TestDeleteNetworkPolicy failed @ ipsMgr.CreateSet, adding kube-system set%+v", err) } - nsObj := &corev1.Namespace{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-nwpolicy", - Labels: map[string]string{ - "app": "test-namespace", - }, - }, - } - - npMgr.Lock() - if err := npMgr.AddNamespace(nsObj); err != nil { - t.Errorf("TestDeleteNetworkPolicy @ npMgr.AddNamespace") - } - npMgr.Unlock() - tcp := corev1.ProtocolTCP allow := &networkingv1.NetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ From c031a9b4b9535bb1573b8c854e08df5ae46adadb Mon Sep 17 00:00:00 2001 From: vakr Date: Mon, 29 Mar 2021 20:49:03 -0700 Subject: [PATCH 03/17] correcting testcases --- npm/nameSpaceController_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index a8ed94c182..a9d5cc45fc 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -198,7 +198,7 @@ func TestAddNamespace(t *testing.T) { } checkNsTestResult("TestAddNamespace", f, testCases) - if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(nsObj.Name)]; exists { + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(nsObj.Name)]; !exists { t.Errorf("TestAddNamespace failed @ npMgr.nsMap check") } } @@ -230,7 +230,7 @@ func TestUpdateNamespace(t *testing.T) { } checkNsTestResult("TestUpdateNamespace", f, testCases) - if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; !exists { t.Errorf("TestUpdateNamespace failed @ npMgr.nsMap check") } @@ -269,7 +269,7 @@ func TestAddNamespaceLabel(t *testing.T) { } checkNsTestResult("TestAddNamespaceLabel", f, testCases) - if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; !exists { t.Errorf("TestAddNamespaceLabel failed @ npMgr.nsMap check") } @@ -309,7 +309,7 @@ func TestAddNamespaceLabelSameRv(t *testing.T) { } checkNsTestResult("TestAddNamespaceLabelSameRv", f, testCases) - if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; !exists { t.Errorf("TestAddNamespaceLabelSameRv failed @ npMgr.nsMap check") } From 35e3cd314bee0f1b8c5e813f5ab37ac80419da7f Mon Sep 17 00:00:00 2001 From: vakr Date: Mon, 29 Mar 2021 21:14:09 -0700 Subject: [PATCH 04/17] correcting testcases --- npm/nameSpaceController_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index a9d5cc45fc..c22738381c 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -122,6 +122,10 @@ func addNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) { t.Logf("Calling add namespace event") f.nsController.addNamespace(nsObj) + if f.nsController.workqueue.Len() == 0 { + t.Logf("Add Namespace: worker queue length is 0 ") + return + } f.nsController.processNextWorkItem() } @@ -134,6 +138,10 @@ func updateNamespace(t *testing.T, f *nameSpaceFixture, oldNsObj, newNsObj *core t.Logf("Calling update namespace event") f.nsController.updateNamespace(oldNsObj, newNsObj) + if f.nsController.workqueue.Len() == 0 { + t.Logf("Update Namespace: worker queue length is 0 ") + return + } f.nsController.processNextWorkItem() } @@ -146,6 +154,10 @@ func deleteNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) t.Logf("Calling delete namespace event") f.nsController.deleteNamespace(nsObj) + if f.nsController.workqueue.Len() == 0 { + t.Logf("Delete Namespace: worker queue length is 0 ") + return + } f.nsController.processNextWorkItem() } From 10d77a969a8f355717b195797848208b6d2d6670 Mon Sep 17 00:00:00 2001 From: vakr Date: Mon, 29 Mar 2021 21:49:37 -0700 Subject: [PATCH 05/17] correcting testcases --- npm/nameSpaceController.go | 10 ++++++---- npm/nameSpaceController_test.go | 8 ++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index 1744036d29..2f68bf9341 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -303,7 +303,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { cachedNs, found := nsc.npMgr.NsMap[nsKey] if found { // TODO: better to use cachedPod when calling cleanUpDeletedPod? - err = nsc.cleanDeletedNamespace(getNamespaceObjFromNsObj(cachedNs)) + err = nsc.cleanDeletedNamespace(cachedNs.name, cachedNs.LabelsMap) if err != nil { // cleaning process was failed, need to requeue and retry later. return fmt.Errorf("cannot delete ipset due to %s\n", err.Error()) @@ -315,7 +315,10 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { } if nsObj.DeletionTimestamp != nil || nsObj.DeletionGracePeriodSeconds != nil { - return nsc.cleanDeletedNamespace(nsObj) + return nsc.cleanDeletedNamespace( + util.GetNSNameWithPrefix(nsObj.Name), + nsObj.Labels, + ) } @@ -470,10 +473,9 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) } // cleanDeletedNamespace handles deleting namespace from ipset. -func (nsc *nameSpaceController) cleanDeletedNamespace(nsObj *corev1.Namespace) error { +func (nsc *nameSpaceController) cleanDeletedNamespace(nsName string, nsLabel map[string]string) error { var err error - nsName, nsLabel := util.GetNSNameWithPrefix(nsObj.ObjectMeta.Name), nsObj.ObjectMeta.Labels log.Logf("NAMESPACE DELETING: [%s/%v]", nsName, nsLabel) cachedNsObj, exists := nsc.npMgr.NsMap[nsName] diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index c22738381c..816b6e20d1 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -363,12 +363,12 @@ func TestDeleteandUpdateNamespaceLabel(t *testing.T) { } checkNsTestResult("TestDeleteandUpdateNamespaceLabel", f, testCases) - if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; exists { + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; !exists { t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.nsMap check") } if !reflect.DeepEqual( - oldNsObj.Labels, + newNsObj.Labels, f.npMgr.NsMap[util.GetNSNameWithPrefix(oldNsObj.Name)].LabelsMap, ) { t.Fatalf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.nsMap labelMap check") @@ -402,10 +402,10 @@ func TestDeleteNamespace(t *testing.T) { func checkNsTestResult(testName string, f *nameSpaceFixture, testCases []expectedNsValues) { for _, test := range testCases { if got := len(f.npMgr.PodMap); got != test.expectedLenOfPodMap { - f.t.Errorf("PodMap length = %d, want %d", got, test.expectedLenOfPodMap) + f.t.Errorf("PodMap length = %d, want %d. Map: %+v", got, test.expectedLenOfPodMap, f.npMgr.PodMap) } if got := len(f.npMgr.NsMap); got != test.expectedLenOfNsMap { - f.t.Errorf("npMgr length = %d, want %d", got, test.expectedLenOfNsMap) + f.t.Errorf("npMgr length = %d, want %d. Map: %+v", got, test.expectedLenOfNsMap, f.npMgr.NsMap) } if got := f.nsController.workqueue.Len(); got != test.expectedLenOfWorkQueue { f.t.Errorf("Workqueue length = %d, want %d", got, test.expectedLenOfWorkQueue) From 39919f21f2b131c3260bbb4a71da3f1ba10d984a Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 30 Mar 2021 09:10:09 -0700 Subject: [PATCH 06/17] Adding in more testcases --- npm/nameSpaceController_test.go | 41 +++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index 816b6e20d1..ac77ee3fb0 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -399,6 +399,47 @@ func TestDeleteNamespace(t *testing.T) { } } +func TestGetNamespaceObjFromNsObj(t *testing.T) { + ns, _ := newNs("test-ns") + ns.LabelsMap = map[string]string{ + "test": "new", + } + + nsObj := getNamespaceObjFromNsObj(ns) + + if !reflect.DeepEqual(ns.LabelsMap, nsObj.ObjectMeta.Labels) { + t.Errorf("TestGetNamespaceObjFromNsObj failed @ nsObj labels check") + } +} + +func TestIsSystemNs(t *testing.T) { + nsObj := newNameSpace("kube-system", "0", map[string]string{"test": "new"}) + + if !isSystemNs(nsObj) { + t.Errorf("TestIsSystemNs failed @ nsObj isSystemNs check") + } +} + +func TestIsInvalidNamespaceUpdate(t *testing.T) { + oldNsObj := newNameSpace("test-ns", "0", map[string]string{"test": "new"}) + newNsObj := newNameSpace("test-ns", "0", map[string]string{"test": "new"}) + + if !isInvalidNamespaceUpdate(oldNsObj, newNsObj) { + t.Errorf("TestIsInvalidNamespaceUpdate failed @ nsObj isInvalidNamespaceUpdate check") + } + + newNsObj.Labels["update"] = "true" + if isInvalidNamespaceUpdate(oldNsObj, newNsObj) { + t.Errorf("TestIsInvalidNamespaceUpdate failed @ updated nsObj isInvalidNamespaceUpdate check") + } + + newNsObj.SetDeletionTimestamp(&metav1.Time{}) + if isInvalidNamespaceUpdate(oldNsObj, newNsObj) { + t.Errorf("TestIsInvalidNamespaceUpdate failed @ deletion time updated nsObj isInvalidNamespaceUpdate check") + } + +} + func checkNsTestResult(testName string, f *nameSpaceFixture, testCases []expectedNsValues) { for _, test := range testCases { if got := len(f.npMgr.PodMap); got != test.expectedLenOfPodMap { From 080314d3c1fe4598761bc6f3c977af0eae15f10d Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 30 Mar 2021 10:43:47 -0700 Subject: [PATCH 07/17] adding threadness to NS --- npm/npm.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/npm/npm.go b/npm/npm.go index 00f22f86b5..223eace1b1 100644 --- a/npm/npm.go +++ b/npm/npm.go @@ -36,6 +36,8 @@ const ( telemetryRetryTimeInSeconds = 60 heartbeatIntervalInMinutes = 30 reconcileChainTimeInMinutes = 5 + // TODO: consider increasing thread number later when logics are correct + threadness = 1 ) // NetworkPolicyManager contains informers for pod, namespace and networkpolicy. @@ -184,6 +186,7 @@ func (npMgr *NetworkPolicyManager) Start(stopCh <-chan struct{}) error { return fmt.Errorf("Network policy informer failed to sync") } + go npMgr.nameSpaceController.Run(threadness, stopCh) go npMgr.reconcileChains() go npMgr.backup() From d6d17b282c5890aad9afee9fb4c6d6b929a6a7b3 Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 30 Mar 2021 12:20:13 -0700 Subject: [PATCH 08/17] minor imporvements wrt RV --- npm/nameSpaceController.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index 2f68bf9341..05fc8a502c 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -164,6 +164,7 @@ func (nsc *nameSpaceController) updateNamespace(old, new interface{}) { nsKey := util.GetNSNameWithPrefix(key) nsc.npMgr.Lock() + defer nsc.npMgr.Unlock() cachedNsObj, nsExists := nsc.npMgr.NsMap[nsKey] if !nsExists { nsc.workqueue.Add(key) @@ -174,7 +175,6 @@ func (nsc *nameSpaceController) updateNamespace(old, new interface{}) { log.Logf("[NAMESPACE UPDATE EVENT] Namespace [%s] labels did not change", key) return } - nsc.npMgr.Unlock() nsc.workqueue.Add(key) } @@ -306,7 +306,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { err = nsc.cleanDeletedNamespace(cachedNs.name, cachedNs.LabelsMap) if err != nil { // cleaning process was failed, need to requeue and retry later. - return fmt.Errorf("cannot delete ipset due to %s\n", err.Error()) + return fmt.Errorf("cannot delete ipset due to %s", err.Error()) } } // for other transient apiserver error requeue with exponential backoff @@ -325,7 +325,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { err = nsc.syncUpdateNameSpace(nsObj) // 1. deal with error code and retry this if err != nil { - return fmt.Errorf("failed to sync namespace due to %s\n", err.Error()) + return fmt.Errorf("failed to sync namespace due to %s", err.Error()) } return nil @@ -374,7 +374,7 @@ func (nsc *nameSpaceController) syncAddNameSpace(nsObj *corev1.Namespace) error ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr // Create ipset for the namespace. - if err = ipsMgr.CreateSet(nsName, append([]string{util.IpsetNetHashFlag})); err != nil { + if err = ipsMgr.CreateSet(nsName, []string{util.IpsetNetHashFlag}); err != nil { metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to create ipset for namespace %s with err: %v", nsName, err) return err } @@ -430,16 +430,6 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) return nil } - newRv := util.ParseResourceVersion(newNsObj.ObjectMeta.ResourceVersion) - if !util.CompareUintResourceVersions(curNsObj.resourceVersion, newRv) { - log.Logf("Cached NameSpace has larger ResourceVersion number than new Obj. NameSpace: %s Cached RV: %d New RV:\n", - newNsNs, - curNsObj.resourceVersion, - newRv, - ) - return nil - } - //If the Namespace is not deleted, delete removed labels and create new labels addToIPSets, deleteFromIPSets := util.GetIPSetListCompareLabels(curNsObj.LabelsMap, newNsLabel) From bf83890c0abf0191c67e6b96709e89b2f957ae58 Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 30 Mar 2021 12:29:45 -0700 Subject: [PATCH 09/17] minor imporvements wrt RV --- npm/nameSpaceController_test.go | 44 +++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index ac77ee3fb0..4af251d78a 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -375,6 +375,50 @@ func TestDeleteandUpdateNamespaceLabel(t *testing.T) { } } +func TestNewNameSpaceUpdate(t *testing.T) { + f := newNsFixture(t) + f.ipSetSave(util.IpsetTestConfigFile) + defer f.ipSetRestore(util.IpsetTestConfigFile) + + oldNsObj := newNameSpace( + "test-namespace", + "10", + map[string]string{ + "app": "old-test-namespace", + "update": "true", + "group": "test", + }, + ) + oldNsObj.SetUID("test1") + + newNsObj := newNameSpace( + "test-namespace", + "9", + map[string]string{ + "app": "old-test-namespace", + "update": "false", + }, + ) + newNsObj.SetUID("test2") + updateNamespace(t, f, oldNsObj, newNsObj) + + testCases := []expectedNsValues{ + {0, 2, 0}, + } + checkNsTestResult("TestDeleteandUpdateNamespaceLabel", f, testCases) + + if _, exists := f.npMgr.NsMap[util.GetNSNameWithPrefix(newNsObj.Name)]; !exists { + t.Errorf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.nsMap check") + } + + if !reflect.DeepEqual( + newNsObj.Labels, + f.npMgr.NsMap[util.GetNSNameWithPrefix(oldNsObj.Name)].LabelsMap, + ) { + t.Fatalf("TestDeleteandUpdateNamespaceLabel failed @ npMgr.nsMap labelMap check") + } +} + func TestDeleteNamespace(t *testing.T) { f := newNsFixture(t) f.ipSetSave(util.IpsetTestConfigFile) From d8f029de486d50026f534011e36e8ffac077d29a Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 30 Mar 2021 12:31:06 -0700 Subject: [PATCH 10/17] minor imporvements wrt RV --- npm/nameSpaceController_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index 4af251d78a..a75d4b9615 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -375,6 +375,9 @@ func TestDeleteandUpdateNamespaceLabel(t *testing.T) { } } +// TestNewNameSpaceUpdate will test the case where the key is same but the object is different. +// this happens when NSA delete event is missed and deleted from NPMLocalCache, +// but NSA gets added again. This will result in an update event with old and new with different UUIDs func TestNewNameSpaceUpdate(t *testing.T) { f := newNsFixture(t) f.ipSetSave(util.IpsetTestConfigFile) From 0af4f4c534d956436c97931e6b12fc216e58f839 Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 30 Mar 2021 12:58:27 -0700 Subject: [PATCH 11/17] Cleaning up some stale code --- npm/nameSpaceController.go | 20 ++------------------ npm/nameSpaceController_test.go | 20 -------------------- 2 files changed, 2 insertions(+), 38 deletions(-) diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index 05fc8a502c..0ef44284b2 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -68,15 +68,6 @@ func isSystemNs(nsObj *corev1.Namespace) bool { return nsObj.ObjectMeta.Name == util.KubeSystemFlag } -func isInvalidNamespaceUpdate(oldNsObj, newNsObj *corev1.Namespace) (isInvalidUpdate bool) { - isInvalidUpdate = oldNsObj.ObjectMeta.Name == newNsObj.ObjectMeta.Name && - newNsObj.ObjectMeta.DeletionTimestamp == nil && - newNsObj.ObjectMeta.DeletionGracePeriodSeconds == nil - isInvalidUpdate = isInvalidUpdate && reflect.DeepEqual(oldNsObj.ObjectMeta.Labels, newNsObj.ObjectMeta.Labels) - - return -} - type nameSpaceController struct { clientset kubernetes.Interface nameSpaceLister corelisters.NamespaceLister @@ -476,21 +467,14 @@ func (nsc *nameSpaceController) cleanDeletedNamespace(nsName string, nsLabel map log.Logf("NAMESPACE DELETING cached labels: [%s/%v]", nsName, cachedNsObj.LabelsMap) // Delete the namespace from its label's ipset list. ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr - nsLabels := cachedNsObj.LabelsMap - for nsLabelKey, nsLabelVal := range nsLabels { + nsLabels := util.GetIPSetListFromLabels(cachedNsObj.LabelsMap) + for _, nsLabelKey := range nsLabels { labelKey := util.GetNSNameWithPrefix(nsLabelKey) log.Logf("Deleting namespace %s from ipset list %s", nsName, labelKey) if err = ipsMgr.DeleteFromList(labelKey, nsName); err != nil { metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, labelKey, err) return err } - - label := util.GetNSNameWithPrefix(nsLabelKey + ":" + nsLabelVal) - log.Logf("Deleting namespace %s from ipset list %s", nsName, label) - if err = ipsMgr.DeleteFromList(label, nsName); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[DeleteNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", nsName, label, err) - return err - } } // Delete the namespace from all-namespace ipset list. diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index a75d4b9615..fe959da768 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -467,26 +467,6 @@ func TestIsSystemNs(t *testing.T) { } } -func TestIsInvalidNamespaceUpdate(t *testing.T) { - oldNsObj := newNameSpace("test-ns", "0", map[string]string{"test": "new"}) - newNsObj := newNameSpace("test-ns", "0", map[string]string{"test": "new"}) - - if !isInvalidNamespaceUpdate(oldNsObj, newNsObj) { - t.Errorf("TestIsInvalidNamespaceUpdate failed @ nsObj isInvalidNamespaceUpdate check") - } - - newNsObj.Labels["update"] = "true" - if isInvalidNamespaceUpdate(oldNsObj, newNsObj) { - t.Errorf("TestIsInvalidNamespaceUpdate failed @ updated nsObj isInvalidNamespaceUpdate check") - } - - newNsObj.SetDeletionTimestamp(&metav1.Time{}) - if isInvalidNamespaceUpdate(oldNsObj, newNsObj) { - t.Errorf("TestIsInvalidNamespaceUpdate failed @ deletion time updated nsObj isInvalidNamespaceUpdate check") - } - -} - func checkNsTestResult(testName string, f *nameSpaceFixture, testCases []expectedNsValues) { for _, test := range testCases { if got := len(f.npMgr.PodMap); got != test.expectedLenOfPodMap { From 772b0f5fad9e061fd3e3250f69e43ac192874f5c Mon Sep 17 00:00:00 2001 From: vakr Date: Tue, 30 Mar 2021 17:32:54 -0700 Subject: [PATCH 12/17] resolving comments --- npm/nameSpaceController.go | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index 0ef44284b2..c43ad8820b 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -138,13 +138,6 @@ func (nsc *nameSpaceController) updateNamespace(old, new interface{}) { } nsObj, _ := new.(*corev1.Namespace) - - if nsObj.ObjectMeta.DeletionTimestamp != nil || - nsObj.ObjectMeta.DeletionGracePeriodSeconds != nil { - nsc.deleteNamespace(new) - return - } - oldNsObj, ok := old.(*corev1.Namespace) if ok { if oldNsObj.ResourceVersion == nsObj.ResourceVersion { @@ -261,7 +254,8 @@ func (nsc *nameSpaceController) processNextWorkItem() bool { if err := nsc.syncNameSpace(key); err != nil { // Put the item back on the workqueue to handle any transient errors. nsc.workqueue.AddRateLimited(key) - return fmt.Errorf("error syncing '%s': %s, requeuing", key, err.Error()) + metrics.SendErrorLogAndMetric(util.NSID, "[processNextWorkItem] Error: failed to syncNameSpace %s. Requeuing with err: %v", key, err) + return err } // Finally, if no error occurs we Forget this item so it does not // get queued again until another change happens. @@ -292,15 +286,18 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { // find the namespace object from a local cache and start cleaning up process (calling cleanUpDeletedPod function) nsKey := util.GetNSNameWithPrefix(key) cachedNs, found := nsc.npMgr.NsMap[nsKey] - if found { - // TODO: better to use cachedPod when calling cleanUpDeletedPod? - err = nsc.cleanDeletedNamespace(cachedNs.name, cachedNs.LabelsMap) - if err != nil { - // cleaning process was failed, need to requeue and retry later. - return fmt.Errorf("cannot delete ipset due to %s", err.Error()) - } + // if the namespace does not exists, we do not need to clean up process and retry it + if !found { + return nil + } + + // Found the namespace object from NsMap local cache and start cleaning up processes + err = nsc.cleanDeletedNamespace(cachedNs.name, cachedNs.LabelsMap) + if err != nil { + // need to retry this cleaning-up process + metrics.SendErrorLogAndMetric(util.NSID, "Error: %v when namespace is not found", err) + return fmt.Errorf("Error: %v when namespace is not found\n", err) } - // for other transient apiserver error requeue with exponential backoff return err } } From ee14e851908af610b51a5baf0b4b1c784c7f401e Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 31 Mar 2021 14:37:16 -0700 Subject: [PATCH 13/17] Addressing some comments --- npm/nameSpaceController.go | 59 ++++++++++++++------------------- npm/nameSpaceController_test.go | 50 ++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 45 deletions(-) diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index c43ad8820b..4fc2035042 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -50,7 +50,7 @@ func newNs(name string) (*Namespace, error) { return ns, nil } -func getNamespaceObjFromNsObj(nsObj *Namespace) *corev1.Namespace { +func (nsObj *Namespace) getNamespaceObjFromNsObj() *corev1.Namespace { return &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ Name: nsObj.name, @@ -60,7 +60,7 @@ func getNamespaceObjFromNsObj(nsObj *Namespace) *corev1.Namespace { } // setResourceVersion setter func for RV -func setResourceVersion(nsObj *Namespace, rv string) { +func (nsObj *Namespace) setResourceVersion(rv string) { nsObj.resourceVersion = util.ParseResourceVersion(rv) } @@ -103,15 +103,14 @@ func (nsc *nameSpaceController) needSync(obj interface{}, event string) (string, nsObj, ok := obj.(*corev1.Namespace) if !ok { - metrics.SendErrorLogAndMetric(util.NSID, "%s Pod: Received unexpected object type: %v", event, obj) + metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE %s EVENT] Received unexpected object type: %v", event, obj) return key, needSync } var err error if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil { utilruntime.HandleError(err) - err = fmt.Errorf("[%sNameSpace] Error: nameSpaceKey is empty for %s namespace", event, util.GetNSNameWithPrefix(nsObj.Name)) - metrics.SendErrorLogAndMetric(util.NSID, err.Error()) + metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE %s EVENT] Error: NameSpaceKey is empty for %s namespace", event, util.GetNSNameWithPrefix(nsObj.Name)) return key, needSync } @@ -147,17 +146,15 @@ func (nsc *nameSpaceController) updateNamespace(old, new interface{}) { } nsKey := util.GetNSNameWithPrefix(key) + nsc.npMgr.Lock() defer nsc.npMgr.Unlock() cachedNsObj, nsExists := nsc.npMgr.NsMap[nsKey] - if !nsExists { - nsc.workqueue.Add(key) - return - } - - if reflect.DeepEqual(cachedNsObj.LabelsMap, nsObj.ObjectMeta.Labels) { - log.Logf("[NAMESPACE UPDATE EVENT] Namespace [%s] labels did not change", key) - return + if nsExists { + if reflect.DeepEqual(cachedNsObj.LabelsMap, nsObj.ObjectMeta.Labels) { + log.Logf("[NAMESPACE UPDATE EVENT] Namespace [%s] labels did not change", key) + return + } } nsc.workqueue.Add(key) @@ -186,8 +183,7 @@ func (nsc *nameSpaceController) deleteNamespace(obj interface{}) { var key string if key, err = cache.MetaNamespaceKeyFunc(obj); err != nil { utilruntime.HandleError(err) - err = fmt.Errorf("[NAMESPACE DELETE EVENT] Error: nameSpaceKey is empty for %s namespace", util.GetNSNameWithPrefix(nsObj.Name)) - metrics.SendErrorLogAndMetric(util.NSID, err.Error()) + metrics.SendErrorLogAndMetric(util.NSID, "[NAMESPACE DELETE EVENT] Error: nameSpaceKey is empty for %s namespace", util.GetNSNameWithPrefix(nsObj.Name)) return } @@ -197,7 +193,7 @@ func (nsc *nameSpaceController) deleteNamespace(obj interface{}) { nsKey := util.GetNSNameWithPrefix(key) _, nsExists := nsc.npMgr.NsMap[nsKey] if !nsExists { - log.Logf("[NAMESPACE Delete EVENT] Namespace [%s] does not exist in case, so returning", key) + log.Logf("[NAMESPACE DELETE EVENT] Namespace [%s] does not exist in case, so returning", key) return } @@ -208,11 +204,9 @@ func (nsc *nameSpaceController) Run(threadiness int, stopCh <-chan struct{}) err defer utilruntime.HandleCrash() defer nsc.workqueue.ShutDown() - // Start the informer factories to begin populating the informer caches log.Logf("Starting Namespace controller\n") - log.Logf("Starting workers") - // Launch two workers to process namespace resources + // Launch workers to process namespace resources for i := 0; i < threadiness; i++ { go wait.Until(nsc.runWorker, time.Second, stopCh) } @@ -248,7 +242,7 @@ func (nsc *nameSpaceController) processNextWorkItem() bool { utilruntime.HandleError(fmt.Errorf("expected string in workqueue but got %#v", obj)) return nil } - // Run the syncHandler, passing it the namespace string of the + // Run the syncNameSpace, passing it the namespace string of the // resource to be synced. // TODO : may consider using "c.queue.AddAfter(key, *requeueAfter)" according to error type later if err := nsc.syncNameSpace(key); err != nil { @@ -283,7 +277,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { if err != nil { if errors.IsNotFound(err) { utilruntime.HandleError(fmt.Errorf("NameSpace '%s' in work queue no longer exists", key)) - // find the namespace object from a local cache and start cleaning up process (calling cleanUpDeletedPod function) + // find the namespace object from a local cache and start cleaning up process (calling cleanDeletedNamespace function) nsKey := util.GetNSNameWithPrefix(key) cachedNs, found := nsc.npMgr.NsMap[nsKey] // if the namespace does not exists, we do not need to clean up process and retry it @@ -303,17 +297,15 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { } if nsObj.DeletionTimestamp != nil || nsObj.DeletionGracePeriodSeconds != nil { - return nsc.cleanDeletedNamespace( - util.GetNSNameWithPrefix(nsObj.Name), - nsObj.Labels, - ) + return nsc.cleanDeletedNamespace(util.GetNSNameWithPrefix(nsObj.Name), nsObj.Labels) } err = nsc.syncUpdateNameSpace(nsObj) // 1. deal with error code and retry this if err != nil { - return fmt.Errorf("failed to sync namespace due to %s", err.Error()) + metrics.SendErrorLogAndMetric(util.NSID, "[syncNameSpace] failed to sync namespace due to %s", err.Error()) + return err } return nil @@ -383,11 +375,8 @@ func (nsc *nameSpaceController) syncAddNameSpace(nsObj *corev1.Namespace) error } } - ns, err := newNs(nsName) - if err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[AddNamespace] Error: failed to create namespace %s with err: %v", nsName, err) - } - setResourceVersion(ns, nsObj.GetObjectMeta().GetResourceVersion()) + ns, _ := newNs(nsName) + ns.setResourceVersion(nsObj.GetObjectMeta().GetResourceVersion()) // Append all labels to the cache NS obj ns.LabelsMap = util.AppendMap(ns.LabelsMap, nsLabel) @@ -444,7 +433,7 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) // Append all labels to the cache NS obj curNsObj.LabelsMap = util.ClearAndAppendMap(curNsObj.LabelsMap, newNsLabel) - setResourceVersion(curNsObj, newNsObj.GetObjectMeta().GetResourceVersion()) + curNsObj.setResourceVersion(newNsObj.GetObjectMeta().GetResourceVersion()) nsc.npMgr.NsMap[newNsNs] = curNsObj return nil @@ -452,8 +441,6 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) // cleanDeletedNamespace handles deleting namespace from ipset. func (nsc *nameSpaceController) cleanDeletedNamespace(nsName string, nsLabel map[string]string) error { - var err error - log.Logf("NAMESPACE DELETING: [%s/%v]", nsName, nsLabel) cachedNsObj, exists := nsc.npMgr.NsMap[nsName] @@ -462,9 +449,11 @@ func (nsc *nameSpaceController) cleanDeletedNamespace(nsName string, nsLabel map } log.Logf("NAMESPACE DELETING cached labels: [%s/%v]", nsName, cachedNsObj.LabelsMap) - // Delete the namespace from its label's ipset list. + + var err error ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr nsLabels := util.GetIPSetListFromLabels(cachedNsObj.LabelsMap) + // Delete the namespace from its label's ipset list. for _, nsLabelKey := range nsLabels { labelKey := util.GetNSNameWithPrefix(nsLabelKey) log.Logf("Deleting namespace %s from ipset list %s", nsName, labelKey) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index fe959da768..066ba6d5ee 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -59,7 +59,7 @@ func newNsFixture(t *testing.T) *nameSpaceFixture { return f } -func (f *nameSpaceFixture) newNsController() { +func (f *nameSpaceFixture) newNsController(stopCh chan struct{}) { f.kubeclient = k8sfake.NewSimpleClientset(f.kubeobjects...) f.kubeInformer = kubeinformers.NewSharedInformerFactory(f.kubeclient, noResyncPeriodFunc()) @@ -69,6 +69,8 @@ func (f *nameSpaceFixture) newNsController() { for _, ns := range f.nsLister { f.kubeInformer.Core().V1().Namespaces().Informer().GetIndexer().Add(ns) } + + f.kubeInformer.Start(stopCh) } func (f *nameSpaceFixture) ipSetSave(ipsetConfigFile string) { @@ -112,14 +114,6 @@ func newNameSpace(name, rv string, labels map[string]string) *corev1.Namespace { } func addNamespace(t *testing.T, f *nameSpaceFixture, nsObj *corev1.Namespace) { - f.nsLister = append(f.nsLister, nsObj) - f.kubeobjects = append(f.kubeobjects, nsObj) - - f.newNsController() - stopCh := make(chan struct{}) - defer close(stopCh) - f.kubeInformer.Start(stopCh) - t.Logf("Calling add namespace event") f.nsController.addNamespace(nsObj) if f.nsController.workqueue.Len() == 0 { @@ -202,6 +196,12 @@ func TestAddNamespace(t *testing.T) { "app": "test-namespace", }, ) + f.nsLister = append(f.nsLister, nsObj) + f.kubeobjects = append(f.kubeobjects, nsObj) + + stopCh := make(chan struct{}) + defer close(stopCh) + f.newNsController(stopCh) addNamespace(t, f, nsObj) @@ -235,6 +235,11 @@ func TestUpdateNamespace(t *testing.T) { "app": "new-test-namespace", }, ) + f.nsLister = append(f.nsLister, oldNsObj) + f.kubeobjects = append(f.kubeobjects, oldNsObj) + + stopCh := make(chan struct{}) + defer close(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -274,6 +279,11 @@ func TestAddNamespaceLabel(t *testing.T) { "update": "true", }, ) + f.nsLister = append(f.nsLister, oldNsObj) + f.kubeobjects = append(f.kubeobjects, oldNsObj) + + stopCh := make(chan struct{}) + defer close(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -314,6 +324,11 @@ func TestAddNamespaceLabelSameRv(t *testing.T) { "update": "true", }, ) + f.nsLister = append(f.nsLister, oldNsObj) + f.kubeobjects = append(f.kubeobjects, oldNsObj) + + stopCh := make(chan struct{}) + defer close(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -356,6 +371,11 @@ func TestDeleteandUpdateNamespaceLabel(t *testing.T) { "update": "false", }, ) + f.nsLister = append(f.nsLister, oldNsObj) + f.kubeobjects = append(f.kubeobjects, oldNsObj) + + stopCh := make(chan struct{}) + defer close(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -402,6 +422,11 @@ func TestNewNameSpaceUpdate(t *testing.T) { "update": "false", }, ) + f.nsLister = append(f.nsLister, oldNsObj) + f.kubeobjects = append(f.kubeobjects, oldNsObj) + + stopCh := make(chan struct{}) + defer close(stopCh) newNsObj.SetUID("test2") updateNamespace(t, f, oldNsObj, newNsObj) @@ -434,6 +459,11 @@ func TestDeleteNamespace(t *testing.T) { "app": "test-namespace", }, ) + f.nsLister = append(f.nsLister, nsObj) + f.kubeobjects = append(f.kubeobjects, nsObj) + + stopCh := make(chan struct{}) + defer close(stopCh) deleteNamespace(t, f, nsObj) testCases := []expectedNsValues{ @@ -452,7 +482,7 @@ func TestGetNamespaceObjFromNsObj(t *testing.T) { "test": "new", } - nsObj := getNamespaceObjFromNsObj(ns) + nsObj := ns.getNamespaceObjFromNsObj() if !reflect.DeepEqual(ns.LabelsMap, nsObj.ObjectMeta.Labels) { t.Errorf("TestGetNamespaceObjFromNsObj failed @ nsObj labels check") From aac8429e207f982bc7b1706c5a898156788f0ba0 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 31 Mar 2021 14:38:06 -0700 Subject: [PATCH 14/17] Addressing some comments --- npm/nameSpaceController_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index 066ba6d5ee..16994c5452 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -10,7 +10,6 @@ import ( "github.com/Azure/azure-container-networking/npm/ipsm" "github.com/Azure/azure-container-networking/npm/util" corev1 "k8s.io/api/core/v1" - networkingv1 "k8s.io/api/networking/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -92,8 +91,6 @@ func newNPMgr(t *testing.T) *NetworkPolicyManager { npMgr := &NetworkPolicyManager{ NsMap: make(map[string]*Namespace), PodMap: make(map[string]*NpmPod), - RawNpMap: make(map[string]*networkingv1.NetworkPolicy), - ProcessedNpMap: make(map[string]*networkingv1.NetworkPolicy), TelemetryEnabled: false, } From 626231fe901c4d8cda5abcdef9b768e8aa717779 Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 31 Mar 2021 15:19:31 -0700 Subject: [PATCH 15/17] Adding missing newcontroller --- npm/nameSpaceController_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index 16994c5452..e972d243bf 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -237,6 +237,7 @@ func TestUpdateNamespace(t *testing.T) { stopCh := make(chan struct{}) defer close(stopCh) + f.newNsController(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -281,6 +282,7 @@ func TestAddNamespaceLabel(t *testing.T) { stopCh := make(chan struct{}) defer close(stopCh) + f.newNsController(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -326,6 +328,7 @@ func TestAddNamespaceLabelSameRv(t *testing.T) { stopCh := make(chan struct{}) defer close(stopCh) + f.newNsController(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -373,6 +376,7 @@ func TestDeleteandUpdateNamespaceLabel(t *testing.T) { stopCh := make(chan struct{}) defer close(stopCh) + f.newNsController(stopCh) updateNamespace(t, f, oldNsObj, newNsObj) testCases := []expectedNsValues{ @@ -424,6 +428,7 @@ func TestNewNameSpaceUpdate(t *testing.T) { stopCh := make(chan struct{}) defer close(stopCh) + f.newNsController(stopCh) newNsObj.SetUID("test2") updateNamespace(t, f, oldNsObj, newNsObj) @@ -461,6 +466,7 @@ func TestDeleteNamespace(t *testing.T) { stopCh := make(chan struct{}) defer close(stopCh) + f.newNsController(stopCh) deleteNamespace(t, f, nsObj) testCases := []expectedNsValues{ From 41a1db0a5416e6e856557f13e6962e45634e808e Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 31 Mar 2021 15:32:13 -0700 Subject: [PATCH 16/17] Addressing more comments --- npm/nameSpaceController.go | 56 +++++++-------------------------- npm/nameSpaceController_test.go | 23 -------------- 2 files changed, 11 insertions(+), 68 deletions(-) diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index 4fc2035042..49b79f500a 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -290,7 +290,7 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { if err != nil { // need to retry this cleaning-up process metrics.SendErrorLogAndMetric(util.NSID, "Error: %v when namespace is not found", err) - return fmt.Errorf("Error: %v when namespace is not found\n", err) + return err } return err } @@ -311,40 +311,6 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { return nil } -// InitAllNsList syncs all-namespace ipset list. -func (npMgr *NetworkPolicyManager) InitAllNsList() error { - allNs := npMgr.NsMap[util.KubeAllNamespacesFlag] - for ns := range npMgr.NsMap { - if ns == util.KubeAllNamespacesFlag { - continue - } - - if err := allNs.IpsMgr.AddToList(util.KubeAllNamespacesFlag, ns); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[InitAllNsList] Error: failed to add namespace set %s to ipset list %s with err: %v", ns, util.KubeAllNamespacesFlag, err) - return err - } - } - - return nil -} - -// UninitAllNsList cleans all-namespace ipset list. -func (npMgr *NetworkPolicyManager) UninitAllNsList() error { - allNs := npMgr.NsMap[util.KubeAllNamespacesFlag] - for ns := range npMgr.NsMap { - if ns == util.KubeAllNamespacesFlag { - continue - } - - if err := allNs.IpsMgr.DeleteFromList(util.KubeAllNamespacesFlag, ns); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[UninitAllNsList] Error: failed to delete namespace set %s from list %s with err: %v", ns, util.KubeAllNamespacesFlag, err) - return err - } - } - - return nil -} - // syncAddNameSpace handles adding namespace to ipset. func (nsc *nameSpaceController) syncAddNameSpace(nsObj *corev1.Namespace) error { var err error @@ -388,15 +354,15 @@ func (nsc *nameSpaceController) syncAddNameSpace(nsObj *corev1.Namespace) error // syncUpdateNameSpace handles updating namespace in ipset. func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) error { var err error - newNsNs, newNsLabel := util.GetNSNameWithPrefix(newNsObj.ObjectMeta.Name), newNsObj.ObjectMeta.Labels + newNsName, newNsLabel := util.GetNSNameWithPrefix(newNsObj.ObjectMeta.Name), newNsObj.ObjectMeta.Labels log.Logf( "NAMESPACE UPDATING:\n namespace: [%s/%v]", - newNsNs, newNsLabel, + newNsName, newNsLabel, ) // If orignal AddNamespace failed for some reason, then NS will not be found // in nsMap, resulting in retry of ADD. - curNsObj, exists := nsc.npMgr.NsMap[newNsNs] + curNsObj, exists := nsc.npMgr.NsMap[newNsName] if !exists { if newNsObj.ObjectMeta.DeletionTimestamp == nil && newNsObj.ObjectMeta.DeletionGracePeriodSeconds == nil { if err = nsc.syncAddNameSpace(newNsObj); err != nil { @@ -414,9 +380,9 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) ipsMgr := nsc.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr for _, nsLabelVal := range deleteFromIPSets { labelKey := util.GetNSNameWithPrefix(nsLabelVal) - log.Logf("Deleting namespace %s from ipset list %s", newNsNs, labelKey) - if err = ipsMgr.DeleteFromList(labelKey, newNsNs); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", newNsNs, labelKey, err) + log.Logf("Deleting namespace %s from ipset list %s", newNsName, labelKey) + if err = ipsMgr.DeleteFromList(labelKey, newNsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to delete namespace %s from ipset list %s with err: %v", newNsName, labelKey, err) return err } } @@ -424,9 +390,9 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) // Add the namespace to its label's ipset list. for _, nsLabelVal := range addToIPSets { labelKey := util.GetNSNameWithPrefix(nsLabelVal) - log.Logf("Adding namespace %s to ipset list %s", newNsNs, labelKey) - if err = ipsMgr.AddToList(labelKey, newNsNs); err != nil { - metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", newNsNs, labelKey, err) + log.Logf("Adding namespace %s to ipset list %s", newNsName, labelKey) + if err = ipsMgr.AddToList(labelKey, newNsName); err != nil { + metrics.SendErrorLogAndMetric(util.NSID, "[UpdateNamespace] Error: failed to add namespace %s to ipset list %s with err: %v", newNsName, labelKey, err) return err } } @@ -434,7 +400,7 @@ func (nsc *nameSpaceController) syncUpdateNameSpace(newNsObj *corev1.Namespace) // Append all labels to the cache NS obj curNsObj.LabelsMap = util.ClearAndAppendMap(curNsObj.LabelsMap, newNsLabel) curNsObj.setResourceVersion(newNsObj.GetObjectMeta().GetResourceVersion()) - nsc.npMgr.NsMap[newNsNs] = curNsObj + nsc.npMgr.NsMap[newNsName] = curNsObj return nil } diff --git a/npm/nameSpaceController_test.go b/npm/nameSpaceController_test.go index e972d243bf..667d834b9d 100644 --- a/npm/nameSpaceController_test.go +++ b/npm/nameSpaceController_test.go @@ -158,29 +158,6 @@ func TestNewNs(t *testing.T) { } } -func TestAllNsList(t *testing.T) { - npMgr := &NetworkPolicyManager{} - - ipsMgr := ipsm.NewIpsetManager() - if err := ipsMgr.Save(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAllNsList failed @ ipsMgr.Save") - } - - defer func() { - if err := ipsMgr.Restore(util.IpsetTestConfigFile); err != nil { - t.Errorf("TestAllNsList failed @ ipsMgr.Restore") - } - }() - - if err := npMgr.InitAllNsList(); err != nil { - t.Errorf("TestAllNsList failed @ InitAllNsList") - } - - if err := npMgr.UninitAllNsList(); err != nil { - t.Errorf("TestAllNsList failed @ UninitAllNsList") - } -} - func TestAddNamespace(t *testing.T) { f := newNsFixture(t) f.ipSetSave(util.IpsetTestConfigFile) From 87241d24210f0d7e588443937f9a11e7db01321a Mon Sep 17 00:00:00 2001 From: vakr Date: Wed, 31 Mar 2021 15:45:42 -0700 Subject: [PATCH 17/17] Addressing more comments --- npm/nameSpaceController.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/npm/nameSpaceController.go b/npm/nameSpaceController.go index 49b79f500a..d02d0a1b96 100644 --- a/npm/nameSpaceController.go +++ b/npm/nameSpaceController.go @@ -290,10 +290,10 @@ func (nsc *nameSpaceController) syncNameSpace(key string) error { if err != nil { // need to retry this cleaning-up process metrics.SendErrorLogAndMetric(util.NSID, "Error: %v when namespace is not found", err) - return err + return fmt.Errorf("Error: %v when namespace is not found", err) } - return err } + return err } if nsObj.DeletionTimestamp != nil || nsObj.DeletionGracePeriodSeconds != nil {