Skip to content

fix(cli): discover ingestor SA from ingestionAuthz ConfigMap; drop --ingestor-sa (#7) - #176

Merged
saadqbal merged 1 commit into
developfrom
fix/7-ingestor-sa-discovery
Jul 8, 2026
Merged

fix(cli): discover ingestor SA from ingestionAuthz ConfigMap; drop --ingestor-sa (#7)#176
saadqbal merged 1 commit into
developfrom
fix/7-ingestor-sa-discovery

Conversation

@LukasWodka

Copy link
Copy Markdown
Contributor

Closes#7.

Problem

The ingestor ServiceAccount name was hardcoded to "ingestor" in DiscoverParentRelease, with a manual --ingestor-sa flag on cluster info and data ingest as the only escape for customers who set a non-default ingestionAuthz.serviceAccountName. A renamed SA made the CLI mint a token jobs-manager then rejected — and data delete had no such flag at all, so it broke with no workaround.

Fix

Discover the SA from the chart's <release>-ingestion-authz ConfigMap (key ingestion-authz.yaml) — the very policy jobs-manager enforces, so it's authoritative. New best-effort cluster.discoverIngestorSAName:

  • reads allowed[], keeps entries whose namespace matches the namespace we mint in (the rendered policy always populates namespace, defaulting to the release ns),
  • requires exactly one distinct service_account;
  • a missing ConfigMap (older chart), RBAC denial, empty, or ambiguous policy → keeps the "ingestor" default. Never errors — discovery is advisory.

With discovery authoritative, --ingestor-sa is removed from cluster info and data ingest (per the issue's acceptance criteria). Everything downstream — stage/teardown pods, the jobs-manager token mint, and now data delete — reads the discovered release.IngestorSAName. Dropping the flag is safe: the ConfigMap is the enforced policy, so an override to an SA absent from it was rejected server-side anyway — the flag never added real capability.

⚠️ Contract correction

The issue sketched a top-level serviceAccountName key. Verified against tracebloc/clienttemplates/ingestion-authz-configmap.yaml, the real contract is allowed[].service_account (a list) with per-entry namespace — building to the sketch would have compiled but never matched.

Tests (internal/cluster/discover_test.go)

  • DiscoversRenamedSAFromConfigMap — renamed SA is discovered (the core fix).
  • AmbiguousSAKeepsDefault — two SAs for one ns → default.
  • CrossNamespaceSAIgnored — entry scoped to another ns is ignored (pins the namespace gate).
  • Existing happy-path (no ConfigMap) proves the "ingestor" fallback.

Full internal/... suite + go vet + gofmt green.

Sequencing: removes the global --ingestor-sa from data.go's flag block — coordinate merge order with the open ingest-path PR #173 and the planned #76/#77, but the regions differ.

🤖 Generated with Claude Code

… drop --ingestor-sa (#7)
The ingestor ServiceAccount name was hardcoded to "ingestor" in
DiscoverParentRelease, with a manual --ingestor-sa flag on both
`cluster info` and `data ingest` as the only escape for customers who
set a non-default `ingestionAuthz.serviceAccountName`. A renamed SA
made the CLI mint a token jobs-manager then rejected — and `data delete`
had no flag at all, so it broke with no workaround.
Discover it instead from the chart's `<release>-ingestion-authz`
ConfigMap (key `ingestion-authz.yaml`) — the very policy jobs-manager
enforces, so it's the authoritative source. New best-effort
cluster.discoverIngestorSAName reads `allowed[]`, keeps entries whose
`namespace` matches the namespace we mint in (the rendered policy
always populates namespace), and requires exactly one distinct
`service_account`; a missing ConfigMap (older chart), RBAC denial,
empty, or ambiguous policy all keep the "ingestor" default. It never
errors — discovery is advisory.
With discovery authoritative, --ingestor-sa is removed from `cluster
info` and `data ingest` (its acceptance criteria); everything
downstream (stage/teardown pods, the jobs-manager token mint, and now
`data delete`) reads the discovered release.IngestorSAName.
Verified the contract against tracebloc/client
templates/ingestion-authz-configmap.yaml: name `<release>-ingestion-authz`,
key `ingestion-authz.yaml`, `allowed[].service_account` + per-entry
`namespace` — NOT the top-level `serviceAccountName` the issue sketched.
Tests: discover_test.go — DiscoversRenamedSAFromConfigMap,
AmbiguousSAKeepsDefault, CrossNamespaceSAIgnored (pins the namespace
gate); the existing happy-path (no ConfigMap) proves the fallback. Full
internal suite + vet + gofmt green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Jul 8, 2026
@LukasWodka
LukasWodka requested a review from saadqbalJuly 8, 2026 04:37
@saadqbal

Copy link
Copy Markdown
Collaborator

Nice — and good catch on the allowed[].service_account contract correction (verified against the chart; namespace always renders, so the ns gate is sound). Built + tested the branch, green.

One thing worth a note: the chart supports multiple allowed[] entries with different SAs scoped by table_prefixes. For that config, discovery hits the ambiguous→default path and falls back to ingestor, which the policy rejects — and with --ingestor-sa gone there's no workaround. So the "flag never added real capability" line isn't quite right: for a multi-SA policy the flag was the selector. Fine to punt if nobody runs multi-SA-per-ns, but then it's worth saying so. Ideal fix later would be to pick the SA whose table_prefixes match the target table rather than requiring exactly one.

@saadqbal
saadqbal merged commit 421ddd0 into developJul 8, 2026
17 checks passed
@saadqbal

Copy link
Copy Markdown
Collaborator

/fr-pass

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

@LukasWodka@saadqbal