OCPBUGS-65629: ensure canary daemon set uses its own service account.

pkg/manifests/assets/canary/cluster-role-binding.yaml

@@ -0,0 +1,18 @@
+# Binds the operator cluster role to its Service Account.
+kind: ClusterRoleBinding
+apiVersion: rbac.authorization.k8s.io/v1
+metadata:
+  name: openshift-ingress-canary
+  annotations:
+    capability.openshift.io/name: Ingress
+    include.release.openshift.io/ibm-cloud-managed: "true"
+    include.release.openshift.io/self-managed-high-availability: "true"
+    include.release.openshift.io/single-node-developer: "true"
+subjects:
+- kind: ServiceAccount
+  name: ingress-canary
+  namespace: openshift-ingress-canary
+roleRef:
+  kind: ClusterRole
+  apiGroup: rbac.authorization.k8s.io
+  name: openshift-ingress-operator

pkg/manifests/assets/canary/service-account.yaml

@@ -0,0 +1,12 @@
+# Account for the operator itself. It should require namespace scoped
+# permissions.
+kind: ServiceAccount
+apiVersion: v1
+metadata:
+  name: ingress-canary
+  namespace: openshift-ingress-canary
+  annotations:
+    capability.openshift.io/name: Ingress
+    include.release.openshift.io/ibm-cloud-managed: "true"
+    include.release.openshift.io/self-managed-high-availability: "true"
+    include.release.openshift.io/single-node-developer: "true"

pkg/manifests/manifests.go

