Refactor the role and rolebinding code again

Signed-off-by: Lubron Zhan <[email protected]>

design/automated-provisioning-design.md

@@ -60,7 +60,7 @@ It will handle the full provisioning lifecycle for these Gateways: creating a Co
 ![drawing](images/gatewayapi-provisioner-overview.png)
 
 Importantly, this provisioning model allows for (a) any number of GatewayClasses to be controlled by the Gateway provisioner; and (b) any number of Gateways per controlled GatewayClass.
-While the exact use cases for many Contour Gateways remain unclear, these one-to-many relationships between controller and GatewayClass, and GatewayClass and Gateway, offer users the full flexibility that the Gateway API spec intends.
+While the exact use cases for many Contour Gateways remain unclear, these one-to-many relationships between controller and GatewayClass, and GatewayClass and Gateway, offer users the full flexibility that the Gateway API spec intends. 
 Additionally, support for this one-to-many relationship is required in order to pass any of the Gateway API conformance tests.
 
 ![drawing](images/gatewayapi-resource-relationships.png)
@@ -77,7 +77,7 @@ Those users can continue to statically provision their Contour + Envoy instances
 
 There will be two ways to configure Contour for Gateway API support in the static provisioning scenario:
 - **Controller name** - this is the model implemented today, where Contour is configured with a controller name, and it continuously looks for the oldest GatewayClass with that controller, and the oldest Gateway using that GatewayClass. This model is appropriate for users who expect their GatewayClasses and Gateways to come and go, and who want their Contour instance to dynamically pick up the appropriate Gateway as those changes occur.
-- **Gateway name** - Contour can alternately be directly configured with a specific Gateway name, which avoids the multiple levels of indirection of the previous model. This model is appropriate for users who expect their Contour instance to correspond to a single static Gateway; the lifecycle of the Gateway and the lifecycle of the Contour instance are tied together.
+- **Gateway name** - Contour can alternately be directly configured with a specific Gateway name, which avoids the multiple levels of indirection of the previous model. This model is appropriate for users who expect their Contour instance to correspond to a single static Gateway; the lifecycle of the Gateway and the lifecycle of the Contour instance are tied together. 
 
 Note that the Gateway provisioner will make use of the **Gateway name** mode of configuring Contour, to tie each instance of Contour it provisions directly to a specific Gateway.
 
@@ -88,13 +88,13 @@ This custom resource definition embeds a ContourConfiguration spec, as well as a
 This ContourDeployment resource serves as a template that defines exactly how to customize each Contour + Envoy instance that is created for this GatewayClass.
 When a Gateway is provisioned, the Gateway provisioner will use the configuration options specified to customize the YAML that is applied, and will pass through a copy of the ContourConfiguration data to the Gateway’s Contour instance.
 
-Note that, according to the Gateway API documentation:
+Note that, according to the Gateway API documentation: 
 > It is recommended that [GatewayClass] be used as a template for Gateways.
 > This means that a Gateway is based on the state of the GatewayClass at the time it was created and changes to the GatewayClass or associated parameters are not propagated down to existing Gateways.
-> This recommendation is intended to limit the blast radius of changes to GatewayClass or associated parameters.
-(ref. [GatewayClass API reference documentation](https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.GatewayClass))
+> This recommendation is intended to limit the blast radius of changes to GatewayClass or associated parameters. 
+(ref. [GatewayClass API reference documentation](https://gateway-api.sigs.k8s.io/v1alpha2/references/spec/#gateway.networking.k8s.io/v1alpha2.GatewayClass)) 
 
-For Contour, this means that after a Gateway has been provisioned, the Gateway provisioner will not apply subsequent changes to the GatewayClass/ContourDeployment to it.
+For Contour, this means that after a Gateway has been provisioned, the Gateway provisioner will not apply subsequent changes to the GatewayClass/ContourDeployment to it.  
 This also means that Contour users can modify the ContourConfigurations used by their running Contours after instantiation, without having those changes overwritten by the Gateway provisioner.
 
 Since the Gateway provisioner supports multiple GatewayClasses, each GatewayClass can have a different ContourDeployment reference, corresponding to different sets of Gateway configuration profiles that the infrastructure provider offers (e.g. an external vs. internal profile).
@@ -109,7 +109,7 @@ This proposal is related to, but separate from, the managed Envoy proposal:
 - If we don’t implement managed Envoy, then the Gateway provisioner implements the Envoy provisioning logic.
 - Either way, the logic needs to be implemented and live somewhere.
 
-Advantages of doing managed Envoy:
+Advantages of doing managed Envoy: 
 - Users who don’t want automated Gateway provisioning, but do want automated Envoy provisioning, can have it
 - Users who don’t want to use Gateway API can still take advantage of automated Envoy provisioning
 - Listener programming (combo of Envoy service + Envoy listeners) can be done in one place
@@ -122,7 +122,7 @@ Disadvantages of doing managed Envoy:
 We considered continuing to invest in the Contour Operator, including the Contour CRD, with an eye towards bringing it to beta and eventually GA, and implementing the Gateway provisioner within the Operator (since they would share much of the underlying logic).
 Our first challenge with this option is that we have failed to establish a community of contributors around the Operator, so the work would need to be done by the core Contour team of maintainers, which would detract from Contour development.
 Our second challenge is that we have seen and heard of only limited usage of the Operator in the wild, so it’s not clear to us that continuing to develop it is an important priority for our users.
-Finally, continuing to maintain the Operator in a separate repository creates development overhead, since various bits of code and configuration must be manually kept in sync between Contour and the Operator.
+Finally, continuing to maintain the Operator in a separate repository creates development overhead, since various bits of code and configuration must be manually kept in sync between Contour and the Operator. 
 
 ### Alternative 2
 We also considered converting the existing Contour Operator into a Gateway provisioner, dropping the Contour CRD, and only supporting a Gateway API-based dynamic provisioning workflow.

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) {