Fix wrong handling of k8s errors causing wrong handling of applying n…

…etpols on non-existing namespaces (#514)
otterize · Nov 10, 2024 · 1152f32 · 1152f32
1 parent 4f75687
commit 1152f32
Show file tree

Hide file tree

Showing 6 changed files with 61 additions and 24 deletions.
diff --git a/src/operator/controllers/external_traffic/network_policy.go b/src/operator/controllers/external_traffic/network_policy.go
@@ -369,8 +369,10 @@ func (r *NetworkPolicyHandler) handleEndpointsWithIngressList(ctx context.Contex
 	foundOtterizeNetpolsAffectingPods := false
 	for _, address := range addresses {
 		pod, err := r.getAffectedPod(ctx, address)
-		if k8serrors.IsNotFound(errors.Unwrap(err)) {
-			continue
+		if k8sErr := &(k8serrors.StatusError{}); errors.As(err, &k8sErr) {
+			if k8serrors.IsNotFound(k8sErr) {
+				continue
+			}
 		}
 
 		if err != nil {

diff --git a/src/operator/controllers/intents_reconcilers/istio_policy.go b/src/operator/controllers/intents_reconcilers/istio_policy.go
@@ -124,9 +124,10 @@ func (r *IstioPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)
 
 	err = r.policyManager.Create(ctx, intents, clientServiceAccountName)
 	if err != nil {
-		errUnwrap := errors.Unwrap(err)
-		if k8serrors.IsConflict(errUnwrap) || k8serrors.IsAlreadyExists(errUnwrap) {
-			return ctrl.Result{Requeue: true}, nil
+		if k8sErr := &(k8serrors.StatusError{}); errors.As(err, &k8sErr) {
+			if k8serrors.IsConflict(k8sErr) || k8serrors.IsAlreadyExists(k8sErr) {
+				return ctrl.Result{Requeue: true}, nil
+			}
 		}
 		return ctrl.Result{}, errors.Wrap(err)
 	}

diff --git a/src/operator/controllers/intents_reconcilers/networkpolicy/reconciler.go b/src/operator/controllers/intents_reconcilers/networkpolicy/reconciler.go
@@ -578,22 +578,23 @@ func (r *Reconciler) handleExistingPolicyRetry(ctx context.Context, ep effective
 
 func (r *Reconciler) handleCreationErrors(ctx context.Context, ep effectivepolicy.ServiceEffectivePolicy, netpol *v1.NetworkPolicy, err error) error {
 	errStr := err.Error()
-	errUnwrap := errors.Unwrap(err)
-	if k8serrors.IsAlreadyExists(errUnwrap) {
-		// Ideally we would just return {Requeue: true} but it is not possible without a mini-refactor
-		return r.handleExistingPolicyRetry(ctx, ep, netpol)
-	}
+	if k8sErr := &(k8serrors.StatusError{}); errors.As(err, &k8sErr) {
+		if k8serrors.IsAlreadyExists(k8sErr) {
+			// Ideally we would just return {Requeue: true} but it is not possible without a mini-refactor
+			return r.handleExistingPolicyRetry(ctx, ep, netpol)
+		}
 
-	if k8serrors.IsForbidden(errUnwrap) && strings.Contains(errStr, "is being terminated") {
-		// Namespace is being deleted, nothing to do further
-		logrus.Debugf("Namespace %s is being terminated, ignoring api server error", netpol.Namespace)
-		return nil
-	}
+		if k8serrors.IsForbidden(k8sErr) && strings.Contains(errStr, "is being terminated") {
+			// Namespace is being deleted, nothing to do further
+			logrus.Debugf("Namespace %s is being terminated, ignoring api server error", netpol.Namespace)
+			return nil
+		}
 
-	if k8serrors.IsNotFound(errUnwrap) && strings.Contains(errStr, netpol.Namespace) {
-		// Namespace was deleted since we started .Create() logic, nothing to do further
-		logrus.Debugf("Namespace %s was deleted, ignoring api server error", netpol.Namespace)
-		return nil
+		if k8serrors.IsNotFound(k8sErr) && strings.Contains(errStr, netpol.Namespace) {
+			// Namespace was deleted since we started .Create() logic, nothing to do further
+			logrus.Debugf("Namespace %s was deleted, ignoring api server error", netpol.Namespace)
+			return nil
+		}
 	}
 
 	return errors.Wrap(err)

diff --git a/src/operator/controllers/intents_reconcilers/pods.go b/src/operator/controllers/intents_reconcilers/pods.go
@@ -51,9 +51,10 @@ func (r *PodLabelReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	if !intents.DeletionTimestamp.IsZero() {
 		err := r.removeLabelsFromPods(ctx, intents)
 		if err != nil {
-			errUnwrap := errors.Unwrap(err)
-			if k8serrors.IsConflict(errUnwrap) || k8serrors.IsNotFound(errUnwrap) || k8serrors.IsForbidden(errUnwrap) {
-				return ctrl.Result{Requeue: true}, nil
+			if k8sErr := &(k8serrors.StatusError{}); errors.As(err, &k8sErr) {
+				if k8serrors.IsConflict(k8sErr) || k8serrors.IsNotFound(k8sErr) || k8serrors.IsForbidden(k8sErr) {
+					return ctrl.Result{Requeue: true}, nil
+				}
 			}
 			r.RecordWarningEventf(intents, ReasonRemovingPodLabelsFailed, "could not remove pod labels: %s", err.Error())
 			return ctrl.Result{}, errors.Wrap(err)

diff --git a/src/shared/reconcilergroup/reconcilergroup.go b/src/shared/reconcilergroup/reconcilergroup.go
@@ -172,8 +172,11 @@ func (g *Group) InjectRecorder(recorder record.EventRecorder) {
 }
 
 func isKubernetesRaceRelatedError(err error) bool {
-	errUnwrap := errors.Unwrap(err)
-	return k8serrors.IsConflict(errUnwrap) || k8serrors.IsNotFound(errUnwrap) || k8serrors.IsForbidden(errUnwrap) || k8serrors.IsAlreadyExists(errUnwrap)
+	if k8sErr := &(k8serrors.StatusError{}); errors.As(err, &k8sErr) {
+		return k8serrors.IsConflict(k8sErr) || k8serrors.IsNotFound(k8sErr) || k8serrors.IsForbidden(k8sErr) || k8serrors.IsAlreadyExists(k8sErr)
+	}
+
+	return false
 }
 
 func shortestRequeue(a, b reconcile.Result) reconcile.Result {

diff --git a/src/shared/reconcilergroup/reconcilergroup_test.go b/src/shared/reconcilergroup/reconcilergroup_test.go
@@ -329,6 +329,35 @@ func (s *ReconcilerGroupTestSuite) TestFinalizerUpdateFailedAfterDelete() {
 	s.Require().True(reconciler.Reconciled)
 }
 
+func (s *ReconcilerGroupTestSuite) TestFinalizerUpdateFailedAfterDelete_NotFound_Requeue() {
+	reconciler := &TestReconciler{Err: nil, Result: reconcile.Result{}}
+	s.group.AddToGroup(reconciler)
+
+	resourceName := types.NamespacedName{
+		Name:      "my-resource",
+		Namespace: "the-happy-place-we-live-in",
+	}
+
+	emptyIntents := &otterizev2alpha1.ClientIntents{}
+	intentsWithFinalizer := emptyIntents.DeepCopy()
+	controllerutil.AddFinalizer(intentsWithFinalizer, testFinalizer)
+	intentsWithFinalizer.DeletionTimestamp = &v1.Time{Time: time.Date(1992, 4, 25, 19, 30, 0, 0, time.UTC)}
+
+	s.client.EXPECT().Get(gomock.Any(), resourceName, gomock.Eq(emptyIntents)).DoAndReturn(
+		func(_ context.Context, _ types.NamespacedName, intents *otterizev2alpha1.ClientIntents, _ ...client.GetOption) error {
+			*intents = *intentsWithFinalizer
+			return nil
+		})
+
+	intentsWithoutFinalizer := intentsWithFinalizer.DeepCopy()
+	controllerutil.RemoveFinalizer(intentsWithoutFinalizer, testFinalizer)
+	s.client.EXPECT().Update(gomock.Any(), gomock.Eq(intentsWithoutFinalizer)).Return(k8serrors.NewNotFound(schema.GroupResource{}, resourceName.Name))
+
+	res, err := s.group.Reconcile(context.Background(), reconcile.Request{NamespacedName: resourceName})
+	s.Require().NoError(err)
+	s.Require().True(res.Requeue)
+}
+
 func (s *ReconcilerGroupTestSuite) TestFinalizerNotDeletedIfReconcilerFailed() {
 	reconciler := &TestReconciler{Err: errors.New("test error"), Result: reconcile.Result{}}
 	s.group.AddToGroup(reconciler)