@@ -37,10 +37,14 @@ const (
 	MetricsRoleAsset               = "assets/router/metrics/role.yaml"
 	MetricsRoleBindingAsset        = "assets/router/metrics/role-binding.yaml"
 
-	CanaryNamespaceAsset = "assets/canary/namespace.yaml"
-	CanaryDaemonSetAsset = "assets/canary/daemonset.yaml"
-	CanaryServiceAsset   = "assets/canary/service.yaml"
-	CanaryRouteAsset     = "assets/canary/route.yaml"
+	CanaryNamespaceAsset          = "assets/canary/namespace.yaml"
+	CanaryDaemonSetAsset          = "assets/canary/daemonset.yaml"
+	CanaryServiceAsset            = "assets/canary/service.yaml"
+	CanaryRouteAsset              = "assets/canary/route.yaml"
+	CanaryClusterRoleBindingAsset = "assets/canary/cluster-role-binding.yaml"
+	CanaryRoleBindingAsset        = "assets/canary/role-binding.yaml"
+	CanaryRoleAsset               = "assets/canary/role.yaml"
+	CanaryServiceAccountAsset     = "assets/canary/service-account.yaml"
 
 	GatewayClassCRDAsset            = "assets/gateway-api/gateway.networking.k8s.io_gatewayclasses.yaml"
 	GatewayCRDAsset                 = "assets/gateway-api/gateway.networking.k8s.io_gateways.yaml"
@@ -258,6 +262,39 @@ func CanaryRoute() *routev1.Route {
 	return route
 }
 
+func CanaryClusterRoleBinding() *rbacv1.ClusterRoleBinding {
+	clusterRoleBinding, err := NewClusterRoleBinding(MustAssetReader(CanaryClusterRoleBindingAsset))
+	if err != nil {
+		panic(err)
+	}
+	return clusterRoleBinding
+
+}
+
+func CanaryRoleBinding() *rbacv1.RoleBinding {
+	roleBinding, err := NewRoleBinding(MustAssetReader(CanaryClusterRoleBindingAsset))
+	if err != nil {
+		panic(err)
+	}
+	return roleBinding
+}
+
+func CanaryRole() *rbacv1.Role {
+	role, err := NewRole(MustAssetReader(CanaryRoleAsset))
+	if err != nil {
+		panic(err)
+	}
+	return role
+}
+
+func CanaryServiceAccount() *corev1.ServiceAccount {
+	serviceAccount, err := NewServiceAccount(MustAssetReader(CanaryServiceAccountAsset))
+	if err != nil {
+		panic(err)
+	}
+	return serviceAccount
+}
+
 func GatewayClassCRD() *apiextensionsv1.CustomResourceDefinition {
 	crd, err := NewCustomResourceDefinition(MustAssetReader(GatewayClassCRDAsset))
 	if err != nil {

pkg/operator/controller/canary/cluster_role_binding.go

@@ -0,0 +1,124 @@
+package canary
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+)
+
+// ensureCanaryClusterRoleBinding ensures the canary cluster role binding exists.
+func (r *reconciler) ensureCanaryClusterRoleBinding() (bool, *rbacv1.ClusterRoleBinding, error) {
+	desired := desiredCanaryClusterRoleBinding()
+	haveCrb, current, err := r.currentCanaryClusterRoleBinding()
+
+	if err != nil {
+		return false, nil, err
+	}
+
+	switch {
+	case !haveCrb:
+		if err := r.createCanaryClusterRoleBinding(desired); err != nil {
+			return false, nil, err
+		}
+		return r.currentCanaryClusterRoleBinding()
+	case haveCrb:
+		if updated, err := r.updateCanaryClusterRoleBinding(current, desired); err != nil {
+			return true, current, err
+		} else if updated {
+			return r.currentCanaryClusterRoleBinding()
+		}
+	}
+
+	return true, current, nil
+}
+
+// currentCanaryClusterRoleBinding returns the current cluster role binding.
+func (r *reconciler) currentCanaryClusterRoleBinding() (bool, *rbacv1.ClusterRoleBinding, error) {
+	clusterRoleBinding := &rbacv1.ClusterRoleBinding{}
+	if err := r.client.Get(context.TODO(), controller.CanaryClusterRoleBindingName(), clusterRoleBinding); err != nil {
+		if errors.IsNotFound(err) {
+			return false, nil, nil
+		}
+		return false, nil, err
+	}
+	return true, clusterRoleBinding, nil
+}
+
+// createCanaryClusterRoleBinding creates the given cluster role binding resource.
+func (r *reconciler) createCanaryClusterRoleBinding(clusterRoleBinding *rbacv1.ClusterRoleBinding) error {
+	if err := r.client.Create(context.TODO(), clusterRoleBinding); err != nil {
+		return fmt.Errorf("failed to create canary cluster role binding %s/%s: %v", clusterRoleBinding.Namespace, clusterRoleBinding.Name, err)
+	}
+	log.Info("created canary cluster role binding", "namespace", clusterRoleBinding.Namespace, "name", clusterRoleBinding.Name)
+	return nil
+}
+
+// updateCanaryClusterBinding updates the canary clusterRoleBinding if an appropriate
+// change has been detected.
+func (r *reconciler) updateCanaryClusterRoleBinding(current, desired *rbacv1.ClusterRoleBinding) (bool, error) {
+	changed, updated := canaryClusterRoleBindingChanged(current, desired)
+	if !changed {
+		return false, nil
+	}
+
+	diff := cmp.Diff(current, updated, cmpopts.EquateEmpty())
+	if err := r.client.Update(context.TODO(), updated); err != nil {
+		return false, fmt.Errorf("failed to update canary cluster role binding %s/%s: %v", updated.Namespace, updated.Name, err)
+	}
+	log.Info("updated canary cluster role binding", "namespace", updated.Namespace, "name", updated.Name, "diff", diff)
+	return true, nil
+}
+
+// desiredCanaryClusterRoleBinding returns the desired canary clusterRoleBinding
+// read in from manifests.
+func desiredCanaryClusterRoleBinding() *rbacv1.ClusterRoleBinding {
+	clusterRoleBinding := manifests.CanaryClusterRoleBinding()
+	name := controller.CanaryClusterRoleBindingName()
+	clusterRoleBinding.Name = name.Name
+	clusterRoleBinding.Namespace = name.Namespace
+
+	clusterRoleBinding.Labels = map[string]string{
+		// associate the cluster role binding with the ingress canary controller
+		manifests.OwningIngressCanaryCheckLabel: canaryControllerName,
+	}
+
+	return clusterRoleBinding
+}
+
+// canaryClusterRoleBindingChanged returns true if current and expected differ by the
+// binding's subjects.
+func canaryClusterRoleBindingChanged(current, expected *rbacv1.ClusterRoleBinding) (bool, *rbacv1.ClusterRoleBinding) {
+	if len(current.Subjects) == 0 && len(expected.Subjects) == 0 {
+		return false, nil
+	}
+
+	if len(current.Subjects) != len(expected.Subjects) {
+		return true, expected.DeepCopy()
+	}
+	currentSubjectsMap := make(map[string]rbacv1.Subject, len(current.Subjects))
+
+	for _, subject := range current.Subjects {
+		currentSubjectsMap[getSubjectKey(subject)] = subject
+	}
+
+	for _, subject := range expected.Subjects {
+		if _, exists := currentSubjectsMap[getSubjectKey(subject)]; !exists {
+			return true, expected.DeepCopy()
+		}
+	}
+
+	return false, nil
+}
+
+// getSubjectKey returns a unique, complete key for a subject object.
+// It is a helper function for canaryClusterRoleBindingChanged.
+func getSubjectKey(s rbacv1.Subject) string {
+	return fmt.Sprintf("%s/%s/%s/%s", s.Kind, s.APIGroup, s.Namespace, s.Name)
+}

pkg/operator/controller/canary/cluster_role_binding_test.go

@@ -0,0 +1,79 @@
+package canary
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/openshift/cluster-ingress-operator/pkg/manifests"
+	"github.com/openshift/cluster-ingress-operator/pkg/operator/controller"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+)
+
+func Test_desiredClusterRoleBinding(t *testing.T) {
+	clusterRoleBinding := desiredCanaryClusterRoleBinding()
+
+	expectedClusterRoleBindingName := controller.CanaryClusterRoleBindingName()
+
+	if !cmp.Equal(clusterRoleBinding.Name, expectedClusterRoleBindingName.Name) {
+		t.Errorf("expected clusterrolebinding name to be %s, but got %s", expectedClusterRoleBindingName.Name, clusterRoleBinding.Name)
+	}
+
+	if !cmp.Equal(clusterRoleBinding.Namespace, expectedClusterRoleBindingName.Namespace) {
+		t.Errorf("expected clusterrolebinding namespace to be %s, but got %s", expectedClusterRoleBindingName.Namespace, clusterRoleBinding.Namespace)
+	}
+
+	expectedLabels := map[string]string{
+		manifests.OwningIngressCanaryCheckLabel: canaryControllerName,
+	}
+
+	if !cmp.Equal(clusterRoleBinding.Labels, expectedLabels) {
+		t.Errorf("expected clusterrolebinding labels to be %q, but got %q", expectedLabels, clusterRoleBinding.Labels)
+	}
+}
+
+func Test_canaryClusterRoleBindingChanged(t *testing.T) {
+	testCases := []struct {
+		description string
+		mutate      func(*rbacv1.ClusterRoleBinding)
+		expect      bool
+	}{
+		{
+			description: "if nothing changes",
+			mutate:      func(_ *rbacv1.ClusterRoleBinding) {},
+			expect:      false,
+		},
+		{
+			description: "if subjects change",
+			mutate: func(crb *rbacv1.ClusterRoleBinding) {
+				crb.Subjects = []rbacv1.Subject{
+					{
+						Kind:      "test",
+						APIGroup:  "foo",
+						Name:      "bar",
+						Namespace: "foobar",
+					},
+				}
+			},
+			expect: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			original := desiredCanaryClusterRoleBinding()
+			mutated := original.DeepCopy()
+			tc.mutate(mutated)
+			if changed, updated := canaryClusterRoleBindingChanged(original, mutated); changed != tc.expect {
+				t.Errorf("expected canaryClusterRoleBindingChanged to be %t, got %t", tc.expect, changed)
+			} else if changed {
+				if updatedChanged, _ := canaryClusterRoleBindingChanged(original, updated); !updatedChanged {
+					t.Error("canaryClusterRoleBindingChanged reported changes but did not make any update")
+				}
+				if changedAgain, _ := canaryClusterRoleBindingChanged(mutated, updated); changedAgain {
+					t.Error("canaryClusterRoleBindingChanged does not behave as a fixed point function")
+				}
+			}
+		})
+	}
+}

