Skip to content

Commit

Permalink
Don't log error when namespace is being deleted (openshift-service-me…
Browse files Browse the repository at this point in the history 
…sh#88)

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 <mluksa@redhat.com>
  • Loading branch information
@luksa
luksa authored Apr 29, 2024
1 parent 861983b commit e6d483e
Show file tree
Hide file tree
Showing 8 changed files with 463 additions and 19 deletions.
8 changes: 4 additions & 4 deletions controllers/istio/istio_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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
}
Expand Down
61 changes: 61 additions & 0 deletions controllers/istio/istio_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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()

Expand Down
13 changes: 5 additions & 8 deletions controllers/istiocni/istiocni_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -110,27 +111,23 @@ 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
}

log.Info("Installing Helm chart")
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
}
Expand Down
87 changes: 87 additions & 0 deletions controllers/istiocni/istiocni_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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
Expand Down
12 changes: 5 additions & 7 deletions controllers/istiorevision/istiorevision_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}

Expand All @@ -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 {
Expand Down
Loading

0 comments on commit e6d483e

Please sign in to comment.