From 3c66f9a367a8ac25fcff4b9b826e7570299e1c69 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Tue, 13 Feb 2024 11:27:13 -0800 Subject: [PATCH 01/12] Add initial draft of flag options --- cmd/gateway/commands.go | 1 + go.mod | 2 +- internal/mode/static/config/config.go | 3 + internal/mode/static/manager.go | 1 + internal/mode/static/telemetry/collector.go | 101 +++++++++++++++++--- 5 files changed, 94 insertions(+), 14 deletions(-) diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 32e819c219..6d987a554b 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -215,6 +215,7 @@ func createStaticModeCommand() *cobra.Command { Version: version, ExperimentalFeatures: gwExperimentalFeatures, ImageSource: imageSource, + Flags: cmd.Flags(), } if err := static.StartManager(conf); err != nil { diff --git a/go.mod b/go.mod index f43c39f192..2ce3aa38c6 100644 --- a/go.mod +++ b/go.mod @@ -17,6 +17,7 @@ require ( github.com/prometheus/client_golang v1.18.0 github.com/prometheus/common v0.48.0 github.com/spf13/cobra v1.8.0 + github.com/spf13/pflag v1.0.5 github.com/tsenart/vegeta/v12 v12.11.1 go.uber.org/zap v1.27.0 k8s.io/api v0.29.2 @@ -71,7 +72,6 @@ require ( github.com/prometheus/client_model v0.5.0 // indirect github.com/prometheus/procfs v0.12.0 // indirect github.com/rs/dnscache v0.0.0-20211102005908-e0241e321417 // indirect - github.com/spf13/pflag v1.0.5 // indirect github.com/stretchr/testify v1.8.4 // indirect go.uber.org/multierr v1.11.0 // indirect golang.org/x/exp v0.0.0-20231006140011-7918f672742d // indirect diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index 5f93a3e3d6..c3df848314 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -4,6 +4,7 @@ import ( "time" "github.com/go-logr/logr" + "github.com/spf13/pflag" "go.uber.org/zap" "k8s.io/apimachinery/pkg/types" ) @@ -15,6 +16,8 @@ type Config struct { ImageSource string // AtomicLevel is an atomically changeable, dynamic logging level. AtomicLevel zap.AtomicLevel + // Flags are all the NGF flags. + Flags *pflag.FlagSet // GatewayNsName is the namespaced name of a Gateway resource that the Gateway will use. // The Gateway will ignore all other Gateway resources. GatewayNsName *types.NamespacedName diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index f13d24cc53..c4dcba32ab 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -253,6 +253,7 @@ func StartManager(cfg config.Config) error { Name: cfg.GatewayPodConfig.Name, }, ImageSource: cfg.ImageSource, + Flags: cfg.Flags, }) if err = mgr.Add(createTelemetryJob(cfg, dataCollector, nginxChecker.getReadyCh())); err != nil { return fmt.Errorf("cannot register telemetry job: %w", err) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 74d8d2ebf1..6de1596ca4 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -5,7 +5,9 @@ import ( "errors" "fmt" "runtime" + "strconv" + "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -47,17 +49,49 @@ type ProjectMetadata struct { Version string } +//type DeploymentFlagOptions struct { +// GatewayClass string +// GatewayCtlrName string +// Gateway string +// Config string +// Service string +// UpdateGCStatus bool +// MetricsDisable bool +// MetricsSecure bool +// MetricsPort string +// HealthDisable bool +// HealthPort string +// LeaderElectionDisable bool +// LeaderElectionLockName string +// Plus bool +//} + +type DeploymentFlagOptions struct { + NonBooleanFlags []NonBooleanFlag + BooleanFlags []BooleanFlag +} + +type BooleanFlag struct { + Name string + Value bool +} +type NonBooleanFlag struct { + Name string + Value string +} + // Data is telemetry data. // Note: this type might change once https://github.com/nginxinc/nginx-gateway-fabric/issues/1318 is implemented. type Data struct { - ProjectMetadata ProjectMetadata - ClusterID string - Arch string - DeploymentID string - ImageSource string - NGFResourceCounts NGFResourceCounts - NodeCount int - NGFReplicaCount int + ProjectMetadata ProjectMetadata + ClusterID string + Arch string + DeploymentID string + ImageSource string + NGFResourceCounts NGFResourceCounts + NodeCount int + NGFReplicaCount int + DeploymentFlagOptions DeploymentFlagOptions } // DataCollectorConfig holds configuration parameters for DataCollectorImpl. @@ -68,6 +102,8 @@ type DataCollectorConfig struct { GraphGetter GraphGetter // ConfigurationGetter allows us to get the Configuration. ConfigurationGetter ConfigurationGetter + // Flags are all the NGF flags. + Flags *pflag.FlagSet // Version is the NGF version. Version string // PodNSName is the NamespacedName of the NGF Pod. @@ -111,6 +147,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { if err != nil { return Data{}, fmt.Errorf("failed to collect NGF replica count: %w", err) } + deploymentFlagOptions := collectDeploymentFlagOptions(c.cfg.Flags) deploymentID, err := getDeploymentID(replicaSet) if err != nil { @@ -129,11 +166,12 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { Name: "NGF", Version: c.cfg.Version, }, - NGFReplicaCount: replicaCount, - ClusterID: clusterID, - ImageSource: c.cfg.ImageSource, - Arch: runtime.GOARCH, - DeploymentID: deploymentID, + NGFReplicaCount: replicaCount, + ClusterID: clusterID, + ImageSource: c.cfg.ImageSource, + Arch: runtime.GOARCH, + DeploymentID: deploymentID, + DeploymentFlagOptions: deploymentFlagOptions, } return data, nil @@ -258,3 +296,40 @@ func CollectClusterID(ctx context.Context, k8sClient client.Reader) (string, err } return string(kubeNamespace.GetUID()), nil } + +func collectDeploymentFlagOptions(flags *pflag.FlagSet) DeploymentFlagOptions { + deploymentFlagOptions := DeploymentFlagOptions{ + NonBooleanFlags: []NonBooleanFlag{}, + BooleanFlags: []BooleanFlag{}, + } + flags.Visit( + func(flag *pflag.Flag) { + switch flag.Value.Type() { + case "bool": + val, err := strconv.ParseBool(flag.Value.String()) + if err != nil { + return + } + deploymentFlagOptions.BooleanFlags = append(deploymentFlagOptions.BooleanFlags, BooleanFlag{ + Name: flag.Name, + Value: val, + }) + + default: + var val string + if flag.Value.String() == flag.DefValue { + val = "default" + } else { + val = "user-defined" + } + + deploymentFlagOptions.NonBooleanFlags = append(deploymentFlagOptions.NonBooleanFlags, NonBooleanFlag{ + Name: flag.Name, + Value: val, + }) + } + }, + ) + + return deploymentFlagOptions +} From 2187af7549e92ae6f05171fe9e68a73f5d284bd7 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 14 Feb 2024 08:53:11 -0800 Subject: [PATCH 02/12] Add different Deployment Flag Options --- internal/mode/static/telemetry/collector.go | 81 +++++++++++++-------- 1 file changed, 52 insertions(+), 29 deletions(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 6de1596ca4..7cab874471 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "runtime" - "strconv" "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" @@ -49,6 +48,7 @@ type ProjectMetadata struct { Version string } +// Option 1: list off each Flag and its value //type DeploymentFlagOptions struct { // GatewayClass string // GatewayCtlrName string @@ -66,18 +66,35 @@ type ProjectMetadata struct { // Plus bool //} -type DeploymentFlagOptions struct { - NonBooleanFlags []NonBooleanFlag - BooleanFlags []BooleanFlag -} +// Option 2 doesn't work with exporter, can't handle slices of structs. +//type DeploymentFlagOptions struct { +// NonBooleanFlags []NonBooleanFlag +// BooleanFlags []BooleanFlag +//} +// +//type BooleanFlag struct { +// Name string +// Value bool +//} +//type NonBooleanFlag struct { +// Name string +// Value string +//} -type BooleanFlag struct { - Name string - Value bool -} -type NonBooleanFlag struct { - Name string - Value string +// Option 2.1: separate Boolean and Non-boolean flags +//type DeploymentFlagOptions struct { +// NonBooleanFlagKeys []string +// NonBooleanFlagValues []string +// +// BooleanFlagKeys []string +// BooleanFlagValues []bool +//} + +// Option 3: Don't separate the boolean and non-boolean flags but simply keep the value of the boolean flag +// as a string, e.g. "true", "false". +type DeploymentFlagOptions struct { + FlagKeys []string + FlagValues []string } // Data is telemetry data. @@ -88,10 +105,10 @@ type Data struct { Arch string DeploymentID string ImageSource string + DeploymentFlagOptions DeploymentFlagOptions NGFResourceCounts NGFResourceCounts NodeCount int NGFReplicaCount int - DeploymentFlagOptions DeploymentFlagOptions } // DataCollectorConfig holds configuration parameters for DataCollectorImpl. @@ -298,22 +315,30 @@ func CollectClusterID(ctx context.Context, k8sClient client.Reader) (string, err } func collectDeploymentFlagOptions(flags *pflag.FlagSet) DeploymentFlagOptions { + //deploymentFlagOptions := DeploymentFlagOptions{ + // NonBooleanFlagKeys: []string{}, + // NonBooleanFlagValues: []string{}, + // BooleanFlagKeys: []string{}, + // BooleanFlagValues: []bool{}, + //} + deploymentFlagOptions := DeploymentFlagOptions{ - NonBooleanFlags: []NonBooleanFlag{}, - BooleanFlags: []BooleanFlag{}, + FlagKeys: []string{}, + FlagValues: []string{}, } - flags.Visit( + flags.VisitAll( func(flag *pflag.Flag) { + deploymentFlagOptions.FlagKeys = append(deploymentFlagOptions.FlagKeys, flag.Name) switch flag.Value.Type() { case "bool": - val, err := strconv.ParseBool(flag.Value.String()) - if err != nil { - return - } - deploymentFlagOptions.BooleanFlags = append(deploymentFlagOptions.BooleanFlags, BooleanFlag{ - Name: flag.Name, - Value: val, - }) + //val, err := strconv.ParseBool(flag.Value.String()) + //if err != nil { + // return + //} + //deploymentFlagOptions.BooleanFlagKeys = append(deploymentFlagOptions.BooleanFlagKeys, flag.Name) + //deploymentFlagOptions.BooleanFlagValues = append(deploymentFlagOptions.BooleanFlagValues, val) + + deploymentFlagOptions.FlagValues = append(deploymentFlagOptions.FlagValues, flag.Value.String()) default: var val string @@ -322,11 +347,9 @@ func collectDeploymentFlagOptions(flags *pflag.FlagSet) DeploymentFlagOptions { } else { val = "user-defined" } - - deploymentFlagOptions.NonBooleanFlags = append(deploymentFlagOptions.NonBooleanFlags, NonBooleanFlag{ - Name: flag.Name, - Value: val, - }) + deploymentFlagOptions.FlagValues = append(deploymentFlagOptions.FlagValues, val) + // deploymentFlagOptions.NonBooleanFlagKeys = append(deploymentFlagOptions.NonBooleanFlagKeys, flag.Name) + // deploymentFlagOptions.NonBooleanFlagValues = append(deploymentFlagOptions.NonBooleanFlagValues, val) } }, ) From 1c8cf8b8c60a7b7b795ab7324c2eb783822663f1 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 14 Feb 2024 14:06:31 -0800 Subject: [PATCH 03/12] Add temporary collector tests --- .../mode/static/telemetry/collector_test.go | 22 ++++++++++++------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 3a8da82260..664d02b5fc 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -9,6 +9,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -79,6 +80,7 @@ var _ = Describe("Collector", Ordered, func() { kubeNamespace *v1.Namespace baseGetCalls getCallsFunc + flagset *pflag.FlagSet ) BeforeAll(func() { @@ -129,16 +131,19 @@ var _ = Describe("Collector", Ordered, func() { BeforeEach(func() { expData = telemetry.Data{ - ProjectMetadata: telemetry.ProjectMetadata{Name: "NGF", Version: version}, - NodeCount: 0, - NGFResourceCounts: telemetry.NGFResourceCounts{}, - NGFReplicaCount: 1, - ClusterID: string(kubeNamespace.GetUID()), - ImageSource: "local", - Arch: runtime.GOARCH, - DeploymentID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), + ProjectMetadata: telemetry.ProjectMetadata{Name: "NGF", Version: version}, + NodeCount: 0, + NGFResourceCounts: telemetry.NGFResourceCounts{}, + NGFReplicaCount: 1, + ClusterID: string(kubeNamespace.GetUID()), + ImageSource: "local", + Arch: runtime.GOARCH, + DeploymentID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), + DeploymentFlagOptions: telemetry.DeploymentFlagOptions{FlagKeys: []string{}, FlagValues: []string{}}, } + flagset = pflag.NewFlagSet("flagset", 0) + k8sClientReader = &eventsfakes.FakeReader{} fakeGraphGetter = &telemetryfakes.FakeGraphGetter{} fakeConfigurationGetter = &telemetryfakes.FakeConfigurationGetter{} @@ -153,6 +158,7 @@ var _ = Describe("Collector", Ordered, func() { Version: version, PodNSName: podNSName, ImageSource: "local", + Flags: flagset, }) baseGetCalls = createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) From 402d2a0241acff454bccfc912ec6eff715a31bcb Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Thu, 15 Feb 2024 09:33:54 -0800 Subject: [PATCH 04/12] WIP --- .goreleaser.yml | 2 +- Makefile | 2 +- .../mode/static/telemetry/collector_test.go | 54 +++++++++++++++++++ 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 8433bbc985..1642ee5266 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -15,7 +15,7 @@ builds: asmflags: - all=-trimpath={{.Env.GOPATH}} ldflags: - - -s -w -X main.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.Date}} -X main.telemetryReportPeriod=24h + - -s -w -X main.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.Date}} -X main.telemetryReportPeriod=15s main: ./cmd/gateway/ binary: gateway diff --git a/Makefile b/Makefile index 87303395e1..67076a67fc 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ NGINX_CONF_DIR = internal/mode/static/nginx/conf NJS_DIR = internal/mode/static/nginx/modules/src NGINX_DOCKER_BUILD_PLUS_ARGS = --secret id=nginx-repo.crt,src=nginx-repo.crt --secret id=nginx-repo.key,src=nginx-repo.key BUILD_AGENT=local -TELEMETRY_REPORT_PERIOD = 24h # also configured in goreleaser.yml +TELEMETRY_REPORT_PERIOD = 15s # also configured in goreleaser.yml GW_API_VERSION = 1.0.0 INSTALL_WEBHOOK = false diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 664d02b5fc..cf93063e92 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -6,6 +6,7 @@ import ( "fmt" "reflect" "runtime" + "strconv" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -273,6 +274,26 @@ var _ = Describe("Collector", Ordered, func() { }, } + var boolFlag bool + flagset.BoolVar( + &boolFlag, + "boolFlag", + true, + "boolean test flag", + ) + intFlag := flagValue[int]{value: 8080} + flagset.Var( + &intFlag, + "intFlag", + "int test flag", + ) + stringFlag := flagValue[string]{value: "string-flag"} + flagset.Var( + &stringFlag, + "stringFlag", + "string test flag", + ) + fakeGraphGetter.GetLatestGraphReturns(graph) fakeConfigurationGetter.GetLatestConfigurationReturns(config) @@ -285,6 +306,10 @@ var _ = Describe("Collector", Ordered, func() { Services: 3, Endpoints: 4, } + expData.DeploymentFlagOptions = telemetry.DeploymentFlagOptions{ + FlagKeys: []string{"boolFlag", "intFlag", "stringFlag"}, + FlagValues: []string{"true", "default", "default"}, + } data, err := dataCollector.Collect(ctx) @@ -673,3 +698,32 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) + +// flagValue holds a generic value and implements the pflag.Value interface. +type flagValue[T comparable] struct { + value T +} + +func (v *flagValue[T]) String() string { + switch val := any(v.value).(type) { + case int: + return strconv.Itoa(val) + case string: + return val + } + return "" +} + +func (v *flagValue[T]) Set(_ string) error { + return nil +} + +func (v *flagValue[T]) Type() string { + switch any(v.value).(type) { + case int: + return "int" + case string: + return "string" + } + return "" +} From 17deecce2e8dcdfb92b34eb0981ba45edd83366c Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 16 Feb 2024 09:59:59 -0800 Subject: [PATCH 05/12] Move flag parsing to commands --- .goreleaser.yml | 2 +- Makefile | 2 +- cmd/gateway/commands.go | 34 ++++- cmd/gateway/commands_test.go | 133 ++++++++++++++++++ internal/mode/static/config/config.go | 15 +- internal/mode/static/manager.go | 4 +- internal/mode/static/telemetry/collector.go | 129 +++-------------- .../mode/static/telemetry/collector_test.go | 88 +++--------- 8 files changed, 218 insertions(+), 189 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 1642ee5266..8433bbc985 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -15,7 +15,7 @@ builds: asmflags: - all=-trimpath={{.Env.GOPATH}} ldflags: - - -s -w -X main.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.Date}} -X main.telemetryReportPeriod=15s + - -s -w -X main.version={{.Version}} -X main.commit={{.Commit}} -X main.date={{.Date}} -X main.telemetryReportPeriod=24h main: ./cmd/gateway/ binary: gateway diff --git a/Makefile b/Makefile index 67076a67fc..87303395e1 100644 --- a/Makefile +++ b/Makefile @@ -8,7 +8,7 @@ NGINX_CONF_DIR = internal/mode/static/nginx/conf NJS_DIR = internal/mode/static/nginx/modules/src NGINX_DOCKER_BUILD_PLUS_ARGS = --secret id=nginx-repo.crt,src=nginx-repo.crt --secret id=nginx-repo.key,src=nginx-repo.key BUILD_AGENT=local -TELEMETRY_REPORT_PERIOD = 15s # also configured in goreleaser.yml +TELEMETRY_REPORT_PERIOD = 24h # also configured in goreleaser.yml GW_API_VERSION = 1.0.0 INSTALL_WEBHOOK = false diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 6d987a554b..3bb4cc08ff 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -7,6 +7,7 @@ import ( "time" "github.com/spf13/cobra" + "github.com/spf13/pflag" "go.uber.org/zap" "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" @@ -178,6 +179,8 @@ func createStaticModeCommand() *cobra.Command { } } + flagKeys, flagValues := parseFlagKeysAndValues(cmd.Flags()) + conf := config.Config{ GatewayCtlrName: gatewayCtlrName.value, ConfigName: configName.String(), @@ -215,7 +218,10 @@ func createStaticModeCommand() *cobra.Command { Version: version, ExperimentalFeatures: gwExperimentalFeatures, ImageSource: imageSource, - Flags: cmd.Flags(), + FlagKeyValues: config.FlagKeyValues{ + FlagKeys: flagKeys, + FlagValues: flagValues, + }, } if err := static.StartManager(conf); err != nil { @@ -451,3 +457,29 @@ func createSleepCommand() *cobra.Command { return cmd } + +func parseFlagKeysAndValues(flags *pflag.FlagSet) (flagKeys, flagValues []string) { + flagKeys = []string{} + flagValues = []string{} + + flags.VisitAll( + func(flag *pflag.Flag) { + flagKeys = append(flagKeys, flag.Name) + switch flag.Value.Type() { + case "bool": + flagValues = append(flagValues, flag.Value.String()) + default: + var val string + if flag.Value.String() == flag.DefValue { + val = "default" + } else { + val = "user-defined" + } + + flagValues = append(flagValues, val) + } + }, + ) + + return flagKeys, flagValues +} diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index 05da529d49..2a8fe12e0a 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -6,6 +6,8 @@ import ( . "github.com/onsi/gomega" "github.com/spf13/cobra" + "github.com/spf13/pflag" + "k8s.io/apimachinery/pkg/types" ) type flagTestCase struct { @@ -454,3 +456,134 @@ func TestSleepCmdFlagValidation(t *testing.T) { }) } } + +func TestParseFlagKeysAndValues(t *testing.T) { + g := NewWithT(t) + + flagSet := pflag.NewFlagSet("flagSet", 0) + // set SortFlags to false for testing purposes so when parseFlagKeysAndValues loops over the flagSet it + // goes off of primordial order. + flagSet.SortFlags = false + + var boolFlagTrue bool + flagSet.BoolVar( + &boolFlagTrue, + "boolFlagTrue", + true, + "boolean true test flag", + ) + + var boolFlagFalse bool + flagSet.BoolVar( + &boolFlagFalse, + "boolFlagFalse", + false, + "boolean false test flag", + ) + + intFlagDefault := intValidatingValue{ + validator: validatePort, + value: 8080, + } + flagSet.Var( + &intFlagDefault, + "intFlagDefault", + "default int test flag", + ) + + intFlagUserDefined := intValidatingValue{ + validator: validatePort, + value: 8080, + } + flagSet.Var( + &intFlagUserDefined, + "intFlagUserDefined", + "user defined int test flag", + ) + err := flagSet.Set("intFlagUserDefined", "8081") + g.Expect(err).To(Not(HaveOccurred())) + + stringFlagDefault := stringValidatingValue{ + validator: validateResourceName, + value: "default-string-test-flag", + } + flagSet.Var( + &stringFlagDefault, + "stringFlagDefault", + "default string test flag", + ) + + stringFlagUserDefined := stringValidatingValue{ + validator: validateResourceName, + value: "user-defined-string-test-flag", + } + flagSet.Var( + &stringFlagUserDefined, + "stringFlagUserDefined", + "user defined string test flag", + ) + err = flagSet.Set("stringFlagUserDefined", "changed-test-flag-value") + g.Expect(err).To(Not(HaveOccurred())) + + namespacedNameFlagDefault := namespacedNameValue{ + value: types.NamespacedName{ + Namespace: "test", + Name: "test-flag", + }, + } + flagSet.Var( + &namespacedNameFlagDefault, + "namespacedNameFlagDefault", + "default namespacedName test flag", + ) + + namespacedNameFlagUserDefined := namespacedNameValue{ + value: types.NamespacedName{ + Namespace: "test", + Name: "test-flag", + }, + } + flagSet.Var( + &namespacedNameFlagUserDefined, + "namespacedNameFlagUserDefined", + "user defined namespacedName test flag", + ) + userDefinedNamespacedName := types.NamespacedName{ + Namespace: "changed-namespace", + Name: "changed-name", + } + err = flagSet.Set("namespacedNameFlagUserDefined", userDefinedNamespacedName.String()) + g.Expect(err).To(Not(HaveOccurred())) + + expectedKeys := []string{ + "boolFlagTrue", + "boolFlagFalse", + + "intFlagDefault", + "intFlagUserDefined", + + "stringFlagDefault", + "stringFlagUserDefined", + + "namespacedNameFlagDefault", + "namespacedNameFlagUserDefined", + } + expectedValues := []string{ + "true", + "false", + + "default", + "user-defined", + + "default", + "user-defined", + + "default", + "user-defined", + } + + flagKeys, flagValues := parseFlagKeysAndValues(flagSet) + + g.Expect(flagKeys).Should(Equal(expectedKeys)) + g.Expect(flagValues).Should(Equal(expectedValues)) +} diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index c3df848314..a59cf480ad 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -4,7 +4,6 @@ import ( "time" "github.com/go-logr/logr" - "github.com/spf13/pflag" "go.uber.org/zap" "k8s.io/apimachinery/pkg/types" ) @@ -16,8 +15,8 @@ type Config struct { ImageSource string // AtomicLevel is an atomically changeable, dynamic logging level. AtomicLevel zap.AtomicLevel - // Flags are all the NGF flags. - Flags *pflag.FlagSet + // FlagKeyValues holds the parsed NGF flag keys and values. + FlagKeyValues FlagKeyValues // GatewayNsName is the namespaced name of a Gateway resource that the Gateway will use. // The Gateway will ignore all other Gateway resources. GatewayNsName *types.NamespacedName @@ -108,3 +107,13 @@ type UsageReportConfig struct { // InsecureSkipVerify controls whether the client verifies the server cert. InsecureSkipVerify bool } + +// FlagKeyValues holds the parsed NGF flag keys and values. +// Flag Key and Value are paired based off of index in slice. +type FlagKeyValues struct { + // FlagKeys holds the name of the flag. + FlagKeys []string + // FlagValues holds the value of the flag in string form. + // Value will be either true or false for boolean flags and default or user-defined for non-boolean flags. + FlagValues []string +} diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index c4dcba32ab..411bcb694b 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -252,8 +252,8 @@ func StartManager(cfg config.Config) error { Namespace: cfg.GatewayPodConfig.Namespace, Name: cfg.GatewayPodConfig.Name, }, - ImageSource: cfg.ImageSource, - Flags: cfg.Flags, + ImageSource: cfg.ImageSource, + FlagKeyValues: cfg.FlagKeyValues, }) if err = mgr.Add(createTelemetryJob(cfg, dataCollector, nginxChecker.getReadyCh())); err != nil { return fmt.Errorf("cannot register telemetry job: %w", err) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 7cab874471..709bc7ea62 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -6,13 +6,13 @@ import ( "fmt" "runtime" - "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" ) @@ -48,67 +48,18 @@ type ProjectMetadata struct { Version string } -// Option 1: list off each Flag and its value -//type DeploymentFlagOptions struct { -// GatewayClass string -// GatewayCtlrName string -// Gateway string -// Config string -// Service string -// UpdateGCStatus bool -// MetricsDisable bool -// MetricsSecure bool -// MetricsPort string -// HealthDisable bool -// HealthPort string -// LeaderElectionDisable bool -// LeaderElectionLockName string -// Plus bool -//} - -// Option 2 doesn't work with exporter, can't handle slices of structs. -//type DeploymentFlagOptions struct { -// NonBooleanFlags []NonBooleanFlag -// BooleanFlags []BooleanFlag -//} -// -//type BooleanFlag struct { -// Name string -// Value bool -//} -//type NonBooleanFlag struct { -// Name string -// Value string -//} - -// Option 2.1: separate Boolean and Non-boolean flags -//type DeploymentFlagOptions struct { -// NonBooleanFlagKeys []string -// NonBooleanFlagValues []string -// -// BooleanFlagKeys []string -// BooleanFlagValues []bool -//} - -// Option 3: Don't separate the boolean and non-boolean flags but simply keep the value of the boolean flag -// as a string, e.g. "true", "false". -type DeploymentFlagOptions struct { - FlagKeys []string - FlagValues []string -} - // Data is telemetry data. // Note: this type might change once https://github.com/nginxinc/nginx-gateway-fabric/issues/1318 is implemented. type Data struct { - ProjectMetadata ProjectMetadata - ClusterID string - Arch string - DeploymentID string - ImageSource string - DeploymentFlagOptions DeploymentFlagOptions - NGFResourceCounts NGFResourceCounts - NodeCount int - NGFReplicaCount int + ProjectMetadata ProjectMetadata + ClusterID string + Arch string + DeploymentID string + ImageSource string + NGFResourceCounts NGFResourceCounts + NodeCount int + NGFReplicaCount int + FlagKeyValues config.FlagKeyValues } // DataCollectorConfig holds configuration parameters for DataCollectorImpl. @@ -119,14 +70,14 @@ type DataCollectorConfig struct { GraphGetter GraphGetter // ConfigurationGetter allows us to get the Configuration. ConfigurationGetter ConfigurationGetter - // Flags are all the NGF flags. - Flags *pflag.FlagSet // Version is the NGF version. Version string // PodNSName is the NamespacedName of the NGF Pod. PodNSName types.NamespacedName // ImageSource is the source of the NGF image. ImageSource string + // FlagKeyValues holds the parsed NGF flag keys and values. + FlagKeyValues config.FlagKeyValues } // DataCollectorImpl is am implementation of DataCollector. @@ -164,7 +115,6 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { if err != nil { return Data{}, fmt.Errorf("failed to collect NGF replica count: %w", err) } - deploymentFlagOptions := collectDeploymentFlagOptions(c.cfg.Flags) deploymentID, err := getDeploymentID(replicaSet) if err != nil { @@ -183,12 +133,12 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { Name: "NGF", Version: c.cfg.Version, }, - NGFReplicaCount: replicaCount, - ClusterID: clusterID, - ImageSource: c.cfg.ImageSource, - Arch: runtime.GOARCH, - DeploymentID: deploymentID, - DeploymentFlagOptions: deploymentFlagOptions, + NGFReplicaCount: replicaCount, + ClusterID: clusterID, + ImageSource: c.cfg.ImageSource, + Arch: runtime.GOARCH, + DeploymentID: deploymentID, + FlagKeyValues: c.cfg.FlagKeyValues, } return data, nil @@ -313,46 +263,3 @@ func CollectClusterID(ctx context.Context, k8sClient client.Reader) (string, err } return string(kubeNamespace.GetUID()), nil } - -func collectDeploymentFlagOptions(flags *pflag.FlagSet) DeploymentFlagOptions { - //deploymentFlagOptions := DeploymentFlagOptions{ - // NonBooleanFlagKeys: []string{}, - // NonBooleanFlagValues: []string{}, - // BooleanFlagKeys: []string{}, - // BooleanFlagValues: []bool{}, - //} - - deploymentFlagOptions := DeploymentFlagOptions{ - FlagKeys: []string{}, - FlagValues: []string{}, - } - flags.VisitAll( - func(flag *pflag.Flag) { - deploymentFlagOptions.FlagKeys = append(deploymentFlagOptions.FlagKeys, flag.Name) - switch flag.Value.Type() { - case "bool": - //val, err := strconv.ParseBool(flag.Value.String()) - //if err != nil { - // return - //} - //deploymentFlagOptions.BooleanFlagKeys = append(deploymentFlagOptions.BooleanFlagKeys, flag.Name) - //deploymentFlagOptions.BooleanFlagValues = append(deploymentFlagOptions.BooleanFlagValues, val) - - deploymentFlagOptions.FlagValues = append(deploymentFlagOptions.FlagValues, flag.Value.String()) - - default: - var val string - if flag.Value.String() == flag.DefValue { - val = "default" - } else { - val = "user-defined" - } - deploymentFlagOptions.FlagValues = append(deploymentFlagOptions.FlagValues, val) - // deploymentFlagOptions.NonBooleanFlagKeys = append(deploymentFlagOptions.NonBooleanFlagKeys, flag.Name) - // deploymentFlagOptions.NonBooleanFlagValues = append(deploymentFlagOptions.NonBooleanFlagValues, val) - } - }, - ) - - return deploymentFlagOptions -} diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index cf93063e92..8cc789e1c3 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -6,11 +6,9 @@ import ( "fmt" "reflect" "runtime" - "strconv" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - "github.com/spf13/pflag" appsv1 "k8s.io/api/apps/v1" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -19,6 +17,7 @@ import ( gatewayv1 "sigs.k8s.io/gateway-api/apis/v1" "github.com/nginxinc/nginx-gateway-fabric/internal/framework/events/eventsfakes" + "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph" "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver" @@ -79,9 +78,8 @@ var _ = Describe("Collector", Ordered, func() { ngfPod *v1.Pod ngfReplicaSet *appsv1.ReplicaSet kubeNamespace *v1.Namespace - - baseGetCalls getCallsFunc - flagset *pflag.FlagSet + baseGetCalls getCallsFunc + flagKeyValues config.FlagKeyValues ) BeforeAll(func() { @@ -128,23 +126,26 @@ var _ = Describe("Collector", Ordered, func() { UID: "test-uid", }, } + + flagKeyValues = config.FlagKeyValues{ + FlagKeys: []string{"boolFlag", "intFlag", "stringFlag"}, + FlagValues: []string{"false", "default", "user-defined"}, + } }) BeforeEach(func() { expData = telemetry.Data{ - ProjectMetadata: telemetry.ProjectMetadata{Name: "NGF", Version: version}, - NodeCount: 0, - NGFResourceCounts: telemetry.NGFResourceCounts{}, - NGFReplicaCount: 1, - ClusterID: string(kubeNamespace.GetUID()), - ImageSource: "local", - Arch: runtime.GOARCH, - DeploymentID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), - DeploymentFlagOptions: telemetry.DeploymentFlagOptions{FlagKeys: []string{}, FlagValues: []string{}}, + ProjectMetadata: telemetry.ProjectMetadata{Name: "NGF", Version: version}, + NodeCount: 0, + NGFResourceCounts: telemetry.NGFResourceCounts{}, + NGFReplicaCount: 1, + ClusterID: string(kubeNamespace.GetUID()), + ImageSource: "local", + Arch: runtime.GOARCH, + DeploymentID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), + FlagKeyValues: flagKeyValues, } - flagset = pflag.NewFlagSet("flagset", 0) - k8sClientReader = &eventsfakes.FakeReader{} fakeGraphGetter = &telemetryfakes.FakeGraphGetter{} fakeConfigurationGetter = &telemetryfakes.FakeConfigurationGetter{} @@ -159,7 +160,7 @@ var _ = Describe("Collector", Ordered, func() { Version: version, PodNSName: podNSName, ImageSource: "local", - Flags: flagset, + FlagKeyValues: flagKeyValues, }) baseGetCalls = createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) @@ -274,26 +275,6 @@ var _ = Describe("Collector", Ordered, func() { }, } - var boolFlag bool - flagset.BoolVar( - &boolFlag, - "boolFlag", - true, - "boolean test flag", - ) - intFlag := flagValue[int]{value: 8080} - flagset.Var( - &intFlag, - "intFlag", - "int test flag", - ) - stringFlag := flagValue[string]{value: "string-flag"} - flagset.Var( - &stringFlag, - "stringFlag", - "string test flag", - ) - fakeGraphGetter.GetLatestGraphReturns(graph) fakeConfigurationGetter.GetLatestConfigurationReturns(config) @@ -306,10 +287,6 @@ var _ = Describe("Collector", Ordered, func() { Services: 3, Endpoints: 4, } - expData.DeploymentFlagOptions = telemetry.DeploymentFlagOptions{ - FlagKeys: []string{"boolFlag", "intFlag", "stringFlag"}, - FlagValues: []string{"true", "default", "default"}, - } data, err := dataCollector.Collect(ctx) @@ -698,32 +675,3 @@ var _ = Describe("Collector", Ordered, func() { }) }) }) - -// flagValue holds a generic value and implements the pflag.Value interface. -type flagValue[T comparable] struct { - value T -} - -func (v *flagValue[T]) String() string { - switch val := any(v.value).(type) { - case int: - return strconv.Itoa(val) - case string: - return val - } - return "" -} - -func (v *flagValue[T]) Set(_ string) error { - return nil -} - -func (v *flagValue[T]) Type() string { - switch any(v.value).(type) { - case int: - return "int" - case string: - return "string" - } - return "" -} From f936282e32724c796b9708973328bea70c71794b Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 16 Feb 2024 10:01:49 -0800 Subject: [PATCH 06/12] Change comment wording --- internal/mode/static/config/config.go | 8 ++++---- internal/mode/static/telemetry/collector.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index a59cf480ad..54b34a7e84 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -15,7 +15,7 @@ type Config struct { ImageSource string // AtomicLevel is an atomically changeable, dynamic logging level. AtomicLevel zap.AtomicLevel - // FlagKeyValues holds the parsed NGF flag keys and values. + // FlagKeyValues contains the parsed NGF flag keys and values. FlagKeyValues FlagKeyValues // GatewayNsName is the namespaced name of a Gateway resource that the Gateway will use. // The Gateway will ignore all other Gateway resources. @@ -108,12 +108,12 @@ type UsageReportConfig struct { InsecureSkipVerify bool } -// FlagKeyValues holds the parsed NGF flag keys and values. +// FlagKeyValues contains the parsed NGF flag keys and values. // Flag Key and Value are paired based off of index in slice. type FlagKeyValues struct { - // FlagKeys holds the name of the flag. + // FlagKeys contains the name of the flag. FlagKeys []string - // FlagValues holds the value of the flag in string form. + // FlagValues contains the value of the flag in string form. // Value will be either true or false for boolean flags and default or user-defined for non-boolean flags. FlagValues []string } diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 709bc7ea62..bfd7db3573 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -76,7 +76,7 @@ type DataCollectorConfig struct { PodNSName types.NamespacedName // ImageSource is the source of the NGF image. ImageSource string - // FlagKeyValues holds the parsed NGF flag keys and values. + // FlagKeyValues contains the parsed NGF flag keys and values. FlagKeyValues config.FlagKeyValues } From 95c465365a3634b360b7e557b90600207ce0e965 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 16 Feb 2024 10:23:10 -0800 Subject: [PATCH 07/12] Fix lint issues --- internal/mode/static/telemetry/collector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index bfd7db3573..3017ab75dc 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -56,10 +56,10 @@ type Data struct { Arch string DeploymentID string ImageSource string + FlagKeyValues config.FlagKeyValues NGFResourceCounts NGFResourceCounts NodeCount int NGFReplicaCount int - FlagKeyValues config.FlagKeyValues } // DataCollectorConfig holds configuration parameters for DataCollectorImpl. From 89e95312a1a1a0fc8b3d7b525ab3bc186bbdc87e Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 23 Feb 2024 10:08:55 -0800 Subject: [PATCH 08/12] Add some review feedback --- cmd/gateway/commands.go | 16 +++++++-------- cmd/gateway/commands_test.go | 6 +++--- internal/mode/static/config/config.go | 20 +++++++++---------- internal/mode/static/manager.go | 4 ++-- internal/mode/static/telemetry/collector.go | 8 ++++---- .../mode/static/telemetry/collector_test.go | 16 +++++++++++---- 6 files changed, 39 insertions(+), 31 deletions(-) diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 3bb4cc08ff..1068858dad 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -179,7 +179,7 @@ func createStaticModeCommand() *cobra.Command { } } - flagKeys, flagValues := parseFlagKeysAndValues(cmd.Flags()) + flagKeys, flagValues := parseFlags(cmd.Flags()) conf := config.Config{ GatewayCtlrName: gatewayCtlrName.value, @@ -218,9 +218,9 @@ func createStaticModeCommand() *cobra.Command { Version: version, ExperimentalFeatures: gwExperimentalFeatures, ImageSource: imageSource, - FlagKeyValues: config.FlagKeyValues{ - FlagKeys: flagKeys, - FlagValues: flagValues, + Flags: config.Flags{ + Names: flagKeys, + Values: flagValues, }, } @@ -458,17 +458,17 @@ func createSleepCommand() *cobra.Command { return cmd } -func parseFlagKeysAndValues(flags *pflag.FlagSet) (flagKeys, flagValues []string) { +func parseFlags(flags *pflag.FlagSet) (flagKeys, flagValues []string) { flagKeys = []string{} flagValues = []string{} flags.VisitAll( func(flag *pflag.Flag) { flagKeys = append(flagKeys, flag.Name) - switch flag.Value.Type() { - case "bool": + + if flag.Value.Type() == "bool" { flagValues = append(flagValues, flag.Value.String()) - default: + } else { var val string if flag.Value.String() == flag.DefValue { val = "default" diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index 2a8fe12e0a..7665c13c34 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -457,11 +457,11 @@ func TestSleepCmdFlagValidation(t *testing.T) { } } -func TestParseFlagKeysAndValues(t *testing.T) { +func TestParseFlags(t *testing.T) { g := NewWithT(t) flagSet := pflag.NewFlagSet("flagSet", 0) - // set SortFlags to false for testing purposes so when parseFlagKeysAndValues loops over the flagSet it + // set SortFlags to false for testing purposes so when parseFlags loops over the flagSet it // goes off of primordial order. flagSet.SortFlags = false @@ -582,7 +582,7 @@ func TestParseFlagKeysAndValues(t *testing.T) { "user-defined", } - flagKeys, flagValues := parseFlagKeysAndValues(flagSet) + flagKeys, flagValues := parseFlags(flagSet) g.Expect(flagKeys).Should(Equal(expectedKeys)) g.Expect(flagValues).Should(Equal(expectedValues)) diff --git a/internal/mode/static/config/config.go b/internal/mode/static/config/config.go index 54b34a7e84..ecc60cb668 100644 --- a/internal/mode/static/config/config.go +++ b/internal/mode/static/config/config.go @@ -15,8 +15,8 @@ type Config struct { ImageSource string // AtomicLevel is an atomically changeable, dynamic logging level. AtomicLevel zap.AtomicLevel - // FlagKeyValues contains the parsed NGF flag keys and values. - FlagKeyValues FlagKeyValues + // Flags contains the NGF command-line flag names and values. + Flags Flags // GatewayNsName is the namespaced name of a Gateway resource that the Gateway will use. // The Gateway will ignore all other Gateway resources. GatewayNsName *types.NamespacedName @@ -108,12 +108,12 @@ type UsageReportConfig struct { InsecureSkipVerify bool } -// FlagKeyValues contains the parsed NGF flag keys and values. -// Flag Key and Value are paired based off of index in slice. -type FlagKeyValues struct { - // FlagKeys contains the name of the flag. - FlagKeys []string - // FlagValues contains the value of the flag in string form. - // Value will be either true or false for boolean flags and default or user-defined for non-boolean flags. - FlagValues []string +// Flags contains the NGF command-line flag names and values. +// Flag Names and Values are paired based off of index in slice. +type Flags struct { + // Names contains the name of the flag. + Names []string + // Values contains the value of the flag in string form. + // Each Value will be either true or false for boolean flags and default or user-defined for non-boolean flags. + Values []string } diff --git a/internal/mode/static/manager.go b/internal/mode/static/manager.go index 411bcb694b..c4dcba32ab 100644 --- a/internal/mode/static/manager.go +++ b/internal/mode/static/manager.go @@ -252,8 +252,8 @@ func StartManager(cfg config.Config) error { Namespace: cfg.GatewayPodConfig.Namespace, Name: cfg.GatewayPodConfig.Name, }, - ImageSource: cfg.ImageSource, - FlagKeyValues: cfg.FlagKeyValues, + ImageSource: cfg.ImageSource, + Flags: cfg.Flags, }) if err = mgr.Add(createTelemetryJob(cfg, dataCollector, nginxChecker.getReadyCh())); err != nil { return fmt.Errorf("cannot register telemetry job: %w", err) diff --git a/internal/mode/static/telemetry/collector.go b/internal/mode/static/telemetry/collector.go index 3017ab75dc..42c5749f0c 100644 --- a/internal/mode/static/telemetry/collector.go +++ b/internal/mode/static/telemetry/collector.go @@ -56,7 +56,7 @@ type Data struct { Arch string DeploymentID string ImageSource string - FlagKeyValues config.FlagKeyValues + Flags config.Flags NGFResourceCounts NGFResourceCounts NodeCount int NGFReplicaCount int @@ -76,8 +76,8 @@ type DataCollectorConfig struct { PodNSName types.NamespacedName // ImageSource is the source of the NGF image. ImageSource string - // FlagKeyValues contains the parsed NGF flag keys and values. - FlagKeyValues config.FlagKeyValues + // Flags contains the command-line NGF flag keys and values. + Flags config.Flags } // DataCollectorImpl is am implementation of DataCollector. @@ -138,7 +138,7 @@ func (c DataCollectorImpl) Collect(ctx context.Context) (Data, error) { ImageSource: c.cfg.ImageSource, Arch: runtime.GOARCH, DeploymentID: deploymentID, - FlagKeyValues: c.cfg.FlagKeyValues, + Flags: c.cfg.Flags, } return data, nil diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index 8cc789e1c3..a2a4050a9b 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -79,7 +79,7 @@ var _ = Describe("Collector", Ordered, func() { ngfReplicaSet *appsv1.ReplicaSet kubeNamespace *v1.Namespace baseGetCalls getCallsFunc - flagKeyValues config.FlagKeyValues + flags config.Flags ) BeforeAll(func() { @@ -127,9 +127,9 @@ var _ = Describe("Collector", Ordered, func() { }, } - flagKeyValues = config.FlagKeyValues{ - FlagKeys: []string{"boolFlag", "intFlag", "stringFlag"}, - FlagValues: []string{"false", "default", "user-defined"}, + flags = config.Flags{ + Names: []string{"boolFlag", "intFlag", "stringFlag"}, + Values: []string{"false", "default", "user-defined"}, } }) @@ -140,10 +140,14 @@ var _ = Describe("Collector", Ordered, func() { NGFResourceCounts: telemetry.NGFResourceCounts{}, NGFReplicaCount: 1, ClusterID: string(kubeNamespace.GetUID()), +<<<<<<< HEAD ImageSource: "local", Arch: runtime.GOARCH, DeploymentID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), FlagKeyValues: flagKeyValues, +======= + Flags: flags, +>>>>>>> 9e04b8c (Add some review feedback) } k8sClientReader = &eventsfakes.FakeReader{} @@ -159,8 +163,12 @@ var _ = Describe("Collector", Ordered, func() { ConfigurationGetter: fakeConfigurationGetter, Version: version, PodNSName: podNSName, +<<<<<<< HEAD ImageSource: "local", FlagKeyValues: flagKeyValues, +======= + Flags: flags, +>>>>>>> 9e04b8c (Add some review feedback) }) baseGetCalls = createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) From 9c405e14f80ac8e4976785c4ec6d682f6ae871fe Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 23 Feb 2024 10:28:34 -0800 Subject: [PATCH 09/12] Fix rebasing issue --- internal/mode/static/telemetry/collector_test.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/internal/mode/static/telemetry/collector_test.go b/internal/mode/static/telemetry/collector_test.go index a2a4050a9b..271bbc0ca8 100644 --- a/internal/mode/static/telemetry/collector_test.go +++ b/internal/mode/static/telemetry/collector_test.go @@ -140,14 +140,10 @@ var _ = Describe("Collector", Ordered, func() { NGFResourceCounts: telemetry.NGFResourceCounts{}, NGFReplicaCount: 1, ClusterID: string(kubeNamespace.GetUID()), -<<<<<<< HEAD ImageSource: "local", Arch: runtime.GOARCH, DeploymentID: string(ngfReplicaSet.ObjectMeta.OwnerReferences[0].UID), - FlagKeyValues: flagKeyValues, -======= Flags: flags, ->>>>>>> 9e04b8c (Add some review feedback) } k8sClientReader = &eventsfakes.FakeReader{} @@ -163,12 +159,8 @@ var _ = Describe("Collector", Ordered, func() { ConfigurationGetter: fakeConfigurationGetter, Version: version, PodNSName: podNSName, -<<<<<<< HEAD ImageSource: "local", - FlagKeyValues: flagKeyValues, -======= Flags: flags, ->>>>>>> 9e04b8c (Add some review feedback) }) baseGetCalls = createGetCallsFunc(ngfPod, ngfReplicaSet, kubeNamespace) From 449adb0195967faa4dac29b36161f9bab517cbe0 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Fri, 23 Feb 2024 11:39:18 -0800 Subject: [PATCH 10/12] Add review feedback --- cmd/gateway/commands_test.go | 80 +++++++++++++++++------------------- 1 file changed, 37 insertions(+), 43 deletions(-) diff --git a/cmd/gateway/commands_test.go b/cmd/gateway/commands_test.go index 7665c13c34..c2ae80a238 100644 --- a/cmd/gateway/commands_test.go +++ b/cmd/gateway/commands_test.go @@ -481,92 +481,86 @@ func TestParseFlags(t *testing.T) { "boolean false test flag", ) - intFlagDefault := intValidatingValue{ + customIntFlagDefault := intValidatingValue{ validator: validatePort, value: 8080, } flagSet.Var( - &intFlagDefault, - "intFlagDefault", - "default int test flag", + &customIntFlagDefault, + "customIntFlagDefault", + "default custom int test flag", ) - intFlagUserDefined := intValidatingValue{ + customIntFlagUserDefined := intValidatingValue{ validator: validatePort, value: 8080, } flagSet.Var( - &intFlagUserDefined, - "intFlagUserDefined", - "user defined int test flag", + &customIntFlagUserDefined, + "customIntFlagUserDefined", + "user defined custom int test flag", ) - err := flagSet.Set("intFlagUserDefined", "8081") + err := flagSet.Set("customIntFlagUserDefined", "8081") g.Expect(err).To(Not(HaveOccurred())) - stringFlagDefault := stringValidatingValue{ + customStringFlagDefault := stringValidatingValue{ validator: validateResourceName, - value: "default-string-test-flag", + value: "default-custom-string-test-flag", } flagSet.Var( - &stringFlagDefault, - "stringFlagDefault", - "default string test flag", + &customStringFlagDefault, + "customStringFlagDefault", + "default custom string test flag", ) - stringFlagUserDefined := stringValidatingValue{ + customStringFlagUserDefined := stringValidatingValue{ validator: validateResourceName, - value: "user-defined-string-test-flag", + value: "user-defined-custom-string-test-flag", } flagSet.Var( - &stringFlagUserDefined, - "stringFlagUserDefined", - "user defined string test flag", + &customStringFlagUserDefined, + "customStringFlagUserDefined", + "user defined custom string test flag", ) - err = flagSet.Set("stringFlagUserDefined", "changed-test-flag-value") + err = flagSet.Set("customStringFlagUserDefined", "changed-test-flag-value") g.Expect(err).To(Not(HaveOccurred())) - namespacedNameFlagDefault := namespacedNameValue{ - value: types.NamespacedName{ - Namespace: "test", - Name: "test-flag", - }, + customStringFlagNoDefaultValueUnset := namespacedNameValue{ + value: types.NamespacedName{}, } flagSet.Var( - &namespacedNameFlagDefault, - "namespacedNameFlagDefault", - "default namespacedName test flag", + &customStringFlagNoDefaultValueUnset, + "customStringFlagNoDefaultValueUnset", + "no default value custom string test flag", ) - namespacedNameFlagUserDefined := namespacedNameValue{ - value: types.NamespacedName{ - Namespace: "test", - Name: "test-flag", - }, + customStringFlagNoDefaultValueUserDefined := namespacedNameValue{ + value: types.NamespacedName{}, } flagSet.Var( - &namespacedNameFlagUserDefined, - "namespacedNameFlagUserDefined", - "user defined namespacedName test flag", + &customStringFlagNoDefaultValueUserDefined, + "customStringFlagNoDefaultValueUserDefined", + "no default value but with user defined namespacedName test flag", ) userDefinedNamespacedName := types.NamespacedName{ Namespace: "changed-namespace", Name: "changed-name", } - err = flagSet.Set("namespacedNameFlagUserDefined", userDefinedNamespacedName.String()) + err = flagSet.Set("customStringFlagNoDefaultValueUserDefined", userDefinedNamespacedName.String()) g.Expect(err).To(Not(HaveOccurred())) expectedKeys := []string{ "boolFlagTrue", "boolFlagFalse", - "intFlagDefault", - "intFlagUserDefined", + "customIntFlagDefault", + "customIntFlagUserDefined", - "stringFlagDefault", - "stringFlagUserDefined", + "customStringFlagDefault", + "customStringFlagUserDefined", - "namespacedNameFlagDefault", - "namespacedNameFlagUserDefined", + "customStringFlagNoDefaultValueUnset", + "customStringFlagNoDefaultValueUserDefined", } expectedValues := []string{ "true", From 3220c93ef2bd17eb4c4a3c10e4ac3d958a35fb08 Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Mon, 26 Feb 2024 13:46:07 -0800 Subject: [PATCH 11/12] Fix declaration of flag slices --- cmd/gateway/commands.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 1068858dad..0dd5db4333 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -458,9 +458,8 @@ func createSleepCommand() *cobra.Command { return cmd } -func parseFlags(flags *pflag.FlagSet) (flagKeys, flagValues []string) { - flagKeys = []string{} - flagValues = []string{} +func parseFlags(flags *pflag.FlagSet) ([]string, []string) { + var flagKeys, flagValues []string flags.VisitAll( func(flag *pflag.Flag) { From 60ac78b9dbb221bbe4a9c8138f384b95abc2221e Mon Sep 17 00:00:00 2001 From: Benjamin Jee Date: Wed, 28 Feb 2024 09:33:39 -0800 Subject: [PATCH 12/12] Refactor code to remove else statement --- cmd/gateway/commands.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cmd/gateway/commands.go b/cmd/gateway/commands.go index 0dd5db4333..0a5d54d43f 100644 --- a/cmd/gateway/commands.go +++ b/cmd/gateway/commands.go @@ -468,11 +468,9 @@ func parseFlags(flags *pflag.FlagSet) ([]string, []string) { if flag.Value.Type() == "bool" { flagValues = append(flagValues, flag.Value.String()) } else { - var val string + val := "user-defined" if flag.Value.String() == flag.DefValue { val = "default" - } else { - val = "user-defined" } flagValues = append(flagValues, val)