[release-1.27] Handle istio-cni cleanup on node restart (#57880)

* Handle istio-cni on node cleanup

Currently on cleanup if safe upgrades are enable we check if the
cni daemonset has a deletion time stamp. If it didn't have a
stamp then we are in the process of upgrade or rebooting the node.
Otherwise we should cleanup. This didn't handle failures on the
get request for the DS (other than not found) which could indicate
the node is in an unhealthy state / restarting. Previously an err
would mean we would cleanup. Now we will retry the get, and assume
we shouldn't cleanup by default.

Signed-off-by: Jackie Elliott <[email protected]>

* fix lint

Signed-off-by: Jackie Elliott <[email protected]>

* Add release note

Signed-off-by: Jackie Elliott <[email protected]>

* Cleanup root

Signed-off-by: Jackie Elliott <[email protected]>

* Refactor StopCleanup to only default to true when using istio owned cni config. Also, check for cni pod in plugin prior to getting k8s client.

Signed-off-by: Jackie Elliott <[email protected]>

* Handle unauthorized get error on cleanup

Signed-off-by: Jackie Elliott <[email protected]>

* Fix releasenotes and string format

Signed-off-by: Jackie Elliott <[email protected]>

* Fix nits

Signed-off-by: Jackie Elliott <[email protected]>

---------

Signed-off-by: Jackie Elliott <[email protected]>
Co-authored-by: Jackie Elliott <[email protected]>

Author: istio-testing

cni/pkg/cmd/root.go

@@ -132,25 +132,27 @@ var rootCmd = &cobra.Command{
 			// if it is, we do NOT remove the plugin, and do
 			// NOT do ambient watch server cleanup
 			defer func() {
-				var isUpgrade bool
+				var shouldStopCleanup bool
 				if cfg.InstallConfig.AmbientDisableSafeUpgrade {
 					log.Info("Ambient node agent safe upgrade explicitly disabled via env")
-					isUpgrade = false
+					shouldStopCleanup = false
 				} else {
-					isUpgrade = ambientAgent.ShouldStopForUpgrade("istio-cni", nodeagent.PodNamespace)
+					shouldStopCleanup = ambientAgent.ShouldStopCleanup("istio-cni", nodeagent.PodNamespace, cfg.InstallConfig.IstioOwnedCNIConfig)
 				}
-				log.Infof("Ambient node agent shutting down - is upgrade shutdown? %t", isUpgrade)
+				log.Infof("Ambient node agent shutting down - should stop cleanup? %t", shouldStopCleanup)
+
+				// TODO(jaellio) - do we want to add support for a partial cleanup
 				// if we are doing an "upgrade shutdown", then
 				// we do NOT want to remove/cleanup the CNI plugin.
 				//
 				// This is important - we want it to remain in place to "stall"
 				// new ambient-enabled pods while our replacement spins up.
-				if !isUpgrade {
+				if !shouldStopCleanup {
 					if cleanErr := installer.Cleanup(); cleanErr != nil {
 						log.Error(cleanErr.Error())
 					}
 				}
-				ambientAgent.Stop(isUpgrade)
+				ambientAgent.Stop(shouldStopCleanup)
 			}()
 
 			ambientAgent.Start()

cni/pkg/nodeagent/server.go

@@ -21,7 +21,9 @@ import (
 	"sync/atomic"
 	"time"
 
+	"github.com/cenkalti/backoff/v4"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/rest"
 
@@ -31,7 +33,10 @@ import (
 
 const defaultZTunnelKeepAliveCheckInterval = 5 * time.Second
 
-var log = scopes.CNIAgent
+var (
+	log              = scopes.CNIAgent
+	tokenWaitBackoff = time.Second
+)
 
 type MeshDataplane interface {
 	// MUST be called first, (even before Start()).
@@ -119,18 +124,47 @@ func (s *Server) Stop(skipCleanup bool) {
 	s.dataplane.Stop(skipCleanup)
 }
 
-func (s *Server) ShouldStopForUpgrade(selfName, selfNamespace string) bool {
+// ShouldStopCleanup of istio-cni config and binary when upgrading or on node reboot
+func (s *Server) ShouldStopCleanup(selfName, selfNamespace string, istioOwnedCNIConfig bool) bool {
 	dsName := fmt.Sprintf("%s-node", selfName)
-	cniDS, err := s.kubeClient.Kube().AppsV1().DaemonSets(selfNamespace).Get(context.Background(), dsName, metav1.GetOptions{})
-	log.Debugf("Daemonset %s has deletion timestamp?: %+v", dsName, cniDS.DeletionTimestamp)
-	if err == nil && cniDS != nil && cniDS.DeletionTimestamp == nil {
-		log.Infof("terminating, but parent DS %s is still present, this is an upgrade, leaving plugin in place", dsName)
-		return true
+	shouldStopCleanup := false
+	var numRetries uint64
+	// use different defaults when using an istio owned CNI config file
+	if istioOwnedCNIConfig {
+		shouldStopCleanup = true
+		numRetries = 2
 	}
-
-	// If the DS is gone, it's definitely not an upgrade, so carry on like normal.
-	log.Infof("parent DS %s is gone or marked for deletion, this is not an upgrade, shutting down normally %s", dsName, err)
-	return false
+	err := backoff.Retry(
+		func() error {
+			cniDS, err := s.kubeClient.Kube().AppsV1().DaemonSets(selfNamespace).Get(context.Background(), dsName, metav1.GetOptions{})
+
+			if err == nil && cniDS != nil && cniDS.DeletionTimestamp == nil {
+				log.Infof("terminating, but parent DaemonSet %s is still present, this is an upgrade or a node reboot, leaving plugin in place", dsName)
+				shouldStopCleanup = true
+				return nil
+			}
+			if errors.IsNotFound(err) || (cniDS != nil && cniDS.DeletionTimestamp != nil) {
+				// If the DS is gone, or marked for deletion, this is not an upgrade.
+				// We can safely shut down the plugin.
+				log.Infof("parent DaemonSet %s is not found or marked for deletion, this is not an upgrade, shutting down normally", dsName)
+				shouldStopCleanup = false
+				return nil
+			}
+			if errors.IsUnauthorized(err) {
+				log.Infof("permission to get parent DaemonSet %s has been revoked manually or due to uninstall, this is not an upgrade, "+
+					"shutting down normally", dsName)
+				shouldStopCleanup = false
+				return nil
+			}
+			log.Infof("failed to get parent DS %s, retrying: %v", dsName, err)
+			return err
+		},
+		// Limiting retries to 3 so other shutdown tasks can complete before the graceful shutdown period ends
+		backoff.WithMaxRetries(backoff.NewConstantBackOff(tokenWaitBackoff), numRetries))
+	if err != nil {
+		log.Infof("failed to get parent DaemonSet %s, returning %t: %v", dsName, shouldStopCleanup, err)
+	}
+	return shouldStopCleanup
 }
 
 // buildKubeClient creates the kube client

cni/pkg/plugin/plugin.go

@@ -164,23 +164,26 @@ func CmdAdd(args *skel.CmdArgs) (err error) {
 		return err
 	}
 
+	// Preemptively check if the pod is a CNI pod.
+	// It is possible that the kubeconfig is not available if it hasn't been written yet
+	// by the CNI pod or it is invalid which cause the CNI pod to be unable to start if
+	// on the creation of a new K8s client. We preemptively check if the pod is a CNI pod
+	// to avoid a deadlock on the kubeconfig when the k8s client is unnecessary to process
+	// the CNI add event for the CNI pod itself.
+	if conf.AmbientEnabled {
+		k8sArgs := K8sArgs{}
+		if err := types.LoadArgs(args.Args, &k8sArgs); err != nil {
+			return fmt.Errorf("failed to load args: %v", err)
+		}
+		if isCNIPod(conf, &k8sArgs) {
+			// If we are in a degraded state and this is our own agent pod, skip
+			return pluginResponse(conf)
+		}
+	}
+
 	// Create a kube client
 	client, err := newK8sClient(*conf)
-	// If creation of a kube client fails, check if the pod is a CNI pod.
-	// It is possible that the kubeconfig is not available if it hasn't been written yet
-	// by the CNI pod which could be unable to start if we failed here. We skip this
-	// failure for the CNI pod to avoid a deadlock.
 	if err != nil {
-		if conf.AmbientEnabled {
-			k8sArgs := K8sArgs{}
-			if err := types.LoadArgs(args.Args, &k8sArgs); err != nil {
-				return fmt.Errorf("failed to load args after failed attempt to get client: %v", err)
-			}
-			if isCNIPod(conf, &k8sArgs) {
-				// If we are in a degraded state and this is our own agent pod, skip
-				return pluginResponse(conf)
-			}
-		}
 		return fmt.Errorf("failed to createNewK8sClient: %v", err)
 	}
 
@@ -404,6 +407,6 @@ func isCNIPod(conf *Config, k8sArgs *K8sArgs) bool {
 		log.Infof("in a degraded state and %v looks like our own agent pod, skipping", k8sArgs.K8S_POD_NAME)
 		return true
 	}
-	log.Warnf("not a CNI pod, podName: %s, podNamespace: %s", k8sArgs.K8S_POD_NAME, k8sArgs.K8S_POD_NAMESPACE)
+	log.Warnf("not a CNI pod, podName: %s, podNamespace: %s, conf pod namespace: %s", k8sArgs.K8S_POD_NAME, k8sArgs.K8S_POD_NAMESPACE, conf.PodNamespace)
 	return false
 }

releasenotes/notes/57316.yaml

@@ -0,0 +1,10 @@
+apiVersion: release-notes/v2
+kind: feature
+area: security
+issue:
+  - 57316
+releaseNotes:
+- |
+  **Fixed** the behavior of istio-cni cleanup when the get daemonset command fails due to an error
+  other than "not found". Defaults to not cleaning up the CNI config and binary when it cannot be
+  concluded if an upgrade, delete, or node reboot is in progress.