Add TLS support to auto-instrumentation #3338

pavolloffay · 2024-10-08T16:26:34Z

Description:

Link to tracking Issue(s):

Resolves: Enable TLS between the collector and application pods #2997

Todos:

e2e tests
more unit tests
follow up issue with proposal for cert-manager - Cert-manager integration for collector-instrumentation mTLS #3342

Testing:

On OpenShifit

kubectl apply -f - <<EOF
apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
metadata:
  name: simplest
  annotations:
    service.beta.openshift.io/serving-cert-secret-name: otelcol
spec:
  volumeMounts:
  - name: otelcol-certs
    mountPath: /otelcol-certs
  volumes:
  - name: otelcol-certs
    secret: 
      secretName: otelcol
    
  config:
    receivers:
      otlp:
        protocols:
          grpc:
            endpoint: 0.0.0.0:4317
            tls:
              cert_file: /otelcol-certs/tls.crt
              key_file: /otelcol-certs/tls.key
          http:
            endpoint: 0.0.0.0:4318
            tls:
              cert_file: /otelcol-certs/tls.crt
              key_file: /otelcol-certs/tls.key
    processors:
    exporters:
      debug: {}

    service:
      pipelines:
        traces:
          receivers: [otlp]
          processors: []
          exporters: [debug]
EOF


kubectl apply -f - <<EOF
apiVersion: opentelemetry.io/v1alpha1
kind: Instrumentation
metadata:
  name: my-instrumentation
spec:
  exporter:
    endpoint: https://simplest-collector.tracing-system.svc.cluster.local:4317
    tls:
      ca: /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
  propagators:
    - tracecontext
    - baggage
    - b3
  sampler:
    type: parentbased_traceidratio
    argument: "1"
EOF

kubectl apply -f https://raw.githubusercontent.com/pavolloffay/kubecon-eu-2024-opentelemetry-kubernetes-tracing-tutorial/main/app/k8s.yaml
kubectl patch deployment frontend-deployment -n tutorial-application -p '{"spec": {"template":{"metadata":{"annotations":{"instrumentation.opentelemetry.io/inject-sdk":"tracing-system/my-instrumentation"}}}} }'
kubectl patch deployment backend1-deployment -n tutorial-application -p '{"spec": {"template":{"metadata":{"annotations":{"instrumentation.opentelemetry.io/inject-python":"tracing-system/my-instrumentation"}}}} }'
kubectl patch deployment backend2-deployment -n tutorial-application -p '{"spec": {"template":{"metadata":{"annotations":{"instrumentation.opentelemetry.io/inject-java":"tracing-system/my-instrumentation"}}}} }'
k get pods -n tutorial-application -w 
kubectl port-forward service/frontend-service -n tutorial-application 4000:4000

Another Example for Openshift Ca bundle and serving certs

kubectl apply -f - <<EOF
apiVersion: tempo.grafana.com/v1alpha1
kind: TempoMonolithic
metadata:
  name: tempo
spec:
  storage:
    traces:
      backend: memory 
      size: 1Gi 
  ingestion:
    otlp:
      grpc:
        enabled: true
        tls:
          enabled: true
      http:
        enabled: true
        tls:
          enabled: true
  jaegerui:
    enabled: true 
    route:
      enabled: true 
EOF


kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
  name: otelcol-cabundle
  annotations:
    service.beta.openshift.io/inject-cabundle: "true"
EOF

kubectl apply -f - <<EOF
apiVersion: opentelemetry.io/v1beta1
kind: OpenTelemetryCollector
metadata:
  name: simplest
  annotations:
    service.beta.openshift.io/serving-cert-secret-name: otelcol
spec:
  volumeMounts:
  - name: otelcol-certs
    mountPath: /otelcol-certs
  - name: otelcol-ca
    mountPath: /otelcol-certs-ca
  volumes:
  - name: otelcol-certs
    secret: 
      secretName: otelcol
  - name: otelcol-ca
    configMap: 
      name: otelcol-cabundle
    
  config:
    receivers:
      otlp:
        protocols:
          grpc:
            endpoint: 0.0.0.0:4317
            tls:
              cert_file: /otelcol-certs/tls.crt
              key_file: /otelcol-certs/tls.key
          http:
            endpoint: 0.0.0.0:4318
            tls:
              cert_file: /otelcol-certs/tls.crt
              key_file: /otelcol-certs/tls.key
    processors:
    exporters:
      otlp:
        endpoint: https://tempo-tempo.tracing-system.svc.cluster.local:4317
        tls:
          ca_file: /otelcol-certs-ca/service-ca.crt
      debug: {}

    service:
      pipelines:
        traces:
          receivers: [otlp]
          processors: []
          exporters: [debug,otlp]
