Feat: Emit lineage.self.namespace; require namespace (contract v1.7) - #918
Conversation
|
Warning Review limit reachedNext included review available in 2 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe lineage plugin now requires a validated namespace from inline configuration or a startup-read file. It emits the namespace on both spans, rejects separator-only identities, updates the v1.7.0 contract, and updates sidecar attachment tooling and documentation. ChangesLineage namespace identity
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~30 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The namespace configuration, validation, emission, contract, and attachment tooling are aligned with no merge-blocking issue identified. Sequence Diagram(s)sequenceDiagram
participant SidecarConfig
participant LineageTelemetry
participant NamespaceFile
participant RequestSpan
participant ResponseSpan
SidecarConfig->>LineageTelemetry: provide namespace or namespace_file
LineageTelemetry->>NamespaceFile: read namespace once when needed
NamespaceFile-->>LineageTelemetry: validated namespace
LineageTelemetry->>RequestSpan: emit lineage.self.namespace
LineageTelemetry->>ResponseSpan: emit lineage.self.namespace
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
abigailgold
added
the
ready-for-ai-review
Two workloads with the same name in two namespaces derived as ONE entity at the consumer: lineage.self.id is the last segment of the SPIFFE ID, and the data-governance sidecar algorithm keyed an entity on kind:self.id. A team1/weather-service and a team2/weather-service therefore shared one row, and every interaction of both pods pointed at it. That collision was made normative in rossoctl#761 (round 5: contract v1.6.1 §4; round 6: the TestServiceLabel row "collides by design") as the documentation half — the reduction is deliberate and stays. This is the identity half: self.id alone was never the whole identity of a pod, so the missing fact is added rather than the shipped reduction changed. The plugin now emits lineage.self.namespace on both spans, from a new required `namespace` config key (or `namespace_file`, read once at start — for the file the kubelet projects from the pod's own metadata, the one source that is right in every copy of a ConfigMap shared across namespaces). The value is resolved before anything else in Init and must be an RFC 1123 DNS label: absent, blank, a "/" (which would make the consumer's {kind}:{namespace}/{self.id} key ambiguous), or any other shape refuses to start and leaves nothing behind — the treatment a blank self_id gets, because a name without a namespace is half an identity. It is never parsed out of the SPIFFE path (a registrar convention; a kit-attached pod has no SPIFFE ID). Neither identity fact is capped by max_attr_bytes any more — self.id was, and a truncated identity keys the pod on a name that is not its own; both are operator configuration, not caller input. lineage.self.id, its reduction, the §4 clause, the by-design test row and the span names are unchanged. The rossoctl#761 round-6 question is closed alongside: a self_id made only of separators, which the reduction emits as-is, now refuses at start like a blank one, and a self_id_file carrying one keeps the plugin not-ready like a blank file. Wire contract v1.7.0 (vendored byte-identical with lab-data-governance): the attribute (§4), the keys (§6), and the consumer commitment that a pod's identity is the (namespace, self.id) pair, natural key {kind}:{namespace}/{self.id}, read from the request span (§7). Additive on the wire; breaking in configuration in both directions — a sidecar older than the key rejects a config that carries it, so image and ConfigMap flip together per pod (no published release carries the plugin yet, so no deployed configuration is affected). The attach kit writes its NAMESPACE, warns when a re-run patches nothing (no pod rolls; the old sidecar only hot-reloads), and the README covers the upgrade, the enrolled-workload route via namespace_file (a literal in the platform's shared ConfigMap would be wrong in every namespace but one), and three troubleshooting rows. The schema marks the key required for abctl and /v1/plugins; the Capabilities cite moves to v1.7. Tests: the namespace on both spans in both directions and through the real decode → Init → emit path (trimmed, otherwise verbatim); never capped; refusals for absent, blank, "/", uppercase, spaces, dots and 64 chars with 63 and padded accepted, and nothing left behind; refusal precedes the identity poll; namespace_file (projected file, inline wins; absent, blank — its own message — and non-label refuse); the schema's required flag; every Init fixture carries the key; the identity table gains the "/" and " // " rows and the file test a separators-only file. Tests that run a full Init shut the plugin down (provider and gRPC client), not only the provider. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NJpNC6yQgZAdfiLAp3RksZ Signed-off-by: YehoshuaSagron <ysagron@gmail.com>
9e3ef03 to
3fda5ce
Compare
huang195
left a comment
huang195
left a comment
There was a problem hiding this comment.
Careful, well-argued change. The namespace is resolved before anything else Init builds (no tracer provider, no poller, no connection behind a refusal), the fact is never derived from the SPIFFE path, and the docs, the wire-contract ladder, the kit scripts and the troubleshooting rows all move together. The breaking half is owned honestly in both the PR body and the contract, and it strands no in-repo config: lineage-telemetry is rendered nowhere in this repo's charts or fixtures, and nowhere at all in rossoctl. Test coverage for the new key is thorough — decode → Init → both spans, plus the file source and the shape refusals.
Claims I verified against source at head rather than taking on faith:
| Claim | Result |
|---|---|
| "the same check the attach kit applies to NAMESPACE" | attach-lineage.sh:139-140 uses the byte-identical regex |
ReadCredentialFile "already trims; zero-length is its error" |
authlib/config/resolve.go:25-38 — so the raw == "" branch is reachable for whitespace-only files |
required:"true" won't reject a namespace_file-only config |
pipeline/schema.go:64-66 — "just metadata" |
p.cfg.Namespace = ns is race-free |
reloader.go builds fresh pipelines per reload (Configure+Init both run) — which also makes sidecar-patch.sh's "old pipeline kept on a rejected reload" note accurate |
RECIPE's … self_id=<x> namespace=<ns> pass line |
matches the slog.Info arg order |
| self.id uncapped, span name still capped | code and both docs agree; the changelog is explicit that span names are unchanged |
One I could not check: "vendored byte-identical with lab-data-governance" — that repo 404s for me, so it rests on the consumer PR.
Nothing blocking. Six inline items, all suggestions or nits; the two worth acting on are the untested self.id uncapping and the automountServiceAccountToken: false caveat on the recommended platform route.
Author: JoshSag (CONTRIBUTOR — returning external, from fork s-and-p-team/cortex)
Areas reviewed: Go (plugin + config + tests), Shell, Docs
Agent/IDE config (.claude/.vscode): none — grepped for both +++ b/ and rename to
Commits: 1, signed-off: yes
CI: passing (23 green, Spellcheck skipped)
Assisted-By: Claude Code
| // hasIdentity is the one rule behind both identity sources: an identity is | ||
| // a string with at least one non-empty "/"-segment, so serviceLabel has a | ||
| // name to emit. Blank, and separator-only values such as "/", carry none. | ||
| func hasIdentity(id string) bool { return strings.Trim(id, "/") != "" } |
There was a problem hiding this comment.
nit — hasIdentity still admits whitespace-only segments. I ran it: " / / " → TrimSpace → "/ /" → true, and "// //" → true. Since serviceLabel returns non-SPIFFE input as-is, those emit a whitespace self.id — the exact "entity keyed on whitespace" the Init comment above says this prevents.
The motivating / case is caught, so this is cosmetic completeness rather than a live hole:
func hasIdentity(id string) bool { return strings.Trim(id, "/ \t\n\r") != "" }| // both of these are operator configuration (self_id / self_id_file, | ||
| // namespace / namespace_file), and the namespace is bounded to a DNS | ||
| // label by Init besides. | ||
| attribute.String("lineage.self.id", self), |
There was a problem hiding this comment.
suggestion — dropping p.capped() here changes behavior on a pre-existing fact, and nothing tests it. TestAttrBytesCapsCallerControlledValues only asserts url.path / lineage.peer.host / mcp.tool / span name, so there was no stale assertion to update — but there's also no new one, and lineage.self.namespace got TestNamespace_NeverCapped while lineage.self.id got nothing.
A self_id-flavored twin of that test (long self_id, MaxAttrBytes = 4, assert the whole value survives) would pin the half of v1.7.0 that is currently unguarded — and it's the half a future refactor is likelier to silently re-cap, since capped() is the default idiom everywhere else in baseAttrs.
| fi | ||
| kubectl apply -f - <<<"$cm" | ||
| kubectl patch deploy "$DEPLOY" -n "$NAMESPACE" --type strategic --patch "$patch" || { | ||
| patched="$(kubectl patch deploy "$DEPLOY" -n "$NAMESPACE" --type strategic --patch "$patch")" || { |
There was a problem hiding this comment.
nit — patched isn't declared local, making it the only variable in apply() that leaks to global scope: cm patch undo restored_image cm_existed are declared on line 175 and dryrun_err gets its own local on line 198. Adding it to the line-175 declaration keeps the function's convention intact.
| # kept. "attached" below would then be true of the objects and false of | ||
| # the spans. Say so, and name the check. | ||
| case "$patched" in | ||
| *"(no change)"*) |
There was a problem hiding this comment.
suggestion — the NOTE itself is genuinely valuable: it catches the case where "attached" is true of the objects and false of the spans, which is exactly the trap a re-run falls into. But it hinges on matching kubectl's human-readable (no change) prose, which isn't part of any API contract and has changed across kubectl versions.
.metadata.generation gives the same signal from a stable surface — read it before the patch, compare after (or -o jsonpath='{.metadata.generation}' on the patch itself); unchanged generation means no roll, regardless of what kubectl chose to print.
|
|
||
| That file is projected by the kubelet from the pod's own metadata into every | ||
| container that mounts the service-account volume; if the injected sidecar | ||
| does not mount it, the plugin refuses to start (loudly, naming the path) |
There was a problem hiding this comment.
suggestion — worth naming the one way this file is absent that isn't a wrong path or a missing mount: automountServiceAccountToken: false on the pod or the ServiceAccount, which suppresses the projection entirely.
The no-poller reasoning is sound — the kubelet projects before the container starts, so absence really is misconfiguration rather than a race. But on a cluster that disables automount, the route recommended here crashloops the whole sidecar and every plugin in its chain, which is the failure mode v1.6.3 deliberately walked back for self_id_file. A troubleshooting row pointing at automount would turn a confusing outage into a one-line fix.
| // all. Empty, blank, or not an RFC 1123 DNS label (the only shape a | ||
| // namespace can have) refuses at start (see resolveNamespace); when empty, | ||
| // NamespaceFile is consulted instead. | ||
| Namespace string `json:"namespace" required:"true" description:"This workload's Kubernetes namespace (an RFC 1123 DNS label), emitted as lineage.self.namespace on every span; refused at start when empty or not a label. Alternatively namespace_file."` |
There was a problem hiding this comment.
nit — namespace is the only required:"true" field in this Config, and the tag carries no "one-of" notion, so abctl's renderer (edit/templates.go:110, collectRequiredPaths) will emit # Required: namespace plus a [REQUIRED] annotation — contradicting the README's platform recipe, which sets only namespace_file.
I checked that this can't actually break that path: pipeline/schema.go:64-66 is explicit that Required is metadata and "boot semantics are the plugin's own concern." And the tag is defensible on its own terms — this is the one key with no default, unlike self_id, which falls back to self_id_file's default. The description does name the alternative. It's purely that an operator generating a template from the schema will be told they can't omit a key the docs tell them to omit.
github-project-automation
Bot
moved this from New/ToDo
to Done
in Rossoctl Issue Prioritization
4 participants
Summary
Fixes #917. Producer half of rossoctl/lab-data-governance#201; consumer: rossoctl/lab-data-governance#230.
lineage-telemetryemitslineage.self.namespaceon both spans, from a new requirednamespacekey, ornamespace_filefor the kubelet-projected file in ConfigMaps shared across namespaces. Resolved first inInit; absent, blank or not an RFC 1123 DNS label refuses at start, nothing left behind; not capped; never parsed out of the SPIFFE path. Wire contract v1.7.0, vendored byte-identical with lab-data-governance.Unchanged:
lineage.self.id, its reduction, the §4 clause and the "collides by design" row inTestServiceLabel(#761 rounds 5–6) — that was the documentation half; this adds the identity half. Closed alongside: #761 round 6's open question — a separators-onlyself_id(/) now refuses at start like a blank one.Compatibility. Additive on the wire; breaking in configuration both ways (an older sidecar rejects the key), so image and ConfigMap flip together per pod — one
sidecar-patch.shre-run with a matchingSIDECAR_IMAGE. The kit rendersNAMESPACEand warns on a no-op re-run; README covers the upgrade, the enrolled route vianamespace_file, and three troubleshooting rows. Schema marks the key required; Capabilities cite → v1.7.Tests. Both spans, both directions, through decode → Init → emit; never capped; refusals (absent, blank,
/, uppercase, spaces, dots, 64 chars; 63 and padded pass) before the identity poll;namespace_file; schema flag;/and//in the identity table. gofmt, vet,go test -race ./...clean.Live. Same weather pair under identical names in
team1andteam2: four entity rows, each trace on its own rows; ateam1agent calling theteam2tool attributes totool:team2/weather-tool; bad or missing keys crashloop the sidecar with the message naming the key;namespace_fileresolvesteam1; the #853 demo passes end to end on this image.Summary by CodeRabbit
lineage.self.namespace.