From e6d483ea9e1297fe6955b70be6300491129316e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20Luk=C5=A1a?= Date: Mon, 29 Apr 2024 11:11:21 +0200 Subject: [PATCH] Don't log error when namespace is being deleted (#88) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously, when the namespace referenced in IstioCni or IstioRevision.spec.namespace was being deleted, the operator log would show this as an error (with a stacktrace). Because of the requeues, the error was also logged many times. Now, if the namespace is being deleted, the operator logs this at INFO level. It also doesn't requeue the object in this case. Signed-off-by: Marko Lukša --- controllers/istio/istio_controller.go | 8 +- controllers/istio/istio_controller_test.go | 61 +++++++ controllers/istiocni/istiocni_controller.go | 13 +- .../istiocni/istiocni_controller_test.go | 87 ++++++++++ .../istiorevision/istiorevision_controller.go | 12 +- .../istiorevision_controller_test.go | 164 ++++++++++++++++++ pkg/validation/namespace.go | 41 +++++ pkg/validation/namespace_test.go | 96 ++++++++++ 8 files changed, 463 insertions(+), 19 deletions(-) create mode 100644 pkg/validation/namespace.go create mode 100644 pkg/validation/namespace_test.go diff --git a/controllers/istio/istio_controller.go b/controllers/istio/istio_controller.go index 1e5f44617..f4ef36853 100644 --- a/controllers/istio/istio_controller.go +++ b/controllers/istio/istio_controller.go @@ -85,7 +85,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, istio *v1alpha1.Istio) (ctrl // doReconcile is the function that actually reconciles the Istio object. Any error reported by this // function should get reported in the status of the Istio object by the caller. func (r *Reconciler) doReconcile(ctx context.Context, istio *v1alpha1.Istio) (result ctrl.Result, err error) { - if err := validateIstio(istio); err != nil { + if err := validate(istio); err != nil { return ctrl.Result{}, err } @@ -101,12 +101,12 @@ func (r *Reconciler) doReconcile(ctx context.Context, istio *v1alpha1.Istio) (re return r.pruneInactiveRevisions(ctx, istio) } -func validateIstio(istio *v1alpha1.Istio) error { +func validate(istio *v1alpha1.Istio) error { if istio.Spec.Version == "" { - return reconciler.NewValidationError("no spec.version set") + return reconciler.NewValidationError("spec.version not set") } if istio.Spec.Namespace == "" { - return reconciler.NewValidationError("no spec.namespace set") + return reconciler.NewValidationError("spec.namespace not set") } return nil } diff --git a/controllers/istio/istio_controller_test.go b/controllers/istio/istio_controller_test.go index 161b41352..a9e40ebd4 100644 --- a/controllers/istio/istio_controller_test.go +++ b/controllers/istio/istio_controller_test.go @@ -30,6 +30,8 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/scheme" "github.com/istio-ecosystem/sail-operator/pkg/test/testtime" + "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" + . "github.com/onsi/gomega" "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -166,6 +168,65 @@ func TestReconcile(t *testing.T) { }) } +func TestValidate(t *testing.T) { + testCases := []struct { + name string + istio *v1alpha1.Istio + expectErr string + }{ + { + name: "success", + istio: &v1alpha1.Istio{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioSpec{ + Version: supportedversion.Default, + Namespace: "istio-system", + }, + }, + expectErr: "", + }, + { + name: "no version", + istio: &v1alpha1.Istio{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioSpec{ + Namespace: "istio-system", + }, + }, + expectErr: "spec.version not set", + }, + { + name: "no namespace", + istio: &v1alpha1.Istio{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioSpec{ + Version: supportedversion.Default, + }, + }, + expectErr: "spec.namespace not set", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + err := validate(tc.istio) + if tc.expectErr == "" { + g.Expect(err).ToNot(HaveOccurred()) + } else { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.expectErr)) + } + }) + } +} + func TestDetermineStatus(t *testing.T) { resourceDir := t.TempDir() diff --git a/controllers/istiocni/istiocni_controller.go b/controllers/istiocni/istiocni_controller.go index bda385748..4584565e3 100644 --- a/controllers/istiocni/istiocni_controller.go +++ b/controllers/istiocni/istiocni_controller.go @@ -30,6 +30,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/kube" "github.com/istio-ecosystem/sail-operator/pkg/profiles" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" + "github.com/istio-ecosystem/sail-operator/pkg/validation" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" @@ -110,7 +111,7 @@ func (r *Reconciler) Finalize(ctx context.Context, cni *v1alpha1.IstioCNI) error func (r *Reconciler) doReconcile(ctx context.Context, cni *v1alpha1.IstioCNI) error { log := logf.FromContext(ctx) - if err := r.validateIstioCNI(ctx, cni); err != nil { + if err := r.validate(ctx, cni); err != nil { return err } @@ -118,19 +119,15 @@ func (r *Reconciler) doReconcile(ctx context.Context, cni *v1alpha1.IstioCNI) er return r.installHelmChart(ctx, cni) } -func (r *Reconciler) validateIstioCNI(ctx context.Context, cni *v1alpha1.IstioCNI) error { +func (r *Reconciler) validate(ctx context.Context, cni *v1alpha1.IstioCNI) error { if cni.Spec.Version == "" { return reconciler.NewValidationError("spec.version not set") } if cni.Spec.Namespace == "" { return reconciler.NewValidationError("spec.namespace not set") } - - if err := r.Client.Get(ctx, types.NamespacedName{Name: cni.Spec.Namespace}, &corev1.Namespace{}); err != nil { - if apierrors.IsNotFound(err) { - return reconciler.NewValidationError(fmt.Sprintf("namespace %q doesn't exist", cni.Spec.Namespace)) - } - return fmt.Errorf("get failed: %w", err) + if err := validation.ValidateTargetNamespace(ctx, r.Client, cni.Spec.Namespace); err != nil { + return err } return nil } diff --git a/controllers/istiocni/istiocni_controller_test.go b/controllers/istiocni/istiocni_controller_test.go index 7c8f77182..fdaae34d0 100644 --- a/controllers/istiocni/istiocni_controller_test.go +++ b/controllers/istiocni/istiocni_controller_test.go @@ -23,8 +23,10 @@ import ( "github.com/istio-ecosystem/sail-operator/api/v1alpha1" "github.com/istio-ecosystem/sail-operator/pkg/config" "github.com/istio-ecosystem/sail-operator/pkg/scheme" + "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" @@ -34,6 +36,91 @@ import ( "istio.io/istio/pkg/ptr" ) +func TestValidate(t *testing.T) { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-cni", + }, + } + + testCases := []struct { + name string + cni *v1alpha1.IstioCNI + objects []client.Object + expectErr string + }{ + { + name: "success", + cni: &v1alpha1.IstioCNI{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioCNISpec{ + Version: supportedversion.Default, + Namespace: "istio-cni", + }, + }, + objects: []client.Object{ns}, + expectErr: "", + }, + { + name: "no version", + cni: &v1alpha1.IstioCNI{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioCNISpec{ + Namespace: "istio-cni", + }, + }, + objects: []client.Object{ns}, + expectErr: "spec.version not set", + }, + { + name: "no namespace", + cni: &v1alpha1.IstioCNI{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioCNISpec{ + Version: supportedversion.Default, + }, + }, + objects: []client.Object{ns}, + expectErr: "spec.namespace not set", + }, + { + name: "namespace not found", + cni: &v1alpha1.IstioCNI{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioCNISpec{ + Version: supportedversion.Default, + Namespace: "istio-cni", + }, + }, + objects: []client.Object{}, + expectErr: `namespace "istio-cni" doesn't exist`, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tc.objects...).Build() + r := NewReconciler(cl, scheme.Scheme, "", nil, "") + + err := r.validate(context.TODO(), tc.cni) + if tc.expectErr == "" { + g.Expect(err).ToNot(HaveOccurred()) + } else { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.expectErr)) + } + }) + } +} + func TestDeriveState(t *testing.T) { testCases := []struct { name string diff --git a/controllers/istiorevision/istiorevision_controller.go b/controllers/istiorevision/istiorevision_controller.go index ccd3263fb..c87f82213 100644 --- a/controllers/istiorevision/istiorevision_controller.go +++ b/controllers/istiorevision/istiorevision_controller.go @@ -29,6 +29,7 @@ import ( "github.com/istio-ecosystem/sail-operator/pkg/helm" "github.com/istio-ecosystem/sail-operator/pkg/kube" "github.com/istio-ecosystem/sail-operator/pkg/reconciler" + "github.com/istio-ecosystem/sail-operator/pkg/validation" admissionv1 "k8s.io/api/admissionregistration/v1" appsv1 "k8s.io/api/apps/v1" autoscalingv2 "k8s.io/api/autoscaling/v2" @@ -112,7 +113,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, rev *v1alpha1.IstioRevision) func (r *Reconciler) doReconcile(ctx context.Context, rev *v1alpha1.IstioRevision) error { log := logf.FromContext(ctx) - if err := r.validateIstioRevision(ctx, rev); err != nil { + if err := r.validate(ctx, rev); err != nil { return err } @@ -124,18 +125,15 @@ func (r *Reconciler) Finalize(ctx context.Context, rev *v1alpha1.IstioRevision) return r.uninstallHelmCharts(ctx, rev) } -func (r *Reconciler) validateIstioRevision(ctx context.Context, rev *v1alpha1.IstioRevision) error { +func (r *Reconciler) validate(ctx context.Context, rev *v1alpha1.IstioRevision) error { if rev.Spec.Version == "" { return reconciler.NewValidationError("spec.version not set") } if rev.Spec.Namespace == "" { return reconciler.NewValidationError("spec.namespace not set") } - if err := r.Client.Get(ctx, types.NamespacedName{Name: rev.Spec.Namespace}, &corev1.Namespace{}); err != nil { - if apierrors.IsNotFound(err) { - return reconciler.NewValidationError(fmt.Sprintf("namespace %q doesn't exist", rev.Spec.Namespace)) - } - return fmt.Errorf("get failed: %w", err) + if err := validation.ValidateTargetNamespace(ctx, r.Client, rev.Spec.Namespace); err != nil { + return err } if rev.Spec.Values == nil { diff --git a/controllers/istiorevision/istiorevision_controller_test.go b/controllers/istiorevision/istiorevision_controller_test.go index 76c5b86b1..aeb947179 100644 --- a/controllers/istiorevision/istiorevision_controller_test.go +++ b/controllers/istiorevision/istiorevision_controller_test.go @@ -22,6 +22,7 @@ import ( "github.com/istio-ecosystem/sail-operator/api/v1alpha1" "github.com/istio-ecosystem/sail-operator/pkg/scheme" + "github.com/istio-ecosystem/sail-operator/pkg/test/util/supportedversion" . "github.com/onsi/gomega" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" @@ -33,6 +34,169 @@ import ( "istio.io/istio/pkg/ptr" ) +func TestValidate(t *testing.T) { + ns := &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "istio-system", + }, + } + + testCases := []struct { + name string + rev *v1alpha1.IstioRevision + objects []client.Object + expectErr string + }{ + { + name: "success", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Version: supportedversion.Default, + Namespace: "istio-system", + Values: &v1alpha1.Values{ + Global: &v1alpha1.GlobalConfig{ + IstioNamespace: "istio-system", + }, + }, + }, + }, + objects: []client.Object{ns}, + expectErr: "", + }, + { + name: "no version", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Namespace: "istio-system", + }, + }, + objects: []client.Object{ns}, + expectErr: "spec.version not set", + }, + { + name: "no namespace", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Version: supportedversion.Default, + }, + }, + objects: []client.Object{ns}, + expectErr: "spec.namespace not set", + }, + { + name: "namespace not found", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Version: supportedversion.Default, + Namespace: "istio-system", + }, + }, + objects: []client.Object{}, + expectErr: `namespace "istio-system" doesn't exist`, + }, + { + name: "no values", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Version: supportedversion.Default, + Namespace: "istio-system", + }, + }, + objects: []client.Object{ns}, + expectErr: "spec.values not set", + }, + { + name: "invalid istioNamespace", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Version: supportedversion.Default, + Namespace: "istio-system", + Values: &v1alpha1.Values{ + Global: &v1alpha1.GlobalConfig{ + IstioNamespace: "other-namespace", + }, + }, + }, + }, + objects: []client.Object{ns}, + expectErr: "spec.values.global.istioNamespace does not match spec.namespace", + }, + { + name: "invalid revision default", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "default", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Version: supportedversion.Default, + Namespace: "istio-system", + Values: &v1alpha1.Values{ + Revision: "my-revision", + Global: &v1alpha1.GlobalConfig{ + IstioNamespace: "other-namespace", + }, + }, + }, + }, + objects: []client.Object{ns}, + expectErr: `spec.values.revision must be "" when IstioRevision name is default`, + }, + { + name: "invalid revision non-default", + rev: &v1alpha1.IstioRevision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-revision", + }, + Spec: v1alpha1.IstioRevisionSpec{ + Version: supportedversion.Default, + Namespace: "istio-system", + Values: &v1alpha1.Values{ + Revision: "other-revision", + Global: &v1alpha1.GlobalConfig{ + IstioNamespace: "other-namespace", + }, + }, + }, + }, + objects: []client.Object{ns}, + expectErr: `spec.values.revision does not match IstioRevision name`, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + cl := fake.NewClientBuilder().WithScheme(scheme.Scheme).WithObjects(tc.objects...).Build() + r := NewReconciler(cl, scheme.Scheme, "", nil) + + err := r.validate(context.TODO(), tc.rev) + if tc.expectErr == "" { + g.Expect(err).ToNot(HaveOccurred()) + } else { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.expectErr)) + } + }) + } +} + func TestDeriveState(t *testing.T) { testCases := []struct { name string diff --git a/pkg/validation/namespace.go b/pkg/validation/namespace.go new file mode 100644 index 000000000..8e95eb9b3 --- /dev/null +++ b/pkg/validation/namespace.go @@ -0,0 +1,41 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validation + +import ( + "context" + "fmt" + + "github.com/istio-ecosystem/sail-operator/pkg/reconciler" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// ValidateTargetNamespace checks if the target namespace exists and is not being deleted. +func ValidateTargetNamespace(ctx context.Context, cl client.Client, namespace string) error { + ns := &corev1.Namespace{} + if err := cl.Get(ctx, types.NamespacedName{Name: namespace}, ns); err != nil { + if apierrors.IsNotFound(err) { + return reconciler.NewValidationError(fmt.Sprintf("namespace %q doesn't exist", namespace)) + } + return fmt.Errorf("get failed: %w", err) + } + if ns.DeletionTimestamp != nil { + return reconciler.NewValidationError(fmt.Sprintf("namespace %q is being deleted", namespace)) + } + return nil +} diff --git a/pkg/validation/namespace_test.go b/pkg/validation/namespace_test.go new file mode 100644 index 000000000..994b3064d --- /dev/null +++ b/pkg/validation/namespace_test.go @@ -0,0 +1,96 @@ +// Copyright Istio Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package validation + +import ( + "context" + "fmt" + "testing" + + "github.com/istio-ecosystem/sail-operator/pkg/scheme" + "github.com/istio-ecosystem/sail-operator/pkg/test/testtime" + . "github.com/onsi/gomega" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +func TestValidateTargetNamespace(t *testing.T) { + testCases := []struct { + name string + objects []client.Object + interceptors interceptor.Funcs + expectErr string + }{ + { + name: "success", + objects: []client.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-namespace", + }, + }, + }, + expectErr: "", + }, + { + name: "namespace not found", + objects: []client.Object{}, + expectErr: `namespace "my-namespace" doesn't exist`, + }, + { + name: "namespace deleted", + objects: []client.Object{ + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: "my-namespace", + DeletionTimestamp: testtime.OneMinuteAgo(), + Finalizers: []string{"dummy"}, // required for fake client builder to accept a deleted object + }, + }, + }, + expectErr: `namespace "my-namespace" is being deleted`, + }, + { + name: "get error", + interceptors: interceptor.Funcs{ + Get: func(ctx context.Context, client client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + return fmt.Errorf("simulated error") + }, + }, + expectErr: "simulated error", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + cl := fake.NewClientBuilder(). + WithScheme(scheme.Scheme). + WithObjects(tc.objects...). + WithInterceptorFuncs(tc.interceptors). + Build() + + err := ValidateTargetNamespace(context.TODO(), cl, "my-namespace") + if tc.expectErr == "" { + g.Expect(err).ToNot(HaveOccurred()) + } else { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tc.expectErr)) + } + }) + } +}