EOF



kubectl apply -f - <<EOF
apiVersion: v1
kind: ConfigMap
metadata:
  name: otelcol-cabundle
  namespace: tutorial-application
  annotations:
    service.beta.openshift.io/inject-cabundle: "true"
EOF

kubectl apply -f - <<EOF
apiVersion: opentelemetry.io/v1alpha1
kind: Instrumentation
metadata:
  name: my-instrumentation
spec:
  exporter:
    endpoint: https://simplest-collector.tracing-system.svc.cluster.local:4317
    tls:
      configMapName: otelcol-cabundle
      ca: service-ca.crt
  propagators:
    - tracecontext
    - baggage
    - b3
  sampler:
    type: parentbased_traceidratio
    argument: "1"
  python:
    env:
      # Required if endpoint is set to 4317.
      # Python autoinstrumentation uses http/proto by default
      # so data must be sent to 4318 instead of 4317.
      - name: OTEL_EXPORTER_OTLP_ENDPOINT
        value: https://simplest-collector.tracing-system.svc.cluster.local:4318
EOF

Documentation:

apis/v1alpha1/instrumentation_types.go

pkg/instrumentation/exporter.go

apis/v1alpha1/instrumentation_types.go

frzifus · 2024-10-09T20:49:36Z

apis/v1alpha1/instrumentation_types.go

+
+	// CA defines the key of certificate in the configmap map, secret or absolute path to a certificate.
+	// The absolute path can be used when certificate is already present on the workload filesystem e.g.
+	// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt


Suggested change

// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt

// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt

// +optional

@frzifus what does the +optional do ? It does not seem to have any effect on the generated bundle or code.

We should get rid of it https://book.kubebuilder.io/reference/markers I will submit a follow up PR to remove +optional

apis/v1alpha1/instrumentation_types.go

pkg/instrumentation/podmutator.go

apis/v1alpha1/instrumentation_types.go

iblancasa · 2024-10-10T10:21:32Z

apis/v1alpha1/instrumentation_types.go

+
+	// ConfigMapName defines configmap name with CA certificate. If it is not defined CA certificate will be
+	// used from the secret defined in SecretName.
+	ConfigMapName string `json:"configMapName,omitempty"`


I'm not sure about this name because it is something associated to the CA directly. Maybe configMapName -> caConfigMap.

pkg/instrumentation/exporter.go

pkg/instrumentation/podmutator.go

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

jaronoff97 · 2024-10-10T15:46:02Z

apis/v1alpha1/instrumentation_webhook.go

@@ -236,6 +236,14 @@ func (w InstrumentationWebhook) validate(r *Instrumentation) (admission.Warnings
 	default:
 		return warnings, fmt.Errorf("spec.sampler.type is not valid: %s", r.Spec.Sampler.Type)
 	}
+
+	if r.Spec.Exporter.TLS != nil {


let's also add a validation that if the scheme for the endpoint contains https TLS must be defined.

no, this can break already existing installations. Users could configure env vars in the env section or directly on their deployments.

let's go the reverse and if TLS is set we should block when https is not set. If HTTPS is set, TLS doesn't need to be set.

pkg/instrumentation/exporter_test.go

jaronoff97 · 2024-10-10T15:49:44Z

pkg/instrumentation/exporter.go

+	}
+	// the name cannot be longer than 63 characters
+	secretVolumeName := naming.Truncate("otel-auto-secret-%s", 63, exporter.TLS.SecretName)
+	secretMountPath := fmt.Sprintf("/otel-auto-instrumentation-secret-%s", exporter.TLS.SecretName)


should we care about the max for a mount path? I'm not 100% there is one (i couldn't find anything in the kube API docs) but given we truncate the names here, i feel like we should try to keep the mount path name minimal too.

that does not seem to be an issue only the volume name

pkg/instrumentation/exporter_test.go

jaronoff97 · 2024-10-10T15:56:25Z

tests/e2e-instrumentation/instrumentation-java-tls/chainsaw-test.yaml

