From 84ab8ba24c280f3a10bbeba28fb6f03db0b8d06e Mon Sep 17 00:00:00 2001 From: Matthew Yacobucci Date: Sun, 14 Nov 2021 07:55:20 -0700 Subject: [PATCH 1/5] Adding CLI validation Validate input parameter. Users can input entire DOMAIN/NS/NAME path, or leave some elements off. Gateway will use defaults for missing elements. --- cmd/gateway/gateway_suite_test.go | 13 ++++ cmd/gateway/main.go | 20 ++++-- cmd/gateway/setup.go | 59 ++++++++++++++++ cmd/gateway/setup_test.go | 108 ++++++++++++++++++++++++++++++ go.mod | 2 + go.sum | 4 ++ 6 files changed, 200 insertions(+), 6 deletions(-) create mode 100644 cmd/gateway/gateway_suite_test.go create mode 100644 cmd/gateway/setup.go create mode 100644 cmd/gateway/setup_test.go diff --git a/cmd/gateway/gateway_suite_test.go b/cmd/gateway/gateway_suite_test.go new file mode 100644 index 0000000000..878d92fbdf --- /dev/null +++ b/cmd/gateway/gateway_suite_test.go @@ -0,0 +1,13 @@ +package main_test + +import ( + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" + + "testing" +) + +func TestGateway(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Gateway Suite") +} diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 39b182bd07..b774e05432 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -10,6 +10,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" ) +const ( + domain string = "k8s-gateway.nginx.org" +) + var ( // Set during go build version string @@ -17,23 +21,27 @@ var ( date string // Command-line flags - gatewayCtlrName = flag.String("gateway-ctlr-name", "", "The name of the Gateway controller") + gatewayCtlrName = flag.String("gateway-ctlr-name", "", "The name of the Gateway controller. The controller name should be of the form: DOMAIN/NAMESPACE/NAME. If omitted, DOMAIN will default to 'k8s-gateway.nginx.org'. If omitted, NAMESPACE will default to the current Pod namespace. NAME MUST be included.") ) func main() { flag.Parse() - if *gatewayCtlrName == "" { - flag.PrintDefaults() - os.Exit(1) - } - logger := zap.New() conf := config.Config{ GatewayCtlrName: *gatewayCtlrName, Logger: logger, } + valid := validateArguments( + logger, + GatewayControllerParam(true, "nginx-gateway" /* TODO dynamically set */, *gatewayCtlrName), + ) + if !valid { + flag.PrintDefaults() + os.Exit(1) + } + logger.Info("Starting NGINX Gateway", "version", version, "commit", commit, diff --git a/cmd/gateway/setup.go b/cmd/gateway/setup.go new file mode 100644 index 0000000000..51a019f763 --- /dev/null +++ b/cmd/gateway/setup.go @@ -0,0 +1,59 @@ +package main + +import ( + "errors" + "fmt" + "strings" + + "github.com/go-logr/logr" +) + +type Validator func() (bool, error) + +func GatewayControllerParam(required bool, namespace string, param string) Validator { + return func() (bool, error) { + if required && len(param) == 0 { + return false, errors.New("gateway-ctlr-name must have a value") + } + + fields := strings.Split(param, "/") + l := len(fields) + if l > 3 { + return false, fmt.Errorf("unsupported path length") + } + + switch len(fields) { + case 3: + if fields[0] != domain { + return false, fmt.Errorf("invalid domain: %s", domain) + } + fields = fields[1:] + fallthrough + case 2: + if fields[0] != namespace { + return false, fmt.Errorf("cannot cross namespace references: %s", namespace) + } + fields = fields[1:] + fallthrough + case 1: + if fields[0] == "" { + return false, fmt.Errorf("must provide a name") + } + } + + return true, nil + } +} + +func validateArguments(logger logr.Logger, validators ...Validator) bool { + valid := true + for _, v := range validators { + if r, err := v(); !r { + logger.Error(err, "failed validation") + if valid { + valid = !valid + } + } + } + return valid +} diff --git a/cmd/gateway/setup_test.go b/cmd/gateway/setup_test.go new file mode 100644 index 0000000000..0435e27ca2 --- /dev/null +++ b/cmd/gateway/setup_test.go @@ -0,0 +1,108 @@ +package main_test + +import ( + . "github.com/nginxinc/nginx-gateway-kubernetes/cmd/gateway" + + . "github.com/onsi/ginkgo" + . "github.com/onsi/gomega" +) + +var _ = Describe("Main", func() { + Describe("CLI argument validation", func() { + It("should parse full gateway-ctlr-name", func() { + gatewayCtlrName := "k8s-gateway.nginx.org/nginx-gateway/my-gateway" + + v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err := v() + Expect(r).To(BeTrue()) + Expect(err).ToNot(HaveOccurred()) + }) // should parse full gateway-ctlr-name + + It("should parse ns/name gateway-ctlr-name", func() { + gatewayCtlrName := "nginx-gateway/my-gateway" + + v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err := v() + Expect(r).To(BeTrue()) + Expect(err).ToNot(HaveOccurred()) + }) // should parse ns/name gateway-ctlr-name + + It("should parse name gateway-ctlr-name", func() { + gatewayCtlrName := "my-gateway" + + v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err := v() + Expect(r).To(BeTrue()) + Expect(err).ToNot(HaveOccurred()) + }) // should parse name gateway-ctlr-name + + It("should fail with too many path elements", func() { + gatewayCtlrName := "nginx.org/nginx/my-gateway/broken" + + v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err := v() + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) + }) // should fail with too many path elements + + It("should verify constraints", func() { + // bad domain + gatewayCtlrName := "invalid-domain/nginx-gateway/my-gateway" + + v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err := v() + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) + + // bad domain + gatewayCtlrName = "/default/my-gateway" + + v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err = v() + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) + + // bad namespace + gatewayCtlrName = "k8s-gateway.nginx.org/default/my-gateway" + + v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err = v() + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) + + // bad namespace + gatewayCtlrName = "k8s-gateway.nginx.org//my-gateway" + + v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err = v() + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) + + // bad name + gatewayCtlrName = "k8s-gateway.nginx.org/default/" + + v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + Expect(v).ToNot(BeNil()) + + r, err = v() + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) + }) // should verify constraints + }) // CLI argument validation +}) // end Main diff --git a/go.mod b/go.mod index 6207e993d6..50f83bc1dc 100644 --- a/go.mod +++ b/go.mod @@ -38,6 +38,7 @@ require ( github.com/google/gofuzz v1.1.0 // indirect github.com/google/uuid v1.1.2 // indirect github.com/googleapis/gnostic v0.5.5 // indirect + github.com/hashicorp/golang-lru v0.5.4 // indirect github.com/imdario/mergo v0.3.12 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect github.com/josharian/intern v1.0.0 // indirect @@ -55,6 +56,7 @@ require ( github.com/prometheus/common v0.28.0 // indirect github.com/prometheus/procfs v0.6.0 // indirect github.com/spf13/cobra v1.2.1 // indirect + github.com/spf13/pflag v1.0.5 // indirect go.uber.org/atomic v1.9.0 // indirect go.uber.org/multierr v1.7.0 // indirect go.uber.org/zap v1.19.1 // indirect diff --git a/go.sum b/go.sum index c947bf52a6..8df48e3c56 100644 --- a/go.sum +++ b/go.sum @@ -247,6 +247,7 @@ github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/ github.com/google/gofuzz v1.0.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/gofuzz v1.1.0 h1:Hsa8mG0dQ46ij8Sl2AYJDUv1oA9/d6Vk+3LG99Oe02g= github.com/google/gofuzz v1.1.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= +github.com/google/martian v2.1.0+incompatible h1:/CP5g8u/VJHijgedC/Legn3BAbAaWPgecwXBIDzw5no= github.com/google/martian v2.1.0+incompatible/go.mod h1:9I4somxYTbIHy5NJKHRl3wXiIaQGbYVAs8BPL6v8lEs= github.com/google/martian/v3 v3.0.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0= github.com/google/martian/v3 v3.1.0/go.mod h1:y5Zk1BBys9G+gd6Jrk0W3cC1+ELVxBWuIGO+w/tUAp0= @@ -287,6 +288,7 @@ github.com/hashicorp/consul/api v1.1.0/go.mod h1:VmuI/Lkw1nC05EYQWNKwWGbkg+FbDBt github.com/hashicorp/consul/sdk v0.1.1/go.mod h1:VKf9jXwCTEY1QZP2MOLRhb5i/I/ssyNV1vwHyQBF0x8= github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4= github.com/hashicorp/go-cleanhttp v0.5.1/go.mod h1:JpRdi6/HCYpAwUzNwuwqhbovhLtngrth3wmdIIUrZ80= +github.com/hashicorp/go-immutable-radix v1.0.0 h1:AKDB1HM5PWEA7i4nhcpwOrO2byshxBjXVn/J/3+z5/0= github.com/hashicorp/go-immutable-radix v1.0.0/go.mod h1:0y9vanUI8NX6FsYoO3zeMjhV/C5i9g4Q3DwcSNZ4P60= github.com/hashicorp/go-msgpack v0.5.3/go.mod h1:ahLV/dePpqEmjfWmKiqvPkv/twdG7iPBM1vqhUKIvfM= github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHhCYQXV3UM06sGGrk= @@ -294,10 +296,12 @@ github.com/hashicorp/go-rootcerts v1.0.0/go.mod h1:K6zTfqpRlCUIjkwsN4Z+hiSfzSTQa github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4= github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= +github.com/hashicorp/go-uuid v1.0.1 h1:fv1ep09latC32wFoVwnqcnKJGnMSdBanPczbHAYm1BE= github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go.net v0.0.1/go.mod h1:hjKkEWcCURg++eb33jQU7oqQcI9XDCnUzHA0oac0k90= github.com/hashicorp/golang-lru v0.5.0/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= github.com/hashicorp/golang-lru v0.5.1/go.mod h1:/m3WP610KZHVQ1SGc6re/UDhFvYD7pJ4Ao+sR/qLZy8= +github.com/hashicorp/golang-lru v0.5.4 h1:YDjusn29QI/Das2iO9M0BHnIbxPeyuCHsjMW+lJfyTc= github.com/hashicorp/golang-lru v0.5.4/go.mod h1:iADmTwqILo4mZ8BN3D2Q6+9jd8WM5uGBxy+E8yxSoD4= github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ= github.com/hashicorp/logutils v1.0.0/go.mod h1:QIAnNjmIWmVIIkWDTG1z5v++HQmx9WQRO+LraFDTW64= From dffca5cdeb19058dc8c3430fe0c46bd2977574d9 Mon Sep 17 00:00:00 2001 From: Matthew Yacobucci Date: Tue, 16 Nov 2021 10:37:56 -0700 Subject: [PATCH 2/5] Code review notes --- cmd/gateway/main.go | 6 +- cmd/gateway/setup.go | 40 ++++++------ cmd/gateway/setup_test.go | 129 +++++++++++++++++++++++++++++++------- 3 files changed, 129 insertions(+), 46 deletions(-) diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index b774e05432..1aed0727d5 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -21,7 +21,7 @@ var ( date string // Command-line flags - gatewayCtlrName = flag.String("gateway-ctlr-name", "", "The name of the Gateway controller. The controller name should be of the form: DOMAIN/NAMESPACE/NAME. If omitted, DOMAIN will default to 'k8s-gateway.nginx.org'. If omitted, NAMESPACE will default to the current Pod namespace. NAME MUST be included.") + gatewayCtlrName = flag.String("gateway-ctlr-name", "", "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'.") ) func main() { @@ -33,9 +33,9 @@ func main() { Logger: logger, } - valid := validateArguments( + valid := ValidateArguments( logger, - GatewayControllerParam(true, "nginx-gateway" /* TODO dynamically set */, *gatewayCtlrName), + GatewayControllerParam(true, domain, "nginx-gateway" /* TODO dynamically set */, *gatewayCtlrName), ) if !valid { flag.PrintDefaults() diff --git a/cmd/gateway/setup.go b/cmd/gateway/setup.go index 51a019f763..90c329c820 100644 --- a/cmd/gateway/setup.go +++ b/cmd/gateway/setup.go @@ -10,7 +10,7 @@ import ( type Validator func() (bool, error) -func GatewayControllerParam(required bool, namespace string, param string) Validator { +func GatewayControllerParam(required bool, domain string, namespace string, param string) Validator { return func() (bool, error) { if required && len(param) == 0 { return false, errors.New("gateway-ctlr-name must have a value") @@ -18,26 +18,26 @@ func GatewayControllerParam(required bool, namespace string, param string) Valid fields := strings.Split(param, "/") l := len(fields) - if l > 3 { - return false, fmt.Errorf("unsupported path length") + if l > 3 || l < 3 { + return false, fmt.Errorf("unsupported path length, must be form DOMAIN/NAMESPACE/NAME") } - switch len(fields) { - case 3: - if fields[0] != domain { - return false, fmt.Errorf("invalid domain: %s", domain) - } - fields = fields[1:] - fallthrough - case 2: - if fields[0] != namespace { - return false, fmt.Errorf("cannot cross namespace references: %s", namespace) - } - fields = fields[1:] - fallthrough - case 1: - if fields[0] == "" { - return false, fmt.Errorf("must provide a name") + for i := len(fields); i > 0; i-- { + switch i { + case 3: + if fields[0] != domain { + return false, fmt.Errorf("invalid domain: %s - %s", domain, param) + } + fields = fields[1:] + case 2: + if fields[0] != namespace { + return false, fmt.Errorf("cross namespace unsupported: %s - %s", namespace, param) + } + fields = fields[1:] + case 1: + if fields[0] == "" { + return false, fmt.Errorf("must provide a name: %s", param) + } } } @@ -45,7 +45,7 @@ func GatewayControllerParam(required bool, namespace string, param string) Valid } } -func validateArguments(logger logr.Logger, validators ...Validator) bool { +func ValidateArguments(logger logr.Logger, validators ...Validator) bool { valid := true for _, v := range validators { if r, err := v(); !r { diff --git a/cmd/gateway/setup_test.go b/cmd/gateway/setup_test.go index 0435e27ca2..551ea2b63a 100644 --- a/cmd/gateway/setup_test.go +++ b/cmd/gateway/setup_test.go @@ -1,18 +1,103 @@ package main_test import ( + "errors" + + "github.com/go-logr/logr" . "github.com/nginxinc/nginx-gateway-kubernetes/cmd/gateway" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" ) +var domain string + +func MockValidator(called *int, succeed bool) Validator { + return func() (bool, error) { + *called++ + + if !succeed { + return succeed, errors.New("Mock error") + } + return succeed, nil + } +} + var _ = Describe("Main", func() { + Describe("Generic Validator", func() { + It("should call all validators", func() { + var called int + table := []struct { + ExpectedCalls int + Success bool + Validators []Validator + }{ + { + 0, + true, + []Validator{}, + }, + { + 1, + true, + []Validator{ + MockValidator(&called, true), + }, + }, + { + 2, + true, + []Validator{ + MockValidator(&called, true), + MockValidator(&called, true), + }, + }, + { + 3, + true, + []Validator{ + MockValidator(&called, true), + MockValidator(&called, true), + MockValidator(&called, true), + }, + }, + { + 3, + false, + []Validator{ + MockValidator(&called, false), + MockValidator(&called, true), + MockValidator(&called, true), + }, + }, + { + 3, + false, + []Validator{ + MockValidator(&called, true), + MockValidator(&called, true), + MockValidator(&called, false), + }, + }, + } + + for i := range table { + called = 0 + ret := ValidateArguments(logr.Discard(), table[i].Validators...) + Expect(ret).To(Equal(table[i].Success)) + Expect(called).To(Equal(table[i].ExpectedCalls)) + } + }) // should call all validators + }) // Generic Validator + Describe("CLI argument validation", func() { + BeforeEach(func() { + domain = "k8s-gateway.nginx.org" + }) It("should parse full gateway-ctlr-name", func() { gatewayCtlrName := "k8s-gateway.nginx.org/nginx-gateway/my-gateway" - v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err := v() @@ -20,44 +105,42 @@ var _ = Describe("Main", func() { Expect(err).ToNot(HaveOccurred()) }) // should parse full gateway-ctlr-name - It("should parse ns/name gateway-ctlr-name", func() { - gatewayCtlrName := "nginx-gateway/my-gateway" + It("should fail with too many path elements", func() { + gatewayCtlrName := "k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken" - v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err := v() - Expect(r).To(BeTrue()) - Expect(err).ToNot(HaveOccurred()) - }) // should parse ns/name gateway-ctlr-name + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) + }) // should fail with too many path elements - It("should parse name gateway-ctlr-name", func() { - gatewayCtlrName := "my-gateway" + It("should fail with too few path elements", func() { + gatewayCtlrName := "nginx-gateway/my-gateway" - v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err := v() - Expect(r).To(BeTrue()) - Expect(err).ToNot(HaveOccurred()) - }) // should parse name gateway-ctlr-name + Expect(r).To(BeFalse()) + Expect(err).To(HaveOccurred()) - It("should fail with too many path elements", func() { - gatewayCtlrName := "nginx.org/nginx/my-gateway/broken" + gatewayCtlrName = "my-gateway" - v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) - r, err := v() + r, err = v() Expect(r).To(BeFalse()) Expect(err).To(HaveOccurred()) - }) // should fail with too many path elements + }) // should fail with too few path elements It("should verify constraints", func() { // bad domain gatewayCtlrName := "invalid-domain/nginx-gateway/my-gateway" - v := GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err := v() @@ -67,7 +150,7 @@ var _ = Describe("Main", func() { // bad domain gatewayCtlrName = "/default/my-gateway" - v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err = v() @@ -77,7 +160,7 @@ var _ = Describe("Main", func() { // bad namespace gatewayCtlrName = "k8s-gateway.nginx.org/default/my-gateway" - v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err = v() @@ -87,7 +170,7 @@ var _ = Describe("Main", func() { // bad namespace gatewayCtlrName = "k8s-gateway.nginx.org//my-gateway" - v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err = v() @@ -97,7 +180,7 @@ var _ = Describe("Main", func() { // bad name gatewayCtlrName = "k8s-gateway.nginx.org/default/" - v = GatewayControllerParam(true, "nginx-gateway", gatewayCtlrName) + v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) Expect(v).ToNot(BeNil()) r, err = v() From 35fa31b90243f675b459f48523f48f3f00078a3a Mon Sep 17 00:00:00 2001 From: Matthew Yacobucci Date: Wed, 17 Nov 2021 21:01:12 -0700 Subject: [PATCH 3/5] Code review notes - Making domain our API group - Removed superfluous 'required' arg to GatewayControllerParam - Single return Validator - Reworked argument validators - standardized output - no mixed structured and unstructured logging to stdout for arguments - added Must validator (exists on error) - param validator dynamically gets its value, only arguments are external constraints --- cmd/gateway/main.go | 19 ++-- cmd/gateway/setup.go | 106 +++++++++++++-------- cmd/gateway/setup_test.go | 189 +++++++++++++++++++++++--------------- 3 files changed, 195 insertions(+), 119 deletions(-) diff --git a/cmd/gateway/main.go b/cmd/gateway/main.go index 1aed0727d5..39e6afe9a5 100644 --- a/cmd/gateway/main.go +++ b/cmd/gateway/main.go @@ -1,6 +1,7 @@ package main import ( + "fmt" "os" "github.com/nginxinc/nginx-gateway-kubernetes/internal/config" @@ -11,7 +12,7 @@ import ( ) const ( - domain string = "k8s-gateway.nginx.org" + domain string = "gateway.nginx.org" ) var ( @@ -21,7 +22,11 @@ var ( date string // Command-line flags - gatewayCtlrName = flag.String("gateway-ctlr-name", "", "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'.") + 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'.", domain), + ) ) func main() { @@ -33,14 +38,10 @@ func main() { Logger: logger, } - valid := ValidateArguments( - logger, - GatewayControllerParam(true, domain, "nginx-gateway" /* TODO dynamically set */, *gatewayCtlrName), + MustValidateArguments( + flag.CommandLine, + GatewayControllerParam(domain, "nginx-gateway" /* TODO dynamically set */), ) - if !valid { - flag.PrintDefaults() - os.Exit(1) - } logger.Info("Starting NGINX Gateway", "version", version, diff --git a/cmd/gateway/setup.go b/cmd/gateway/setup.go index 90c329c820..761e9a3c85 100644 --- a/cmd/gateway/setup.go +++ b/cmd/gateway/setup.go @@ -3,57 +3,91 @@ package main import ( "errors" "fmt" + "os" "strings" - "github.com/go-logr/logr" + flag "github.com/spf13/pflag" ) -type Validator func() (bool, error) +const ( + errTmpl = "failed validation - flag: '--%s' reason: '%s'\n" +) -func GatewayControllerParam(required bool, domain string, namespace string, param string) Validator { - return func() (bool, error) { - if required && len(param) == 0 { - return false, errors.New("gateway-ctlr-name must have a value") - } +type Validator func(*flag.FlagSet) error +type ValidatorContext struct { + Key string + V Validator +} - fields := strings.Split(param, "/") - l := len(fields) - if l > 3 || l < 3 { - return false, fmt.Errorf("unsupported path length, must be form DOMAIN/NAMESPACE/NAME") - } +func GatewayControllerParam(domain string, namespace string) ValidatorContext { + name := "gateway-ctlr-name" + return ValidatorContext{ + name, + func(flagset *flag.FlagSet) error { + param, err := flagset.GetString(name) + if err != nil { + return err + } - for i := len(fields); i > 0; i-- { - switch i { - case 3: - if fields[0] != domain { - return false, fmt.Errorf("invalid domain: %s - %s", domain, param) - } - fields = fields[1:] - case 2: - if fields[0] != namespace { - return false, fmt.Errorf("cross namespace unsupported: %s - %s", namespace, param) - } - fields = fields[1:] - case 1: - if fields[0] == "" { - return false, fmt.Errorf("must provide a name: %s", param) + if len(param) == 0 { + return errors.New("flag must be set") + } + + fields := strings.Split(param, "/") + l := len(fields) + if l != 3 { + return fmt.Errorf("unsupported path length, must be form DOMAIN/NAMESPACE/NAME") + } + + 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 fmt.Errorf("must provide a name") + } } } - } - return true, nil + return nil + }, } } -func ValidateArguments(logger logr.Logger, validators ...Validator) bool { - valid := true +func ValidateArguments(flagset *flag.FlagSet, validators ...ValidatorContext) []string { + var msgs []string for _, v := range validators { - if r, err := v(); !r { - logger.Error(err, "failed validation") - if valid { - valid = !valid + if flagset.Lookup(v.Key) != nil { + err := v.V(flagset) + if err != nil { + msgs = append(msgs, fmt.Sprintf(errTmpl, v.Key, err.Error())) } } } - return valid + + return msgs +} + +func MustValidateArguments(flagset *flag.FlagSet, validators ...ValidatorContext) { + msgs := ValidateArguments(flagset, validators...) + if msgs != nil { + for i := range msgs { + fmt.Printf("%s", msgs[i]) + } + fmt.Println("") + + fmt.Printf("Usage of %s:\n", os.Args[0]) + flag.PrintDefaults() + + os.Exit(1) + } } diff --git a/cmd/gateway/setup_test.go b/cmd/gateway/setup_test.go index 551ea2b63a..a1707b6c7f 100644 --- a/cmd/gateway/setup_test.go +++ b/cmd/gateway/setup_test.go @@ -3,8 +3,8 @@ package main_test import ( "errors" - "github.com/go-logr/logr" . "github.com/nginxinc/nginx-gateway-kubernetes/cmd/gateway" + flag "github.com/spf13/pflag" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" @@ -12,179 +12,220 @@ import ( var domain string -func MockValidator(called *int, succeed bool) Validator { - return func() (bool, error) { - *called++ +func MockValidator(name string, called *int, succeed bool) ValidatorContext { + return ValidatorContext{ + name, + func(_ *flag.FlagSet) error { + *called++ - if !succeed { - return succeed, errors.New("Mock error") - } - return succeed, nil + if !succeed { + return errors.New("Mock error") + } + return nil + }, } } var _ = Describe("Main", func() { Describe("Generic Validator", func() { + var mockFlags *flag.FlagSet + BeforeEach(func() { + mockFlags = flag.NewFlagSet("mock", flag.PanicOnError) + _ = mockFlags.String("validator-1", "", "validator-1") + _ = mockFlags.String("validator-2", "", "validator-2") + _ = mockFlags.String("validator-3", "", "validator-3") + err := mockFlags.Parse([]string{}) + Expect(err).ToNot(HaveOccurred()) + }) + AfterEach(func() { + mockFlags = nil + }) It("should call all validators", func() { var called int table := []struct { ExpectedCalls int Success bool - Validators []Validator + Contexts []ValidatorContext }{ { 0, true, - []Validator{}, + []ValidatorContext{}, + }, + { + 0, + true, + []ValidatorContext{ + MockValidator("no-flag-set", &called, true), + }, }, { 1, true, - []Validator{ - MockValidator(&called, true), + []ValidatorContext{ + MockValidator("validator-1", &called, true), + }, + }, + { + 1, + true, + []ValidatorContext{ + MockValidator("no-flag-set", &called, true), + MockValidator("validator-1", &called, true), }, }, { 2, true, - []Validator{ - MockValidator(&called, true), - MockValidator(&called, true), + []ValidatorContext{ + MockValidator("validator-1", &called, true), + MockValidator("validator-2", &called, true), }, }, { 3, true, - []Validator{ - MockValidator(&called, true), - MockValidator(&called, true), - MockValidator(&called, true), + []ValidatorContext{ + MockValidator("validator-1", &called, true), + MockValidator("validator-2", &called, true), + MockValidator("validator-3", &called, true), }, }, { 3, false, - []Validator{ - MockValidator(&called, false), - MockValidator(&called, true), - MockValidator(&called, true), + []ValidatorContext{ + MockValidator("validator-1", &called, false), + MockValidator("validator-2", &called, true), + MockValidator("validator-3", &called, true), }, }, { 3, false, - []Validator{ - MockValidator(&called, true), - MockValidator(&called, true), - MockValidator(&called, false), + []ValidatorContext{ + MockValidator("validator-1", &called, true), + MockValidator("validator-2", &called, true), + MockValidator("validator-3", &called, false), }, }, } for i := range table { called = 0 - ret := ValidateArguments(logr.Discard(), table[i].Validators...) - Expect(ret).To(Equal(table[i].Success)) + msgs := ValidateArguments(mockFlags, table[i].Contexts...) + Expect(msgs == nil).To(Equal(table[i].Success)) Expect(called).To(Equal(table[i].ExpectedCalls)) } }) // should call all validators }) // Generic Validator Describe("CLI argument validation", func() { + var mockFlags *flag.FlagSet + var gatewayCtlrName string BeforeEach(func() { domain = "k8s-gateway.nginx.org" + gatewayCtlrName = "gateway-ctlr-name" + + mockFlags = flag.NewFlagSet("mock", flag.PanicOnError) + _ = mockFlags.String("gateway-ctlr-name", "", "mock gateway-ctlr-name") + err := mockFlags.Parse([]string{}) + Expect(err).ToNot(HaveOccurred()) + }) + AfterEach(func() { + mockFlags = nil }) It("should parse full gateway-ctlr-name", func() { - gatewayCtlrName := "k8s-gateway.nginx.org/nginx-gateway/my-gateway" + err := mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/nginx-gateway/my-gateway") + Expect(err).ToNot(HaveOccurred()) - v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v := GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err := v() - Expect(r).To(BeTrue()) + err = v.V(mockFlags) Expect(err).ToNot(HaveOccurred()) }) // should parse full gateway-ctlr-name It("should fail with too many path elements", func() { - gatewayCtlrName := "k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken" + err := mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken") + Expect(err).ToNot(HaveOccurred()) - v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v := GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err := v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) }) // should fail with too many path elements It("should fail with too few path elements", func() { - gatewayCtlrName := "nginx-gateway/my-gateway" + err := mockFlags.Set(gatewayCtlrName, "nginx-gateway/my-gateway") + Expect(err).ToNot(HaveOccurred()) - v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v := GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err := v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) - gatewayCtlrName = "my-gateway" + err = mockFlags.Set(gatewayCtlrName, "my-gateway") + Expect(err).ToNot(HaveOccurred()) - v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v = GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err = v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) }) // should fail with too few path elements It("should verify constraints", func() { // bad domain - gatewayCtlrName := "invalid-domain/nginx-gateway/my-gateway" + err := mockFlags.Set(gatewayCtlrName, "invalid-domain/nginx-gateway/my-gateway") + Expect(err).ToNot(HaveOccurred()) - v := GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v := GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err := v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) // bad domain - gatewayCtlrName = "/default/my-gateway" + err = mockFlags.Set(gatewayCtlrName, "/default/my-gateway") + Expect(err).ToNot(HaveOccurred()) - v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v = GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err = v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) // bad namespace - gatewayCtlrName = "k8s-gateway.nginx.org/default/my-gateway" + err = mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/default/my-gateway") + Expect(err).ToNot(HaveOccurred()) - v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v = GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err = v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) // bad namespace - gatewayCtlrName = "k8s-gateway.nginx.org//my-gateway" + err = mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org//my-gateway") + Expect(err).ToNot(HaveOccurred()) - v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v = GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err = v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) // bad name - gatewayCtlrName = "k8s-gateway.nginx.org/default/" + err = mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/default/") + Expect(err).ToNot(HaveOccurred()) - v = GatewayControllerParam(true, domain, "nginx-gateway", gatewayCtlrName) - Expect(v).ToNot(BeNil()) + v = GatewayControllerParam(domain, "nginx-gateway") + Expect(v.V).ToNot(BeNil()) - r, err = v() - Expect(r).To(BeFalse()) + err = v.V(mockFlags) Expect(err).To(HaveOccurred()) }) // should verify constraints }) // CLI argument validation From 4f02f52e9d41d8cfb12e3916888488c96fe56152 Mon Sep 17 00:00:00 2001 From: Matthew Yacobucci Date: Thu, 18 Nov 2021 14:23:34 -0700 Subject: [PATCH 4/5] FIXME --- cmd/gateway/setup.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cmd/gateway/setup.go b/cmd/gateway/setup.go index 761e9a3c85..974a6e1143 100644 --- a/cmd/gateway/setup.go +++ b/cmd/gateway/setup.go @@ -24,6 +24,8 @@ func GatewayControllerParam(domain string, namespace string) ValidatorContext { return ValidatorContext{ name, func(flagset *flag.FlagSet) error { + // FIXME(yacobucci) 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 From ed4ae7732c9808180f1bcd66862ecc0ce31ce8ad Mon Sep 17 00:00:00 2001 From: Matthew Yacobucci Date: Mon, 29 Nov 2021 18:37:10 -0700 Subject: [PATCH 5/5] Code review notes Table testing Stderr Squash --- cmd/gateway/setup.go | 10 +-- cmd/gateway/setup_test.go | 165 ++++++++++++++++++++------------------ 2 files changed, 90 insertions(+), 85 deletions(-) diff --git a/cmd/gateway/setup.go b/cmd/gateway/setup.go index 974a6e1143..9961153cfb 100644 --- a/cmd/gateway/setup.go +++ b/cmd/gateway/setup.go @@ -38,7 +38,7 @@ func GatewayControllerParam(domain string, namespace string) ValidatorContext { fields := strings.Split(param, "/") l := len(fields) if l != 3 { - return fmt.Errorf("unsupported path length, must be form DOMAIN/NAMESPACE/NAME") + return errors.New("unsupported path length, must be form DOMAIN/NAMESPACE/NAME") } for i := len(fields); i > 0; i-- { @@ -55,7 +55,7 @@ func GatewayControllerParam(domain string, namespace string) ValidatorContext { fields = fields[1:] case 1: if fields[0] == "" { - return fmt.Errorf("must provide a name") + return errors.New("must provide a name") } } } @@ -83,11 +83,11 @@ func MustValidateArguments(flagset *flag.FlagSet, validators ...ValidatorContext msgs := ValidateArguments(flagset, validators...) if msgs != nil { for i := range msgs { - fmt.Printf("%s", msgs[i]) + fmt.Fprintf(os.Stderr, "%s", msgs[i]) } - fmt.Println("") + fmt.Fprintln(os.Stderr, "") - fmt.Printf("Usage of %s:\n", os.Args[0]) + fmt.Fprintf(os.Stderr, "Usage of %s:\n", os.Args[0]) flag.PrintDefaults() os.Exit(1) diff --git a/cmd/gateway/setup_test.go b/cmd/gateway/setup_test.go index a1707b6c7f..51e050d894 100644 --- a/cmd/gateway/setup_test.go +++ b/cmd/gateway/setup_test.go @@ -121,8 +121,35 @@ var _ = Describe("Main", func() { }) // Generic Validator Describe("CLI argument validation", func() { + type testCase struct { + Param string + Domain string + ExpError bool + } + var mockFlags *flag.FlagSet var gatewayCtlrName string + + tester := func(t testCase) { + err := mockFlags.Set(gatewayCtlrName, t.Param) + Expect(err).ToNot(HaveOccurred()) + + v := GatewayControllerParam(domain, t.Domain) + Expect(v.V).ToNot(BeNil()) + + err = v.V(mockFlags) + if t.ExpError { + Expect(err).To(HaveOccurred()) + } else { + Expect(err).ToNot(HaveOccurred()) + } + } + runner := func(table []testCase) { + for i := range table { + tester(table[i]) + } + } + BeforeEach(func() { domain = "k8s-gateway.nginx.org" gatewayCtlrName = "gateway-ctlr-name" @@ -136,97 +163,75 @@ var _ = Describe("Main", func() { mockFlags = nil }) It("should parse full gateway-ctlr-name", func() { - err := mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/nginx-gateway/my-gateway") - Expect(err).ToNot(HaveOccurred()) - - v := GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) - - err = v.V(mockFlags) - Expect(err).ToNot(HaveOccurred()) + t := testCase{ + "k8s-gateway.nginx.org/nginx-gateway/my-gateway", + "nginx-gateway", + false, + } + tester(t) }) // should parse full gateway-ctlr-name It("should fail with too many path elements", func() { - err := mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken") - Expect(err).ToNot(HaveOccurred()) - - v := GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) - - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) + t := testCase{ + "k8s-gateway.nginx.org/nginx-gateway/my-gateway/broken", + "nginx-gateway", + true, + } + tester(t) }) // should fail with too many path elements It("should fail with too few path elements", func() { - err := mockFlags.Set(gatewayCtlrName, "nginx-gateway/my-gateway") - Expect(err).ToNot(HaveOccurred()) - - v := GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) - - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) - - err = mockFlags.Set(gatewayCtlrName, "my-gateway") - Expect(err).ToNot(HaveOccurred()) - - v = GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) + table := []testCase{ + { + Param: "nginx-gateway/my-gateway", + Domain: "nginx-gateway", + ExpError: true, + }, + { + Param: "my-gateway", + Domain: "nginx-gateway", + ExpError: true, + }, + } - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) + runner(table) }) // should fail with too few path elements It("should verify constraints", func() { - // bad domain - err := mockFlags.Set(gatewayCtlrName, "invalid-domain/nginx-gateway/my-gateway") - Expect(err).ToNot(HaveOccurred()) - - v := GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) - - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) - - // bad domain - err = mockFlags.Set(gatewayCtlrName, "/default/my-gateway") - Expect(err).ToNot(HaveOccurred()) - - v = GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) - - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) - - // bad namespace - err = mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/default/my-gateway") - Expect(err).ToNot(HaveOccurred()) - - v = GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) - - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) - - // bad namespace - err = mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org//my-gateway") - Expect(err).ToNot(HaveOccurred()) - - v = GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) - - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) - - // bad name - err = mockFlags.Set(gatewayCtlrName, "k8s-gateway.nginx.org/default/") - Expect(err).ToNot(HaveOccurred()) - - v = GatewayControllerParam(domain, "nginx-gateway") - Expect(v.V).ToNot(BeNil()) + table := []testCase{ + { + // bad domain + Param: "invalid-domain/nginx-gateway/my-gateway", + Domain: "nginx-gateway", + ExpError: true, + }, + { + // bad domain + Param: "/default/my-gateway", + Domain: "nginx-gateway", + ExpError: true, + }, + { + // bad namespace + Param: "k8s-gateway.nginx.org/default/my-gateway", + Domain: "nginx-gateway", + ExpError: true, + }, + { + // bad namespace + Param: "k8s-gateway.nginx.org//my-gateway", + Domain: "nginx-gateway", + ExpError: true, + }, + { + // bad name + Param: "k8s-gateway.nginx.org/default/", + Domain: "nginx-gateway", + ExpError: true, + }, + } - err = v.V(mockFlags) - Expect(err).To(HaveOccurred()) + runner(table) }) // should verify constraints }) // CLI argument validation }) // end Main