diff --git a/pkg/manifests/assets/canary/cluster-role-binding.yaml b/pkg/manifests/assets/canary/cluster-role-binding.yaml new file mode 100644 index 0000000000..a39247b5d8 --- /dev/null +++ b/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 \ No newline at end of file diff --git a/pkg/manifests/assets/canary/service-account.yaml b/pkg/manifests/assets/canary/service-account.yaml new file mode 100644 index 0000000000..4531bcfc6b --- /dev/null +++ b/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" \ No newline at end of file diff --git a/pkg/manifests/manifests.go b/pkg/manifests/manifests.go index 76c3f26ec4..c73d650dcd 100644 --- a/pkg/manifests/manifests.go +++ b/pkg/manifests/manifests.go @@ -37,10 +37,12 @@ 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" + 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 +260,23 @@ func CanaryRoute() *routev1.Route { return route } +func CanaryClusterRoleBinding() *rbacv1.ClusterRoleBinding { + clusterRoleBinding, err := NewClusterRoleBinding(MustAssetReader(CanaryClusterRoleBindingAsset)) + if err != nil { + panic(err) + } + return clusterRoleBinding + +} + +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 { diff --git a/pkg/operator/controller/canary/cluster_role_binding.go b/pkg/operator/controller/canary/cluster_role_binding.go new file mode 100644 index 0000000000..5b059d0a19 --- /dev/null +++ b/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) +} diff --git a/pkg/operator/controller/canary/cluster_role_binding_test.go b/pkg/operator/controller/canary/cluster_role_binding_test.go new file mode 100644 index 0000000000..3c1f2d57eb --- /dev/null +++ b/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") + } + } + }) + } +} diff --git a/pkg/operator/controller/canary/controller.go b/pkg/operator/controller/canary/controller.go index a9096f2e7f..473b23c948 100644 --- a/pkg/operator/controller/canary/controller.go +++ b/pkg/operator/controller/canary/controller.go @@ -188,6 +188,20 @@ func (r *reconciler) Reconcile(ctx context.Context, request reconcile.Request) ( // resource creation in a namespace that does not exist will fail. return result, fmt.Errorf("failed to ensure canary namespace: %v", err) } + // Cluster Role Binding + haveCrb, _, err := r.ensureCanaryClusterRoleBinding() + if err != nil { + return result, fmt.Errorf("failed to ensure canary cluster role binding: %v", err) + } else if !haveCrb { + return result, fmt.Errorf("failed to get canary cluster role binding: %v", err) + } + // Service Account + haveSa, _, err := r.ensureCanaryServiceAccount() + if err != nil { + return result, fmt.Errorf("failed to ensure canary service account: %v", err) + } else if !haveSa { + return result, fmt.Errorf("failed to get canary cluster role binding: %v", err) + } haveDs, daemonset, err := r.ensureCanaryDaemonSet() if err != nil { diff --git a/pkg/operator/controller/canary/daemonset.go b/pkg/operator/controller/canary/daemonset.go index ea477b4b13..d48c87f1e5 100644 --- a/pkg/operator/controller/canary/daemonset.go +++ b/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 } diff --git a/pkg/operator/controller/canary/daemonset_test.go b/pkg/operator/controller/canary/daemonset_test.go index d29bc5afca..d6f405b478 100644 --- a/pkg/operator/controller/canary/daemonset_test.go +++ b/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 { diff --git a/pkg/operator/controller/canary/service_account.go b/pkg/operator/controller/canary/service_account.go new file mode 100644 index 0000000000..4930c271be --- /dev/null +++ b/pkg/operator/controller/canary/service_account.go @@ -0,0 +1,123 @@ +package canary + +import ( + "context" + "fmt" + "reflect" + + "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" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" +) + +// ensureCurrentCanaryServiceAccount ensures the canary service account exists. +func (r *reconciler) ensureCanaryServiceAccount() (bool, *corev1.ServiceAccount, error) { + desired := desiredCanaryServiceAccount() + haveSa, current, err := r.currentCanaryServiceAccount() + + if err != nil { + return false, nil, err + } + + switch { + case !haveSa: + if err := r.createCanaryServiceAccount(desired); err != nil { + return false, nil, err + } + return r.currentCanaryServiceAccount() + case haveSa: + if updated, err := r.updateCanaryServiceAccount(current, desired); err != nil { + return true, current, err + } else if updated { + return r.currentCanaryServiceAccount() + } + } + return true, current, nil +} + +// currentCanaryServiceAccount returns the current service account. +func (r *reconciler) currentCanaryServiceAccount() (bool, *corev1.ServiceAccount, error) { + serviceAccount := &corev1.ServiceAccount{} + if err := r.client.Get(context.TODO(), controller.CanaryServiceAccountName(), serviceAccount); err != nil { + if errors.IsNotFound(err) { + return false, nil, nil + } + return false, nil, err + } + return true, serviceAccount, nil +} + +// createCanaryServiceAccount creates the given service account resource. +func (r *reconciler) createCanaryServiceAccount(serviceAccount *corev1.ServiceAccount) error { + if err := r.client.Create(context.TODO(), serviceAccount); err != nil { + return fmt.Errorf("failed to create canary service account %s/%s: %w", serviceAccount.Namespace, serviceAccount.Name, err) + } + log.Info("created canary service account", "namespace", serviceAccount.Namespace, "name", serviceAccount.Name) + return nil +} + +// updateCanaryServiceaccount updates the canary ServiceAccount if an appropriate +// change has been detected. +func (r *reconciler) updateCanaryServiceAccount(current, desired *corev1.ServiceAccount) (bool, error) { + changed, updated := canaryServiceAccountChanged(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 service account %s/%s: %v", updated.Namespace, updated.Name, err) + } + log.Info("updated canary service account", "namespace", updated.Namespace, "name", updated.Name, "diff", diff) + + return true, nil +} + +// desiredServiceAccount returns the desired canary ServiceAccount +// read in from manifests. +func desiredCanaryServiceAccount() *corev1.ServiceAccount { + serviceAccount := manifests.CanaryServiceAccount() + name := controller.CanaryServiceAccountName() + serviceAccount.Name = name.Name + serviceAccount.Namespace = name.Namespace + + serviceAccount.Labels = map[string]string{ + // associate the service account with the ingress canary controller + manifests.OwningIngressCanaryCheckLabel: canaryControllerName, + } + + return serviceAccount +} + +// canaryServiceAccountChanged returns true if current and expected differ by the +// service account's secrets, image pull secrets, and/or +// AutomountServiceAccountToken. +func canaryServiceAccountChanged(current, expected *corev1.ServiceAccount) (bool, *corev1.ServiceAccount) { + currentSpec := SASpecComparison{ + Secrets: current.Secrets, + ImagePullSecrets: current.ImagePullSecrets, + AutomountServiceAccountToken: current.AutomountServiceAccountToken, + } + + expectedSpec := SASpecComparison{ + Secrets: expected.Secrets, + ImagePullSecrets: expected.ImagePullSecrets, + AutomountServiceAccountToken: expected.AutomountServiceAccountToken, + } + + if reflect.DeepEqual(currentSpec, expectedSpec) { + return false, nil + } + + return true, expected.DeepCopy() +} + +// SASpecComparison is a helper struct for comparing two service account objects. +type SASpecComparison struct { + Secrets []corev1.ObjectReference + ImagePullSecrets []corev1.LocalObjectReference + AutomountServiceAccountToken *bool +} diff --git a/pkg/operator/controller/canary/service_account_test.go b/pkg/operator/controller/canary/service_account_test.go new file mode 100644 index 0000000000..654eff65c9 --- /dev/null +++ b/pkg/operator/controller/canary/service_account_test.go @@ -0,0 +1,103 @@ +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" + + corev1 "k8s.io/api/core/v1" +) + +func Test_desiredServiceAccount(t *testing.T) { + serviceAccount := desiredCanaryServiceAccount() + + expectedServiceAccountName := controller.CanaryServiceAccountName() + + if !cmp.Equal(serviceAccount.Name, expectedServiceAccountName.Name) { + t.Errorf("expected service account name to be %s, but got %s", expectedServiceAccountName.Name, serviceAccount.Name) + } + + if !cmp.Equal(serviceAccount.Namespace, expectedServiceAccountName.Namespace) { + t.Errorf("expected service account namespace to be %s, but got %s", expectedServiceAccountName.Namespace, serviceAccount.Namespace) + } + + expectedLabels := map[string]string{ + manifests.OwningIngressCanaryCheckLabel: canaryControllerName, + } + + if !cmp.Equal(serviceAccount.Labels, expectedLabels) { + t.Errorf("expected service account labels to be %q, but got %q", expectedLabels, serviceAccount.Labels) + } +} + +func Test_canaryServiceAccountChanged(t *testing.T) { + testCases := []struct { + description string + mutate func(*corev1.ServiceAccount) + expect bool + }{ + { + description: "if nothing changes", + mutate: func(_ *corev1.ServiceAccount) {}, + expect: false, + }, + { + description: "if secrets change", + mutate: func(sa *corev1.ServiceAccount) { + sa.Secrets = []corev1.ObjectReference{ + { + Kind: "foo", + FieldPath: "foo/bar", + }, + } + }, + expect: true, + }, + { + description: "if image pull secrets change", + mutate: func(sa *corev1.ServiceAccount) { + sa.ImagePullSecrets = []corev1.LocalObjectReference{ + { + Name: "foo", + }, + } + }, + expect: true, + }, + { + description: "if auto mount service account token changes", + mutate: func(sa *corev1.ServiceAccount) { + val := true + if sa.AutomountServiceAccountToken == nil { + sa.AutomountServiceAccountToken = &val + } else { + val = *sa.AutomountServiceAccountToken + invertVal := !val + sa.AutomountServiceAccountToken = &invertVal + } + }, + expect: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.description, func(t *testing.T) { + original := desiredCanaryServiceAccount() + mutated := original.DeepCopy() + tc.mutate(mutated) + if changed, updated := canaryServiceAccountChanged(original, mutated); changed != tc.expect { + t.Errorf("expected canaryServiceAccountChanged to be %t, but got %t", tc.expect, changed) + } else if changed { + if updatedChanged, _ := canaryServiceAccountChanged(original, updated); !updatedChanged { + t.Error("canaryServiceAccountChanged reported changes but did not make any update") + } + if changedAgain, _ := canaryServiceAccountChanged(mutated, updated); changedAgain { + t.Error("canaryServiceAccountChanged does not behave as a fixed point function") + } + } + }) + } +} diff --git a/pkg/operator/controller/names.go b/pkg/operator/controller/names.go index 749bbf8a23..98f60fca58 100644 --- a/pkg/operator/controller/names.go +++ b/pkg/operator/controller/names.go @@ -292,6 +292,20 @@ func CanaryRouteName() types.NamespacedName { } } +func CanaryClusterRoleBindingName() types.NamespacedName { + return types.NamespacedName{ + Namespace: DefaultCanaryNamespace, + Name: "openshift-ingress-canary", + } +} + +func CanaryServiceAccountName() types.NamespacedName { + return types.NamespacedName{ + Namespace: DefaultCanaryNamespace, + Name: "ingress-canary", + } +} + func IngressClassName(ingressControllerName string) types.NamespacedName { return types.NamespacedName{Name: "openshift-" + ingressControllerName} }