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

Inject otelcol sidecar into any namespace #1395

Merged
merged 8 commits into from Feb 1, 2023

Conversation

pavolloffay
Copy link
Member

@pavolloffay pavolloffay commented Jan 25, 2023

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

Resolves #199

  • This PR adds support for injecting otelcol sidecar into any namespace - currently the sidecar can be injected into workloads that are in the same namespace as the otelcol CR.
  • The injection is enabled via the same annotation - sidecar.opentelemetry.io/inject: "default/simplest" (default is the namespace)
  • The PR changes how the sidecar config is handled - now the collector config is passed an env var to to the collector container. ---- This approach was chosen to eliminate issues with cleaning the config maps needed for the collector.

Tested on with:

kubectl apply -f - <<EOF
apiVersion: opentelemetry.io/v1alpha1
kind: OpenTelemetryCollector
metadata:
  name: simplest
spec:
  mode: sidecar
  config: |
    receivers:
      jaeger:
        protocols:
          thrift_compact:
    processors:

    exporters:
      logging:

    service:
      pipelines:
        traces:
          receivers: [jaeger]
          processors: []
          exporters: [logging]
EOF
kubectl apply -f - <<EOF
apiVersion: v1
kind: Pod
metadata:
  name: myapp
  annotations:
    sidecar.opentelemetry.io/inject: "default/simplest"
spec:
  containers:
  - name: myapp
    image: jaegertracing/vertx-create-span:operator-e2e-tests
    ports:
      - containerPort: 8080
        protocol: TCP
EOF

Result:

apiVersion: v1
kind: Pod
metadata:
  annotations:
    kubectl.kubernetes.io/last-applied-configuration: |
      {"apiVersion":"v1","kind":"Pod","metadata":{"annotations":{"sidecar.opentelemetry.io/inject":"default/simplest"},"name":"myapp","namespace":"ploffay"},"spec":{"containers":[{"image":"jaegertracing/vertx-create-span:operator-e2e-tests","name":"myapp","ports":[{"containerPort":8080,"protocol":"TCP"}]}]}}
    sidecar.opentelemetry.io/inject: default/simplest
  creationTimestamp: "2023-02-01T10:54:42Z"
  labels:
    sidecar.opentelemetry.io/injected: default.simplest
  name: myapp
  namespace: ploffay
  resourceVersion: "6276"
  uid: 232aea7f-3630-4255-b259-43355a404013
spec:
  containers:
  - image: jaegertracing/vertx-create-span:operator-e2e-tests
    imagePullPolicy: IfNotPresent
    name: myapp
    ports:
    - containerPort: 8080
      protocol: TCP
    resources: {}
    terminationMessagePath: /dev/termination-log
    terminationMessagePolicy: File
    volumeMounts:
    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
      name: kube-api-access-hllwm
      readOnly: true
  - args:
    - --config=env:OTEL_CONFIG
    env:
    - name: POD_NAME
      valueFrom:
        fieldRef:
          apiVersion: v1
          fieldPath: metadata.name
    - name: OTEL_CONFIG
      value: |
        receivers:
          jaeger:
            protocols:
              thrift_compact:
        processors:

        exporters:
          logging:

        service:
          pipelines:
            traces:
              receivers: [jaeger]
              processors: []
              exporters: [logging]
    - name: OTEL_RESOURCE_ATTRIBUTES_POD_NAME
      valueFrom:
        fieldRef:
          apiVersion: v1
          fieldPath: metadata.name
    - name: OTEL_RESOURCE_ATTRIBUTES_POD_UID
      valueFrom:
        fieldRef:
          apiVersion: v1
          fieldPath: metadata.uid
    - name: OTEL_RESOURCE_ATTRIBUTES_NODE_NAME
      valueFrom:
        fieldRef:
          apiVersion: v1
          fieldPath: spec.nodeName
    - name: OTEL_RESOURCE_ATTRIBUTES
      value: k8s.namespace.name=ploffay,k8s.node.name=$(OTEL_RESOURCE_ATTRIBUTES_NODE_NAME),k8s.pod.name=$(OTEL_RESOURCE_ATTRIBUTES_POD_NAME),k8s.pod.uid=$(OTEL_RESOURCE_ATTRIBUTES_POD_UID)
    image: ghcr.io/open-telemetry/opentelemetry-collector-releases/opentelemetry-collector:0.68.0
    imagePullPolicy: IfNotPresent
    name: otc-container
    ports:
    - containerPort: 6831
      name: jaeger-thrift-c
      protocol: UDP
    - containerPort: 8888
      name: metrics
      protocol: TCP
    resources: {}
    terminationMessagePath: /dev/termination-log
    terminationMessagePolicy: File
    volumeMounts:
    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
      name: kube-api-access-hllwm
      readOnly: true
  dnsPolicy: ClusterFirst
  enableServiceLinks: true
  nodeName: minikube
  preemptionPolicy: PreemptLowerPriority
  priority: 0
  restartPolicy: Always
  schedulerName: default-scheduler
  securityContext: {}
  serviceAccount: default
  serviceAccountName: default
  terminationGracePeriodSeconds: 30
  tolerations:
  - effect: NoExecute
    key: node.kubernetes.io/not-ready
    operator: Exists
    tolerationSeconds: 300
  - effect: NoExecute
    key: node.kubernetes.io/unreachable
    operator: Exists
    tolerationSeconds: 300
  volumes:
  - name: kube-api-access-hllwm
    projected:
      defaultMode: 420
      sources:
      - serviceAccountToken:
          expirationSeconds: 3607
          path: token
      - configMap:
          items:
          - key: ca.crt
            path: ca.crt
          name: kube-root-ca.crt
      - downwardAPI:
          items:
          - fieldRef:
              apiVersion: v1
              fieldPath: metadata.namespace
            path: namespace

