Add default name to IngressClass and tidy SCC handling

Author: soneillf5

Diff for: controllers/ingressclass.go

@@ -7,9 +7,13 @@ import (
 )
 
 func ingressClassForNginxIngressController(instance *k8sv1alpha1.NginxIngressController) *networking.IngressClass {
+	ingressClassName := "nginx"
+	if instance.Spec.IngressClass != "" {
+		ingressClassName = instance.Spec.IngressClass
+	}
 	ic := &networking.IngressClass{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: instance.Spec.IngressClass,
+			Name: ingressClassName,
 		},
 		Spec: networking.IngressClassSpec{
 			Controller: "nginx.org/ingress-controller",

Diff for: controllers/ingressclass_test.go

@@ -29,3 +29,22 @@ func TestIngressClassForNginxIngressController(t *testing.T) {
 		t.Errorf("ingressClassForNginxIngressController() mismatch (-want +got):\n%s", diff)
 	}
 }
+
+func TestIngressClassForNginxIngressControllerDefault(t *testing.T) {
+	instance := &k8sv1alpha1.NginxIngressController{
+		Spec: k8sv1alpha1.NginxIngressControllerSpec{},
+	}
+	expected := &networking.IngressClass{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "nginx",
+		},
+		Spec: networking.IngressClassSpec{
+			Controller: "nginx.org/ingress-controller",
+		},
+	}
+
+	result := ingressClassForNginxIngressController(instance)
+	if diff := cmp.Diff(expected, result); diff != "" {
+		t.Errorf("ingressClassForNginxIngressController() mismatch (-want +got):\n%s", diff)
+	}
+}

Diff for: controllers/nginxingresscontroller_controller.go

@@ -21,6 +21,8 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/nginxinc/nginx-ingress-operator/controllers/scc"
+
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
@@ -39,7 +41,6 @@ import (
 
 const (
 	clusterRoleName = "nginx-ingress-role"
-	sccName         = "nginx-ingress-scc"
 	finalizer       = "nginxingresscontroller.k8s.nginx.org/finalizer"
 )
 
@@ -287,33 +288,12 @@ func (r *NginxIngressControllerReconciler) finalizeNginxIngressController(log lo
 	}
 
 	if r.SccAPIExists {
-		scc := sccForNginxIngressController(sccName)
-
-		err := r.Get(context.TODO(), types.NamespacedName{Name: sccName, Namespace: v1.NamespaceAll}, scc)
-		if err != nil {
-			return err
-		}
-
-		var users []string
-		for _, u := range scc.Users {
-			if u != userForSCC(instance.Namespace, instance.Name) {
-				users = append(users, u)
-			}
-		}
-
-		scc.Users = users
-
-		err = r.Update(context.TODO(), scc)
+		err := scc.RemoveServiceAccount(r.Client, instance.Namespace, instance.Name)
 		if err != nil {
-			return err
+			return fmt.Errorf("failed to remove service account user from SCC: %w", err)
 		}
 	}
 
-	ic := ingressClassForNginxIngressController(instance)
-	if err := r.Delete(context.TODO(), ic); client.IgnoreNotFound(err) != nil {
-		return err
-	}
-
 	log.Info("Successfully finalized NginxIngressController")
 	return nil
 }

Diff for: controllers/prerequisites.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"fmt"
 
+	"github.com/nginxinc/nginx-ingress-operator/controllers/scc"
+
 	"github.com/go-logr/logr"
 	k8sv1alpha1 "github.com/nginxinc/nginx-ingress-operator/api/v1alpha1"
 	v1 "k8s.io/api/core/v1"
