Remote otel collector configuration #934

jvanz · 2024-12-06T18:49:04Z

Description

Updates the collector to allow the communication with a remote OpenTelemetry collector. Also updates the policy server reconciler to replicate the same configuration in the policy server deployment enabling it to send data to the same remote Otel collector.

Fix #933

Tests

Warning

This PR was tested together with the changes from this Helm chart changes

Create a simple cluster with required dependencies:

k3d cluster create 

helm repo add jetstack https://charts.jetstack.io --force-update
helm upgrade -i --wait cert-manager jetstack/cert-manager \
        -n cert-manager --create-namespace \
        --set crds.enabled=true

helm repo add --force-update open-telemetry https://open-telemetry.github.io/opentelemetry-helm-charts
helm upgrade -i --wait my-opentelemetry-operator open-telemetry/opentelemetry-operator \
        --set "manager.collectorImage.repository=otel/opentelemetry-collector-contrib" \
        -n open-telemetry --create-namespace

I've tested this changes with this Otel collector configuration

# cert-manager resources
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: my-client-certificate
  namespace: kubewarden
spec:
  dnsNames:
  - my-collector-collector.kubewarden.svc
  - my-collector-collector.kubewarden.svc.cluster.local
  issuerRef:
    kind: Issuer
    name: my-client-selfsigned-issuer
  secretName: my-client-cert
---
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
  name: my-client-selfsigned-issuer
  namespace: kubewarden
spec:
  selfSigned: {}
---
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: my-certificate
  namespace: kubewarden
spec:
  dnsNames:
  - my-collector-collector.kubewarden.svc
  - my-collector-collector.kubewarden.svc.cluster.local
  issuerRef:
    kind: Issuer
    name: my-selfsigned-issuer
  secretName: my-server-cert
---
apiVersion: cert-manager.io/v1
kind: Issuer
metadata:
  name: my-selfsigned-issuer
  namespace: kubewarden
spec:
  selfSigned: {}
---
apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
metadata:
  name: my-collector
  namespace: kubewarden
spec:
  mode: deployment # This configuration is omittable.
  volumes:
    - name: server-certificate
      secret:
        secretName: my-server-cert
    - name: client-certificate
      secret:
        secretName: my-client-cert
  volumeMounts:
    - name: server-certificate
      mountPath: /tmp/etc/ssl/certs/my-server-cert
      readOnly: true
    - name: client-certificate
      mountPath: /tmp/etc/ssl/certs/my-client-cert
      readOnly: true
  config:
    receivers:
      otlp:
        protocols:
          grpc:
            tls:
              cert_file: /tmp/etc/ssl/certs/my-server-cert/tls.crt
              key_file: /tmp/etc/ssl/certs/my-server-cert/tls.key
              client_ca_file: /tmp/etc/ssl/certs/my-client-cert/ca.crt
              # ca_file: /tmp/etc/ssl/certs/my-server-cert/ca.crt
              # client_ca_file_reload: true
    processors: {}
    exporters:
      debug:
        verbosity: normal
      prometheus:
        endpoint: ":8080"
    service:
      pipelines:
        metrics:
          receivers: [otlp]
          processors: []
          exporters: [debug, prometheus]
        traces:
          receivers: [otlp]
          processors: []
          exporters: [debug]

codecov · 2024-12-06T18:52:11Z

Codecov Report

Attention: Patch coverage is 3.09278% with 94 lines in your changes missing coverage. Please review.

Project coverage is 67.35%. Comparing base (1eca73e) to head (9b4ddd0).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...l/controller/policyserver_controller_deployment.go	3.19%	89 Missing and 2 partials ⚠️
internal/metrics/metrics.go	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #934      +/-   ##
==========================================
- Coverage   69.44%   67.35%   -2.09%     
==========================================
  Files          30       30              
  Lines        3040     3128      +88     
==========================================
- Hits         2111     2107       -4     
- Misses        763      852      +89     
- Partials      166      169       +3

