Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Separate validating types & review feedback
Browse files Browse the repository at this point in the history
ciarams87 committed Aug 30, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent fa29023 commit 7cdc73b
Showing 6 changed files with 137 additions and 63 deletions.
70 changes: 9 additions & 61 deletions cmd/gateway/commands.go
Original file line number Diff line number Diff line change
@@ -39,68 +39,20 @@ var (
}

// Backing values for static subcommand cli flags.
updateGCStatus bool
disableMetrics bool
metricsListenPort int
metricsSecure bool
updateGCStatus bool
disableMetrics bool
metricsSecure bool

metricsListenPort = intValidatingValue{
validator: validatePort,
value: 9113,
}
gateway = namespacedNameValue{}
configName = stringValidatingValue{
validator: validateResourceName,
}
)

// stringValidatingValue is a string flag value with custom validation logic.
// it implements the pflag.Value interface.
type stringValidatingValue struct {
validator func(v string) error
value string
}

func (v *stringValidatingValue) String() string {
return v.value
}

func (v *stringValidatingValue) Set(param string) error {
if err := v.validator(param); err != nil {
return err
}
v.value = param
return nil
}

func (v *stringValidatingValue) Type() string {
return "string"
}

// namespacedNameValue is a string flag value that represents a namespaced name.
// it implements the pflag.Value interface.
type namespacedNameValue struct {
value types.NamespacedName
}

func (v *namespacedNameValue) String() string {
if (v.value == types.NamespacedName{}) {
// if we don't do that, the default value in the help message will be printed as "/"
return ""
}
return v.value.String()
}

func (v *namespacedNameValue) Set(param string) error {
nsname, err := parseNamespacedResourceName(param)
if err != nil {
return err
}

v.value = nsname
return nil
}

func (v *namespacedNameValue) Type() string {
return "string"
}

