Refactor the role and rolebinding code again

Signed-off-by: Lubron Zhan <lubronzhan@gmail.com>

internal/provisioner/objects/rbac/clusterrole/cluster_role.go

@@ -21,17 +21,10 @@ import (
 	"github.com/projectcontour/contour/internal/provisioner/labels"
 	"github.com/projectcontour/contour/internal/provisioner/model"
 	"github.com/projectcontour/contour/internal/provisioner/objects"
-	corev1 "k8s.io/api/core/v1"
-	discoveryv1 "k8s.io/api/discovery/v1"
-	networkingv1 "k8s.io/api/networking/v1"
+	"github.com/projectcontour/contour/internal/provisioner/objects/rbac/util"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/client"
-	gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
-)
-
-const (
-	contourV1GroupName = "projectcontour.io"
 )
 
 // EnsureClusterRole ensures a ClusterRole resource exists with the provided name
@@ -50,40 +43,7 @@ func EnsureClusterRole(ctx context.Context, cli client.Client, name string, cont
 // desiredClusterRole constructs an instance of the desired ClusterRole resource with
 // the provided name and contour namespace/name for the owning contour labels.
 func desiredClusterRole(name string, contour *model.Contour, gatewayClassOnly bool) *rbacv1.ClusterRole {
-	var (
-		createGetUpdate = []string{"create", "get", "update"}
-		getListWatch    = []string{"get", "list", "watch"}
-		update          = []string{"update"}
-	)
-
-	policyRuleFor := func(apiGroup string, verbs []string, resources ...string) rbacv1.PolicyRule {
-		return rbacv1.PolicyRule{
-			Verbs:     verbs,
-			APIGroups: []string{apiGroup},
-			Resources: resources,
-		}
-	}
-
-	if gatewayClassOnly {
-		return &rbacv1.ClusterRole{
-			TypeMeta: metav1.TypeMeta{
-				Kind: "Role",
-			},
-			ObjectMeta: metav1.ObjectMeta{
-				Name:        name,
-				Labels:      contour.CommonLabels(),
-				Annotations: contour.CommonAnnotations(),
-			},
-			Rules: []rbacv1.PolicyRule{
-				// Gateway API resources.
-				// Note, ReferenceGrant does not currently have a .status field so it's omitted from the status rule.
-				policyRuleFor(gatewayv1alpha2.GroupName, getListWatch, "gatewayclasses"),
-				policyRuleFor(gatewayv1alpha2.GroupName, update, "gatewayclasses/status"),
-			},
-		}
-	}
-
-	return &rbacv1.ClusterRole{
+	role := &rbacv1.ClusterRole{
 		TypeMeta: metav1.TypeMeta{
 			Kind: "Role",
 		},
@@ -92,27 +52,15 @@ func desiredClusterRole(name string, contour *model.Contour, gatewayClassOnly bo
 			Labels:      contour.CommonLabels(),
 			Annotations: contour.CommonAnnotations(),
 		},
-		Rules: []rbacv1.PolicyRule{
-			// Core Contour-watched resources.
-			policyRuleFor(corev1.GroupName, getListWatch, "secrets", "endpoints", "services", "namespaces"),
-
-			// Discovery Contour-watched resources.
-			policyRuleFor(discoveryv1.GroupName, getListWatch, "endpointslices"),
-
-			// Gateway API resources.
-			// Note, ReferenceGrant does not currently have a .status field so it's omitted from the status rule.
-			policyRuleFor(gatewayv1alpha2.GroupName, getListWatch, "gatewayclasses", "gateways", "httproutes", "tlsroutes", "grpcroutes", "tcproutes", "referencegrants"),
-			policyRuleFor(gatewayv1alpha2.GroupName, update, "gatewayclasses/status", "gateways/status", "httproutes/status", "tlsroutes/status", "grpcroutes/status", "tcproutes/status"),
-
-			// Ingress resources.
-			policyRuleFor(networkingv1.GroupName, getListWatch, "ingresses"),
-			policyRuleFor(networkingv1.GroupName, createGetUpdate, "ingresses/status"),
-
-			// Contour CRDs.
-			policyRuleFor(contourV1GroupName, getListWatch, "httpproxies", "tlscertificatedelegations", "extensionservices", "contourconfigurations"),
-			policyRuleFor(contourV1GroupName, createGetUpdate, "httpproxies/status", "extensionservices/status", "contourconfigurations/status"),
-		},
+		Rules: util.ClusterScopePolicyRulesForContour(),
+	}
+	if gatewayClassOnly {
+		return role
 	}
+
+	// add basic rules to role
+	role.Rules = append(role.Rules, util.BasicPolicyRulesForContour()...)
+	return role
 }
 
 // updateClusterRoleIfNeeded updates a ClusterRole resource if current does not match desired,

internal/provisioner/objects/rbac/rbac.go

@@ -54,25 +54,26 @@ func ensureContourRBAC(ctx context.Context, cli client.Client, contour *model.Co
 	}
 
 	// By default, Contour watches all namespaces, use default cluster role and rolebinding
+	clusterRoleForGatewayclassOnly := true
 	if contour.WatchAllNamespaces() {
 		// Ensure cluster role & binding.
-		if err := clusterrole.EnsureClusterRole(ctx, cli, names.ClusterRole, contour, false); err != nil {
+		if err := clusterrole.EnsureClusterRole(ctx, cli, names.ClusterRole, contour, !clusterRoleForGatewayclassOnly); err != nil {
 			return fmt.Errorf("failed to ensure cluster role %s: %w", names.ClusterRole, err)
 		}
 	} else {
 		// Ensure cluster role & cluster binding for gatewayclass first since it's cluster scope variables
-		if err := clusterrole.EnsureClusterRole(ctx, cli, names.ClusterRole, contour, true); err != nil {
+		if err := clusterrole.EnsureClusterRole(ctx, cli, names.ClusterRole, contour, clusterRoleForGatewayclassOnly); err != nil {
 			return fmt.Errorf("failed to ensure cluster role %s: %w", names.ClusterRole, err)
 		}
 
 		// Ensures role and rolebinding for set of namespaces in contour.spec.watchNamespaces variable
 		// Ensure cluster role & cluster binding for gatewayclass first since it's cluster scope variables
-		if err := role.EnsureRolesForNamespaces(ctx, cli, names.Role, contour); err != nil {
+		if err := role.EnsureRolesInNamespaces(ctx, cli, names.Role, contour, contour.Spec.WatchNamespaces); err != nil {
 			return fmt.Errorf("failed to ensure cluster role %s: %w", names.ClusterRole, err)
 		}
 		// Ensures role and rolebinding for set of namespaces in contour.spec.watchNamespaces variable
 		// Ensure cluster role & cluster binding for gatewayclass first since it's cluster scope variables
-		if err := rolebinding.EnsureRoleBindingsForNamespaces(ctx, cli, names.RoleBinding, names.ServiceAccount, names.Role, contour); err != nil {
+		if err := rolebinding.EnsureRoleBindingsInNamespaces(ctx, cli, names.RoleBinding, names.ServiceAccount, names.Role, contour, contour.Spec.WatchNamespaces); err != nil {
 			return fmt.Errorf("failed to ensure cluster role %s: %w", names.ClusterRole, err)
 		}
 	}

internal/provisioner/objects/rbac/role/role.go

@@ -21,10 +21,12 @@ import (
 	"github.com/projectcontour/contour/internal/provisioner/labels"
 	"github.com/projectcontour/contour/internal/provisioner/model"
 	"github.com/projectcontour/contour/internal/provisioner/objects"
+	"github.com/projectcontour/contour/internal/provisioner/objects/rbac/util"
 	coordinationv1 "k8s.io/api/coordination/v1"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -41,21 +43,29 @@ func EnsureControllerRole(ctx context.Context, cli client.Client, name string, c
 	return objects.EnsureObject(ctx, cli, desired, updater, &rbacv1.Role{})
 }
 
-// EnsureRolesForNamespaces ensures a Role resource exists with the for the Contour
-// controller.
-func EnsureRolesForNamespaces(ctx context.Context, cli client.Client, name string, contour *model.Contour) error {
-	desired := desiredControllerRole(name, contour)
+// EnsureRolesInNamespaces ensures a set of Role resources exist in namespaces
+// specified, for contour to manage resources under these namespaces. And
+// contour namespace/name for the owning contour labels for the Contour
+// controller
+func EnsureRolesInNamespaces(ctx context.Context, cli client.Client, name string, contour *model.Contour, namespaces []string) error {
+	errs := []error{}
+	for _, ns := range namespaces {
+		desired := desiredRoleForContourInNamespace(name, ns, contour)
 
-	updater := func(ctx context.Context, cli client.Client, current, desired *rbacv1.Role) error {
-		_, err := updateRoleIfNeeded(ctx, cli, contour, current, desired)
-		return err
+		updater := func(ctx context.Context, cli client.Client, current, desired *rbacv1.Role) error {
+			_, err := updateRoleIfNeeded(ctx, cli, contour, current, desired)
+			return err
+		}
+		if err := objects.EnsureObject(ctx, cli, desired, updater, &rbacv1.Role{}); err != nil {
+			errs = append(errs, err)
+		}
 	}
 
-	return objects.EnsureObject(ctx, cli, desired, updater, &rbacv1.Role{})
+	return kerrors.NewAggregate(errs)
 }
 
 // desiredControllerRole constructs an instance of the desired Role resource with the
-// provided ns/name and contour namespace/name for the owning contour labels for
+// provided ns/name and using contour namespace/name for the owning contour labels for
 // the Contour controller.
 func desiredControllerRole(name string, contour *model.Contour) *rbacv1.Role {
 	role := &rbacv1.Role{
@@ -85,6 +95,23 @@ func desiredControllerRole(name string, contour *model.Contour) *rbacv1.Role {
 	return role
 }
 
+// desiredRoleForContour constructs an instance of the desired Role resource with the
+// provided ns/name and using contour namespace/name for the corresponding Countour instance
+func desiredRoleForContourInNamespace(name, namespace string, contour *model.Contour) *rbacv1.Role {
+	return &rbacv1.Role{
+		TypeMeta: metav1.TypeMeta{
+			Kind: "Role",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        name,
+			Namespace:   contour.Namespace,
+			Labels:      contour.CommonLabels(),
+			Annotations: contour.CommonAnnotations(),
+		},
+		Rules: util.BasicPolicyRulesForContour(),
+	}
+}
+
 // updateRoleIfNeeded updates a Role resource if current does not match desired,
 // using contour to verify the existence of owner labels.
 func updateRoleIfNeeded(ctx context.Context, cli client.Client, contour *model.Contour, current, desired *rbacv1.Role) (*rbacv1.Role, error) {

internal/provisioner/objects/rbac/rolebinding/role_binding.go

@@ -21,18 +21,18 @@ import (
 	"github.com/projectcontour/contour/internal/provisioner/labels"
 	"github.com/projectcontour/contour/internal/provisioner/model"
 	"github.com/projectcontour/contour/internal/provisioner/objects"
-
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kerrors "k8s.io/apimachinery/pkg/util/errors"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 // EnsureRoleBinding ensures a RoleBinding resource exists with the provided
-// ns/name and contour namespace/name for the owning contour labels.
+// ns/name and using contour namespace/name for the owning contour labels.
 // The RoleBinding will use svcAct for the subject and role for the role reference.
 func EnsureRoleBinding(ctx context.Context, cli client.Client, name, svcAct, role string, contour *model.Contour) error {
-	desired := desiredRoleBinding(name, svcAct, role, contour)
+	desired := desiredRoleBindingInNamespace(name, svcAct, role, contour.Namespace, contour)
 
 	// Enclose contour.
 	updater := func(ctx context.Context, cli client.Client, current, desired *rbacv1.RoleBinding) error {
@@ -42,40 +42,45 @@ func EnsureRoleBinding(ctx context.Context, cli client.Client, name, svcAct, rol
 	return objects.EnsureObject(ctx, cli, desired, updater, &rbacv1.RoleBinding{})
 }
 
-// EnsureRoleBindingsForNamespaces ensures a RoleBinding resource exists with the provided
-// ns/name and contour namespace/name for the owning contour labels.
-// The RoleBinding will use svcAct for the subject and role for the role reference.
-func EnsureRoleBindingsForNamespaces(ctx context.Context, cli client.Client, name, svcAct, role string, contour *model.Contour) error {
-	desired := desiredRoleBinding(name, svcAct, role, contour)
+// EnsureRoleBindingsInNamespaces ensures a set of RoleBindings resource exist with the provided
+// namespaces/name using contour namespace/name for the owning contour labels.
+// The RoleBindings will use same svcAct for the subject and role for the role reference.
+func EnsureRoleBindingsInNamespaces(ctx context.Context, cli client.Client, name, svcAct, role string, contour *model.Contour, namespaces []string) error {
+	errs := []error{}
+	for _, ns := range namespaces {
+		desired := desiredRoleBindingInNamespace(name, svcAct, role, ns, contour)
 
-	// Enclose contour.
-	updater := func(ctx context.Context, cli client.Client, current, desired *rbacv1.RoleBinding) error {
-		return updateRoleBindingIfNeeded(ctx, cli, contour, current, desired)
+		// Enclose contour.
+		updater := func(ctx context.Context, cli client.Client, current, desired *rbacv1.RoleBinding) error {
+			return updateRoleBindingIfNeeded(ctx, cli, contour, current, desired)
+		}
+		objects.EnsureObject(ctx, cli, desired, updater, &rbacv1.RoleBinding{})
 	}
 
-	return objects.EnsureObject(ctx, cli, desired, updater, &rbacv1.RoleBinding{})
+	return kerrors.NewAggregate(errs)
 }
 
-// desiredRoleBinding constructs an instance of the desired RoleBinding resource
-// with the provided name in Contour spec Namespace, using contour namespace/name
+// desiredRoleBindingInNamespace constructs an instance of the desired RoleBinding resource
+// with the provided name in provided namespace, using contour namespace/name
 // for the owning contour labels. The RoleBinding will use svcAct for the subject
 // and role for the role reference.
-func desiredRoleBinding(name, svcAcctRef, roleRef string, contour *model.Contour) *rbacv1.RoleBinding {
+func desiredRoleBindingInNamespace(name, svcAcctRef, roleRef, namespace string, contour *model.Contour) *rbacv1.RoleBinding {
 	rb := &rbacv1.RoleBinding{
 		TypeMeta: metav1.TypeMeta{
 			Kind: "RoleBinding",
 		},
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace:   contour.Namespace,
+			Namespace:   namespace,
 			Name:        name,
 			Labels:      contour.CommonLabels(),
 			Annotations: contour.CommonAnnotations(),
 		},
 	}
 	rb.Subjects = []rbacv1.Subject{{
-		Kind:      "ServiceAccount",
-		APIGroup:  corev1.GroupName,
-		Name:      svcAcctRef,
+		Kind:     "ServiceAccount",
+		APIGroup: corev1.GroupName,
+		Name:     svcAcctRef,
+		// service account will be the same one
 		Namespace: contour.Namespace,
 	}}
 	rb.RoleRef = rbacv1.RoleRef{

internal/provisioner/objects/rbac/rolebinding/role_binding_test.go

@@ -33,6 +33,16 @@ func checkRoleBindingName(t *testing.T, rb *rbacv1.RoleBinding, expected string)
 	t.Errorf("role binding %q has unexpected name", rb.Name)
 }
 
+func checkRoleBindingNamespace(t *testing.T, rb *rbacv1.RoleBinding, expected string) {
+	t.Helper()
+
+	if rb.Namespace == expected {
+		return
+	}
+
+	t.Errorf("role binding %q has unexpected namespace", rb.Namespace)
+}
+
 func checkRoleBindingLabels(t *testing.T, rb *rbacv1.RoleBinding, expected map[string]string) {
 	t.Helper()
 
@@ -66,11 +76,13 @@ func checkRoleBindingRole(t *testing.T, rb *rbacv1.RoleBinding, expected string)
 func TestDesiredRoleBinding(t *testing.T) {
 	name := "job-test"
 	cntr := model.Default(fmt.Sprintf("%s-ns", name), name)
+	testns := "test-ns"
 	rbName := "test-rb"
 	svcAcct := "test-svc-acct-ref"
 	roleRef := "test-role-ref"
-	rb := desiredRoleBinding(rbName, svcAcct, roleRef, cntr)
+	rb := desiredRoleBindingInNamespace(rbName, svcAcct, roleRef, testns, cntr)
 	checkRoleBindingName(t, rb, rbName)
+	checkRoleBindingNamespace(t, rb, testns)
 	ownerLabels := map[string]string{
 		model.ContourOwningGatewayNameLabel:    cntr.Name,
 		model.GatewayAPIOwningGatewayNameLabel: cntr.Name,

internal/provisioner/objects/rbac/util/util.go

@@ -0,0 +1,74 @@
+// Copyright Project Contour Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package util
+
+import (
+	corev1 "k8s.io/api/core/v1"
+	discoveryv1 "k8s.io/api/discovery/v1"
+	networkingv1 "k8s.io/api/networking/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
+	gatewayv1alpha2 "sigs.k8s.io/gateway-api/apis/v1alpha2"
+)
+
+const (
+	ContourV1GroupName = "projectcontour.io"
+)
+
+var (
+	createGetUpdate = []string{"create", "get", "update"}
+	getListWatch    = []string{"get", "list", "watch"}
+	update          = []string{"update"}
+)
+
+// PolicyRuleFor returns PolicyRule object with provided apiGroup, verbs and resources
+func PolicyRuleFor(apiGroup string, verbs []string, resources ...string) rbacv1.PolicyRule {
+	return rbacv1.PolicyRule{
+		Verbs:     verbs,
+		APIGroups: []string{apiGroup},
+		Resources: resources,
+	}
+}
+
+// BasicPolicyRulesForContour returns set of basic rules that contour requires
+func BasicPolicyRulesForContour() []rbacv1.PolicyRule {
+	return []rbacv1.PolicyRule{
+		// Core Contour-watched resources.
+		PolicyRuleFor(corev1.GroupName, getListWatch, "secrets", "endpoints", "services", "namespaces"),
+
+		// Discovery Contour-watched resources.
+		PolicyRuleFor(discoveryv1.GroupName, getListWatch, "endpointslices"),
+
+		// Gateway API resources.
+		// Note, ReferenceGrant does not currently have a .status field so it's omitted from the status rule.
+		PolicyRuleFor(gatewayv1alpha2.GroupName, getListWatch, "gateways", "httproutes", "tlsroutes", "grpcroutes", "tcproutes", "referencegrants"),
+		PolicyRuleFor(gatewayv1alpha2.GroupName, update, "gateways/status", "httproutes/status", "tlsroutes/status", "grpcroutes/status", "tcproutes/status"),
+
+		// Ingress resources.
+		PolicyRuleFor(networkingv1.GroupName, getListWatch, "ingresses"),
+		PolicyRuleFor(networkingv1.GroupName, createGetUpdate, "ingresses/status"),
+
+		// Contour CRDs.
+		PolicyRuleFor(ContourV1GroupName, getListWatch, "httpproxies", "tlscertificatedelegations", "extensionservices", "contourconfigurations"),
+		PolicyRuleFor(ContourV1GroupName, createGetUpdate, "httpproxies/status", "extensionservices/status", "contourconfigurations/status"),
+	}
+}
+
+// ClusterScopePolicyRulesForContour returns set of rules only for cluster scope object
+func ClusterScopePolicyRulesForContour() []rbacv1.PolicyRule {
+	return []rbacv1.PolicyRule{
+		// GatewayClass only.
+		PolicyRuleFor(gatewayv1alpha2.GroupName, getListWatch, "gatewayclasses"),
+		PolicyRuleFor(gatewayv1alpha2.GroupName, update, "gatewayclasses/status"),
+	}
+}