Flag	Coverage Δ
integration-tests	`56.59% <3.09%> (-1.79%)`	⬇️
unit-tests	`40.41% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cmd/main.go

internal/controller/policyserver_controller.go

internal/controller/policyserver_controller_deployment.go

jvanz · 2024-12-10T22:08:35Z

cmd/main.go

@@ -86,7 +89,10 @@ func main() {
 		"Enable metrics collection for all Policy Servers and the Kubewarden Controller")
 	flag.BoolVar(&enableTracing, "enable-tracing", false,
 		"Enable tracing collection for all Policy Servers")
-	flag.StringVar(&openTelemetryEndpoint, "opentelemetry-endpoint", "127.0.0.1:4317", "The OpenTelemetry connection endpoint")


This is a breaking change. But considering that we are doing a major change on how we integrate with OpenTelemetry, I think is the right time and worth it. It will simplify the configuration and avoid confusion.

flavio

It looks good. I left some minor comments, some of them are not mandatory.

Please address the linter warnings before merging the PR

internal/controller/policyserver_controller_deployment.go

flavio · 2024-12-11T10:51:11Z

internal/controller/policyserver_controller_deployment.go

+				if index := envVarsContainVariable(admissionContainer.Env, envVar); index >= 0 {
+					admissionContainer.Env[index] = envvar
+				} else {
+					admissionContainer.Env = append(admissionContainer.Env, envvar)


This is generic reflection. I've double checked the code, we create the deployment definition by add some default env variables and then we append the ones provided by the user via the .spec.Env list. Then we keep adding more env variables to the list if needed, like we are doing here.

Kubernetes' uses a list, not a hash to store these env variables. Hence there's the chance of the user defining variables we need to control. Should we change all the code to ensure our env variables have precedence over the ones provided by the user?

Maybe this is something to be done with another PR.

What do you think @kubewarden/kubewarden-developers ?

Yes, I was thinking about that as well. Another option is to add some validation in the webhook to prevent the users from deploying a policy server with a configuration that can cause issues. Therefore, we can at least tell them why the controller cannot deploy the policy server. Instead of allow they to deploy and notice that something is not working as they expected afterwards.

cmd/main.go

Tiltfile

viccuad · 2024-12-11T16:53:03Z

internal/controller/policyserver_controller_deployment.go

+
+	certificatePath := filepath.Dir(os.Getenv("OTEL_EXPORTER_OTLP_CERTIFICATE"))
+	if otelCertificateSecret != "" {
+		policyServerDeployment.Spec.Template.Spec.Volumes = append(policyServerDeployment.Spec.Template.Spec.Volumes, corev1.Volume{


Does this need to be idempotent? (same for otelClientCertificateSecret).
If the values change, to me it seems that we append again, instead of removing the old ones?

This is idempotent, give we always start from an empty Deployment and add all the data defined inside of the PolicyServer spec, plus all the values we deem required.

@kubewarden/kubewarden-developers am I right?

I think you are right, I consider this resolved.

flavio

I think this looks good. Please wait for everybody's approvals before merging

Tiltfile

viccuad

LGTM!

Updates the collector to allow the communication with a remote OpenTelemetry collector. Also updates the policy server reconciler to replicate the same configuration in the policy server deployment enabling it to send data to the same remote Otel collector. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>

flavio · 2024-12-12T14:35:06Z

Merging, addressing the code coverage is tough, we will track this technical debt and address that after the 1.20 release

jvanz self-assigned this Dec 6, 2024

jvanz force-pushed the issue933 branch from a77560b to 34cbd89 Compare December 10, 2024 02:21

jvanz commented Dec 10, 2024

View reviewed changes

cmd/main.go Outdated Show resolved Hide resolved

jvanz force-pushed the issue933 branch from 34cbd89 to b2feead Compare December 10, 2024 02:58

flavio reviewed Dec 10, 2024

View reviewed changes

jvanz force-pushed the issue933 branch from b2feead to c290742 Compare December 10, 2024 21:57

jvanz added the kind/breaking-change label Dec 10, 2024

jvanz force-pushed the issue933 branch 2 times, most recently from fd99270 to e483818 Compare December 10, 2024 22:04

jvanz requested review from flavio and fabriziosestito December 10, 2024 22:06

jvanz commented Dec 10, 2024

View reviewed changes

jvanz marked this pull request as ready for review December 10, 2024 22:10

jvanz requested a review from a team as a code owner December 10, 2024 22:10

jvanz mentioned this pull request Dec 10, 2024

feat: extra OPTEL collector configuration. kubewarden/helm-charts#581

Merged

jvanz force-pushed the issue933 branch 2 times, most recently from 5e75bba to cebdb5b Compare December 11, 2024 01:44

flavio requested changes Dec 11, 2024

View reviewed changes

fabriziosestito reviewed Dec 11, 2024

View reviewed changes

Tiltfile Outdated Show resolved Hide resolved

jvanz force-pushed the issue933 branch from cebdb5b to a5280bf Compare December 11, 2024 13:11

jvanz requested review from fabriziosestito and flavio December 11, 2024 13:15

viccuad reviewed Dec 11, 2024

View reviewed changes

flavio approved these changes Dec 11, 2024

View reviewed changes

jvanz mentioned this pull request Dec 11, 2024

feat: remote otel collector tests kubewarden/kubewarden-end-to-end-tests#127

Merged

fabriziosestito reviewed Dec 12, 2024

View reviewed changes

Tiltfile Outdated Show resolved Hide resolved

fabriziosestito approved these changes Dec 12, 2024

View reviewed changes

viccuad approved these changes Dec 12, 2024

View reviewed changes

jvanz force-pushed the issue933 branch from a5280bf to 5901016 Compare December 12, 2024 12:21

jvanz force-pushed the issue933 branch from 5901016 to 9b4ddd0 Compare December 12, 2024 14:09

flavio merged commit f503bbd into kubewarden:main Dec 12, 2024
7 of 9 checks passed

jvanz mentioned this pull request Dec 12, 2024

Allow testing Telemetry configuration in integration tests #937

Open

jvanz deleted the issue933 branch December 12, 2024 14:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remote otel collector configuration #934

Remote otel collector configuration #934

jvanz commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

jvanz Dec 10, 2024

flavio left a comment

flavio Dec 11, 2024

jvanz Dec 11, 2024

viccuad Dec 11, 2024 •

edited

Loading

flavio Dec 11, 2024

viccuad Dec 12, 2024

flavio left a comment

viccuad left a comment

flavio commented Dec 12, 2024

Remote otel collector configuration #934

Remote otel collector configuration #934

Conversation

jvanz commented Dec 6, 2024 • edited Loading

Description

Tests

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

jvanz Dec 10, 2024

Choose a reason for hiding this comment

flavio left a comment

Choose a reason for hiding this comment

flavio Dec 11, 2024

Choose a reason for hiding this comment

jvanz Dec 11, 2024

Choose a reason for hiding this comment

viccuad Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

flavio Dec 11, 2024

Choose a reason for hiding this comment

viccuad Dec 12, 2024

Choose a reason for hiding this comment

flavio left a comment

Choose a reason for hiding this comment

viccuad left a comment

Choose a reason for hiding this comment

flavio commented Dec 12, 2024

jvanz commented Dec 6, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

viccuad Dec 11, 2024 •

edited

Loading