func createRootCommand() *cobra.Command {
rootCmd := &cobra.Command{
Use: "gateway",
@@ -158,11 +110,8 @@ func createStaticModeCommand() *cobra.Command {

metricsConfig := config.MetricsConfig{}
if !disableMetrics {
if err := validatePort(metricsListenPort); err != nil {
return fmt.Errorf("error validating metrics listen port: %w", err)
}
metricsConfig.Enabled = true
metricsConfig.Port = metricsListenPort
metricsConfig.Port = metricsListenPort.value
metricsConfig.Secure = metricsSecure
}

@@ -219,10 +168,9 @@ func createStaticModeCommand() *cobra.Command {
"Disable exposing metrics in the Prometheus format.",
)

cmd.Flags().IntVar(
cmd.Flags().Var(
&metricsListenPort,
"metrics-port",
9113,
"Set the port where the metrics are exposed. Format: [1023 - 65535]",
)

39 changes: 39 additions & 0 deletions cmd/gateway/commands_test.go
Original file line number Diff line number Diff line change
@@ -118,6 +118,9 @@ func TestStaticModeCmdFlagValidation(t *testing.T) {
"--gateway=nginx-gateway/nginx",
"--config=nginx-gateway-config",
"--update-gatewayclass-status=true",
"--metrics-port=9114",
"--metrics-disable",
"--metrics-secure-serving",
},
wantErr: false,
},
@@ -175,6 +178,42 @@ func TestStaticModeCmdFlagValidation(t *testing.T) {
wantErr: true,
expectedErrPrefix: `invalid argument "invalid" for "--update-gatewayclass-status" flag: strconv.ParseBool`,
},
{
name: "metrics-port is invalid type",
args: []string{
"--metrics-port=invalid", // not an int
},
wantErr: true,
expectedErrPrefix: `invalid argument "invalid" for "--metrics-port" flag: failed to parse int value:` +
` strconv.ParseInt: parsing "invalid": invalid syntax`,
},
{
name: "metrics-port is outside of range",
args: []string{
"--metrics-port=999", // outside of range
},
wantErr: true,
expectedErrPrefix: `invalid argument "999" for "--metrics-port" flag:` +
` port outside of valid port range [1024 - 65535]: 999`,
},
{
name: "metrics-disable is not a bool",
args: []string{
"--metrics-disable=999", // not a bool
},
wantErr: true,
expectedErrPrefix: `invalid argument "999" for "--metrics-disable" flag: strconv.ParseBool:` +
` parsing "999": invalid syntax`,
},
{
name: "metrics-secure-serving is not a bool",
args: []string{
"--metrics-secure-serving=999", // not a bool
},
wantErr: true,
expectedErrPrefix: `invalid argument "999" for "--metrics-secure-serving" flag: strconv.ParseBool:` +
` parsing "999": invalid syntax`,
},
}

for _, test := range tests {
86 changes: 86 additions & 0 deletions cmd/gateway/validating_types.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package main

import (
"fmt"
"strconv"

"k8s.io/apimachinery/pkg/types"
)

// stringValidatingValue is a string flag value with custom validation logic.
// it implements the pflag.Value interface.
type stringValidatingValue struct {
validator func(v string) error
value string
}

func (v *stringValidatingValue) String() string {
return v.value
}

func (v *stringValidatingValue) Set(param string) error {
if err := v.validator(param); err != nil {
return err
}
v.value = param
return nil
}

func (v *stringValidatingValue) Type() string {
return "string"
}

type intValidatingValue struct {
validator func(v int) error
value int
}

func (v *intValidatingValue) String() string {
return strconv.Itoa(v.value)
}

func (v *intValidatingValue) Set(param string) error {
intVal, err := strconv.ParseInt(param, 10, 32)
if err != nil {
return fmt.Errorf("failed to parse int value: %w", err)
}

if err := v.validator(int(intVal)); err != nil {
return err
}

v.value = int(intVal)
return nil
}

func (v *intValidatingValue) Type() string {
return "int"
}

// namespacedNameValue is a string flag value that represents a namespaced name.
// it implements the pflag.Value interface.
type namespacedNameValue struct {
value types.NamespacedName
}

func (v *namespacedNameValue) String() string {
if (v.value == types.NamespacedName{}) {
// if we don't do that, the default value in the help message will be printed as "/"
return ""
}
return v.value.String()
}

func (v *namespacedNameValue) Set(param string) error {
nsname, err := parseNamespacedResourceName(param)
if err != nil {
return err
}

v.value = nsname
return nil
}

func (v *namespacedNameValue) Type() string {
return "string"
}
2 changes: 1 addition & 1 deletion docs/architecture.md
Original file line number Diff line number Diff line change
@@ -96,7 +96,7 @@ parentheses. To enhance readability, the suffix "process" has been omitted from

1. (HTTPS) *NKG* reads the *Kubernetes API* to get the latest versions of the resources in the cluster and writes to the
API to update the handled resources' statuses and emit events.
2. (HTTP) *Prometheus* fetches the `controller-runtime` and NGINX metrics via an HTTP endpoint that *NKG* exposes.
2. (HTTP, HTTPS) *Prometheus* fetches the `controller-runtime` and NGINX metrics via an HTTP endpoint that *NKG* exposes.
The default is :9113/metrics. Note: Prometheus is not required by NKG, the endpoint can be turned off.
3. (File I/O)
- Write: *NKG* generates NGINX *configuration* based on the cluster resources and writes them as `.conf` files to the
1 change: 1 addition & 0 deletions internal/mode/static/nginx/runtime/manager.go
Original file line number Diff line number Diff line change
@@ -70,6 +70,7 @@ func (m *ManagerImpl) Reload(ctx context.Context) error {
return nil
}

// EnsureNginxRunning ensures NGINX is running by locating the main process.
func EnsureNginxRunning(ctx context.Context) error {
if _, err := findMainProcess(ctx, os.Stat, os.ReadFile, pidFileTimeout); err != nil {
return fmt.Errorf("failed to find NGINX main process: %w", err)
2 changes: 1 addition & 1 deletion internal/mode/static/state/graph/gateway_listener_test.go
Original file line number Diff line number Diff line change
@@ -114,7 +114,7 @@ func TestValidateHTTPSListener(t *testing.T) {
l: v1beta1.Listener{
Port: 9113,
TLS: &v1beta1.GatewayTLSConfig{
Mode: helpers.GetTLSModePointer(v1beta1.TLSModeTerminate),
Mode: helpers.GetPointer(v1beta1.TLSModeTerminate),
CertificateRefs: []v1beta1.SecretObjectReference{validSecretRef},
},
},

0 comments on commit 7cdc73b

Please sign in to comment.