Resolve comments

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

Author: lubronzhan

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/model/names.go

@@ -67,7 +67,9 @@ func (c *Contour) ContourRBACNames() RBACNames {
 		Role:               fmt.Sprintf("contour-%s", c.Name),
 
 		// this one has a different prefix to differentiate from the certgen role binding (see below).
-		RoleBinding: fmt.Sprintf("contour-rolebinding-%s", c.Name),
+		RoleBinding:                        fmt.Sprintf("contour-rolebinding-%s", c.Name),
+		NamespaceScopedResourceRole:        fmt.Sprintf("contour-resource-%s-%s", c.Namespace, c.Name),
+		NamespaceScopedResourceRoleBinding: fmt.Sprintf("contour-resource-%s-%s", c.Namespace, c.Name),
 	}
 }
 
@@ -129,9 +131,11 @@ func (c *Contour) CommonAnnotations() map[string]string {
 
 // RBACNames holds a set of names of related RBAC resources.
 type RBACNames struct {
-	ServiceAccount     string
-	ClusterRole        string
-	ClusterRoleBinding string
-	Role               string
-	RoleBinding        string
+	ServiceAccount                     string
+	ClusterRole                        string
+	ClusterRoleBinding                 string
+	Role                               string
+	RoleBinding                        string
+	NamespaceScopedResourceRole        string
+	NamespaceScopedResourceRoleBinding string
 }

internal/provisioner/objects/rbac/rbac.go

@@ -79,10 +79,10 @@ func ensureContourRBAC(ctx context.Context, cli client.Client, contour *model.Co
 			ns = append(ns, contour.Namespace)
 		}
 		// Ensures role and rolebinding for namespaced scope resources in namespaces specified in contour.spec.watchNamespaces variable and contour's namespace
-		if err := role.EnsureRolesInNamespaces(ctx, cli, names.Role, contour, ns); err != nil {
+		if err := role.EnsureRolesInNamespaces(ctx, cli, names.NamespaceScopedResourceRole, contour, ns); err != nil {
 			return fmt.Errorf("failed to ensure roles in namespace %s: %w", contour.Spec.WatchNamespaces, err)
 		}
-		if err := rolebinding.EnsureRoleBindingsInNamespaces(ctx, cli, names.RoleBinding, names.ServiceAccount, names.Role, contour, ns); err != nil {
+		if err := rolebinding.EnsureRoleBindingsInNamespaces(ctx, cli, names.NamespaceScopedResourceRoleBinding, names.ServiceAccount, names.NamespaceScopedResourceRole, contour, ns); err != nil {
 			return fmt.Errorf("failed to ensure rolebindings in namespace %s: %w", contour.Spec.WatchNamespaces, err)
 		}
 	}

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

@@ -50,7 +50,7 @@ func EnsureControllerRole(ctx context.Context, cli client.Client, name string, c
 func EnsureRolesInNamespaces(ctx context.Context, cli client.Client, name string, contour *model.Contour, namespaces []string) error {
 	errs := []error{}
 	for _, ns := range namespaces {
-		desired := desiredRoleForResourceInNamespace(util.ContourResourceName(name), ns, contour)
+		desired := desiredRoleForResourceInNamespace(name, ns, contour)
 
 		updater := func(ctx context.Context, cli client.Client, current, desired *rbacv1.Role) error {
 			err := updateRoleIfNeeded(ctx, cli, contour, current, desired)

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

@@ -21,7 +21,6 @@ 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"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -49,7 +48,7 @@ func EnsureControllerRoleBinding(ctx context.Context, cli client.Client, name, s
 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(util.ContourResourceName(name), svcAct, util.ContourResourceName(role), ns, contour)
+		desired := desiredRoleBindingInNamespace(name, svcAct, role, ns, contour)
 
 		// Enclose contour.
 		updater := func(ctx context.Context, cli client.Client, current, desired *rbacv1.RoleBinding) error {

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

@@ -14,8 +14,6 @@
 package util
 
 import (
-	"fmt"
-
 	corev1 "k8s.io/api/core/v1"
 	discoveryv1 "k8s.io/api/discovery/v1"
 	networkingv1 "k8s.io/api/networking/v1"
@@ -79,8 +77,3 @@ func ClusterScopedResourcePolicyRules() []rbacv1.PolicyRule {
 		PolicyRuleFor(corev1.GroupName, getListWatch, "namespaces"),
 	}
 }
-
-// ContourResourceName returns names for roles/rolebindings for contour resources
-func ContourResourceName(name string) string {
-	return fmt.Sprintf("contour-resource-%s", name)
-}

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

@@ -542,7 +542,7 @@ var _ = Describe("Gateway provisioner", func() {
 				},
 			}, false))
 		})
-		Specify("A gateway with Envoy as a deployment can be provisioned, only routes in watch namespace can be reconciled", func() {
+		Specify("A gateway can be provisioned that only reconciles routes in a subset of namespaces", func() {
 			By("This tests deploy 3 dev namespaces testns-1, testns-2, testns-3")
 			By("Deploy gateway that referencing above gatewayclass")
 			gateway := &gatewayapi_v1beta1.Gateway{
@@ -620,6 +620,9 @@ var _ = Describe("Gateway provisioner", func() {
 				if t.expectReconcile {
 					// set route's parentRef's namespace to the gateway's namespace
 					route.Spec.CommonRouteSpec.ParentRefs[0].Namespace = (*gatewayapi_v1.Namespace)(&namespace)
+					// set the route's hostnames to custom name with namespace inside
+					route.Spec.Hostnames = []gatewayapi_v1beta1.Hostname{gatewayapi_v1beta1.Hostname("provisioner.projectcontour.io." + t.namespace)}
+
 					By(fmt.Sprintf("Expect namespace %s to be watched by contour", t.namespace))
 					hr, ok := f.CreateHTTPRouteAndWaitFor(route, e2e.HTTPRouteAccepted)
 					By(fmt.Sprintf("Expect httproute under namespace %s is accepted", t.namespace))
@@ -636,15 +639,22 @@ var _ = Describe("Gateway provisioner", func() {
 					body := f.GetEchoResponseBody(res.Body)
 					assert.Equal(f.T(), t.namespace, body.Namespace)
 					assert.Equal(f.T(), "echo", body.Service)
-
-					require.NoError(f.T(), f.DeleteHTTPRoute(route, true))
 				} else {
 					// Root proxy in non-watched namespace should fail
 					By(fmt.Sprintf("Expect namespace %s not to be watched by contour", t.namespace))
 					hr, ok := f.CreateHTTPRouteAndWaitFor(route, e2e.HTTPRouteIngnoredByContour)
 					By(fmt.Sprintf("Expect httproute under namespace %s is not accepted", t.namespace))
 					require.True(f.T(), ok, fmt.Sprintf("httproute's is %v", hr))
-					require.NoError(f.T(), f.DeleteHTTPRoute(route, true))
+
+					By(fmt.Sprintf("Expect httproute under namespace %s is not accepted for a period of time", t.namespace))
+					require.Never(f.T(), func() bool {
+						r := &gatewayapi_v1beta1.HTTPRoute{}
+						if err := f.Client.Get(context.Background(), k8s.NamespacedNameOf(hr), r); err != nil {
+							return false
+						}
+						return e2e.HTTPRouteAccepted(r)
+					}, 10*time.Second, time.Second)
+					require.True(f.T(), ok, fmt.Sprintf("httproute's is %v", hr))
 				}
 			}
 		})