sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka
, 'i'); if (__m === '*' || __re.test(location.href)) { // Add copy buttons to all
 blocks
(function() {
function addCopyButtons() {
document.querySelectorAll('pre code').forEach(function(codeBlock) {
if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;
codeBlock.parentElement.setAttribute('data-copy-added', 'true');
var btn = document.createElement('button');
btn.textContent = 'Copy';
btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';
btn.onmouseover = function() { this.style.opacity = '1'; };
btn.onmouseout = function() { this.style.opacity = '0.7'; };
btn.onclick = function() {
navigator.clipboard.writeText(codeBlock.textContent).then(function() {
btn.textContent = 'Copied!';
setTimeout(function() { btn.textContent = 'Copy'; }, 1500);
});
};
codeBlock.parentElement.style.position = 'relative';
codeBlock.parentElement.appendChild(btn);
});
}
addCopyButtons();
// Re-run on dynamic content
var observer = new MutationObserver(addCopyButtons);
observer.observe(document.body, { childList: true, subtree: true });
})();
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka
, 'i'); if (__m === '*' || __re.test(location.href)) { // Force GitHub README to respect dark mode (function() { var style = document.createElement('style'); style.textContent = ' .markdown-body { color-scheme: dark light; } .markdown-body pre { background: #161b22 !important; } .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; } .markdown-body table th, .markdown-body table td { border-color: #30363d !important; } .markdown-body img { background: #0d1117; } .markdown-body blockquote { border-left-color: #8b949e; } .markdown-body hr { border-color: #30363d; } '; document.head.appendChild(style); })(); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka
, 'i'); if (__m === '*' || __re.test(location.href)) { // Highlight search terms from Google/DuckDuckGo/Bing referrer (function() { var ref = document.referrer; var terms = []; if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) { var url = new URL(ref); var q = url.searchParams.get('q') || url.searchParams.get('p'); if (q) { terms = q.split(/\s+/).filter(function(t) { return t.length > 2; }); } } if (terms.length === 0) return; var style = document.createElement('style'); style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }'; document.head.appendChild(style); function highlight(node) { if (node.nodeType === 3) { // text node var text = node.textContent; var found = false; terms.forEach(function(term) { var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\]\\]/g, '\\') + ')', 'gi'); if (regex.test(text)) { found = true; var frag = document.createDocumentFragment(); var parts = text.split(regex); parts.forEach(function(part, i) { if (i % 2 === 0) { frag.appendChild(document.createTextNode(part)); } else { var span = document.createElement('span'); span.className = 'userscript-highlight'; span.textContent = part; frag.appendChild(span); } }); node.parentNode.replaceChild(frag, node); } }); } else if (node.nodeType === 1 && node.childNodes) { // element var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT']; if (!skipTags.includes(node.tagName)) { Array.from(node.childNodes).forEach(highlight); } } } highlight(document.body); // Re-highlight on dynamic content var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1 || node.nodeType === 3) highlight(node); }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka
, 'i'); if (__m === '*' || __re.test(location.href)) { // Strip utm_, fbclid, gclid, etc. from all links on page (function() { var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid', 'ref', 'ref_src', 'source', 'medium', 'campaign']; function cleanUrl(url) { try { var u = new URL(url, window.location.origin); var changed = false; trackingParams.forEach(function(p) { if (u.searchParams.has(p)) { u.searchParams.delete(p); changed = true; } }); return changed ? u.toString() : url; } catch (e) { return url; } } function cleanLinks() { document.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } cleanLinks(); var observer = new MutationObserver(function(mutations) { mutations.forEach(function(m) { m.addedNodes.forEach(function(node) { if (node.nodeType === 1) { if (node.tagName === 'A') cleanLinks(); node.querySelectorAll('a[href]').forEach(function(a) { var clean = cleanUrl(a.href); if (clean !== a.href) a.href = clean; }); } }); }); }); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka
, 'i'); if (__m === '*' || __re.test(location.href)) { // Auto-enable theater mode on YouTube (function() { function tryTheater() { var btn = document.querySelector('button[aria-label="Theater mode"], ytd-player #player button[title="Theater mode"]'); if (btn && !btn.classList.contains('activated')) { btn.click(); } } // Try immediately tryTheater(); // Try after navigation (SPA) var lastUrl = location.href; setInterval(function() { if (location.href !== lastUrl) { lastUrl = location.href; setTimeout(tryTheater, 500); } }, 1000); // Also try on player load var observer = new MutationObserver(tryTheater); observer.observe(document.body, { childList: true, subtree: true }); })(); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka
, 'i'); if (__m === '*' || __re.test(location.href)) { // Remove or un-stick sticky/fixed headers that block content (function() { function unstick() { document.querySelectorAll('header, nav, [role="banner"], .header, .navbar, .sticky, .fixed-top, [style*="position: fixed"], [style*="position:sticky"]').forEach(function(el) { if (el.style.position === 'fixed' || el.style.position === 'sticky' || getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') { el.style.position = 'static'; el.style.top = 'auto'; el.style.zIndex = 'auto'; } }); } unstick(); var observer = new MutationObserver(unstick); observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] }); })(); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka
, 'i'); if (__m === '*' || __re.test(location.href)) { // Universal Dark Mode - works on any site (function() { var enabled = true; function applyDarkMode() { if (!enabled) return; // Create style element if it doesn't exist var style = document.getElementById('universal-dark-mode-style'); if (!style) { style = document.createElement('style'); style.id = 'universal-dark-mode-style'; document.head.appendChild(style); } // Dark mode CSS - inverts colors but preserves images/video style.textContent = ' /* Invert everything except media */ html { filter: invert(1) hue-rotate(180deg) !important; background: #1a1a2e !important; } /* Restore images, videos, iframes, canvas */ img, video, iframe, canvas, svg, picture, [style*="background-image"] { filter: invert(1) hue-rotate(180deg) !important; } /* Preserve specific elements that should not be inverted */ .no-dark-mode, .no-dark-mode *, [data-theme="light"], [data-theme="light"], .ace_editor, .ace_editor *, .CodeMirror, .CodeMirror *, .monaco-editor, .monaco-editor *, .markdown-body pre, .markdown-body pre *, .highlight, .highlight *, pre code, pre code * { filter: none !important; } /* Fix common UI elements */ .modal, .popup, .dropdown-menu, .tooltip, .popover { filter: invert(1) hue-rotate(180deg) !important; background: #2d2d44 !important; border-color: #444 !important; } /* Scrollbars */ ::-webkit-scrollbar { background: #1a1a2e !important; } ::-webkit-scrollbar-thumb { background: #444 !important; } ::-webkit-scrollbar-thumb:hover { background: #555 !important; } /* Selection */ ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; } ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; } '; } function removeDarkMode() { var style = document.getElementById('universal-dark-mode-style'); if (style) style.remove(); } // Toggle with Alt+Shift+D document.addEventListener('keydown', function(e) { if (e.altKey && e.shiftKey && e.key === 'D') { e.preventDefault(); enabled = !enabled; if (enabled) { applyDarkMode(); console.log('[Universal Dark Mode] Enabled'); } else { removeDarkMode(); console.log('[Universal Dark Mode] Disabled'); } } }); // Apply on load applyDarkMode(); // Re-apply on dynamic content var observer = new MutationObserver(function(mutations) { if (enabled && !document.getElementById('universal-dark-mode-style')) { applyDarkMode(); } }); observer.observe(document.head, { childList: true }); console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle'); })(); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625) - #865

Merged
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release
Aug 27, 2026
Merged

sec(telemetry): release-scope the Collector token Secret, migration-safe (backend#2625)#865
LukasWodka merged 4 commits into
developfrom
fix/2625-telemetry-token-per-release

Conversation

@aptracebloc

@aptraceblocaptracebloc commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Fixes tracebloc/backend#2625.

The bug

telemetryCollector.tokenSecret.name defaulted to a fixedtracebloc-telemetry-token in the shared node-agents namespace. Two edges on one cluster collide: two jobs-managers write one Secret, last writer wins, and the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry misattribution. Latent today only because a single Collector is enabled fleet-wide; the collision arrives the moment #1906's prod half enables a second.

The naive fix (template the default to {{ .Release.Name }}-telemetry-token) wedges the working edge: the daemonset's own pre-flight (fail: … Secret %q does not exist) refuses the upgrade on the one edge already collecting, because jobs-manager hasn't written the new name yet — the #2400 deadlock in a new costume.

The fix — Option A (migration-safe rename)

  • One resolvertracebloc.telemetryTokenSecretName — behind all four consumers (writer env jobs-manager-deployment.yaml, reader guard + volume telemetry-collector-daemonset.yaml, RBAC resourceName telemetry-token-rbac.yaml), so they can never disagree. Default is release-scoped <release>-telemetry-token.
  • The legacy fixed name is a "migrate me" sentinel, not an override.helm upgrade --reuse-values bakes the old default into existing releases' stored values, so defaulting only an absent name would strand every installed edge on the colliding fixed name forever. A genuinely custom name is honoured verbatim.
  • jobs-manager writes the release-scoped name; the pre-flight also accepts the legacy name while it exists, so an edge already collecting under the legacy name is not wedged on upgrade. The legacy acceptance and its helper carry, at their declaration, the condition for their own removal.

Acceptance

  • (a) two releases resolve to distinct, release-scoped Secrets — asserted across two renders in telemetry_collector_test.yaml (per-release release.name) and in scripts/tests/telemetry-token-agreement.sh (renders two releases, asserts the resolved names differ and each scopes to its release).
  • (b) an edge on the legacy name upgrades without the pre-flight tripping — the guard is a lookup-backed fail, invisible to helm template (the suite says as much), so it's exercised live in scripts/tests/telemetry-token-migration.sh, invoked from the k3d auto-upgrade e2e (e2e-auto-upgrade.sh) and self-skipping with no cluster. Three cases: no Secret → refused; legacy only → accepted; release-scoped only → accepted.
  • (c) the legacy acceptance declares its own removal condition at the declaration (helper + guard).

Verification (run locally against a throwaway k3d cluster)

  • helm unittest ./client — telemetry suite green, 56 tests (4 unrelated suites fail identically on clean develop — a local helm-4 schema quirk on image tagnot constraints; CI pins helm 3.15.4).
  • telemetry-token-agreement.sh / telemetry-token-bootstrap.sh and the other 8 collector derived checks — green.
  • telemetry-token-migration.sh against live k3d — all three cases pass; verified the guard accepts the legacy name (acceptance b) and still refuses when nothing exists.
  • helm lint, check-style.sh, shellcheck — clean.

Ran /code-review on the diff: one robustness finding fixed (name extraction now reads from the ungated RBAC template so it can't itself trip the guard), two low-severity non-issues.

🤖 Generated with Claude Code


Note

Medium Risk
Changes credential Secret naming and RBAC scoping for telemetry ingest—security-sensitive—but migration paths and live e2e reduce wedge risk; main residual is fleet timing until release-scoped Secrets are populated on every collecting edge.

Overview
Fixes cross-tenant telemetry misattribution when multiple Helm releases share the node-agents namespace: the ingest token Secret no longer defaults to a single fixed tracebloc-telemetry-token name that every jobs-manager could overwrite.

Central change:tracebloc.telemetryTokenSecretName resolves the effective name for jobs-manager (writer env), the Collector DaemonSet (volume + pre-flight), and token RBAC (get/patch on one Secret). Default is <release>-telemetry-token. Empty tokenSecret.name or the legacy sentinel tracebloc-telemetry-token (including values replayed by helm upgrade --reuse-values) is rewritten to that release-scoped name; genuinely custom names stay verbatim.

Upgrade safety: The DaemonSet pre-flight still refuses installs with no token Secret, but now accepts either the release-scoped name or the legacy fixed name so an edge already collecting under the old Secret is not wedged before jobs-manager writes the new one. Chart 1.9.75.

Tests: Helm unittest cases for per-release names, legacy migration, and custom overrides; telemetry-token-agreement.sh asserts two releases resolve to distinct Secrets; new telemetry-token-migration.sh exercises the lookup-based guard on a live cluster (wired into e2e-auto-upgrade.sh with --require and helm-ci path triggers).

Reviewed by Cursor Bugbot for commit d8c3a43. Bugbot is set up for automated code reviews on this repo. Configure here.

…afe (backend#2625)
telemetryCollector.tokenSecret.name defaulted to a fixed
`tracebloc-telemetry-token` in the SHARED node-agents namespace. Two edges on
one cluster collide: two jobs-managers write one Secret, last writer wins, and
the loser's Collector authenticates as the wrong tenant — cross-tenant telemetry
misattribution. Latent today only because a single Collector is enabled fleet-
wide; the collision arrives the moment #1906's prod half enables a second.
Option A (migration-safe rename):
- Resolve the name in one helper, tracebloc.telemetryTokenSecretName, behind all
four consumers (writer env, reader guard + volume, RBAC resourceName), so they
can never disagree. Default is release-scoped `<release>-telemetry-token`.
- The legacy fixed name is a MIGRATE-ME sentinel, not an override: `helm upgrade
--reuse-values` bakes the old default into existing releases' stored values, so
only rewriting the sentinel actually migrates them. A genuinely custom name is
honoured verbatim.
- jobs-manager writes the release-scoped name; the daemonset pre-flight ALSO
accepts the legacy name while it exists, so an edge already collecting under the
legacy name is not wedged on upgrade (the #2400 deadlock in a new costume). The
legacy acceptance and its helper carry, at their declaration, the condition for
their own removal.
Acceptance:
(a) two releases resolve to distinct, release-scoped Secrets — asserted across two
renders in telemetry_collector_test.yaml and telemetry-token-agreement.sh;
(b) an edge on the legacy name upgrades without the pre-flight tripping — the guard
is a lookup-backed `fail`, invisible to `helm template`, so it is exercised
live in scripts/tests/telemetry-token-migration.sh (run from the k3d
auto-upgrade e2e; self-skips with no cluster);
(c) the legacy acceptance declares its own removal condition at the declaration.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Chart content changed (telemetry token rename), so the chart-version-guard
requires a new Chart.yaml version — a Helm repo only publishes on version change.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment threadscripts/tests/telemetry-token-migration.sh Outdated
Comment threadscripts/tests/e2e-auto-upgrade.sh
… guard check
The pipefail early-close house-rule flags `printf | grep -q` under errexit+pipefail
(the reader closes early and SIGPIPEs the producer). Feed the captured output via a
here-string instead — same match, no pipe. (backend#2625)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 7a65049. Configure here.

Comment threadscripts/tests/telemetry-token-migration.sh Outdated
…able in e2e (backend#2625)
Cursor Bugbot (2 findings) on PR #865:
- Unbounded `kubectl cluster-info` hangs the k3d job on a wedged API instead of
failing fast. Every kubectl call now goes through a `--request-timeout=15s` wrapper.
- A skip exited 0, so from e2e-auto-upgrade.sh a skip counted as acceptance (b)
passing while the PASS line claimed it was verified. New `--require` flag turns
every skip condition into a hard failure; the e2e passes it (a cluster is
guaranteed there). Standalone/drift runs still self-skip.
Verified live on k3d: three cases green under --require; teardown clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@LukasWodkaLukasWodka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving d8c3a43e — 44 passing, 4 path-skipped, nothing pending or failing, no open threads, mergeable=MERGEABLE.

I checked the three load-bearing claims against the chart rather than the description, because on a cross-tenant issue the description being right isn't the same as the templates being right.

"One resolver behind all four consumers" is literally true. All four include "tracebloc.telemetryTokenSecretName":

jobs-manager-deployment.yaml writer env
telemetry-token-rbac.yaml RBAC resourceName
telemetry-collector-daemonset.yaml reader guard + volume

and every remaining mention of tracebloc-telemetry-token in the chart is either prose or the sentinel's own definition. The name exists in exactly one place, so the four cannot drift — which is the property, not a comment about the property.

The sentinel reasoning is the part I'd have got wrong. Defaulting only an absent name looks correct and would have stranded every installed edge forever, because --reuse-values bakes the old default into stored values — so the legacy name is present, not absent, on exactly the releases that need migrating. Rewriting it is what actually moves them. And the comparison uses include "tracebloc.telemetryTokenLegacyName" rather than the literal, so even the sentinel check derives from one definition. A different explicit name is honoured verbatim; the trade-off (an operator who deliberately chose the old default gets renamed) is the right way round and you said so out loud.

The removal condition is the best part of this PR. A temporary compatibility shim usually becomes permanent because nobody writes down when it can go. This one names both things that must be deleted together, states what premature removal re-breaks (the #2400 deadlock, in its new costume), and gives an observable exit criterion:

kubectl get secret -A --field-selector metadata.name=tracebloc-telemetry-token

Empty output means the shim can go. That's a machine-checkable condition rather than a promise to remember.

And the test proves the security property, not the code shape. I ran it:

ok: writer, reader, key and RBAC all name the same Secret
release tenant-alpha -> tenant-alpha-telemetry-token
release tenant-beta -> tenant-beta-telemetry-token
ok: two releases resolve to distinct, release-scoped Secrets
telemetry token agreement: green

It pulls the resolved names out of two real helm template renders — the writer's own env and the reader's own volume — rather than grepping the templates for the helper. That's the difference between asserting the four agree and asserting the four render in agreement, and it's why the guard will still mean something after someone refactors the helper. set -euo pipefail plus the fail-closed-on-unlocatable block means a field it can't find is a failure, not a quiet pass.

Value: removes a cross-tenant telemetry misattribution that is latent only because a single Collector is enabled fleet-wide — and would have become real the moment #1906's prod half enabled a second. It does it without wedging the one edge already collecting, which is the trap the obvious fix falls into.

@LukasWodka
LukasWodka merged commit d3732c0 into developAug 27, 2026
48 checks passed
@LukasWodka
LukasWodka deleted the fix/2625-telemetry-token-per-release branch August 27, 2026 09:30
Sign up for freeto 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.

2 participants

@aptracebloc@LukasWodka