pkg/operator/controller/canary/daemonset.go

@@ -97,6 +97,8 @@ func desiredCanaryDaemonSet(canaryImage string) *appsv1.DaemonSet {
 	daemonset.Spec.Template.Spec.Containers[0].Image = canaryImage
 	daemonset.Spec.Template.Spec.Containers[0].Command = []string{"ingress-operator", CanaryHealthcheckCommand}
 
+	daemonset.Spec.Template.Spec.ServiceAccountName = "ingress-canary"
+
 	return daemonset
 }
 
@@ -163,6 +165,11 @@ func canaryDaemonSetChanged(current, expected *appsv1.DaemonSet) (bool, *appsv1.
 		changed = true
 	}
 
+	if current.Spec.Template.Spec.ServiceAccountName != expected.Spec.Template.Spec.ServiceAccountName {
+		updated.Spec.Template.Spec.ServiceAccountName = expected.Spec.Template.Spec.ServiceAccountName
+		changed = true
+	}
+
 	if !changed {
 		return false, nil
 	}

pkg/operator/controller/canary/daemonset_test.go

@@ -83,6 +83,13 @@ func Test_desiredCanaryDaemonSet(t *testing.T) {
 	if !cmp.Equal(tolerations, expectedTolerations) {
 		t.Errorf("expected daemonset tolerations to be %v, but got %v", expectedTolerations, tolerations)
 	}
+
+	serviceAccountName := daemonset.Spec.Template.Spec.ServiceAccountName
+	expectedServiceAccountName := controller.CanaryServiceAccountName()
+
+	if !cmp.Equal(serviceAccountName, expectedServiceAccountName.Name) {
+		t.Errorf("expected service account name to be %s, but got %s", serviceAccountName, expectedServiceAccountName.Name)
+	}
 }
 
 func Test_canaryDaemonsetChanged(t *testing.T) {
@@ -225,6 +232,13 @@ func Test_canaryDaemonsetChanged(t *testing.T) {
 			},
 			expect: true,
 		},
+		{
+			description: "if canary service account name changes",
+			mutate: func(ds *appsv1.DaemonSet) {
+				ds.Spec.Template.Spec.ServiceAccountName = "default"
+			},
+			expect: true,
+		},
 	}
 
 	for _, tc := range testCases {