From 1152f3293b9d1dcb5df0ee794d26293ff112e9d0 Mon Sep 17 00:00:00 2001 From: Amit Lichtenberg Date: Sun, 10 Nov 2024 15:06:21 +0200 Subject: [PATCH] Fix wrong handling of k8s errors causing wrong handling of applying netpols on non-existing namespaces (#514) --- .../external_traffic/network_policy.go | 6 ++-- .../intents_reconcilers/istio_policy.go | 7 +++-- .../networkpolicy/reconciler.go | 29 ++++++++++--------- .../controllers/intents_reconcilers/pods.go | 7 +++-- src/shared/reconcilergroup/reconcilergroup.go | 7 +++-- .../reconcilergroup/reconcilergroup_test.go | 29 +++++++++++++++++++ 6 files changed, 61 insertions(+), 24 deletions(-) diff --git a/src/operator/controllers/external_traffic/network_policy.go b/src/operator/controllers/external_traffic/network_policy.go index 61f3332c0..8f7c99a45 100644 --- 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 index 341bdb551..03f13b3e1 100644 --- 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 index 6d47be4b3..98fd2f84d 100644 --- 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 index 5b6f6480b..7bb432e7b 100644 --- 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 index e5f5541e7..d2a2b544c 100644 --- 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 index 029ece4fe..93b17dc29 100644 --- 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)