Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Telemetry Job #4896

Merged
merged 29 commits into from
Feb 7, 2024
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
838c94a
Add base telemetry job
shaun-nx Jan 5, 2024
ca9b9f7
Ensure only leader pod reports data
shaun-nx Jan 9, 2024
5e3d349
Allow deployments to opt-out of telemetry collection
shaun-nx Jan 10, 2024
a9224db
Add log line for when telemetry is collected
shaun-nx Jan 10, 2024
54b896b
gofumpt files
shaun-nx Jan 10, 2024
8011a08
Revert deployment yaml and fake manager
shaun-nx Jan 10, 2024
079a906
Fix nginx version assignment
shaun-nx Jan 10, 2024
3a69f51
Merge branch 'main' into feat/telemetry
shaun-nx Jan 10, 2024
82a91a6
Resolve lint issues
shaun-nx Jan 10, 2024
ff7515e
Placeholder for telemetry collector
jjngx Jan 22, 2024
9448ae9
Simplify telemetry reporting flags
jjngx Jan 23, 2024
dc634ac
Limit reporting period to min 1m
jjngx Jan 23, 2024
3fa1756
Set min reporting period to 1h
jjngx Jan 24, 2024
c78e3fd
Merge branch 'main' into feat/telemetry
jjngx Jan 24, 2024
922bbe6
Use temp exporter for sending data
jjngx Jan 24, 2024
da52eed
Merge branch 'main' into feat/telemetry
jjngx Jan 24, 2024
2b8e4ad
Merge branch 'main' into feat/telemetry
jjngx Jan 25, 2024
301ed1e
Merge branch 'main' into feat/telemetry
jjngx Jan 26, 2024
cf31f97
Return fake nginx version
jjngx Jan 29, 2024
e249b56
Revert nginx version check changes
shaun-nx Jan 31, 2024
22ca1e8
Merge branch 'main' into feat/telemetry
shaun-nx Jan 31, 2024
37bf9fe
Merge branch 'main' into feat/telemetry
jjngx Feb 6, 2024
e248aa9
Set min reporting time to 1m
jjngx Feb 6, 2024
ec8678d
Merge branch 'main' into feat/telemetry
jjngx Feb 6, 2024
a56bc49
Merge branch 'main' into feat/telemetry
jjngx Feb 6, 2024
2cae174
Set default reporting period and add unit test for new telemetry coll…
shaun-nx Feb 6, 2024
d401d49
Add telemetry reporting flag to helm values
shaun-nx Feb 6, 2024
1d04236
Fix telemetry unit test
shaun-nx Feb 6, 2024
cebd70c
Merge branch 'main' into feat/telemetry
shaun-nx Feb 6, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion charts/nginx-ingress/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
1 change: 1 addition & 0 deletions charts/nginx-ingress/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -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 -}}
8 changes: 8 additions & 0 deletions charts/nginx-ingress/values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1367,6 +1367,14 @@
"examples": [
true
]
},
"enableTelemetryReporting": {
"type": "boolean",
"default": true,
"title": "Enable telemetry reporting",
"examples": [
true
]
}
},
"examples": [
Expand Down
3 changes: 3 additions & 0 deletions charts/nginx-ingress/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,9 @@ controller:
## Enable dynamic reloading of certificates
enableSSLDynamicReload: true

## Enable telemetry reporting
enableTelemetryReporting: true

rbac:
## Configures RBAC.
create: true
Expand Down
18 changes: 18 additions & 0 deletions cmd/nginx-ingress/flags.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package main

import (
"errors"
"flag"
"fmt"
"net"
"os"
"regexp"
"strconv"
"strings"
"time"

"github.com/golang/glog"
api_v1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -201,6 +203,8 @@ 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.")
oseoin marked this conversation as resolved.
Show resolved Hide resolved

startupCheckFn func() error
)

Expand Down Expand Up @@ -489,3 +493,17 @@ func validateLocation(location string) error {
}
return nil
}

// 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() < 1 {
return errors.New("invalid reporting period, expected minimum 1m")
}
return nil
}
24 changes: 24 additions & 0 deletions cmd/nginx-ingress/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,3 +172,27 @@ func TestValidateNamespaces(t *testing.T) {
}
}
}

func TestValidateReportingPeriodWithInvalidInput(t *testing.T) {
t.Parallel()

periods := []string{"", "-1", "1x", "abc", "-", "30s", "10ms", "0h"}
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{"1m", "1h", "24h"}
for _, p := range periods {
err := validateReportingPeriod(p)
if err != nil {
t.Error(err)
}
}
}
5 changes: 4 additions & 1 deletion cmd/nginx-ingress/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ import (
)

// Injected during build
var version string
var (
version string
)

const (
nginxVersionLabel = "app.nginx.org/version"
Expand Down Expand Up @@ -199,6 +201,7 @@ func main() {
ExternalDNSEnabled: *enableExternalDNS,
IsIPV6Disabled: *disableIPV6,
WatchNamespaceLabel: *watchNamespaceLabel,
EnableTelemetryReporting: *enableTelemetryReporting,
}

lbc := k8s.NewLoadBalancerController(lbcInput)
Expand Down
29 changes: 29 additions & 0 deletions internal/k8s/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ import (
"sync"
"time"

"github.com/nginxinc/kubernetes-ingress/internal/telemetry"

"github.com/nginxinc/kubernetes-ingress/pkg/apis/dos/v1beta1"
"golang.org/x/exp/maps"

Expand Down Expand Up @@ -161,6 +163,8 @@ type LoadBalancerController struct {
enableBatchReload bool
isIPV6Disabled bool
namespaceWatcherController cache.Controller
telemetryCollector *telemetry.Collector
telemetryChan chan struct{}
}

var keyFunc = cache.DeletionHandlingMetaNamespaceKeyFunc
Expand Down Expand Up @@ -206,6 +210,7 @@ type NewLoadBalancerControllerInput struct {
ExternalDNSEnabled bool
IsIPV6Disabled bool
WatchNamespaceLabel string
EnableTelemetryReporting bool
}

// NewLoadBalancerController creates a controller
Expand Down Expand Up @@ -271,6 +276,18 @@ 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 {
lbc.telemetryChan = make(chan struct{})
collector, err := telemetry.NewCollector(
telemetry.WithTimePeriod(24 * time.Hour),
)
if err != nil {
glog.Fatalf("failed to initialize telemetry collector: %v", err)
}
lbc.telemetryCollector = collector
}

glog.V(3).Infof("Nginx Ingress Controller has class: %v", input.IngressClass)

lbc.namespacedInformers = make(map[string]*namespacedInformer)
Expand Down Expand Up @@ -683,10 +700,22 @@ 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)
}

if lbc.telemetryCollector != nil {
go func(ctx context.Context) {
select {
case <-lbc.telemetryChan:
lbc.telemetryCollector.Start(lbc.ctx)
case <-ctx.Done():
return
}
}(lbc.ctx)
}

for _, nif := range lbc.namespacedInformers {
nif.start()
}
Expand Down
39 changes: 39 additions & 0 deletions internal/k8s/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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)
}
}
}
4 changes: 4 additions & 0 deletions internal/k8s/leader.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ 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
if lbc.telemetryChan != nil {
close(lbc.telemetryChan)
}
if lbc.reportIngressStatus {
ingresses := lbc.configuration.GetResourcesWithFilter(resourceFilter{Ingresses: true})

Expand Down
2 changes: 1 addition & 1 deletion internal/nginx/fake_manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)")
jjngx marked this conversation as resolved.
Show resolved Hide resolved
}

// Start provides a fake implementation of Start.
Expand Down
Loading
Loading