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

Update Telemetry Job with GRPC endpoint #5237

Merged
merged 16 commits into from
Mar 14, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
6 changes: 3 additions & 3 deletions charts/nginx-ingress/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ The following tables lists the configurable parameters of the NGINX Ingress Cont
|`controller.replicaCount` | The number of replicas of the Ingress Controller deployment. | 1 |
|`controller.ingressClass.name` | A class of the Ingress Controller. An IngressClass resource with the name equal to the class must be deployed. Otherwise, the Ingress Controller will fail to start. The Ingress Controller only processes resources that belong to its class - i.e. have the "ingressClassName" field resource equal to the class. The Ingress Controller processes all the VirtualServer/VirtualServerRoute/TransportServer resources that do not have the "ingressClassName" field for all versions of Kubernetes. | nginx |
|`controller.ingressClass.create` | Creates a new IngressClass object with the name `controller.ingressClass.name`. Set to `false` to use an existing ingressClass created using `kubectl` with the same name. If you use `helm upgrade`, do not change the values from the previous release as helm will delete IngressClass objects managed by helm. If you are upgrading from a release earlier than 3.4.3, do not set the value to false. | true |
|`controller.ingressClass.setAsDefaultIngress` | New Ingresses without an `"ingressClassName"` field specified will be assigned the class specified in `controller.ingressClass.name`. Requires `controller.ingressClass.create`. | false |
|`controller.ingressClass.setAsDefaultIngress` | New Ingresses without an `"ingressClassName"` field specified will be assigned the class specified in `controller.ingressClass.name`. Requires `controller.ingressClass.create`. | false |
|`controller.watchNamespace` | Comma separated list of namespaces the Ingress Controller should watch for resources. By default the Ingress Controller watches all namespaces. Mutually exclusive with `controller.watchNamespaceLabel`. Please note that if configuring multiple namespaces using the Helm cli `--set` option, the string needs to wrapped in double quotes and the commas escaped using a backslash - e.g. `--set controller.watchNamespace="default\,nginx-ingress"`. | "" |
|`controller.watchNamespaceLabel` | Configures the Ingress Controller to watch only those namespaces with label foo=bar. By default the Ingress Controller watches all namespaces. Mutually exclusive with `controller.watchNamespace`. | "" |
|`controller.watchSecretNamespace` | Comma separated list of namespaces the Ingress Controller should watch for resources of type Secret. If this arg is not configured, the Ingress Controller watches the same namespaces for all resources. See `controller.watchNamespace` and `controller.watchNamespaceLabel`. Please note that if configuring multiple namespaces using the Helm cli `--set` option, the string needs to wrapped in double quotes and the commas escaped using a backslash - e.g. `--set controller.watchSecretNamespace="default\,nginx-ingress"`. | "" |
Expand Down Expand Up @@ -466,11 +466,11 @@ The following tables lists the configurable parameters of the NGINX Ingress Cont
|`controller.podDisruptionBudget.maxUnavailable` | The number of Ingress Controller pods that can be unavailable. This is a mutually exclusive setting with "minAvailable". | 0 |
|`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.defaultHTTPListenerPort` | Sets the port for the HTTP `default_server` listener. | 80 |
|`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. Three major releases after 3.5.x this argument will be moved permanently to the `controller.securityContext` section. | false |
|`controller.enableSSLDynamicReload` | Enable lazy loading for SSL Certificates. | true |
|`controller.enableTelemetryReporting` | Enable telemetry reporting. | true |
|`controller.telemetryReporting.enable` | Enable telemetry reporting. | true |
shaun-nx marked this conversation as resolved.
Show resolved Hide resolved
|`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
2 changes: 1 addition & 1 deletion charts/nginx-ingress/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,7 @@ 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}}
- -enable-telemetry-reporting={{ .Values.controller.telemetryReporting.enable}}
{{- if .Values.nginxAgent.enable }}
- -agent=true
- -agent-instance-group={{ default (include "nginx-ingress.controller.fullname" .) .Values.nginxAgent.instanceGroup }}
Expand Down
22 changes: 15 additions & 7 deletions charts/nginx-ingress/values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -1417,13 +1417,21 @@
true
]
},
"enableTelemetryReporting": {
"type": "boolean",
"default": true,
"title": "Enable telemetry reporting",
"examples": [
true
]
"telemetryReporting": {
"type": "object",
"default": {},
"title": "Configure telemetry reporting options",
"required": [],
"properties": {
"enable": {
"type": "boolean",
"default": true,
"title": "Enable telemetry reporting",
"examples": [
true
]
},
}
}
},
"examples": [
Expand Down
6 changes: 4 additions & 2 deletions charts/nginx-ingress/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -487,8 +487,10 @@ controller:
## Enable dynamic reloading of certificates
enableSSLDynamicReload: true

## Enable telemetry reporting
enableTelemetryReporting: true
## Configure telemetry reporting options
telemetryReporting:
oseoin marked this conversation as resolved.
Show resolved Hide resolved
## Enable telemetry reporting
enable: true

rbac:
## Configures RBAC.
Expand Down
23 changes: 0 additions & 23 deletions cmd/nginx-ingress/flags.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,13 @@
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 @@ -207,7 +205,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 = flag.String("telemetry-reporting-period", "24h", "Sets a telemetry reporting period.")

startupCheckFn func() error
)
Expand Down Expand Up @@ -396,12 +393,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
Expand Down Expand Up @@ -507,17 +498,3 @@ 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: 0 additions & 24 deletions cmd/nginx-ingress/flags_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,27 +172,3 @@ 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)
}
}
}
1 change: 0 additions & 1 deletion cmd/nginx-ingress/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,6 @@ func main() {
IsIPV6Disabled: *disableIPV6,
WatchNamespaceLabel: *watchNamespaceLabel,
EnableTelemetryReporting: *enableTelemetryReporting,
TelemetryReportingPeriod: *telemetryReportingPeriod,
NICVersion: version,
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,14 @@ The default value is `true`.

<a name="cmdoption-ssl-dynamic-reload"></a>

### -enable-telemetry-reporting

Enable gathering and reporting of product related telemetry.
shaun-nx marked this conversation as resolved.
Show resolved Hide resolved

The default value is `true`.

<a name="cmdoption-enable-telemetry-reporting"></a>

### -agent

Enable NGINX Agent which can used with `-enable-app-protect` to send events to Security Monitoring.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ h2 {
- A [Kubernetes Version Supported by the Ingress Controller](https://docs.nginx.com/nginx-ingress-controller/technical-specifications/#supported-kubernetes-versions)
- Helm 3.0+.
- If you’d like to use NGINX Plus:
- To pull from the F5 Container registry, configure a docker registry secret using your JWT token from the MyF5 portal by following the instructions from [here](https://docs.nginx.com/nginx-ingress-controller/installation/nic-images/using-the-jwt-token-docker-secret). Make sure to specify the secret using `controller.serviceAccount.imagePullSecretName` or `controller.serviceAccount.imagePullSecretsNames` parameter.
- Alternatively, pull an Ingress Controller image with NGINX Plus and push it to your private registry by following the instructions from [here]({{< relref "installation/nic-images/pulling-ingress-controller-image" >}}).
- Alternatively, you can build an Ingress Controller image with NGINX Plus and push it to your private registry by following the instructions from [here]({{< relref "installation/building-nginx-ingress-controller.md" >}}).
- Update the `controller.image.repository` field of the `values-plus.yaml` accordingly.
- To pull from the F5 Container registry, configure a docker registry secret using your JWT token from the MyF5 portal by following the instructions from [here](https://docs.nginx.com/nginx-ingress-controller/installation/nic-images/using-the-jwt-token-docker-secret). Make sure to specify the secret using `controller.serviceAccount.imagePullSecretName` or `controller.serviceAccount.imagePullSecretsNames` parameter.
- Alternatively, pull an Ingress Controller image with NGINX Plus and push it to your private registry by following the instructions from [here]({{< relref "installation/nic-images/pulling-ingress-controller-image" >}}).
- Alternatively, you can build an Ingress Controller image with NGINX Plus and push it to your private registry by following the instructions from [here]({{< relref "installation/building-nginx-ingress-controller.md" >}}).
shaun-nx marked this conversation as resolved.
Show resolved Hide resolved
- Update the `controller.image.repository` field of the `values-plus.yaml` accordingly.
- If you’d like to use App Protect DoS, please install App Protect DoS Arbitrator [helm chart](https://github.com/nginxinc/nap-dos-arbitrator-helm-chart). Make sure to install in the same namespace as the NGINX Ingress Controller. Note that if you install multiple NGINX Ingress Controllers in the same namespace, they will need to share the same Arbitrator because it is not possible to install more than one Arbitrator in a single namespace.
shaun-nx marked this conversation as resolved.
Show resolved Hide resolved

## CRDs
Expand Down Expand Up @@ -204,7 +204,7 @@ The steps you should follow depend on the Helm release name:
kubectl describe deployments -n <namespace>
```

Copy the key=value under `Selector`, such as:
Copy the key=value under `Selector`, such as:

```shell
Selector: app=nginx-ingress-nginx-ingress
Expand All @@ -225,7 +225,7 @@ The steps you should follow depend on the Helm release name:
--set controller.name=""
--set fullnameOverride="nginx-ingress-nginx-ingress"
```
It could look as follows:
It could look as follows:

```shell
helm upgrade nginx-ingress oci://ghcr.io/nginxinc/charts/nginx-ingress --version 0.19.0 --set controller.kind=deployment/daemonset --set controller.nginxplus=false/true --set controller.image.pullPolicy=Always --set serviceNameOverride="nginx-ingress-nginx-ingress" --set controller.name="" --set fullnameOverride="nginx-ingress-nginx-ingress" -f values.yaml
Expand All @@ -249,7 +249,7 @@ The steps you should follow depend on the Helm release name:
kubectl describe deployment/daemonset -n <namespace>
```

Copy the key=value under ```Selector```, such as:
Copy the key=value under ```Selector```, such as:

```shell
Selector: app=<helm_release_name>-nginx-ingress
Expand All @@ -272,7 +272,7 @@ The steps you should follow depend on the Helm release name:
--set controller.name=""
```

It could look as follows:
It could look as follows:

```shell
helm upgrade test-release oci://ghcr.io/nginxinc/charts/nginx-ingress --version 0.19.0 --set controller.kind=deployment/daemonset --set controller.nginxplus=false/true --set controller.image.pullPolicy=Always --set serviceNameOverride="test-release-nginx-ingress" --set controller.name="" -f values.yaml
Expand Down Expand Up @@ -433,6 +433,7 @@ The following tables lists the configurable parameters of the NGINX Ingress Cont
| **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. Three major releases after 3.5.x this argument will be moved permanently to the `controller.securityContext` section. | false |
| **controller.enableSSLDynamicReload** | Enable lazy loading for SSL Certificates. | true |
| **controller.telemetryReporting.enable** | 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
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ require (
github.com/spiffe/go-spiffe/v2 v2.1.7
github.com/stretchr/testify v1.9.0
go.opentelemetry.io/otel v1.24.0
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.24.0
golang.org/x/exp v0.0.0-20231226003508-02704c960a9b
k8s.io/api v0.29.2
k8s.io/apimachinery v0.29.2
Expand Down Expand Up @@ -101,7 +102,6 @@ require (
go.opentelemetry.io/contrib/instrumentation/google.golang.org/grpc/otelgrpc v0.46.1 // indirect
go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.46.1 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace v1.24.0 // indirect
go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc v1.24.0 // indirect
go.opentelemetry.io/otel/metric v1.24.0 // indirect
go.opentelemetry.io/otel/sdk v1.24.0 // indirect
go.opentelemetry.io/otel/trace v1.24.0 // indirect
Expand Down
16 changes: 12 additions & 4 deletions internal/k8s/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"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 @@ -211,7 +210,6 @@ type NewLoadBalancerControllerInput struct {
IsIPV6Disabled bool
WatchNamespaceLabel string
EnableTelemetryReporting bool
TelemetryReportingPeriod string
NICVersion string
}

Expand Down Expand Up @@ -347,21 +345,31 @@ func NewLoadBalancerController(input NewLoadBalancerControllerInput) *LoadBalanc

// NIC Telemetry Reporting
if input.EnableTelemetryReporting {

exporterCfg := telemetry.ExporterCfg{
Endpoint: "oss.edge.df.f5.com:443",
}

exporter, err := telemetry.NewExporter(exporterCfg)
if err != nil {
glog.Fatalf("failed to initialize telemetry exporter: %v", err)
}
collectorConfig := telemetry.CollectorConfig{
K8sClientReader: input.KubeClient,
CustomK8sClientReader: input.ConfClient,
Period: 5 * time.Second,
Period: 24 * time.Hour,
Configurator: lbc.configurator,
Version: input.NICVersion,
}
lbc.telemetryChan = make(chan struct{})
collector, err := telemetry.NewCollector(
collectorConfig,
telemetry.WithExporter(exporter),
)
if err != nil {
glog.Fatalf("failed to initialize telemetry collector: %v", err)
}
lbc.telemetryCollector = collector
lbc.telemetryChan = make(chan struct{})
}

return lbc
Expand Down
15 changes: 14 additions & 1 deletion internal/k8s/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3758,6 +3758,20 @@ func TestPreSyncSecrets(t *testing.T) {
}
}

//func TestNewTelemetryExporter(t *testing.T) {
shaun-nx marked this conversation as resolved.
Show resolved Hide resolved
// t.Parallel()
//
// testCases := []struct {
// testCase string
// input NewLoadBalancerControllerInput
// expectedExporter telemetry.Exporter
// }{
// {
//
// },
// }
//}

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

Expand All @@ -3772,7 +3786,6 @@ func TestNewTelemetryCollector(t *testing.T) {
input: NewLoadBalancerControllerInput{
KubeClient: fake.NewSimpleClientset(),
EnableTelemetryReporting: true,
TelemetryReportingPeriod: "24h",
},
expectedCollector: telemetry.Collector{
Config: telemetry.CollectorConfig{
Expand Down
2 changes: 1 addition & 1 deletion internal/telemetry/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ func (c *Collector) Collect(ctx context.Context) {
if err != nil {
glog.Errorf("Error exporting telemetry data: %v", err)
}
glog.V(3).Infof("Exported telemetry data: %+v", nicData)
glog.V(3).Infof("Telemetry data collected: %+v", nicData)
}

// Report holds collected NIC telemetry data. It is the package internal
Expand Down
26 changes: 26 additions & 0 deletions internal/telemetry/exporter.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"
"io"

"go.opentelemetry.io/otel/exporters/otlp/otlptrace/otlptracegrpc"

tel "github.com/nginxinc/telemetry-exporter/pkg/telemetry"
)

Expand All @@ -24,6 +26,30 @@ func (e *StdoutExporter) Export(_ context.Context, data tel.Exportable) error {
return nil
}

// ExporterCfg is a configuration struct for an Exporter.
type ExporterCfg struct {
Endpoint string
}

// NewExporter creates an Exporter with the provided ExporterCfg.
func NewExporter(cfg ExporterCfg) (Exporter, error) {
providerOptions := []otlptracegrpc.Option{
otlptracegrpc.WithEndpoint(cfg.Endpoint),
// This header option will be removed when https://github.com/nginxinc/telemetry-exporter/issues/41 is resolved.
otlptracegrpc.WithHeaders(map[string]string{
"X-F5-OTEL": "GRPC",
}),
}

exporter, err := tel.NewExporter(
tel.ExporterConfig{
SpanProvider: tel.CreateOTLPSpanProvider(providerOptions...),
},
)

return exporter, err
}

// Data holds collected telemetry data.
//
//go:generate go run -tags=generator github.com/nginxinc/telemetry-exporter/cmd/generator -type Data -scheme -scheme-protocol=NICProductTelemetry -scheme-df-datatype=nic-product-telemetry -scheme-namespace=ingress.nginx.com
Expand Down
5 changes: 4 additions & 1 deletion tests/suite/utils/resources_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1182,7 +1182,10 @@ def create_ingress_controller(v1: CoreV1Api, apps_v1_api: AppsV1Api, cli_argumen
dep["spec"]["template"]["spec"]["containers"][0]["image"] = cli_arguments["image"]
dep["spec"]["template"]["spec"]["containers"][0]["imagePullPolicy"] = cli_arguments["image-pull-policy"]
dep["spec"]["template"]["spec"]["containers"][0]["args"].extend(
["-default-server-tls-secret=$(POD_NAMESPACE)/default-server-secret"]
[
f"-default-server-tls-secret=$(POD_NAMESPACE)/default-server-secret",
f"-enable-telemetry-reporting=false",
]
)
if args is not None:
dep["spec"]["template"]["spec"]["containers"][0]["args"].extend(args)
Expand Down
Loading