+        file: 01-assert.yaml
+    catch:
+      - podLogs:
+          selector: app=my-java


should we run a curl against the collector to see that span traffic made it successfully to the collector?

We don't do this for auto-instrumentation tests there is a booked ticket for it #552

okay fine :P I can do this post my refactor.

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

* `secrets`: Add TLS support to auto-instrumentation [#3338](open-telemetry/opentelemetry-operator#3338) * `targetallocators`: Generate only TargetAllocator CR from Collector CR [#3402](open-telemetry/opentelemetry-operator#3402)

* Release operator 0.114.1 Signed-off-by: Alex Raiu <raiu.andrei@protonmail.com> * Update operator clusterrole * `secrets`: Add TLS support to auto-instrumentation [#3338](open-telemetry/opentelemetry-operator#3338) * `targetallocators`: Generate only TargetAllocator CR from Collector CR [#3402](open-telemetry/opentelemetry-operator#3402) * Add `targetAllocatorFallbackStrategy` feature flag Feature flag available since `v0.114.0` (Add allocation_fallback_strategy option as fallback strategy for per-node strategy [#3482](open-telemetry/opentelemetry-operator#3482)) * Update operator-test with manager label Adding `control-plane: controller-manager` label to match [operator-restart e2e test](open-telemetry/opentelemetry-operator#3486) --------- Signed-off-by: Alex Raiu <raiu.andrei@protonmail.com>

pavolloffay requested a review from a team as a code owner October 8, 2024 16:26

pavolloffay marked this pull request as draft October 8, 2024 16:26

pavolloffay force-pushed the inst-tls branch from 78fc63e to c414808 Compare October 9, 2024 17:10

pavolloffay marked this pull request as ready for review October 9, 2024 17:12

jaronoff97 reviewed Oct 9, 2024

View reviewed changes

apis/v1alpha1/instrumentation_types.go Outdated Show resolved Hide resolved

apis/v1alpha1/instrumentation_types.go Outdated Show resolved Hide resolved

apis/v1alpha1/instrumentation_types.go Outdated Show resolved Hide resolved

pkg/instrumentation/exporter.go Show resolved Hide resolved

frzifus reviewed Oct 9, 2024

View reviewed changes

iblancasa reviewed Oct 10, 2024

View reviewed changes

pavolloffay force-pushed the inst-tls branch from 37f8193 to d5d8360 Compare October 10, 2024 13:35

pavolloffay mentioned this pull request Oct 10, 2024

Cert-manager integration for collector-instrumentation mTLS #3342

Open

Add TLS support to auto-instrumentation

4e8d00b

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

pavolloffay force-pushed the inst-tls branch from 5340b3f to 4e8d00b Compare October 10, 2024 15:32

jaronoff97 reviewed Oct 10, 2024

View reviewed changes

Fix

ed2db96

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

pavolloffay mentioned this pull request Oct 11, 2024

Release the operator v0.111.0 #3323

Closed

pavolloffay added 2 commits October 11, 2024 09:46

Fix

2849547

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

More validation

6bed105

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>

jaronoff97 approved these changes Oct 11, 2024

View reviewed changes

jaronoff97 requested review from frzifus and iblancasa October 11, 2024 16:07

frzifus approved these changes Oct 11, 2024

View reviewed changes

pavolloffay merged commit a3feb2c into open-telemetry:main Oct 11, 2024
35 checks passed

alexkruc mentioned this pull request Dec 5, 2024

Merge from Upstream (#1) #3514

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TLS support to auto-instrumentation #3338

Add TLS support to auto-instrumentation #3338

pavolloffay commented Oct 8, 2024 •

edited

Loading

frzifus Oct 9, 2024

pavolloffay Oct 10, 2024

iblancasa Oct 10, 2024

jaronoff97 Oct 10, 2024

pavolloffay Oct 10, 2024

jaronoff97 Oct 10, 2024

jaronoff97 Oct 10, 2024

pavolloffay Oct 10, 2024

jaronoff97 Oct 10, 2024

pavolloffay Oct 10, 2024

jaronoff97 Oct 10, 2024

	// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
	// /var/run/secrets/kubernetes.io/serviceaccount/service-ca.crt
	// +optional

Add TLS support to auto-instrumentation #3338

Add TLS support to auto-instrumentation #3338

Conversation

pavolloffay commented Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavolloffay commented Oct 8, 2024 •

edited

Loading