@@ -56,12 +58,7 @@ func (r *NginxIngressControllerReconciler) checkPrerequisites(log logr.Logger, i
 	// IngressClass is available from k8s 1.18+
 	minVersion, _ := version.ParseGeneric("v1.18.0")
 	if RunningK8sVersion.AtLeast(minVersion) {
-		if instance.Spec.IngressClass == "" {
-			instance.Spec.IngressClass = "nginx"
-			log.Info("Warning! IngressClass not set, using default", "IngressClass.Name", instance.Spec.IngressClass)
-		}
 		ic := ingressClassForNginxIngressController(instance)
-
 		existed, err = r.createIfNotExists(ic)
 		if err != nil {
 			return err
@@ -96,30 +93,9 @@ func (r *NginxIngressControllerReconciler) checkPrerequisites(log logr.Logger, i
 	}
 
 	if r.SccAPIExists {
-		// Assign this new User to the SCC (if is not present already)
-		scc := sccForNginxIngressController(sccName)
-
-		err = r.Get(context.TODO(), types.NamespacedName{Name: sccName, Namespace: v1.NamespaceAll}, scc)
+		err := scc.AddServiceAccount(r.Client, sa.Namespace, sa.Name)
 		if err != nil {
-			return err
-		}
-
-		user := userForSCC(sa.Namespace, sa.Name)
-		found := false
-		for _, u := range scc.Users {
-			if u == user {
-				found = true
-				break
-			}
-		}
-
-		if !found {
-			scc.Users = append(scc.Users, user)
-
-			err = r.Update(context.TODO(), scc)
-			if err != nil {
-				return err
-			}
+			return fmt.Errorf("failed to add service account user to scc: %w", err)
 		}
 	}
 
@@ -175,16 +151,9 @@ func (r *NginxIngressControllerReconciler) createCommonResources(log logr.Logger
 	if r.SccAPIExists {
 		log.Info("OpenShift detected as platform.")
 
-		scc := sccForNginxIngressController(sccName)
-
-		err = r.Get(context.TODO(), types.NamespacedName{Name: sccName, Namespace: v1.NamespaceAll}, scc)
-		if err != nil && errors.IsNotFound(err) {
-			log.Info("no previous SecurityContextConstraints found, creating a new one.")
-			err = r.Create(context.TODO(), scc)
-		}
-
+		err := scc.Create(r.Client, log)
 		if err != nil {
-			return fmt.Errorf("error creating SecurityContextConstraints: %w", err)
+			return fmt.Errorf("failed to create SecurityContextConstraints: %w", err)
 		}
 	}
 

Diff for: controllers/scc.go

@@ -0,0 +1,125 @@
+package scc
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/go-logr/logr"
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	secv1 "github.com/openshift/api/security/v1"
+	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+const defaultName = "nginx-ingress-scc"
+
+func sccConfigTemplate() *secv1.SecurityContextConstraints {
+	var uid int64 = 101
+	allowPrivilegeEscalation := true
+
+	return &secv1.SecurityContextConstraints{
+		ObjectMeta: v1.ObjectMeta{
+			Name: defaultName,
+		},
+		AllowHostPorts:           false,
+		AllowPrivilegedContainer: false,
+		RunAsUser: secv1.RunAsUserStrategyOptions{
+			Type: "MustRunAs",
+			UID:  &uid,
+		},
+		Users:                    nil,
+		AllowHostDirVolumePlugin: false,
+		AllowHostIPC:             false,
+		SELinuxContext: secv1.SELinuxContextStrategyOptions{
+			Type: "MustRunAs",
+		},
+		ReadOnlyRootFilesystem: false,
+		FSGroup: secv1.FSGroupStrategyOptions{
+			Type: "MustRunAs",
+		},
+		SupplementalGroups: secv1.SupplementalGroupsStrategyOptions{
+			Type: "MustRunAs",
+		},
+		Volumes:                  []secv1.FSType{"secret"},
+		AllowHostPID:             false,
+		AllowHostNetwork:         false,
+		AllowPrivilegeEscalation: &allowPrivilegeEscalation,
+		RequiredDropCapabilities: []corev1.Capability{"ALL"},
+		DefaultAddCapabilities:   []corev1.Capability{"NET_BIND_SERVICE"},
+		AllowedCapabilities:      nil,
+	}
+}
+
+func serviceAccountName(namespace string, name string) string {
+	return fmt.Sprintf("system:serviceaccount:%v:%v", namespace, name)
+}
+
+func Create(client client.Client, log logr.Logger) error {
+	scc := sccConfigTemplate()
+	err := client.Get(context.TODO(), types.NamespacedName{Name: defaultName, Namespace: v1.NamespaceAll}, scc)
+	if err != nil {
+		if errors.IsNotFound(err) {
+			log.Info("no previous SecurityContextConstraints found, creating a new one.")
+			err = client.Create(context.TODO(), scc)
+			if err != nil {
+				return fmt.Errorf("error creating SecurityContextConstraints: %w", err)
+			}
+		}
+		return fmt.Errorf("error getting scc: %w", err)
+	}
+
+	return nil
+}
+
+func AddServiceAccount(client client.Client, namespace string, name string) error {
+	scc := sccConfigTemplate()
+	err := client.Get(context.TODO(), types.NamespacedName{Name: defaultName, Namespace: v1.NamespaceAll}, scc)
+	if err != nil {
+		return fmt.Errorf("failed to get scc: %w", err)
+	}
+
+	saName := serviceAccountName(namespace, name)
+	for _, u := range scc.Users {
+		if u == saName {
+			// scc already has the service account name
+			return nil
+		}
+	}
+
+	scc.Users = append(scc.Users, saName)
+	err = client.Update(context.TODO(), scc)
+	if err != nil {
+		return fmt.Errorf("failed to update scc: %w", err)
+	}
+
+	return nil
+}
+
+func RemoveServiceAccount(client client.Client, namespace string, name string) error {
+	scc := sccConfigTemplate()
+	err := client.Get(context.TODO(), types.NamespacedName{Name: defaultName, Namespace: v1.NamespaceAll}, scc)
+	if err != nil {
+		return fmt.Errorf("failed to get scc: %w", err)
+	}
+
+	scc.Users = removeStringValue(scc.Users, serviceAccountName(namespace, name))
+
+	err = client.Update(context.TODO(), scc)
+	if err != nil {
+		return fmt.Errorf("failed to update scc: %w", err)
+	}
+	return nil
+}
+
+func removeStringValue(values []string, value string) []string {
+	var filtered []string
+	for _, v := range values {
+		if v != value {
+			filtered = append(filtered, v)
+		}
+	}
+	return filtered
+}

Diff for: controllers/scc/scc.go

@@ -1,4 +1,4 @@
-package controllers
+package scc
 
 import (
 	"fmt"
@@ -12,12 +12,11 @@ import (
 
 func TestSccForNginxIngressController(t *testing.T) {
 	var uid int64 = 101
-	name := "my-nginx-ingress"
 	allowPrivilegeEscalation := true
 
 	expected := &secv1.SecurityContextConstraints{
 		ObjectMeta: v1.ObjectMeta{
-			Name: name,
+			Name: "nginx-ingress-scc",
 		},
 		AllowHostPorts:           false,
 		AllowPrivilegedContainer: false,
@@ -47,19 +46,19 @@ func TestSccForNginxIngressController(t *testing.T) {
 		AllowedCapabilities:      nil,
 	}
 
-	result := sccForNginxIngressController(name)
+	result := sccConfigTemplate()
 	if diff := cmp.Diff(expected, result); diff != "" {
-		t.Errorf("sccForNginxIngressController() mismatch (-want +got):\n%s", diff)
+		t.Errorf("sccConfigTemplate() mismatch (-want +got):\n%s", diff)
 	}
 }
 
-func TestUserForSCC(t *testing.T) {
+func TestServiceAccountName(t *testing.T) {
 	namespace := "my-nginx-ingress"
 	name := "my-nginx-ingress-controller"
 	expected := fmt.Sprintf("system:serviceaccount:%v:%v", namespace, name)
 
-	result := userForSCC(namespace, name)
+	result := serviceAccountName(namespace, name)
 	if expected != result {
-		t.Errorf("userForSCC(%v, %v) returned %v but expected %v", namespace, name, result, expected)
+		t.Errorf("serviceAccountName(%v, %v) returned %v but expected %v", namespace, name, result, expected)
 	}
 }