chore(otel): Ensure daemonset uses local policy#41
Conversation
📝 WalkthroughWalkthroughThe PR adds conditional ChangesOTEL Service Internal Traffic Policy Feature
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/ctrlplane/charts/otel/Chart.yaml (1)
1-8: ⚡ Quick winAdd
kubeVersionto Chart.yaml to declare minimum Kubernetes compatibility.
This chart usesspec.internalTrafficPolicy(in service.yaml), which is GA only in Kubernetes v1.26+. DeclaringkubeVersion: ">=1.26.0"prevents installations on incompatible clusters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/ctrlplane/charts/otel/Chart.yaml` around lines 1 - 8, Chart.yaml is missing a kubeVersion declaration which allows installs on clusters older than v1.26 where spec.internalTrafficPolicy is not GA; update the Chart.yaml for the otel chart by adding a kubeVersion: ">=1.26.0" entry (next to apiVersion/name/version/appVersion) to declare minimum Kubernetes compatibility so installations are prevented on incompatible clusters; note the chart uses spec.internalTrafficPolicy in service.yaml so target Kubernetes v1.26+.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@charts/ctrlplane/charts/otel/Chart.yaml`:
- Around line 1-8: Chart.yaml is missing a kubeVersion declaration which allows
installs on clusters older than v1.26 where spec.internalTrafficPolicy is not
GA; update the Chart.yaml for the otel chart by adding a kubeVersion: ">=1.26.0"
entry (next to apiVersion/name/version/appVersion) to declare minimum Kubernetes
compatibility so installations are prevented on incompatible clusters; note the
chart uses spec.internalTrafficPolicy in service.yaml so target Kubernetes
v1.26+.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47fbf96e-6458-4c2c-8cc1-5a66fe793e9a
⛔ Files ignored due to path filters (1)
charts/ctrlplane/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
charts/ctrlplane/Chart.yamlcharts/ctrlplane/charts/otel/Chart.yamlcharts/ctrlplane/charts/otel/templates/service.yamlcharts/ctrlplane/tests/otel_internal_traffic_policy_test.yaml
Eh I checked and this was in beta from 1.22 which was EOL 4 years ago. Given we don't have version constraints in this repo already I don't think this is the trigger for adding them. Open to it if others disagree |
We run OTEL as a DaemonSet. When spans hit the collector, it uses the pod's IP as a cache key to lookup the pods information and get all the details like namespace, deployment, etc. We set the OTEL collector endpoint to
OTEL_EXPORTER_OTLP_ENDPOINT=http://ctrlplane-otel:4318which hits the Service. By default, the Service traffic policy will load balance traffic to pods on different nodes. So when a span from node A hits a OTEL pod in node B, the pod IP is a cache miss and we don't get the enriched details. In that case, the spans look like this (see only pod IP is there)Using Default Cluster mode
When fixed to use
Localtraffic policy, the Service sends traffic to the pods on the same node as the caller. This is more efficient and causes the cache to properly return the pod data. See the results here with full k8s infoUsing Correct Local mode
This PR defaults traffic policy to Local when it runs as a DaemonSet as is the proper config.
Summary by CodeRabbit