move collide function to crd package

api/v1alpha1/nginxingresscontroller_types.go

@@ -1,11 +1,17 @@
 package v1alpha1
 
 import (
+	"context"
 	"fmt"
 	"unicode"
 
+	"github.com/go-logr/logr"
+	netv1 "k8s.io/api/networking/v1"
+	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func init() {
@@ -195,6 +201,7 @@ func (n *NginxIngressController) Valid() string {
 	return ""
 }
 
+// Default sets default spec values for this NginxIngressController
 func (n *NginxIngressController) Default() {
 	if n.Spec.IngressClassName == "" {
 		n.Spec.IngressClassName = n.Name
@@ -205,6 +212,52 @@ func (n *NginxIngressController) Default() {
 	}
 }
 
+// Collides returns whether the fields in this NginxIngressController would collide with an existing resources making it
+// impossible for this NginxIngressController to become available. This should be run before an NginxIngressController is created.
+// Returns whether there's a collision, the collision reason, and an error if one occurred. The collision reason is something that
+// the user can use to understand and resolve.
+func (n *NginxIngressController) Collides(ctx context.Context, cl client.Client) (bool, string, error) {
+	lgr := logr.FromContextOrDiscard(ctx).WithValues("name", n.Name, "ingressClassName", n.Spec.IngressClassName)
+	lgr.Info("checking for NginxIngressController collisions")
+
+	// check for NginxIngressController collisions
+	lgr.Info("checking for NginxIngressController collision")
+	var nginxIngressControllerList NginxIngressControllerList
+	if err := cl.List(ctx, &nginxIngressControllerList); err != nil {
+		lgr.Error(err, "listing NginxIngressControllers")
+		return false, "", fmt.Errorf("listing NginxIngressControllers: %w", err)
+	}
+
+	for _, nic := range nginxIngressControllerList.Items {
+		if nic.Spec.IngressClassName == n.Spec.IngressClassName && nic.Name != n.Name {
+			lgr.Info("NginxIngressController collision found")
+			return true, fmt.Sprintf("spec.ingressClassName \"%s\" is invalid because NginxIngressController \"%s\" already uses IngressClass \"%[1]s\"", n.Spec.IngressClassName, nic.Name), nil
+		}
+	}
+
+	// Check for an IngressClass collision.
+	// This is purposefully after the NginxIngressController check because if the collision is through an NginxIngressController
+	// that's the one we want to report as the reason since the user action for fixing that would involve working with the NginxIngressController
+	// resource rather than the IngressClass resource.
+	lgr.Info("checking for IngressClass collision")
+	ic := &netv1.IngressClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: n.Spec.IngressClassName,
+		},
+	}
+	err := cl.Get(ctx, types.NamespacedName{Name: ic.Name}, ic)
+	if err == nil {
+		lgr.Info("IngressClass collision found")
+		return true, fmt.Sprintf("spec.ingressClassName \"%s\" is invalid because IngressClass \"%[1]s\" already exists", n.Spec.IngressClassName), nil
+	}
+	if !k8serrors.IsNotFound(err) {
+		lgr.Error(err, "checking for IngressClass collisions")
+		return false, "", fmt.Errorf("checking for IngressClass collisions: %w", err)
+	}
+
+	return false, "", nil
+}
+
 //+kubebuilder:object:root=true
 //+kubebuilder:resource:scope=Cluster
 

api/v1alpha1/nginxingresscontroller_types_test.go

@@ -1,12 +1,21 @@
 package v1alpha1
 
 import (
+	"context"
+	"errors"
 	"fmt"
 	"strings"
 	"testing"
 
+	"github.com/stretchr/testify/require"
+	netv1 "k8s.io/api/networking/v1"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 )
 
 func validNginxIngressController() NginxIngressController {
@@ -316,6 +325,145 @@ func TestNginxIngressControllerSetCondition(t *testing.T) {
 	}
 }
 
+func TestNginxIngressControllerCollides(t *testing.T) {
+	scheme := runtime.NewScheme()
+	require.NoError(t, AddToScheme(scheme))
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	cl := fake.NewClientBuilder().WithScheme(scheme).Build()
+
+	existingIc := &netv1.IngressClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "existing",
+		},
+	}
+	require.NoError(t, cl.Create(context.Background(), existingIc))
+	existingNic := &NginxIngressController{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "existing2",
+		},
+		Spec: NginxIngressControllerSpec{
+			IngressClassName:     "existing2",
+			ControllerNamePrefix: "prefix3",
+		},
+	}
+	require.NoError(t, cl.Create(context.Background(), existingNic))
+	existingNicWithExistingIc := &NginxIngressController{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "existing3",
+		},
+		Spec: NginxIngressControllerSpec{
+			IngressClassName:     "existing3",
+			ControllerNamePrefix: "prefix3",
+		},
+	}
+	require.NoError(t, cl.Create(context.Background(), existingNicWithExistingIc))
+	existingIcWithExistingNic := &netv1.IngressClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: existingNicWithExistingIc.Spec.IngressClassName,
+		},
+	}
+	require.NoError(t, cl.Create(context.Background(), existingIcWithExistingNic))
+
+	cases := []struct {
+		name          string
+		reqNic        *NginxIngressController
+		wantCollision bool
+		wantReason    string
+		wantErr       error
+	}{
+		{
+			name: "no collision",
+			reqNic: &NginxIngressController{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "new",
+				},
+				Spec: NginxIngressControllerSpec{
+					IngressClassName: "new",
+				},
+			},
+		},
+		{
+			name: "collision with existing IngressClass",
+			reqNic: &NginxIngressController{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "new",
+				},
+				Spec: NginxIngressControllerSpec{
+					IngressClassName: existingIc.Name,
+				},
+			},
+			wantCollision: true,
+			wantReason:    "spec.ingressClassName \"existing\" is invalid because IngressClass \"existing\" already exists",
+		},
+		{
+			name: "collision with existing NginxIngressController",
+			reqNic: &NginxIngressController{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "new",
+				},
+				Spec: NginxIngressControllerSpec{
+					IngressClassName: existingNic.Spec.IngressClassName,
+				},
+			},
+			wantCollision: true,
+			wantReason:    "spec.ingressClassName \"existing2\" is invalid because NginxIngressController \"existing2\" already uses IngressClass \"existing2\"",
+		},
+		{
+			name: "collision with existing NginxIngressController and IngressClass should show NginxIngressController reason",
+			reqNic: &NginxIngressController{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "new",
+				},
+				Spec: NginxIngressControllerSpec{
+					IngressClassName: existingNicWithExistingIc.Spec.IngressClassName,
+				},
+			},
+			wantCollision: true,
+			wantReason:    "spec.ingressClassName \"existing3\" is invalid because NginxIngressController \"existing3\" already uses IngressClass \"existing3\"",
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			gotCollision, gotReason, gotErr := c.reqNic.Collides(context.Background(), cl)
+			if gotCollision != c.wantCollision {
+				t.Errorf("NginxIngressController.Collides() gotCollision = %v, want %v", gotCollision, c.wantCollision)
+			}
+			if gotReason != c.wantReason {
+				t.Errorf("NginxIngressController.Collides() gotReason = %v, want %v", gotReason, c.wantReason)
+			}
+			if gotErr != c.wantErr {
+				t.Errorf("NginxIngressController.Collides() gotErr = %v, want %v", gotErr, c.wantErr)
+			}
+		})
+	}
+
+	t.Run("client errors", func(t *testing.T) {
+		listErr := errors.New("list error")
+		listErrCl := fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{
+			List: func(_ context.Context, _ client.WithWatch, _ client.ObjectList, _ ...client.ListOption) error {
+				return listErr
+			},
+		}).WithScheme(scheme).Build()
+		getErr := errors.New("get error")
+		getErrCl := fake.NewClientBuilder().WithInterceptorFuncs(interceptor.Funcs{
+			Get: func(_ context.Context, _ client.WithWatch, _ client.ObjectKey, _ client.Object, _ ...client.GetOption) error {
+				return getErr
+			},
+		}).WithScheme(scheme).Build()
+
+		collision, reason, err := existingNic.Collides(context.Background(), listErrCl)
+		require.False(t, collision)
+		require.Empty(t, reason)
+		require.True(t, errors.Is(err, listErr), "expected error \"%v\", to be type \"%v\"", err, listErr)
+
+		collision, reason, err = existingNic.Collides(context.Background(), getErrCl)
+		require.False(t, collision)
+		require.Empty(t, reason)
+		require.True(t, errors.Is(err, getErr), "expected error \"%v\", to be type \"%v\"", err, getErr)
+	})
+}
+
 func TestStartsWithAlphaNum(t *testing.T) {
 	cases := []struct {
 		name string

pkg/webhook/nginxingress.go

@@ -16,9 +16,6 @@ import (
 	admissionv1 "k8s.io/api/admission/v1"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	authv1 "k8s.io/api/authorization/v1"
-	netv1 "k8s.io/api/networking/v1"
-	k8serrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -174,6 +171,7 @@ func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admissio
 	}()
 
 	lgr := logr.FromContextOrDiscard(ctx).WithValues("resourceName", req.Name, "namespace", req.Namespace, "operation", req.Operation).WithName(nginxResourceValidationName.LoggerName())
+	lgr.Info("validating NginxIngressController request")
 
 	// ensure user has permissions required
 	var cantPerform string
@@ -209,41 +207,20 @@ func (n *nginxIngressResourceValidator) Handle(ctx context.Context, req admissio
 			return admission.Allowed("")
 		}
 
-		lgr.Info("checking if IngressClass already exists")
-		ic := &netv1.IngressClass{
-			ObjectMeta: metav1.ObjectMeta{
-				Name: nginxIngressController.Spec.IngressClassName,
-			},
-		}
-		lgr.Info("attempting to get IngressClass " + ic.Name)
-		err = n.client.Get(ctx, client.ObjectKeyFromObject(ic), ic)
-		if err == nil {
-			lgr.Info("denied because IngressClass already exists")
-			return admission.Denied(fmt.Sprintf("IngressClass %s already exists. Delete or use a different spec.IngressClassName field", ic.Name))
-		}
-		// err can still be populated as not found after this so be careful to overwrite for accurate metrics if needed
-		if !k8serrors.IsNotFound(err) {
-			lgr.Error(err, "get IngressClass")
-			return admission.Errored(http.StatusInternalServerError, fmt.Errorf("getting IngressClass %s: %w", ic.Name, err))
+		lgr.Info("checking if it collides")
+		collides, reason, err := nginxIngressController.Collides(ctx, n.client)
+		if err != nil {
+			lgr.Error(err, "checking if it collides")
+			return admission.Errored(http.StatusInternalServerError, fmt.Errorf("checking if it collides: %w", err))
 		}
 
-		// list nginx ingress controllers
-		lgr.Info("listing NginxIngressControllers to check for collisions")
-		var nginxIngressControllerList approutingv1alpha1.NginxIngressControllerList
-		if err = n.client.List(ctx, &nginxIngressControllerList); err != nil {
-			lgr.Error(err, "listing NginxIngressControllers")
-			return admission.Errored(http.StatusInternalServerError, fmt.Errorf("listing NginxIngressControllers: %w", err))
+		if collides {
+			lgr.Info("denied due to collision", "reason", reason)
+			return admission.Denied(reason)
 		}
-
-		for _, nic := range nginxIngressControllerList.Items {
-			if nic.Spec.IngressClassName == nginxIngressController.Spec.IngressClassName {
-				lgr.Info("denied admission. IngressClass already exists on NginxIngressController " + nic.Name)
-				return admission.Denied(fmt.Sprintf("IngressClass %s already exists on NginxIngressController %s. Use a different spec.IngressClassName field", nic.Spec.IngressClassName, nic.Name))
-			}
-		}
-
 	}
 
+	lgr.Info("admission allowed")
 	return admission.Allowed("")
 }
 

pkg/webhook/nginxingress_test.go

@@ -86,7 +86,7 @@ func TestNginxIngressResourceValidator(t *testing.T) {
 			Name: "existing",
 		},
 		Spec: approutingv1alpha1.NginxIngressControllerSpec{
-			IngressClassName:     "existing",
+			IngressClassName:     "existing2",
 			ControllerNamePrefix: "prefix",
 		},
 	}
@@ -156,7 +156,7 @@ func TestNginxIngressResourceValidator(t *testing.T) {
 				},
 			},
 			authenticator: validUser,
