From 838c94afaae79dc4c0053e766c936ca90c58c880 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Fri, 5 Jan 2024 12:17:06 +0000 Subject: [PATCH 01/19] Add base telemetry job --- cmd/nginx-ingress/main.go | 9 ++++-- internal/k8s/controller.go | 15 ++++++++++ internal/nginx/fake_manager.go | 4 ++- internal/telemetry/telemetry.go | 53 +++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 4 deletions(-) create mode 100644 internal/telemetry/telemetry.go diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index fbd82aed21..7a1c0e38bf 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -790,9 +790,12 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf } func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, nginxVersion, appProtectVersion string, maxRetries int, waitTime time.Duration) { - nginxVer := strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n") - replacer := strings.NewReplacer(" ", "-", "(", "", ")", "") - nginxVer = replacer.Replace(nginxVer) + var nginxVer string + if nginxVersion != "" { + nginxVer := strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n") + replacer := strings.NewReplacer(" ", "-", "(", "", ")", "") + nginxVer = replacer.Replace(nginxVer) + } podUpdated := false for i := 0; (i < maxRetries || maxRetries == 0) && !podUpdated; i++ { diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 45620e30b4..cf0cfd3cf3 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -19,6 +19,7 @@ package k8s import ( "context" "fmt" + "github.com/nginxinc/kubernetes-ingress/internal/telemetry" "net" "strconv" "strings" @@ -161,6 +162,7 @@ type LoadBalancerController struct { enableBatchReload bool isIPV6Disabled bool namespaceWatcherController cache.Controller + telemetryReporter telemetry.Reporter } var keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc @@ -271,6 +273,10 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc lbc.externalDNSController = ed_controller.NewController(ed_controller.BuildOpts(context.TODO(), lbc.namespaceList, lbc.recorder, lbc.confClient, input.ResyncPeriod, isDynamicNs)) } + // Placeholder exporter. + exporter := &telemetry.StdOutExporter{} + lbc.telemetryReporter = telemetry.NewTelemetryReporter(10*time.Second, exporter) + glog.V(3).Infof("Nginx Ingress Controller has class: %v", input.IngressClass) lbc.namespacedInformers = make(map[string]*namespacedInformer) @@ -687,6 +693,15 @@ func (lbc *LoadBalancerController) Run() { go lbc.leaderElector.Run(lbc.ctx) } + glog.V(1).Info("Checking if leader is set...") + if lbc.isLeaderElectionEnabled { + if lbc.leaderElector != nil && lbc.leaderElector.IsLeader() { + glog.V(1).Info("BEFORE Starting Telemetry Reporter...") + go lbc.telemetryReporter.Start(lbc.ctx) + glog.V(1).Info("AFTER Starting Telemetry Reporter...") + } + } + for _, nif := range lbc.namespacedInformers { nif.start() } diff --git a/internal/nginx/fake_manager.go b/internal/nginx/fake_manager.go index 1e42c51ed8..14365fd41a 100644 --- a/internal/nginx/fake_manager.go +++ b/internal/nginx/fake_manager.go @@ -103,7 +103,9 @@ func (fm *FakeManager) CreateDHParam(_ string) (string, error) { // Version provides a fake implementation of Version. func (*FakeManager) Version() Version { glog.V(3).Info("Printing nginx version") - return Version{} + return Version{ + raw: "nginx/1.25.1 (nginx-plus-r30-p1)", + } } // Start provides a fake implementation of Start. diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go new file mode 100644 index 0000000000..cb4781577a --- /dev/null +++ b/internal/telemetry/telemetry.go @@ -0,0 +1,53 @@ +package telemetry + +import ( + "context" + "fmt" + "github.com/golang/glog" + "k8s.io/apimachinery/pkg/util/wait" + "time" +) + +type Data struct { +} + +type Exporter interface { + Export(data Data) +} + +type StdOutExporter struct { +} + +func (s *StdOutExporter) Export(data Data) { + fmt.Printf("Exporting data %v", data) +} + +type Reporter interface { + report(ctx context.Context) + Start(ctx context.Context) +} + +type TraceTelemetryReporter struct { + exporter Exporter + period time.Duration +} + +func NewTelemetryReporter(reportingPeriod time.Duration, exporter Exporter) *TraceTelemetryReporter { + return &TraceTelemetryReporter{ + exporter: exporter, + period: reportingPeriod, + } +} + +func (t *TraceTelemetryReporter) Start(ctx context.Context) { + glog.V(1).Info("Starting Telemetry Job...") + wait.UntilWithContext(ctx, t.report, t.period) + glog.V(1).Info("Stopping Telemetry Job...") +} + +// ctx is blank for now during POC. +func (t *TraceTelemetryReporter) report(_ context.Context) { + t.exporter.Export(Data{}) + + glog.V(1).Info("Data exported...") +} From ca9b9f77829cb6114cae2db0398d368bfefcdf05 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Tue, 9 Jan 2024 09:49:16 +0000 Subject: [PATCH 02/19] Ensure only leader pod reports data --- cmd/nginx-ingress/main.go | 6 +++- internal/k8s/controller.go | 35 ++++++++++++++----- internal/k8s/leader.go | 2 ++ internal/telemetry/exporter.go | 25 ++++++++++++++ internal/telemetry/exporter_test.go | 18 ++++++++++ internal/telemetry/telemetry.go | 52 +++++++++++++++-------------- 6 files changed, 104 insertions(+), 34 deletions(-) create mode 100644 internal/telemetry/exporter.go create mode 100644 internal/telemetry/exporter_test.go diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 7a1c0e38bf..abdc37dc13 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -41,7 +41,10 @@ import ( ) // Injected during build -var version string +var ( + version string + telemetryReportPeriod string +) const ( nginxVersionLabel = "app.nginx.org/version" @@ -199,6 +202,7 @@ func main() { ExternalDNSEnabled: *enableExternalDNS, IsIPV6Disabled: *disableIPV6, WatchNamespaceLabel: *watchNamespaceLabel, + TelemetryReportPeriod: telemetryReportPeriod, } lbc := k8s.NewLoadBalancerController(lbcInput) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index cf0cfd3cf3..b0c8ecd89a 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -163,6 +163,8 @@ type LoadBalancerController struct { isIPV6Disabled bool namespaceWatcherController cache.Controller telemetryReporter telemetry.Reporter + telemetryChan chan struct{} + telemetryReportPeriod string } var keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc @@ -208,6 +210,7 @@ type NewLoadBalancerControllerInput struct { ExternalDNSEnabled bool IsIPV6Disabled bool WatchNamespaceLabel string + TelemetryReportPeriod string } // NewLoadBalancerController creates a controller @@ -240,6 +243,7 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc isPrometheusEnabled: input.IsPrometheusEnabled, isLatencyMetricsEnabled: input.IsLatencyMetricsEnabled, isIPV6Disabled: input.IsIPV6Disabled, + telemetryReportPeriod: input.TelemetryReportPeriod, } eventBroadcaster := record.NewBroadcaster() @@ -275,7 +279,19 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc // Placeholder exporter. exporter := &telemetry.StdOutExporter{} - lbc.telemetryReporter = telemetry.NewTelemetryReporter(10*time.Second, exporter) + lbc.telemetryChan = make(chan struct{}) + duration, err := time.ParseDuration(lbc.telemetryReportPeriod) + + if err != nil { + glog.Fatalf("failed to parse telemetry reporting period: %v", err) + } + + config := telemetry.TraceTelemetryReporterConfig{ + Exporter: exporter, + Period: duration, + Data: telemetry.Data{}, + } + lbc.telemetryReporter = telemetry.NewTelemetryReporter(config) glog.V(3).Infof("Nginx Ingress Controller has class: %v", input.IngressClass) @@ -689,18 +705,21 @@ func (lbc *LoadBalancerController) Run() { if lbc.externalDNSController != nil { go lbc.externalDNSController.Run(lbc.ctx.Done()) } + if lbc.leaderElector != nil { go lbc.leaderElector.Run(lbc.ctx) } - glog.V(1).Info("Checking if leader is set...") - if lbc.isLeaderElectionEnabled { - if lbc.leaderElector != nil && lbc.leaderElector.IsLeader() { - glog.V(1).Info("BEFORE Starting Telemetry Reporter...") - go lbc.telemetryReporter.Start(lbc.ctx) - glog.V(1).Info("AFTER Starting Telemetry Reporter...") + go func(ctx context.Context) { + glog.V(1).Info("-- Checking if leader is set --") + select { + case <-lbc.telemetryChan: + lbc.telemetryReporter.Start(lbc.ctx) + case <-ctx.Done(): + glog.V(1).Info("-- DONE Reporting Telemetry --") + return } - } + }(lbc.ctx) for _, nif := range lbc.namespacedInformers { nif.start() diff --git a/internal/k8s/leader.go b/internal/k8s/leader.go index f87186ad5f..368866b043 100644 --- a/internal/k8s/leader.go +++ b/internal/k8s/leader.go @@ -58,6 +58,8 @@ func createLeaderHandler(lbc *LoadBalancerController) leaderelection.LeaderCallb return leaderelection.LeaderCallbacks{ OnStartedLeading: func(ctx context.Context) { glog.V(3).Info("started leading") + // Closing this channel allows the leader to start the telemetry reporting process + close(lbc.telemetryChan) if lbc.reportIngressStatus { ingresses := lbc.configuration.GetResourcesWithFilter(resourceFilter{Ingresses: true}) diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go new file mode 100644 index 0000000000..c563a5b3e0 --- /dev/null +++ b/internal/telemetry/exporter.go @@ -0,0 +1,25 @@ +package telemetry + +import ( + "context" + "github.com/golang/glog" +) + +type Data struct { +} + +type Exporter interface { + Export(ctx context.Context, data Data) +} + +type StdOutExporter struct { +} + +func NewStdOutExporter() *StdOutExporter { + return &StdOutExporter{} +} + +func (s *StdOutExporter) Export(_ context.Context, data Data) error { + glog.V(1).Infof("Exporting data %v", data) + return nil +} diff --git a/internal/telemetry/exporter_test.go b/internal/telemetry/exporter_test.go new file mode 100644 index 0000000000..d759159d41 --- /dev/null +++ b/internal/telemetry/exporter_test.go @@ -0,0 +1,18 @@ +package telemetry + +import ( + "context" + "testing" +) +import . "github.com/onsi/gomega" + +func TestExportData(t *testing.T) { + t.Parallel() + g := NewWithT(t) + + exporter := NewStdOutExporter() + + err := exporter.Export(context.Background(), Data{}) + + g.Expect(err).To(BeNil()) +} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index cb4781577a..7432cf8b4a 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -2,52 +2,54 @@ package telemetry import ( "context" - "fmt" "github.com/golang/glog" "k8s.io/apimachinery/pkg/util/wait" "time" ) -type Data struct { -} - -type Exporter interface { - Export(data Data) -} - -type StdOutExporter struct { -} - -func (s *StdOutExporter) Export(data Data) { - fmt.Printf("Exporting data %v", data) -} +const ( + jitterFactor = 0.1 // If the period is 10 seconds, the jitter will be up to 1 second. + sliding = true // The period with jitter will be calculated after each report() call. +) type Reporter interface { - report(ctx context.Context) Start(ctx context.Context) } +type TraceTelemetryReporterConfig struct { + Data Data + Exporter Exporter + Period time.Duration +} + type TraceTelemetryReporter struct { - exporter Exporter - period time.Duration + config TraceTelemetryReporterConfig } -func NewTelemetryReporter(reportingPeriod time.Duration, exporter Exporter) *TraceTelemetryReporter { +func NewTelemetryReporter(config TraceTelemetryReporterConfig) *TraceTelemetryReporter { return &TraceTelemetryReporter{ - exporter: exporter, - period: reportingPeriod, + config: config, } } func (t *TraceTelemetryReporter) Start(ctx context.Context) { glog.V(1).Info("Starting Telemetry Job...") - wait.UntilWithContext(ctx, t.report, t.period) + wait.JitterUntilWithContext(ctx, t.report, t.config.Period, jitterFactor, sliding) glog.V(1).Info("Stopping Telemetry Job...") } -// ctx is blank for now during POC. -func (t *TraceTelemetryReporter) report(_ context.Context) { - t.exporter.Export(Data{}) +func (t *TraceTelemetryReporter) report(ctx context.Context) { + // Gather data here + t.setProductName() + t.setProductVersion() + + t.config.Exporter.Export(ctx, t.config.Data) +} + +func (t *TraceTelemetryReporter) setProductVersion() { + // Placeholder function +} - glog.V(1).Info("Data exported...") +func (t *TraceTelemetryReporter) setProductName() { + // Placeholder function } From 5e3d349d97e2f868bdf6a0940659674775298bb6 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Wed, 10 Jan 2024 12:23:26 +0000 Subject: [PATCH 03/19] Allow deployments to opt-out of telemetry collection --- cmd/nginx-ingress/flags.go | 18 ++++--- cmd/nginx-ingress/main.go | 6 +-- .../deployment/nginx-plus-ingress.yaml | 6 +-- internal/k8s/controller.go | 51 ++++++++++--------- internal/k8s/leader.go | 4 +- internal/telemetry/exporter.go | 2 +- internal/telemetry/exporter_test.go | 6 +-- internal/telemetry/telemetry.go | 26 +++++----- internal/telemetry/telemetry_test.go | 30 +++++++++++ 9 files changed, 95 insertions(+), 54 deletions(-) create mode 100644 internal/telemetry/telemetry_test.go diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index da51ac0ebd..dd65807594 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -3,20 +3,22 @@ package main import ( "flag" "fmt" + "github.com/golang/glog" + api_v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/validation" "net" "os" "regexp" "strconv" "strings" - - "github.com/golang/glog" - api_v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/util/validation" ) const ( - dynamicSSLReloadParam = "ssl-dynamic-reload" + dynamicSSLReloadParam = "ssl-dynamic-reload" + enableTelemetryReportingParam = "enable-telemetry-reporting" + telemetryReportingPeriodParam = "telemetry-reporting-period" + defaultTelemetryReportingPeriod = "24h" ) var ( @@ -201,6 +203,10 @@ var ( enableDynamicSSLReload = flag.Bool(dynamicSSLReloadParam, true, "Enable reloading of SSL Certificates without restarting the NGINX process.") + enableTelemetryReporting = flag.Bool(enableTelemetryReportingParam, true, "Enable gathering and reporting of product related telemetry.") + + telemetryReportingPeriod = flag.String(telemetryReportingPeriodParam, defaultTelemetryReportingPeriod, "Period at which product telemetry is reported.") + startupCheckFn func() error ) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index abdc37dc13..d77c28a60f 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -42,8 +42,7 @@ import ( // Injected during build var ( - version string - telemetryReportPeriod string + version string ) const ( @@ -202,7 +201,8 @@ func main() { ExternalDNSEnabled: *enableExternalDNS, IsIPV6Disabled: *disableIPV6, WatchNamespaceLabel: *watchNamespaceLabel, - TelemetryReportPeriod: telemetryReportPeriod, + TelemetryReportPeriod: *telemetryReportingPeriod, + EnableTelemetryReporting: *enableTelemetryReporting, } lbc := k8s.NewLoadBalancerController(lbcInput) diff --git a/deployments/deployment/nginx-plus-ingress.yaml b/deployments/deployment/nginx-plus-ingress.yaml index 91e54cb613..f81118c753 100644 --- a/deployments/deployment/nginx-plus-ingress.yaml +++ b/deployments/deployment/nginx-plus-ingress.yaml @@ -4,7 +4,7 @@ metadata: name: nginx-ingress namespace: nginx-ingress spec: - replicas: 1 + replicas: 2 selector: matchLabels: app: nginx-ingress @@ -33,7 +33,7 @@ spec: # - name: nginx-log # emptyDir: {} containers: - - image: nginx-plus-ingress:3.4.0 + - image: nginx-plus-ingress:telemetry imagePullPolicy: IfNotPresent name: nginx-plus-ingress ports: @@ -96,7 +96,7 @@ spec: #- -enable-external-dns #- -enable-app-protect #- -enable-app-protect-dos - #- -v=3 # Enables extensive logging. Useful for troubleshooting. + - -v=1 # Enables extensive logging. Useful for troubleshooting. #- -report-ingress-status #- -external-service=nginx-ingress #- -enable-prometheus-metrics diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index b0c8ecd89a..6dee4d521f 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -164,7 +164,6 @@ type LoadBalancerController struct { namespaceWatcherController cache.Controller telemetryReporter telemetry.Reporter telemetryChan chan struct{} - telemetryReportPeriod string } var keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc @@ -211,6 +210,7 @@ type NewLoadBalancerControllerInput struct { IsIPV6Disabled bool WatchNamespaceLabel string TelemetryReportPeriod string + EnableTelemetryReporting bool } // NewLoadBalancerController creates a controller @@ -243,7 +243,6 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc isPrometheusEnabled: input.IsPrometheusEnabled, isLatencyMetricsEnabled: input.IsLatencyMetricsEnabled, isIPV6Disabled: input.IsIPV6Disabled, - telemetryReportPeriod: input.TelemetryReportPeriod, } eventBroadcaster := record.NewBroadcaster() @@ -277,21 +276,23 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc lbc.externalDNSController = ed_controller.NewController(ed_controller.BuildOpts(context.TODO(), lbc.namespaceList, lbc.recorder, lbc.confClient, input.ResyncPeriod, isDynamicNs)) } - // Placeholder exporter. - exporter := &telemetry.StdOutExporter{} - lbc.telemetryChan = make(chan struct{}) - duration, err := time.ParseDuration(lbc.telemetryReportPeriod) + if input.EnableTelemetryReporting { + // Placeholder exporter. + exporter := &telemetry.StdOutExporter{} + lbc.telemetryChan = make(chan struct{}) + period, err := time.ParseDuration(input.TelemetryReportPeriod) - if err != nil { - glog.Fatalf("failed to parse telemetry reporting period: %v", err) - } + if err != nil { + glog.Fatalf("Error parsing duration for telemetry: %v", err) + } - config := telemetry.TraceTelemetryReporterConfig{ - Exporter: exporter, - Period: duration, - Data: telemetry.Data{}, + config := telemetry.TraceTelemetryReporterConfig{ + Exporter: exporter, + ReportingPeriod: period, + Data: telemetry.Data{}, + } + lbc.telemetryReporter = telemetry.NewTelemetryReporter(config) } - lbc.telemetryReporter = telemetry.NewTelemetryReporter(config) glog.V(3).Infof("Nginx Ingress Controller has class: %v", input.IngressClass) @@ -710,16 +711,18 @@ func (lbc *LoadBalancerController) Run() { go lbc.leaderElector.Run(lbc.ctx) } - go func(ctx context.Context) { - glog.V(1).Info("-- Checking if leader is set --") - select { - case <-lbc.telemetryChan: - lbc.telemetryReporter.Start(lbc.ctx) - case <-ctx.Done(): - glog.V(1).Info("-- DONE Reporting Telemetry --") - return - } - }(lbc.ctx) + if lbc.telemetryReporter != nil { + go func(ctx context.Context) { + glog.V(1).Info("-- Checking if leader is set --") + select { + case <-lbc.telemetryChan: + lbc.telemetryReporter.Start(lbc.ctx) + case <-ctx.Done(): + glog.V(1).Info("-- DONE Reporting Telemetry --") + return + } + }(lbc.ctx) + } for _, nif := range lbc.namespacedInformers { nif.start() diff --git a/internal/k8s/leader.go b/internal/k8s/leader.go index 368866b043..7d3cf5ae45 100644 --- a/internal/k8s/leader.go +++ b/internal/k8s/leader.go @@ -59,7 +59,9 @@ func createLeaderHandler(lbc *LoadBalancerController) leaderelection.LeaderCallb OnStartedLeading: func(ctx context.Context) { glog.V(3).Info("started leading") // Closing this channel allows the leader to start the telemetry reporting process - close(lbc.telemetryChan) + if lbc.telemetryChan != nil { + close(lbc.telemetryChan) + } if lbc.reportIngressStatus { ingresses := lbc.configuration.GetResourcesWithFilter(resourceFilter{Ingresses: true}) diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index c563a5b3e0..3e3cc6f1fc 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -9,7 +9,7 @@ type Data struct { } type Exporter interface { - Export(ctx context.Context, data Data) + Export(ctx context.Context, data Data) error } type StdOutExporter struct { diff --git a/internal/telemetry/exporter_test.go b/internal/telemetry/exporter_test.go index d759159d41..af3f9db3fa 100644 --- a/internal/telemetry/exporter_test.go +++ b/internal/telemetry/exporter_test.go @@ -4,15 +4,15 @@ import ( "context" "testing" ) -import . "github.com/onsi/gomega" func TestExportData(t *testing.T) { t.Parallel() - g := NewWithT(t) exporter := NewStdOutExporter() err := exporter.Export(context.Background(), Data{}) - g.Expect(err).To(BeNil()) + if err != nil { + t.Fatalf("Expeceted no error, but got %s", err.Error()) + } } diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 7432cf8b4a..5a6898f1df 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -8,8 +8,8 @@ import ( ) const ( - jitterFactor = 0.1 // If the period is 10 seconds, the jitter will be up to 1 second. - sliding = true // The period with jitter will be calculated after each report() call. + jitterFactor = 0.1 + sliding = true ) type Reporter interface { @@ -17,9 +17,9 @@ type Reporter interface { } type TraceTelemetryReporterConfig struct { - Data Data - Exporter Exporter - Period time.Duration + Data Data + Exporter Exporter + ReportingPeriod time.Duration } type TraceTelemetryReporter struct { @@ -33,23 +33,23 @@ func NewTelemetryReporter(config TraceTelemetryReporterConfig) *TraceTelemetryRe } func (t *TraceTelemetryReporter) Start(ctx context.Context) { - glog.V(1).Info("Starting Telemetry Job...") - wait.JitterUntilWithContext(ctx, t.report, t.config.Period, jitterFactor, sliding) - glog.V(1).Info("Stopping Telemetry Job...") + wait.JitterUntilWithContext(ctx, t.report, t.config.ReportingPeriod, jitterFactor, sliding) } func (t *TraceTelemetryReporter) report(ctx context.Context) { // Gather data here - t.setProductName() - t.setProductVersion() + t.setVirtualServerCount() + t.setTransportServerCount() - t.config.Exporter.Export(ctx, t.config.Data) + if err := t.config.Exporter.Export(ctx, t.config.Data); err != nil { + glog.Errorf("Error exporting telemetry data: %v", err) + } } -func (t *TraceTelemetryReporter) setProductVersion() { +func (t *TraceTelemetryReporter) setVirtualServerCount() { // Placeholder function } -func (t *TraceTelemetryReporter) setProductName() { +func (t *TraceTelemetryReporter) setTransportServerCount() { // Placeholder function } diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go new file mode 100644 index 0000000000..ff6ace27eb --- /dev/null +++ b/internal/telemetry/telemetry_test.go @@ -0,0 +1,30 @@ +package telemetry + +import ( + "context" + "reflect" + "testing" +) + +type MockTelemetryReport struct { + data Data +} + +func (m *MockTelemetryReport) Start(_ context.Context) { + m.data = Data{} +} + +func TestCollectData(t *testing.T) { + t.Parallel() + + ctx := context.Background() + mtr := &MockTelemetryReport{ + Data{}, + } + expectedData := Data{} + mtr.Start(ctx) + + if !reflect.DeepEqual(mtr.data, expectedData) { + t.Fatalf("expected %v, but got %v", expectedData, mtr.data) + } +} From a9224db8428bd175d4497d44225a9f2dbffc25b4 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Wed, 10 Jan 2024 12:38:01 +0000 Subject: [PATCH 04/19] Add log line for when telemetry is collected --- deployments/deployment/nginx-plus-ingress.yaml | 4 ++-- internal/k8s/controller.go | 2 -- internal/telemetry/exporter.go | 2 +- internal/telemetry/telemetry.go | 1 + 4 files changed, 4 insertions(+), 5 deletions(-) diff --git a/deployments/deployment/nginx-plus-ingress.yaml b/deployments/deployment/nginx-plus-ingress.yaml index f81118c753..df60d4c7f8 100644 --- a/deployments/deployment/nginx-plus-ingress.yaml +++ b/deployments/deployment/nginx-plus-ingress.yaml @@ -33,7 +33,7 @@ spec: # - name: nginx-log # emptyDir: {} containers: - - image: nginx-plus-ingress:telemetry + - image: nginx-plus-ingress:3.4.0 imagePullPolicy: IfNotPresent name: nginx-plus-ingress ports: @@ -96,7 +96,7 @@ spec: #- -enable-external-dns #- -enable-app-protect #- -enable-app-protect-dos - - -v=1 # Enables extensive logging. Useful for troubleshooting. + #- -v=1 # Enables extensive logging. Useful for troubleshooting. #- -report-ingress-status #- -external-service=nginx-ingress #- -enable-prometheus-metrics diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 6dee4d521f..2e3b2cdfc7 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -713,12 +713,10 @@ func (lbc *LoadBalancerController) Run() { if lbc.telemetryReporter != nil { go func(ctx context.Context) { - glog.V(1).Info("-- Checking if leader is set --") select { case <-lbc.telemetryChan: lbc.telemetryReporter.Start(lbc.ctx) case <-ctx.Done(): - glog.V(1).Info("-- DONE Reporting Telemetry --") return } }(lbc.ctx) diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 3e3cc6f1fc..713295a991 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -20,6 +20,6 @@ func NewStdOutExporter() *StdOutExporter { } func (s *StdOutExporter) Export(_ context.Context, data Data) error { - glog.V(1).Infof("Exporting data %v", data) + glog.V(3).Infof("Exporting data %v", data) return nil } diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 5a6898f1df..537b78718c 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -37,6 +37,7 @@ func (t *TraceTelemetryReporter) Start(ctx context.Context) { } func (t *TraceTelemetryReporter) report(ctx context.Context) { + glog.V(3).Infof("Collecting Telemetry Data") // Gather data here t.setVirtualServerCount() t.setTransportServerCount() From 54b896bdeab6959920a7619c6ce9c6acfd145d07 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Wed, 10 Jan 2024 12:45:20 +0000 Subject: [PATCH 05/19] gofumpt files --- cmd/nginx-ingress/flags.go | 9 +++++---- internal/k8s/controller.go | 4 ++-- internal/telemetry/exporter.go | 7 +++---- internal/telemetry/exporter_test.go | 1 - internal/telemetry/telemetry.go | 3 ++- 5 files changed, 12 insertions(+), 12 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index dd65807594..cdeff495cc 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -3,15 +3,16 @@ package main import ( "flag" "fmt" - "github.com/golang/glog" - api_v1 "k8s.io/api/core/v1" - "k8s.io/apimachinery/pkg/labels" - "k8s.io/apimachinery/pkg/util/validation" "net" "os" "regexp" "strconv" "strings" + + "github.com/golang/glog" + api_v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/validation" ) const ( diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 2e3b2cdfc7..357437daab 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -19,13 +19,14 @@ package k8s import ( "context" "fmt" - "github.com/nginxinc/kubernetes-ingress/internal/telemetry" "net" "strconv" "strings" "sync" "time" + "github.com/nginxinc/kubernetes-ingress/internal/telemetry" + "github.com/nginxinc/kubernetes-ingress/pkg/apis/dos/v1beta1" "golang.org/x/exp/maps" @@ -281,7 +282,6 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc exporter := &telemetry.StdOutExporter{} lbc.telemetryChan = make(chan struct{}) period, err := time.ParseDuration(input.TelemetryReportPeriod) - if err != nil { glog.Fatalf("Error parsing duration for telemetry: %v", err) } diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 713295a991..2a1f2960ea 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -2,18 +2,17 @@ package telemetry import ( "context" + "github.com/golang/glog" ) -type Data struct { -} +type Data struct{} type Exporter interface { Export(ctx context.Context, data Data) error } -type StdOutExporter struct { -} +type StdOutExporter struct{} func NewStdOutExporter() *StdOutExporter { return &StdOutExporter{} diff --git a/internal/telemetry/exporter_test.go b/internal/telemetry/exporter_test.go index af3f9db3fa..947d728f10 100644 --- a/internal/telemetry/exporter_test.go +++ b/internal/telemetry/exporter_test.go @@ -11,7 +11,6 @@ func TestExportData(t *testing.T) { exporter := NewStdOutExporter() err := exporter.Export(context.Background(), Data{}) - if err != nil { t.Fatalf("Expeceted no error, but got %s", err.Error()) } diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 537b78718c..1d2a804906 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -2,9 +2,10 @@ package telemetry import ( "context" + "time" + "github.com/golang/glog" "k8s.io/apimachinery/pkg/util/wait" - "time" ) const ( From 8011a085f06723e37207710aba21a373487030aa Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Wed, 10 Jan 2024 12:58:17 +0000 Subject: [PATCH 06/19] Revert deployment yaml and fake manager --- deployments/deployment/nginx-plus-ingress.yaml | 4 ++-- internal/nginx/fake_manager.go | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/deployments/deployment/nginx-plus-ingress.yaml b/deployments/deployment/nginx-plus-ingress.yaml index df60d4c7f8..91e54cb613 100644 --- a/deployments/deployment/nginx-plus-ingress.yaml +++ b/deployments/deployment/nginx-plus-ingress.yaml @@ -4,7 +4,7 @@ metadata: name: nginx-ingress namespace: nginx-ingress spec: - replicas: 2 + replicas: 1 selector: matchLabels: app: nginx-ingress @@ -96,7 +96,7 @@ spec: #- -enable-external-dns #- -enable-app-protect #- -enable-app-protect-dos - #- -v=1 # Enables extensive logging. Useful for troubleshooting. + #- -v=3 # Enables extensive logging. Useful for troubleshooting. #- -report-ingress-status #- -external-service=nginx-ingress #- -enable-prometheus-metrics diff --git a/internal/nginx/fake_manager.go b/internal/nginx/fake_manager.go index 14365fd41a..1e42c51ed8 100644 --- a/internal/nginx/fake_manager.go +++ b/internal/nginx/fake_manager.go @@ -103,9 +103,7 @@ func (fm *FakeManager) CreateDHParam(_ string) (string, error) { // Version provides a fake implementation of Version. func (*FakeManager) Version() Version { glog.V(3).Info("Printing nginx version") - return Version{ - raw: "nginx/1.25.1 (nginx-plus-r30-p1)", - } + return Version{} } // Start provides a fake implementation of Start. From 079a906b69b4386cad4ea3609492901b4c10cb60 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Wed, 10 Jan 2024 13:32:09 +0000 Subject: [PATCH 07/19] Fix nginx version assignment --- cmd/nginx-ingress/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index d77c28a60f..e01ee57265 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -796,7 +796,7 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, nginxVersion, appProtectVersion string, maxRetries int, waitTime time.Duration) { var nginxVer string if nginxVersion != "" { - nginxVer := strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n") + nginxVer = strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n") replacer := strings.NewReplacer(" ", "-", "(", "", ")", "") nginxVer = replacer.Replace(nginxVer) } From 82a91a63c9a49c44017d90aa568bdde1807038c4 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Wed, 10 Jan 2024 13:42:14 +0000 Subject: [PATCH 08/19] Resolve lint issues --- internal/k8s/controller.go | 2 +- internal/telemetry/exporter.go | 13 +++++++++---- internal/telemetry/exporter_test.go | 2 +- internal/telemetry/telemetry.go | 5 +++++ 4 files changed, 16 insertions(+), 6 deletions(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 357437daab..0a34542d2f 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -279,7 +279,7 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc if input.EnableTelemetryReporting { // Placeholder exporter. - exporter := &telemetry.StdOutExporter{} + exporter := &telemetry.LogExporter{} lbc.telemetryChan = make(chan struct{}) period, err := time.ParseDuration(input.TelemetryReportPeriod) if err != nil { diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go index 2a1f2960ea..dc6aee8592 100644 --- a/internal/telemetry/exporter.go +++ b/internal/telemetry/exporter.go @@ -6,19 +6,24 @@ import ( "github.com/golang/glog" ) +// Data represents the telemetry data that will be exported type Data struct{} +// Exporter defines an interface for telemetry exporters type Exporter interface { Export(ctx context.Context, data Data) error } -type StdOutExporter struct{} +// LogExporter is an exporter that will log out exported data +type LogExporter struct{} -func NewStdOutExporter() *StdOutExporter { - return &StdOutExporter{} +// NewLogExporter creates a new logging exporter +func NewLogExporter() *LogExporter { + return &LogExporter{} } -func (s *StdOutExporter) Export(_ context.Context, data Data) error { +// Export will send exported data level 3 logs +func (s *LogExporter) Export(_ context.Context, data Data) error { glog.V(3).Infof("Exporting data %v", data) return nil } diff --git a/internal/telemetry/exporter_test.go b/internal/telemetry/exporter_test.go index 947d728f10..b28850cefc 100644 --- a/internal/telemetry/exporter_test.go +++ b/internal/telemetry/exporter_test.go @@ -8,7 +8,7 @@ import ( func TestExportData(t *testing.T) { t.Parallel() - exporter := NewStdOutExporter() + exporter := NewLogExporter() err := exporter.Export(context.Background(), Data{}) if err != nil { diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 1d2a804906..7a7c9f8801 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -13,26 +13,31 @@ const ( sliding = true ) +// Reporter is an interface that represents a telemetry style reporter type Reporter interface { Start(ctx context.Context) } +// TraceTelemetryReporterConfig contains configuration data for the Telemetry Reporter type TraceTelemetryReporterConfig struct { Data Data Exporter Exporter ReportingPeriod time.Duration } +// TraceTelemetryReporter reports telemety data that will be exported as a trace type TraceTelemetryReporter struct { config TraceTelemetryReporterConfig } +// NewTelemetryReporter creates a new TraceTelemetryReporter func NewTelemetryReporter(config TraceTelemetryReporterConfig) *TraceTelemetryReporter { return &TraceTelemetryReporter{ config: config, } } +// Start starts the telemetry reporting job func (t *TraceTelemetryReporter) Start(ctx context.Context) { wait.JitterUntilWithContext(ctx, t.report, t.config.ReportingPeriod, jitterFactor, sliding) } From ff7515ec839b3f0454b9d35b1eadb9d71c45e63d Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 22 Jan 2024 19:51:23 +0000 Subject: [PATCH 09/19] Placeholder for telemetry collector --- cmd/nginx-ingress/flags.go | 16 ++++ cmd/nginx-ingress/main_test.go | 24 ++++++ internal/k8s/controller.go | 23 +++--- internal/telemetry/exporter.go | 29 ------- internal/telemetry/exporter_test.go | 17 ---- internal/telemetry/telemetry.go | 111 +++++++++++++++++++-------- internal/telemetry/telemetry_test.go | 49 ++++++++---- 7 files changed, 163 insertions(+), 106 deletions(-) delete mode 100644 internal/telemetry/exporter.go delete mode 100644 internal/telemetry/exporter_test.go diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index cdeff495cc..a2f53d50d7 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -8,6 +8,7 @@ import ( "regexp" "strconv" "strings" + "time" "github.com/golang/glog" api_v1 "k8s.io/api/core/v1" @@ -391,6 +392,12 @@ func validationChecks() { glog.Fatalf("Invalid value for app-protect-log-level: %v", *appProtectLogLevel) } } + + if telemetryReportingPeriod != nil { + if err := validateReportingPeriod(*telemetryReportingPeriod); err != nil { + glog.Fatalf("Invalid value for telemetry-reporting-period: %v", err) + } + } } // validateNamespaceNames validates the namespaces are in the correct format @@ -496,3 +503,12 @@ func validateLocation(location string) error { } return nil } + +// validateReportingPeriod checks if the reporting period parameter can be parsed. +func validateReportingPeriod(period string) error { + _, err := time.ParseDuration(period) + if err != nil { + return err + } + return nil +} diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 1715d2a3e8..35e95a04b9 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -172,3 +172,27 @@ func TestValidateNamespaces(t *testing.T) { } } } + +func TestValidateReportingPeriodWithInvalidInput(t *testing.T) { + t.Parallel() + + periods := []string{"", "-1", "1x", "abc", "-"} + for _, p := range periods { + err := validateReportingPeriod(p) + if err == nil { + t.Errorf("want error on invalid period %s, got nil", p) + } + } +} + +func TestValidateReportingPeriodWithValidInput(t *testing.T) { + t.Parallel() + + periods := []string{"1h", "24h", "30m"} + for _, p := range periods { + err := validateReportingPeriod(p) + if err != nil { + t.Error(err) + } + } +} diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 0a34542d2f..d946293c4a 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -163,7 +163,7 @@ type LoadBalancerController struct { enableBatchReload bool isIPV6Disabled bool namespaceWatcherController cache.Controller - telemetryReporter telemetry.Reporter + telemetryCollector *telemetry.Collector telemetryChan chan struct{} } @@ -277,21 +277,16 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc lbc.externalDNSController = ed_controller.NewController(ed_controller.BuildOpts(context.TODO(), lbc.namespaceList, lbc.recorder, lbc.confClient, input.ResyncPeriod, isDynamicNs)) } + // NIC Telemetry Reporting if input.EnableTelemetryReporting { - // Placeholder exporter. - exporter := &telemetry.LogExporter{} lbc.telemetryChan = make(chan struct{}) - period, err := time.ParseDuration(input.TelemetryReportPeriod) + collector, err := telemetry.NewCollector( + telemetry.WithTimePeriod(input.TelemetryReportPeriod), + ) if err != nil { - glog.Fatalf("Error parsing duration for telemetry: %v", err) + glog.Fatalf("failed to initialize telemetry collector: %v", err) } - - config := telemetry.TraceTelemetryReporterConfig{ - Exporter: exporter, - ReportingPeriod: period, - Data: telemetry.Data{}, - } - lbc.telemetryReporter = telemetry.NewTelemetryReporter(config) + lbc.telemetryCollector = collector } glog.V(3).Infof("Nginx Ingress Controller has class: %v", input.IngressClass) @@ -711,11 +706,11 @@ func (lbc *LoadBalancerController) Run() { go lbc.leaderElector.Run(lbc.ctx) } - if lbc.telemetryReporter != nil { + if lbc.telemetryCollector != nil { go func(ctx context.Context) { select { case <-lbc.telemetryChan: - lbc.telemetryReporter.Start(lbc.ctx) + lbc.telemetryCollector.Run(lbc.ctx) case <-ctx.Done(): return } diff --git a/internal/telemetry/exporter.go b/internal/telemetry/exporter.go deleted file mode 100644 index dc6aee8592..0000000000 --- a/internal/telemetry/exporter.go +++ /dev/null @@ -1,29 +0,0 @@ -package telemetry - -import ( - "context" - - "github.com/golang/glog" -) - -// Data represents the telemetry data that will be exported -type Data struct{} - -// Exporter defines an interface for telemetry exporters -type Exporter interface { - Export(ctx context.Context, data Data) error -} - -// LogExporter is an exporter that will log out exported data -type LogExporter struct{} - -// NewLogExporter creates a new logging exporter -func NewLogExporter() *LogExporter { - return &LogExporter{} -} - -// Export will send exported data level 3 logs -func (s *LogExporter) Export(_ context.Context, data Data) error { - glog.V(3).Infof("Exporting data %v", data) - return nil -} diff --git a/internal/telemetry/exporter_test.go b/internal/telemetry/exporter_test.go deleted file mode 100644 index b28850cefc..0000000000 --- a/internal/telemetry/exporter_test.go +++ /dev/null @@ -1,17 +0,0 @@ -package telemetry - -import ( - "context" - "testing" -) - -func TestExportData(t *testing.T) { - t.Parallel() - - exporter := NewLogExporter() - - err := exporter.Export(context.Background(), Data{}) - if err != nil { - t.Fatalf("Expeceted no error, but got %s", err.Error()) - } -} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 7a7c9f8801..27c50e8a28 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -1,62 +1,113 @@ +// Package telemetry provides functionality for collecting and exporting NIC telemetry data. package telemetry import ( "context" + "sync" "time" "github.com/golang/glog" "k8s.io/apimachinery/pkg/util/wait" ) -const ( - jitterFactor = 0.1 - sliding = true -) +// Export takes context and data and sends it to the Otel endpoint. +func Export(_ context.Context, _ TraceData) error { + // Note: exporting functionality will be implemented in a separate module. + return nil +} + +// TraceData holds collected NIC telemetry data. +type TraceData struct { + // Numer of VirtualServers + VSCount int + // Number of TransportServers + TSCount int -// Reporter is an interface that represents a telemetry style reporter -type Reporter interface { - Start(ctx context.Context) + // TODO + // Add more fields for NIC data points } -// TraceTelemetryReporterConfig contains configuration data for the Telemetry Reporter -type TraceTelemetryReporterConfig struct { - Data Data - Exporter Exporter - ReportingPeriod time.Duration +// Option is a functional option used for configuring TraceReporter. +type Option func(*Collector) error + +// WithTimePeriod configures reporting time on TraceReporter. +func WithTimePeriod(period string) Option { + return func(c *Collector) error { + d, err := time.ParseDuration(period) + if err != nil { + return err + } + c.Period = d + return nil + } } -// TraceTelemetryReporter reports telemety data that will be exported as a trace -type TraceTelemetryReporter struct { - config TraceTelemetryReporterConfig +// Collector is NIC telemetry data collector. +type Collector struct { + Period time.Duration + + mu sync.Mutex + Data TraceData } -// NewTelemetryReporter creates a new TraceTelemetryReporter -func NewTelemetryReporter(config TraceTelemetryReporterConfig) *TraceTelemetryReporter { - return &TraceTelemetryReporter{ - config: config, +// NewCollector takes 0 or more options and creates a new TraceReporter. +// If no options are provided, NewReporter returns TraceReporter +// configured to gather data every 24h. +func NewCollector(opts ...Option) (*Collector, error) { + c := Collector{ + Period: 24 * time.Hour, + Data: TraceData{}, } + for _, o := range opts { + if err := o(&c); err != nil { + return nil, err + } + } + return &c, nil } // Start starts the telemetry reporting job -func (t *TraceTelemetryReporter) Start(ctx context.Context) { - wait.JitterUntilWithContext(ctx, t.report, t.config.ReportingPeriod, jitterFactor, sliding) +func (c *Collector) Start(ctx context.Context) { + wait.JitterUntilWithContext(ctx, c.Collect, c.Period, 0.1, true) } -func (t *TraceTelemetryReporter) report(ctx context.Context) { - glog.V(3).Infof("Collecting Telemetry Data") - // Gather data here - t.setVirtualServerCount() - t.setTransportServerCount() +// BuildReport takes context and builds report from gathered telemetry data. +func (c *Collector) BuildReport(_ context.Context) error { + dt := TraceData{} + + // TODO: Implement handling and logging errors for each collected data point + + c.mu.Lock() + c.Data = dt + c.mu.Unlock() + return nil +} - if err := t.config.Exporter.Export(ctx, t.config.Data); err != nil { - glog.Errorf("Error exporting telemetry data: %v", err) +// Collect runs data builder. +func (c *Collector) Collect(ctx context.Context) { + if err := c.BuildReport(ctx); err != nil { + glog.Errorf("error exporting telemetry data: %v", err) } } -func (t *TraceTelemetryReporter) setVirtualServerCount() { +// GetVSCount returns number of VirtualServers in watched namespaces. +// +// Note: this is a placeholder function. +func (c *Collector) GetVSCount() int { // Placeholder function + return 0 } -func (t *TraceTelemetryReporter) setTransportServerCount() { +// GetTSCount returns number of TransportServers in watched namespaces. +func (c *Collector) GetTSCount() int { // Placeholder function + return 0 +} + +// Run starts running NIC Telemetry Collector. +// +// This is a placeholder for implementing collector runner. +func (c *Collector) Run(ctx context.Context) { + fn := func(ctx context.Context) {} + wait.JitterUntilWithContext(ctx, fn, c.Period, 0.1, true) } diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index ff6ace27eb..f56e4d3491 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -1,30 +1,47 @@ -package telemetry +package telemetry_test import ( - "context" - "reflect" "testing" + + "github.com/google/go-cmp/cmp" + "github.com/nginxinc/kubernetes-ingress/internal/telemetry" ) -type MockTelemetryReport struct { - data Data -} +func TestCreateNewDefaultCollector(t *testing.T) { + t.Parallel() + + c, err := telemetry.NewCollector() + if err != nil { + t.Fatal(err) + } + + want := 24.0 + got := c.Period.Hours() -func (m *MockTelemetryReport) Start(_ context.Context) { - m.data = Data{} + if !cmp.Equal(want, got) { + t.Error(cmp.Diff(want, got)) + } + + wantData := telemetry.TraceData{} + gotData := c.Data + + if !cmp.Equal(wantData, gotData) { + t.Error(cmp.Diff(wantData, gotData)) + } } -func TestCollectData(t *testing.T) { +func TestCreateNewCollectorWithCustomReportingPeriod(t *testing.T) { t.Parallel() - ctx := context.Background() - mtr := &MockTelemetryReport{ - Data{}, + c, err := telemetry.NewCollector(telemetry.WithTimePeriod("4h")) + if err != nil { + t.Fatal(err) } - expectedData := Data{} - mtr.Start(ctx) - if !reflect.DeepEqual(mtr.data, expectedData) { - t.Fatalf("expected %v, but got %v", expectedData, mtr.data) + want := 4.0 + got := c.Period.Hours() + + if !cmp.Equal(want, got) { + t.Error(cmp.Diff(want, got)) } } From 9448ae91dc397161799d58a6abbba13a2ca3972c Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 23 Jan 2024 13:15:27 +0000 Subject: [PATCH 10/19] Simplify telemetry reporting flags --- cmd/nginx-ingress/flags.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index a2f53d50d7..f1d3e48413 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -17,10 +17,7 @@ import ( ) const ( - dynamicSSLReloadParam = "ssl-dynamic-reload" - enableTelemetryReportingParam = "enable-telemetry-reporting" - telemetryReportingPeriodParam = "telemetry-reporting-period" - defaultTelemetryReportingPeriod = "24h" + dynamicSSLReloadParam = "ssl-dynamic-reload" ) var ( @@ -205,9 +202,8 @@ var ( enableDynamicSSLReload = flag.Bool(dynamicSSLReloadParam, true, "Enable reloading of SSL Certificates without restarting the NGINX process.") - enableTelemetryReporting = flag.Bool(enableTelemetryReportingParam, true, "Enable gathering and reporting of product related telemetry.") - - telemetryReportingPeriod = flag.String(telemetryReportingPeriodParam, defaultTelemetryReportingPeriod, "Period at which product telemetry is reported.") + enableTelemetryReporting = flag.Bool("enable-telemetry-reporting", true, "Enable gathering and reporting of product related telemetry.") + telemetryReportingPeriod = flag.String("telemetry-reporting-period", "24h", "Period at which product telemetry is reported.") startupCheckFn func() error ) From dc634ac0b3ff4a5f0f71291b23f5f18a79744341 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 23 Jan 2024 15:19:37 +0000 Subject: [PATCH 11/19] Limit reporting period to min 1m --- cmd/nginx-ingress/flags.go | 6 +++++- cmd/nginx-ingress/main_test.go | 4 ++-- internal/k8s/controller.go | 1 + internal/telemetry/telemetry.go | 16 ++++++++-------- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index f1d3e48413..bf5f3bade2 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -1,6 +1,7 @@ package main import ( + "errors" "flag" "fmt" "net" @@ -502,9 +503,12 @@ func validateLocation(location string) error { // validateReportingPeriod checks if the reporting period parameter can be parsed. func validateReportingPeriod(period string) error { - _, err := time.ParseDuration(period) + duration, err := time.ParseDuration(period) if err != nil { return err } + if duration.Seconds() < 60 { + return errors.New("invalid reporting period, expected minimum 1m") + } return nil } diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index 35e95a04b9..be83fe45c7 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -176,7 +176,7 @@ func TestValidateNamespaces(t *testing.T) { func TestValidateReportingPeriodWithInvalidInput(t *testing.T) { t.Parallel() - periods := []string{"", "-1", "1x", "abc", "-"} + periods := []string{"", "-1", "1x", "abc", "-", "30s", "10ms"} for _, p := range periods { err := validateReportingPeriod(p) if err == nil { @@ -188,7 +188,7 @@ func TestValidateReportingPeriodWithInvalidInput(t *testing.T) { func TestValidateReportingPeriodWithValidInput(t *testing.T) { t.Parallel() - periods := []string{"1h", "24h", "30m"} + periods := []string{"1h", "24h", "30m", "1m"} for _, p := range periods { err := validateReportingPeriod(p) if err != nil { diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index d946293c4a..71c46d26dd 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -710,6 +710,7 @@ func (lbc *LoadBalancerController) Run() { go func(ctx context.Context) { select { case <-lbc.telemetryChan: + glog.V(3).Infof("Starting telemetry collector job") lbc.telemetryCollector.Run(lbc.ctx) case <-ctx.Done(): return diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 27c50e8a28..7ca196ef43 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -66,27 +66,26 @@ func NewCollector(opts ...Option) (*Collector, error) { return &c, nil } -// Start starts the telemetry reporting job -func (c *Collector) Start(ctx context.Context) { - wait.JitterUntilWithContext(ctx, c.Collect, c.Period, 0.1, true) -} - // BuildReport takes context and builds report from gathered telemetry data. func (c *Collector) BuildReport(_ context.Context) error { + glog.V(3).Info("Building telemetry report") dt := TraceData{} // TODO: Implement handling and logging errors for each collected data point c.mu.Lock() c.Data = dt + + glog.V(3).Infof("%+v", c.Data) c.mu.Unlock() return nil } // Collect runs data builder. func (c *Collector) Collect(ctx context.Context) { + glog.V(3).Info("Collecting telemetry data") if err := c.BuildReport(ctx); err != nil { - glog.Errorf("error exporting telemetry data: %v", err) + glog.Errorf("Error exporting telemetry data: %v", err) } } @@ -99,6 +98,8 @@ func (c *Collector) GetVSCount() int { } // GetTSCount returns number of TransportServers in watched namespaces. +// +// Note: this is a placeholder function. func (c *Collector) GetTSCount() int { // Placeholder function return 0 @@ -108,6 +109,5 @@ func (c *Collector) GetTSCount() int { // // This is a placeholder for implementing collector runner. func (c *Collector) Run(ctx context.Context) { - fn := func(ctx context.Context) {} - wait.JitterUntilWithContext(ctx, fn, c.Period, 0.1, true) + wait.JitterUntilWithContext(ctx, c.Collect, c.Period, 0.1, true) } From 3fa1756a104657c8b988d3f963b73903fd318067 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 24 Jan 2024 10:02:07 +0000 Subject: [PATCH 12/19] Set min reporting period to 1h --- cmd/nginx-ingress/flags.go | 4 ++-- cmd/nginx-ingress/main_test.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index bf5f3bade2..077e6a348c 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -507,8 +507,8 @@ func validateReportingPeriod(period string) error { if err != nil { return err } - if duration.Seconds() < 60 { - return errors.New("invalid reporting period, expected minimum 1m") + if duration.Minutes() < 60 { + return errors.New("invalid reporting period, expected minimum 1h") } return nil } diff --git a/cmd/nginx-ingress/main_test.go b/cmd/nginx-ingress/main_test.go index be83fe45c7..7c4ce0f117 100644 --- a/cmd/nginx-ingress/main_test.go +++ b/cmd/nginx-ingress/main_test.go @@ -176,7 +176,7 @@ func TestValidateNamespaces(t *testing.T) { func TestValidateReportingPeriodWithInvalidInput(t *testing.T) { t.Parallel() - periods := []string{"", "-1", "1x", "abc", "-", "30s", "10ms"} + periods := []string{"", "-1", "1x", "abc", "-", "30s", "10ms", "30m", "59m", "0h"} for _, p := range periods { err := validateReportingPeriod(p) if err == nil { @@ -188,7 +188,7 @@ func TestValidateReportingPeriodWithInvalidInput(t *testing.T) { func TestValidateReportingPeriodWithValidInput(t *testing.T) { t.Parallel() - periods := []string{"1h", "24h", "30m", "1m"} + periods := []string{"1h", "24h"} for _, p := range periods { err := validateReportingPeriod(p) if err != nil { From 922bbe6930710e232876cb64841a52436e844c69 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Wed, 24 Jan 2024 15:34:18 +0000 Subject: [PATCH 13/19] Use temp exporter for sending data --- internal/k8s/controller.go | 3 +- internal/telemetry/telemetry.go | 70 ++++++++++++++++++---------- internal/telemetry/telemetry_test.go | 28 ++++++++--- 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 71c46d26dd..7f72a1b168 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -710,8 +710,7 @@ func (lbc *LoadBalancerController) Run() { go func(ctx context.Context) { select { case <-lbc.telemetryChan: - glog.V(3).Infof("Starting telemetry collector job") - lbc.telemetryCollector.Run(lbc.ctx) + lbc.telemetryCollector.Start(lbc.ctx) case <-ctx.Done(): return } diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 7ca196ef43..e5f83da84c 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -3,16 +3,27 @@ package telemetry import ( "context" - "sync" + "fmt" + "io" "time" "github.com/golang/glog" "k8s.io/apimachinery/pkg/util/wait" ) -// Export takes context and data and sends it to the Otel endpoint. -func Export(_ context.Context, _ TraceData) error { +// DiscardExporter is a temporary exporter +// for discarding collected telemetry data. +var DiscardExporter = Exporter{Endpoint: io.Discard} + +// Exporter represents a temporary telemetry data exporter. +type Exporter struct { + Endpoint io.Writer +} + +// Export takes context and trace data and writes to the endpoint. +func (e *Exporter) Export(_ context.Context, td TraceData) error { // Note: exporting functionality will be implemented in a separate module. + fmt.Fprintf(e.Endpoint, "%+v", td) return nil } @@ -42,12 +53,24 @@ func WithTimePeriod(period string) Option { } } +// WithExporter configures telemetry collector to use given exporter. +// +// This may change in the future when we use exporter implemented +// in the external module. +func WithExporter(e Exporter) Option { + return func(c *Collector) error { + c.Exporter = e + return nil + } +} + // Collector is NIC telemetry data collector. type Collector struct { Period time.Duration - mu sync.Mutex - Data TraceData + // Exporter is a temp exporter for exporting telemetry data. + // The concrete implementation will be implemented in a separate module. + Exporter Exporter } // NewCollector takes 0 or more options and creates a new TraceReporter. @@ -55,8 +78,8 @@ type Collector struct { // configured to gather data every 24h. func NewCollector(opts ...Option) (*Collector, error) { c := Collector{ - Period: 24 * time.Hour, - Data: TraceData{}, + Period: 24 * time.Hour, + Exporter: DiscardExporter, // Use DiscardExporter until the real exporter is available. } for _, o := range opts { if err := o(&c); err != nil { @@ -67,26 +90,32 @@ func NewCollector(opts ...Option) (*Collector, error) { } // BuildReport takes context and builds report from gathered telemetry data. -func (c *Collector) BuildReport(_ context.Context) error { - glog.V(3).Info("Building telemetry report") +func (c *Collector) BuildReport(context.Context) (TraceData, error) { dt := TraceData{} // TODO: Implement handling and logging errors for each collected data point - c.mu.Lock() - c.Data = dt - - glog.V(3).Infof("%+v", c.Data) - c.mu.Unlock() - return nil + return dt, nil } -// Collect runs data builder. +// Collect collects and exports telemetry data. +// It exports data using provided exporter. func (c *Collector) Collect(ctx context.Context) { glog.V(3).Info("Collecting telemetry data") - if err := c.BuildReport(ctx); err != nil { + traceData, err := c.BuildReport(ctx) + if err != nil { + glog.Errorf("Error collecting telemetry data: %v", err) + } + err = c.Exporter.Export(ctx, traceData) + if err != nil { glog.Errorf("Error exporting telemetry data: %v", err) } + glog.V(3).Info("Exported telemetry data") +} + +// Start starts running NIC Telemetry Collector. +func (c *Collector) Start(ctx context.Context) { + wait.JitterUntilWithContext(ctx, c.Collect, c.Period, 0.1, true) } // GetVSCount returns number of VirtualServers in watched namespaces. @@ -104,10 +133,3 @@ func (c *Collector) GetTSCount() int { // Placeholder function return 0 } - -// Run starts running NIC Telemetry Collector. -// -// This is a placeholder for implementing collector runner. -func (c *Collector) Run(ctx context.Context) { - wait.JitterUntilWithContext(ctx, c.Collect, c.Period, 0.1, true) -} diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index f56e4d3491..99ca921ae7 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -1,6 +1,8 @@ package telemetry_test import ( + "bytes" + "context" "testing" "github.com/google/go-cmp/cmp" @@ -21,13 +23,6 @@ func TestCreateNewDefaultCollector(t *testing.T) { if !cmp.Equal(want, got) { t.Error(cmp.Diff(want, got)) } - - wantData := telemetry.TraceData{} - gotData := c.Data - - if !cmp.Equal(wantData, gotData) { - t.Error(cmp.Diff(wantData, gotData)) - } } func TestCreateNewCollectorWithCustomReportingPeriod(t *testing.T) { @@ -45,3 +40,22 @@ func TestCreateNewCollectorWithCustomReportingPeriod(t *testing.T) { t.Error(cmp.Diff(want, got)) } } + +func TestCreateNewCollectorWithCustomExporter(t *testing.T) { + t.Parallel() + + buf := &bytes.Buffer{} + exp := telemetry.Exporter{Endpoint: buf} + + c, err := telemetry.NewCollector(telemetry.WithExporter(exp)) + if err != nil { + t.Fatal(err) + } + c.Collect(context.Background()) + + want := "{VSCount:0 TSCount:0}" + got := buf.String() + if !cmp.Equal(want, got) { + t.Error(cmp.Diff(want, got)) + } +} From cf31f9716b38aee78319b94f69fb4c06a0f0ec6d Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Mon, 29 Jan 2024 12:03:02 +0000 Subject: [PATCH 14/19] Return fake nginx version --- internal/nginx/fake_manager.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/nginx/fake_manager.go b/internal/nginx/fake_manager.go index 1e42c51ed8..65ccbcc578 100644 --- a/internal/nginx/fake_manager.go +++ b/internal/nginx/fake_manager.go @@ -103,7 +103,7 @@ func (fm *FakeManager) CreateDHParam(_ string) (string, error) { // Version provides a fake implementation of Version. func (*FakeManager) Version() Version { glog.V(3).Info("Printing nginx version") - return Version{} + return NewVersion("nginx version: nginx/1.25.3 (nginx-plus-r31)") } // Start provides a fake implementation of Start. From e249b56e8ab6af1fd1757ed07e3f5c48e4d95332 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Wed, 31 Jan 2024 12:10:14 +0000 Subject: [PATCH 15/19] Revert nginx version check changes --- cmd/nginx-ingress/main.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index e01ee57265..9a0f5b714e 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -794,12 +794,9 @@ func processConfigMaps(kubeClient *kubernetes.Clientset, cfgParams *configs.Conf } func updateSelfWithVersionInfo(kubeClient *kubernetes.Clientset, version, nginxVersion, appProtectVersion string, maxRetries int, waitTime time.Duration) { - var nginxVer string - if nginxVersion != "" { - nginxVer = strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n") - replacer := strings.NewReplacer(" ", "-", "(", "", ")", "") - nginxVer = replacer.Replace(nginxVer) - } + nginxVer := strings.TrimSuffix(strings.Split(nginxVersion, "/")[1], "\n") + replacer := strings.NewReplacer(" ", "-", "(", "", ")", "") + nginxVer = replacer.Replace(nginxVer) podUpdated := false for i := 0; (i < maxRetries || maxRetries == 0) && !podUpdated; i++ { From e248aa9f1a8097b057752b39fa1403b9c6042d59 Mon Sep 17 00:00:00 2001 From: Jakub Jarosz Date: Tue, 6 Feb 2024 11:42:18 +0000 Subject: [PATCH 16/19] Set min reporting time to 1m Reportign param value 1 minute is set now for testing and demo purpose. It will be removed in v3.5 --- cmd/nginx-ingress/flags.go | 7 +++++-- cmd/nginx-ingress/flags_test.go | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 077e6a348c..24f5e4a83e 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -204,6 +204,7 @@ var ( enableDynamicSSLReload = flag.Bool(dynamicSSLReloadParam, true, "Enable reloading of SSL Certificates without restarting the NGINX process.") enableTelemetryReporting = flag.Bool("enable-telemetry-reporting", true, "Enable gathering and reporting of product related telemetry.") + // telemetryReportingPeriod exists only until NIC v3.5 is released. It is used only for demo and testing. telemetryReportingPeriod = flag.String("telemetry-reporting-period", "24h", "Period at which product telemetry is reported.") startupCheckFn func() error @@ -502,13 +503,15 @@ func validateLocation(location string) error { } // validateReportingPeriod checks if the reporting period parameter can be parsed. +// +// This function will be deprecated in NIC v3.5. It is used only for demo and testing purpose. func validateReportingPeriod(period string) error { duration, err := time.ParseDuration(period) if err != nil { return err } - if duration.Minutes() < 60 { - return errors.New("invalid reporting period, expected minimum 1h") + if duration.Minutes() < 1 { + return errors.New("invalid reporting period, expected minimum 1m") } return nil } diff --git a/cmd/nginx-ingress/flags_test.go b/cmd/nginx-ingress/flags_test.go index 7c4ce0f117..07a32a3f7f 100644 --- a/cmd/nginx-ingress/flags_test.go +++ b/cmd/nginx-ingress/flags_test.go @@ -176,7 +176,7 @@ func TestValidateNamespaces(t *testing.T) { func TestValidateReportingPeriodWithInvalidInput(t *testing.T) { t.Parallel() - periods := []string{"", "-1", "1x", "abc", "-", "30s", "10ms", "30m", "59m", "0h"} + periods := []string{"", "-1", "1x", "abc", "-", "30s", "10ms", "0h"} for _, p := range periods { err := validateReportingPeriod(p) if err == nil { @@ -188,7 +188,7 @@ func TestValidateReportingPeriodWithInvalidInput(t *testing.T) { func TestValidateReportingPeriodWithValidInput(t *testing.T) { t.Parallel() - periods := []string{"1h", "24h"} + periods := []string{"1m", "1h", "24h"} for _, p := range periods { err := validateReportingPeriod(p) if err != nil { From 2cae1742354ac7fbce77396024bb3707e595ffeb Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Tue, 6 Feb 2024 14:31:34 +0000 Subject: [PATCH 17/19] Set default reporting period and add unit test for new telemetry collector --- cmd/nginx-ingress/flags.go | 8 ------- cmd/nginx-ingress/main.go | 1 - internal/k8s/controller.go | 3 +-- internal/k8s/controller_test.go | 39 +++++++++++++++++++++++++++++++++ internal/telemetry/telemetry.go | 14 +++++------- 5 files changed, 45 insertions(+), 20 deletions(-) diff --git a/cmd/nginx-ingress/flags.go b/cmd/nginx-ingress/flags.go index 24f5e4a83e..6972c4104e 100644 --- a/cmd/nginx-ingress/flags.go +++ b/cmd/nginx-ingress/flags.go @@ -204,8 +204,6 @@ var ( enableDynamicSSLReload = flag.Bool(dynamicSSLReloadParam, true, "Enable reloading of SSL Certificates without restarting the NGINX process.") enableTelemetryReporting = flag.Bool("enable-telemetry-reporting", true, "Enable gathering and reporting of product related telemetry.") - // telemetryReportingPeriod exists only until NIC v3.5 is released. It is used only for demo and testing. - telemetryReportingPeriod = flag.String("telemetry-reporting-period", "24h", "Period at which product telemetry is reported.") startupCheckFn func() error ) @@ -390,12 +388,6 @@ func validationChecks() { glog.Fatalf("Invalid value for app-protect-log-level: %v", *appProtectLogLevel) } } - - if telemetryReportingPeriod != nil { - if err := validateReportingPeriod(*telemetryReportingPeriod); err != nil { - glog.Fatalf("Invalid value for telemetry-reporting-period: %v", err) - } - } } // validateNamespaceNames validates the namespaces are in the correct format diff --git a/cmd/nginx-ingress/main.go b/cmd/nginx-ingress/main.go index 086f35f45c..5df8f2c17b 100644 --- a/cmd/nginx-ingress/main.go +++ b/cmd/nginx-ingress/main.go @@ -201,7 +201,6 @@ func main() { ExternalDNSEnabled: *enableExternalDNS, IsIPV6Disabled: *disableIPV6, WatchNamespaceLabel: *watchNamespaceLabel, - TelemetryReportPeriod: *telemetryReportingPeriod, EnableTelemetryReporting: *enableTelemetryReporting, } diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 7f72a1b168..413c20b68d 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -210,7 +210,6 @@ type NewLoadBalancerControllerInput struct { ExternalDNSEnabled bool IsIPV6Disabled bool WatchNamespaceLabel string - TelemetryReportPeriod string EnableTelemetryReporting bool } @@ -281,7 +280,7 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc if input.EnableTelemetryReporting { lbc.telemetryChan = make(chan struct{}) collector, err := telemetry.NewCollector( - telemetry.WithTimePeriod(input.TelemetryReportPeriod), + telemetry.WithTimePeriod(24 * time.Hour), ) if err != nil { glog.Fatalf("failed to initialize telemetry collector: %v", err) diff --git a/internal/k8s/controller_test.go b/internal/k8s/controller_test.go index 4987336e3e..67b32a73d7 100644 --- a/internal/k8s/controller_test.go +++ b/internal/k8s/controller_test.go @@ -3,10 +3,12 @@ package k8s import ( "errors" "fmt" + "github.com/nginxinc/kubernetes-ingress/internal/telemetry" "reflect" "sort" "strings" "testing" + "time" discovery_v1 "k8s.io/api/discovery/v1" @@ -3747,3 +3749,40 @@ func TestPreSyncSecrets(t *testing.T) { t.Errorf("GetSecret(%q) returned a reference without an expected error", unsupportedKey) } } + +func TestNewTelemetryCollector(t *testing.T) { + t.Parallel() + + testCases := []struct { + testCase string + input NewLoadBalancerControllerInput + expectedCollector telemetry.Collector + }{ + { + testCase: "New Telemetry Collector with default values", + input: NewLoadBalancerControllerInput{ + KubeClient: fake.NewSimpleClientset(), + EnableTelemetryReporting: true, + }, + expectedCollector: telemetry.Collector{ + Period: 24 * time.Hour, + Exporter: telemetry.DiscardExporter, + }, + }, + { + testCase: "New Telemetry Collector with Telemetry Reporting set to false", + input: NewLoadBalancerControllerInput{ + KubeClient: fake.NewSimpleClientset(), + EnableTelemetryReporting: false, + }, + expectedCollector: telemetry.Collector{}, + }, + } + + for _, tc := range testCases { + lbc := NewLoadBalancerController(tc.input) + if reflect.DeepEqual(tc.expectedCollector, lbc.telemetryCollector) { + t.Fatalf("Expected %x, but got %x", tc.expectedCollector, lbc.telemetryCollector) + } + } +} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index e5f83da84c..8bc2e166f8 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -29,9 +29,9 @@ func (e *Exporter) Export(_ context.Context, td TraceData) error { // TraceData holds collected NIC telemetry data. type TraceData struct { - // Numer of VirtualServers + // Count of VirtualServers VSCount int - // Number of TransportServers + // Count of TransportServers TSCount int // TODO @@ -42,13 +42,9 @@ type TraceData struct { type Option func(*Collector) error // WithTimePeriod configures reporting time on TraceReporter. -func WithTimePeriod(period string) Option { +func WithTimePeriod(period time.Duration) Option { return func(c *Collector) error { - d, err := time.ParseDuration(period) - if err != nil { - return err - } - c.Period = d + c.Period = period return nil } } @@ -110,7 +106,7 @@ func (c *Collector) Collect(ctx context.Context) { if err != nil { glog.Errorf("Error exporting telemetry data: %v", err) } - glog.V(3).Info("Exported telemetry data") + glog.V(3).Infof("Exported telemetry data: %x", traceData) } // Start starts running NIC Telemetry Collector. From d401d496c134908c2eb0d1ccfa264101fdf964e9 Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Tue, 6 Feb 2024 14:51:02 +0000 Subject: [PATCH 18/19] Add telemetry reporting flag to helm values --- charts/nginx-ingress/README.md | 3 ++- charts/nginx-ingress/templates/_helpers.tpl | 1 + charts/nginx-ingress/values.schema.json | 8 ++++++++ charts/nginx-ingress/values.yaml | 3 +++ 4 files changed, 14 insertions(+), 1 deletion(-) diff --git a/charts/nginx-ingress/README.md b/charts/nginx-ingress/README.md index 2804458bf5..ceeb03bc9e 100644 --- a/charts/nginx-ingress/README.md +++ b/charts/nginx-ingress/README.md @@ -464,9 +464,10 @@ The following tables lists the configurable parameters of the NGINX Ingress Cont |`controller.strategy` | Specifies the strategy used to replace old Pods with new ones. Docs for [Deployment update strategy](https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#strategy) and [Daemonset update strategy](https://kubernetes.io/docs/tasks/manage-daemon/update-daemon-set/#daemonset-update-strategy) | {} | |`controller.disableIPV6` | Disable IPV6 listeners explicitly for nodes that do not support the IPV6 stack. | false | |`controller.defaultHTTPListenerPort` | Sets the port for the HTTP `default_server` listener. | 80 | -|`controller.defaultHTTPSListenerPort` | Sets the port for the HTTPS `default_server` listener. | 443 | +|`controller.defaultHTTPSListenerPort` | Sets the port for the HTTPS `default_server` listener. | 443 | |`controller.readOnlyRootFilesystem` | Configure root filesystem as read-only and add volumes for temporary data. | false | |`controller.enableSSLDynamicReload` | Enable lazy loading for SSL Certificates. | true | +|`controller.enableTelemetryReporting` | Enable telemetry reporting. | true | |`rbac.create` | Configures RBAC. | true | |`prometheus.create` | Expose NGINX or NGINX Plus metrics in the Prometheus format. | true | |`prometheus.port` | Configures the port to scrape the metrics. | 9113 | diff --git a/charts/nginx-ingress/templates/_helpers.tpl b/charts/nginx-ingress/templates/_helpers.tpl index 2f5add833d..88a3d5c2bc 100644 --- a/charts/nginx-ingress/templates/_helpers.tpl +++ b/charts/nginx-ingress/templates/_helpers.tpl @@ -223,4 +223,5 @@ Build the args for the service binary. - -ready-status-port={{ .Values.controller.readyStatus.port }} - -enable-latency-metrics={{ .Values.controller.enableLatencyMetrics }} - -ssl-dynamic-reload={{ .Values.controller.enableSSLDynamicReload }} +- -enable-telemetry-reporting={{ .Values.controller.enableTelemetryReporting}} {{- end -}} diff --git a/charts/nginx-ingress/values.schema.json b/charts/nginx-ingress/values.schema.json index b0fd55061f..15ceaeeec1 100644 --- a/charts/nginx-ingress/values.schema.json +++ b/charts/nginx-ingress/values.schema.json @@ -1367,6 +1367,14 @@ "examples": [ true ] + }, + "enableTelemetryReporting": { + "type": "boolean", + "default": true, + "title": "Enable telemetry reporting", + "examples": [ + true + ] } }, "examples": [ diff --git a/charts/nginx-ingress/values.yaml b/charts/nginx-ingress/values.yaml index 8b1571b197..5e98237194 100644 --- a/charts/nginx-ingress/values.yaml +++ b/charts/nginx-ingress/values.yaml @@ -465,6 +465,9 @@ controller: ## Enable dynamic reloading of certificates enableSSLDynamicReload: true + ## Enable telemetry reporting + enableTelemetryReporting: true + rbac: ## Configures RBAC. create: true From 1d0423660bf0f5ac63493069ebb69b7e65b4078d Mon Sep 17 00:00:00 2001 From: shaun-nx Date: Tue, 6 Feb 2024 16:20:13 +0000 Subject: [PATCH 19/19] Fix telemetry unit test --- internal/k8s/controller.go | 2 +- internal/k8s/controller_test.go | 3 ++- internal/telemetry/telemetry.go | 8 ++++++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go index 413c20b68d..b99b2e66d7 100644 --- a/internal/k8s/controller.go +++ b/internal/k8s/controller.go @@ -280,7 +280,7 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc if input.EnableTelemetryReporting { lbc.telemetryChan = make(chan struct{}) collector, err := telemetry.NewCollector( - telemetry.WithTimePeriod(24 * time.Hour), + telemetry.WithTimePeriod("24h"), ) if err != nil { glog.Fatalf("failed to initialize telemetry collector: %v", err) diff --git a/internal/k8s/controller_test.go b/internal/k8s/controller_test.go index 67b32a73d7..d34eef47c6 100644 --- a/internal/k8s/controller_test.go +++ b/internal/k8s/controller_test.go @@ -3,13 +3,14 @@ package k8s import ( "errors" "fmt" - "github.com/nginxinc/kubernetes-ingress/internal/telemetry" "reflect" "sort" "strings" "testing" "time" + "github.com/nginxinc/kubernetes-ingress/internal/telemetry" + discovery_v1 "k8s.io/api/discovery/v1" "github.com/google/go-cmp/cmp" diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 8bc2e166f8..a1c1a5272e 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -42,9 +42,13 @@ type TraceData struct { type Option func(*Collector) error // WithTimePeriod configures reporting time on TraceReporter. -func WithTimePeriod(period time.Duration) Option { +func WithTimePeriod(period string) Option { return func(c *Collector) error { - c.Period = period + d, err := time.ParseDuration(period) + if err != nil { + return err + } + c.Period = d return nil } }