open-telemetry / opentelemetry-helm-charts

OpenTelemetry Helm Charts
https://opentelemetry.io
Apache License 2.0
390 stars 479 forks source link

[collector] kubernetesAttributes preset ClusterRole does not include "nodes" #1377

Open lindeskar opened 1 week ago

lindeskar commented 1 week ago

The kubernetesAttributes preset does not include "nodes" in it's ClusterRole, leading to errors when trying to access Node metadata:

User "system:serviceaccount:opentelemetry-collector:otelcol-opentelemetry-collector" cannot list resource "nodes" in API group "" at the cluster scope

Example chart values causing the error:

mode: "daemonset"
image:
  repository: otel/opentelemetry-collector-contrib
config:
  processors:
    k8sattributes:
      extract:
        labels:
          - { tag_name: zone, key: "topology.kubernetes.io/zone", from: node }
presets:
  logsCollection:
    enabled: true
  kubernetesAttributes:
    enabled: true

This addition to values fixes the problem:

clusterRole:
  rules:
    - apiGroups: [""]
      resources: ["nodes"]
      verbs: ["get", "list", "watch"]

Is this by design? I think we could update the preset documentation, or simply add "nodes" to the template:

https://github.com/open-telemetry/opentelemetry-helm-charts/blob/694c38d175d72fe8e3bc6cc5c182728615f72cbd/charts/opentelemetry-collector/templates/clusterrole.yaml#L18-L28

--

From the k8sattributesprocessor README:

When using k8s.node.uid or extracting metadata from node, the processor needs get, watch and list permissions for nodes resources.

TylerHelmuth commented 1 week ago

@lindeskar you are correct, the preset only sets the RBAC to meet the permission requirements of the k8sattributes processor as the preset defines it. Since you added additional scope (extracting node labels) you must also supply the additional RBAC for that feature.

We do this to ensure the preset configures the minimum permissions necessary. We do not want to give more permissions that the components need as that is a security risk.

lindeskar commented 1 week ago

Thanks. I agree that's a reasonable default.

Then I wish for a comment about this in:

https://github.com/open-telemetry/opentelemetry-helm-charts/blob/694c38d175d72fe8e3bc6cc5c182728615f72cbd/charts/opentelemetry-collector/values.yaml#L45-L47

and:

https://github.com/open-telemetry/opentelemetry-helm-charts/blob/694c38d175d72fe8e3bc6cc5c182728615f72cbd/charts/opentelemetry-collector/README.md?plain=1#L144-L146

Can I create PR for it?

TylerHelmuth commented 1 week ago

Yes please