-			expected:      admission.Denied(fmt.Sprintf("IngressClass %s already exists. Delete or use a different spec.IngressClassName field", existingIc.Name)),
+			expected:      admission.Denied("spec.ingressClassName \"existing\" is invalid because IngressClass \"existing\" already exists"),
 		},
 		{
 			name: "valid nginx ingress controller, valid user, existing ingress class, update",
@@ -200,15 +200,15 @@ func TestNginxIngressResourceValidator(t *testing.T) {
 					Object: runtime.RawExtension{
 						Raw: toRaw(func() *approutingv1alpha1.NginxIngressController {
 							copy := validNginxIngressController.DeepCopy()
-							copy.Spec.IngressClassName = existingIc.Name
+							copy.Spec.IngressClassName = existingNic.Spec.IngressClassName
 							copy.Name = "other"
 							return copy
 						}()),
 					},
 				},
 			},
 			authenticator: validUser,
-			expected:      admission.Denied(fmt.Sprintf("IngressClass %s already exists. Delete or use a different spec.IngressClassName field", existingNic.Spec.IngressClassName)),
+			expected:      admission.Denied("spec.ingressClassName \"existing2\" is invalid because NginxIngressController \"existing\" already uses IngressClass \"existing2\""),
 		},
 		{
 			name: "valid nginx ingress controller, valid user, existing ingress class on other nginx ingress controller, updating",