@pavolloffay pavolloffay force-pushed the inject-any-namespace branch 2 times, most recently from b162b23 to d208aee Compare January 25, 2023 13:45
func ReplaceConfig(params Params) (string, error) {
if !params.Instance.Spec.TargetAllocator.Enabled {
return params.Instance.Spec.Config, nil
func ReplaceConfig(instance v1alpha1.OpenTelemetryCollector) (string, error) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing the API bc the function uses only the otelcol instance

otelColCfgStr := base64.StdEncoding.EncodeToString([]byte(otelColCfg))

// add the container
pod.Spec.InitContainers = append(pod.Spec.InitContainers, corev1.Container{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaronoff97 @Aneurysm9 could you please take a look and give me 👍🏼 on the approach?

The approach was chosen to avoid copying otelcol config map to the namespace where the injection is enabled and as well avoid issues with cleaning the config map.

My notes:

  • owner reference cannot reference objects from a different namespace
  • when a pod is created directly it does not have any owner references

Once we agree I will make the init container image configurable via a flag on the operator (if needed expose it in the CR as well).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on the concerns with using config maps? While the proposed system appears to work, it's got more moving parts than I like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically I have nothing against the initcontainer solution, but I would prefer a uniform solution for internal and external namespaces. Otherwise there is the possibility that the implementations drift apart.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand on the concerns with using config maps?

Correctly implementing the config map approach for the sidecar is tricky:

  • The sidecar config map would have to be created in the target namespace.
  • A custom cleaning would have to be implemented. An owner reference to otelcol cannot be used (it's a cross namespace) nor owner reference to a pod (a pod UID is not known in the admission webhook).

The init container approach is a cleaner approach because no cleaning is required and the exact sidecar configuration is known (via the OTEL_CONFIG env var and cat config.yaml in the initContainer). In the current implementation in the main branch it is hard to figure out what is the already injected sidecar configuration if the otelcol CR changes.

but I would prefer a uniform solution for internal and external namespaces. Otherwise there is the possibility that the implementations drift apart.

The init container approach that this PR introduces is used for both internal and external namespaces. There are as well tests (unit+e2e) to ensure the solution works. The deployment/statefulset/daemonset deployment modes keep using the mounted configmap as before - which makes perfect sense bc there is only a single version of the "instance".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @pavolloffay OOB and I recommend setting the configuration in an environment variable on the collector container and using the config from there by setting the argument --config=env:OTEL_CONFIG. This avoids the need for an init container simply to copy the config and leaves nothing additional to clean up.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was a great suggestion, I didn't know that --config=env:OTEL_CONFIG is supported.

I have changed the PR to use this approach without the init container.

Copy link
Contributor

@jaronoff97 jaronoff97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just had two questions/concerns, otherwise I think this approach makes sense.

pkg/sidecar/pod.go Outdated Show resolved Hide resolved
pkg/sidecar/pod.go Outdated Show resolved Hide resolved
@pavolloffay pavolloffay marked this pull request as ready for review January 25, 2023 15:03
@pavolloffay pavolloffay requested a review from a team as a code owner January 25, 2023 15:03
main.go Outdated Show resolved Hide resolved
pkg/sidecar/pod.go Outdated Show resolved Hide resolved
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
@pavolloffay pavolloffay merged commit 41938a9 into open-telemetry:main Feb 1, 2023
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
* Inject otelcol sidecar into any namespace

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

* review comments

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

* review comments

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

* Consume otelcol config directly from env var

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

* Fix lint

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

* revert

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

* revert init prepper image

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

* Change order

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

---------

Signed-off-by: Pavol Loffay <p.loffay@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support sidecar injection in another pod namespace different from the otel colletor instance.
4 participants