Resolve FIXMEs for gateway-ctlr-name CLI arg (#235)

- Removes the requirement for namespace in gateway-ctlr-name CLI arg. The expected format
is now DOMAIN/PATH. Valid examples include:
"k8s-gateway.nginx.org/nginx-gateway-controller",
"k8s-gateway.nginx.org/nginx-gateway/nginx-gateway-controller", and
"k8s-gateway.nginx.org/nginx-gateway/nginx-gateway-controller/v2".

- Updates the validation of the argument to use the regex provided by the kubebuilder
annotation for GatewayClass.Spec.controllerName.

- Updates manifests to use the controller name "k8s-gateway.nginx.org/nginx-gateway-controller".

- Removes unused GatewayConfig CRD.

Co-authored-by: Michael Pleshakov <[email protected]>

Author: kate-osborn

cmd/gateway/main.go

@@ -12,8 +12,7 @@ import (
 )
 
 const (
-	domain    string = "k8s-gateway.nginx.org"
-	namespace string = "nginx-gateway"
+	domain string = "k8s-gateway.nginx.org"
 )
 
 var (
@@ -26,13 +25,14 @@ var (
 	gatewayCtlrName = flag.String(
 		"gateway-ctlr-name",
 		"",
-		fmt.Sprintf("The name of the Gateway controller. The controller name must be of the form: DOMAIN/NAMESPACE/NAME. The controller's domain is '%s'; the namespace is '%s'", domain, namespace),
+		fmt.Sprintf("The name of the Gateway controller. The controller name must be of the form: DOMAIN/PATH. The controller's domain is '%s'", domain),
 	)
 
 	gatewayClassName = flag.String(
 		"gatewayclass",
 		"",
-		"The name of the GatewayClass resource. Every NGINX Gateway must have a unique corresponding GatewayClass resource")
+		"The name of the GatewayClass resource. Every NGINX Gateway must have a unique corresponding GatewayClass resource",
+	)
 )
 
 func main() {
@@ -47,7 +47,7 @@ func main() {
 
 	MustValidateArguments(
 		flag.CommandLine,
-		GatewayControllerParam(domain, namespace /* FIXME(f5yacobucci) dynamically set */),
+		GatewayControllerParam(domain),
 		GatewayClassParam(),
 	)
 

cmd/gateway/setup.go

@@ -4,14 +4,19 @@ import (
 	"errors"
 	"fmt"
 	"os"
+	"regexp"
 	"strings"
 
 	flag "github.com/spf13/pflag"
 	"k8s.io/apimachinery/pkg/util/validation"
+
+	// Adding a dummy import here to remind us to check the controllerNameRegex when we update the Gateway API version.
+	_ "sigs.k8s.io/gateway-api/apis/v1beta1"
 )
 
 const (
-	errTmpl = "failed validation - flag: '--%s' reason: '%s'\n"
+	errTmpl             = "failed validation - flag: '--%s' reason: '%s'\n"
+	controllerNameRegex = `^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*\/[A-Za-z0-9\/\-._~%!$&'()*+,;=:]+$`
 )
 
 type (
@@ -22,13 +27,11 @@ type (
 	}
 )
 
-func GatewayControllerParam(domain string, namespace string) ValidatorContext {
+func GatewayControllerParam(domain string) ValidatorContext {
 	name := "gateway-ctlr-name"
 	return ValidatorContext{
 		name,
 		func(flagset *flag.FlagSet) error {
-			// FIXME(f5yacobucci) this does not provide the same regex validation as
-			// GatewayClass.ControllerName. provide equal and then specific validation
 			param, err := flagset.GetString(name)
 			if err != nil {
 				return err
@@ -40,34 +43,28 @@ func GatewayControllerParam(domain string, namespace string) ValidatorContext {
 
 			fields := strings.Split(param, "/")
 			l := len(fields)
-			if l != 3 {
-				return errors.New("unsupported path length, must be form DOMAIN/NAMESPACE/NAME")
+			if l < 2 {
+				return errors.New("invalid format; must be DOMAIN/PATH")
 			}
 
-			for i := len(fields); i > 0; i-- {
-				switch i {
-				case 3:
-					if fields[0] != domain {
-						return fmt.Errorf("invalid domain: %s", fields[0])
-					}
-					fields = fields[1:]
-				case 2:
-					if fields[0] != namespace {
-						return fmt.Errorf("cross namespace unsupported: %s", fields[0])
-					}
-					fields = fields[1:]
-				case 1:
-					if fields[0] == "" {
-						return errors.New("must provide a name")
-					}
-				}
+			if fields[0] != domain {
+				return fmt.Errorf("invalid domain: %s; domain must be: %s", fields[0], domain)
 			}
 
-			return nil
+			return validateControllerName(param)
 		},
 	}
 }
 
+func validateControllerName(name string) error {
+	// Regex from: https://github.com/kubernetes-sigs/gateway-api/blob/547122f7f55ac0464685552898c560658fb40073/apis/v1alpha2/shared_types.go#L462
+	re := regexp.MustCompile(controllerNameRegex)
+	if !re.MatchString(name) {
+		return fmt.Errorf("invalid gateway controller name: %s; expected format is DOMAIN/PATH", name)
+	}
+	return nil
+}
+
 func GatewayClassParam() ValidatorContext {
 	name := "gatewayclass"
 	return ValidatorContext{

cmd/gateway/setup_test.go

@@ -156,7 +156,7 @@ var _ = Describe("Main", func() {
 				return testCase{
 					Flag:             "gateway-ctlr-name",
 					Value:            value,
-					ValidatorContext: GatewayControllerParam("k8s-gateway.nginx.org", "nginx-gateway"),
+					ValidatorContext: GatewayControllerParam("k8s-gateway.nginx.org"),
 					ExpError:         expError,
 				}
 			}
@@ -171,29 +171,32 @@ var _ = Describe("Main", func() {
 				mockFlags = nil
 			})
 
-			It("should parse full gateway-ctlr-name", func() {
-				t := prepareTestCase(
-					"k8s-gateway.nginx.org/nginx-gateway/my-gateway",
-					expectSuccess,
-				)
-				tester(t)
-			}) // should parse full gateway-ctlr-name
-
-			It("should fail with too many path elements", func() {
-				t := prepareTestCase(
-					"k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken",
-					expectError)
-				tester(t)
-			}) // should fail with too many path elements
+			It("should parse valid gateway-ctlr-name", func() {
+				table := []testCase{
+					prepareTestCase(
+						"k8s-gateway.nginx.org/my-gateway",
+						expectSuccess,
+					),
+					prepareTestCase(
+						"k8s-gateway.nginx.org/nginx-gateway/my-gateway",
+						expectSuccess,
+					),
+					prepareTestCase(
+						"k8s-gateway.nginx.org/nginx-gateway/my-gateway/v1",
+						expectSuccess,
+					),
+				}
+				runner(table)
+			}) // should parse valid gateway-ctlr-name
 
 			It("should fail with too few path elements", func() {
 				table := []testCase{
 					prepareTestCase(
-						"nginx-gateway/my-gateway",
+						"k8s-gateway.nginx.org",
 						expectError,
 					),
 					prepareTestCase(
-						"my-gateway",
+						"k8s-gateway.nginx.org/",
 						expectError,
 					),
 				}
@@ -205,26 +208,17 @@ var _ = Describe("Main", func() {
 				table := []testCase{
 					prepareTestCase(
 						// bad domain
-						"invalid-domain/nginx-gateway/my-gateway",
+						"invalid-domain/my-gateway",
 						expectError,
 					),
 					prepareTestCase(
 						// bad domain
 						"/default/my-gateway",
 						expectError,
 					),
-					prepareTestCase(
-						// bad namespace
-						"k8s-gateway.nginx.org/default/my-gateway",
-						expectError),
-					prepareTestCase(
-						// bad namespace
-						"k8s-gateway.nginx.org//my-gateway",
-						expectError,
-					),
 					prepareTestCase(
 						// bad name
-						"k8s-gateway.nginx.org/default/",
+						"k8s-gateway.nginx.org/",
 						expectError,
 					),
 				}
@@ -266,7 +260,7 @@ var _ = Describe("Main", func() {
 					"$nginx",
 					expectError)
 				tester(t)
-			}) // should fail with invalid name"
+			}) // should fail with invalid name
 		}) // gatewayclass validation
 	}) // CLI argument validation
 }) // end Main

deploy/manifests/crds/gateway.nginx.org_gatewayconfigs.yaml

@@ -3,4 +3,4 @@ kind: GatewayClass
 metadata:
   name: nginx
 spec:
-  controllerName: k8s-gateway.nginx.org/nginx-gateway/gateway
+  controllerName: k8s-gateway.nginx.org/nginx-gateway-controller

deploy/manifests/gatewayclass.yaml

@@ -98,7 +98,7 @@ spec:
           # dropping ALL and adding only CAP_KILL doesn't work
           # Note: CAP_KILL is needed for sending HUP signal to NGINX main process
         args:
-        - --gateway-ctlr-name=k8s-gateway.nginx.org/nginx-gateway/gateway
+        - --gateway-ctlr-name=k8s-gateway.nginx.org/nginx-gateway-controller
         - --gatewayclass=nginx
       - image: nginx:1.23.1
         imagePullPolicy: IfNotPresent

deploy/manifests/gatewayconfig.yaml

@@ -4,5 +4,5 @@ The table below describes the command-line arguments of the `gateway` binary fro
 
 | Name | Type | Description |
 |-|-|-|
-|`gateway-ctlr-name` | `string` |  The name of the Gateway controller. The controller name must be of the form: `DOMAIN/NAMESPACE/NAME`. The controller's domain is `k8s-gateway.nginx.org`; the namespace is `nginx-ingress`. |
+|`gateway-ctlr-name` | `string` |  The name of the Gateway controller. The controller name must be of the form: `DOMAIN/PATH`. The controller's domain is `k8s-gateway.nginx.org`. |
 |`gatewayclass`| `string` | The name of the GatewayClass resource. Every NGINX Gateway must have a unique corresponding GatewayClass resource. |