diff --git a/internal/cli/cluster.go b/internal/cli/cluster.go index 76a0952a..51af04dc 100644 --- a/internal/cli/cluster.go +++ b/internal/cli/cluster.go @@ -59,7 +59,6 @@ func newClusterInfoCmd() *cobra.Command { kubeconfigPath string contextOverride string nsOverride string - ingestorSAName string tokenExpiry int64 ) @@ -93,7 +92,6 @@ Exit codes: cmd.Context(), printerFor(cmd), kubeconfigPath, contextOverride, nsOverride, - ingestorSAName, tokenExpiry, ) }, @@ -105,9 +103,6 @@ Exit codes: "name of the kubeconfig context to use (default: kubeconfig's current-context)") cmd.Flags().StringVarP(&nsOverride, "namespace", "n", "", "namespace where your tracebloc client is installed (default: the context's namespace, or 'default')") - cmd.Flags().StringVar(&ingestorSAName, "ingestor-sa", "", - "override the ingestor ServiceAccount name (default: \"ingestor\", the chart default; "+ - "set this if you customized `ingestionAuthz.serviceAccountName` in your client's install)") cmd.Flags().Int64Var(&tokenExpiry, "token-expiry-seconds", 600, "requested SA token expiration in seconds (default 600 = 10 min; ignored for static-secret fallback)") @@ -118,7 +113,6 @@ func runClusterInfo( ctx context.Context, p *ui.Printer, kubeconfigPath, contextOverride, nsOverride string, - ingestorSAOverride string, tokenExpiry int64, ) error { p.Banner("tracebloc", "cluster diagnostics") @@ -171,12 +165,8 @@ func runClusterInfo( // `data ingest` will target, so it must not show the pre-scan default. p.Field("namespace", resolved.Namespace) - // Apply the SA-name override here. Discovery doesn't read the - // name from the cluster (see #7); customers with a non-default - // name pass --ingestor-sa. - if ingestorSAOverride != "" { - release.IngestorSAName = ingestorSAOverride - } + // release.IngestorSAName is discovered from the ingestionAuthz ConfigMap by + // DiscoverParentRelease (#7) — no --ingestor-sa override needed. p.Section("Client install") p.Field("name", release.ReleaseName) diff --git a/internal/cli/coverage_test.go b/internal/cli/coverage_test.go index 2939324e..76cfde76 100644 --- a/internal/cli/coverage_test.go +++ b/internal/cli/coverage_test.go @@ -226,7 +226,7 @@ func TestRunClusterInfo_BadKubeconfigExitsThree(t *testing.T) { } var buf bytes.Buffer - err := runClusterInfo(context.Background(), ui.New(&buf), bad, "", "", "", 600) + err := runClusterInfo(context.Background(), ui.New(&buf), bad, "", "", 600) if err == nil { t.Fatal("runClusterInfo with a broken kubeconfig returned nil; want an exitError") } diff --git a/internal/cli/data.go b/internal/cli/data.go index cca5bdc1..6d1315ad 100644 --- a/internal/cli/data.go +++ b/internal/cli/data.go @@ -103,12 +103,6 @@ func newDataIngestCmd() *cobra.Command { noInput bool outputJSON bool - // Ingestor SA name override. Used as the ServiceAccountName - // of the ephemeral stage Pod, so the Pod inherits whatever - // imagePullSecrets + PSA exemptions the admin already - // configured for that SA. - ingestorSAName string - // Stage Pod image override. Defaults to the digest-pinned // alpine that ships with the CLI; air-gapped customers // override this to an image their registry mirror serves. @@ -210,7 +204,6 @@ Exit codes: SchemaFlag: schemaFlag, DryRun: dryRun, Overwrite: overwrite, - IngestorSAName: ingestorSAName, StagePodImage: stagePodImage, Detach: detach, IdempotencyKey: idempotencyKey, @@ -267,9 +260,6 @@ Exit codes: "disable interactive prompts; fail on missing required values (for CI/scripts)") cmd.Flags().BoolVar(&outputJSON, "output-json", false, "emit a machine-readable JSON result on stdout (human output → stderr; implies --no-input)") - cmd.Flags().StringVar(&ingestorSAName, "ingestor-sa", "", - "override the ingestor ServiceAccount name (default: \"ingestor\"); "+ - "set this if you customized ingestionAuthz.serviceAccountName in your client's install") cmd.Flags().StringVar(&stagePodImage, "stage-pod-image", "", "override the ephemeral stage Pod's image (default: digest-pinned alpine 3.20 baked into the CLI). "+ "Pin by digest in your override too — tag-only refs drift silently.") @@ -302,7 +292,6 @@ type runDataIngestArgs struct { SchemaFlag string // raw --schema; resolved or inferred after Discover (tabular) DryRun bool Overwrite bool - IngestorSAName string StagePodImage string // Printer renders the pre-flight summary + status output. Built in @@ -652,9 +641,9 @@ other collaborators train against it without ever seeing the raw files.`)) return binding.explain(err) } resolved, cs, release, pvc := target.Resolved, target.Clientset, target.Release, target.PVC - if a.IngestorSAName != "" { - release.IngestorSAName = a.IngestorSAName - } + // release.IngestorSAName is discovered from the ingestionAuthz ConfigMap by + // DiscoverParentRelease (#7) and flows into the stage/teardown pods + the + // jobs-manager token mint below — no --ingestor-sa override. // 7. Show what we found on the cluster — the customer's last look // before any bytes move. diff --git a/internal/cluster/discover.go b/internal/cluster/discover.go index 3f6ad697..00a7b7ff 100644 --- a/internal/cluster/discover.go +++ b/internal/cluster/discover.go @@ -10,6 +10,8 @@ import ( appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" + + "gopkg.in/yaml.v3" ) // ErrNoParentRelease is the sentinel for DiscoverParentRelease's "the namespace @@ -57,14 +59,12 @@ type ParentRelease struct { JobsManagerServiceName string JobsManagerPort int - // IngestorSAName is the name of the ServiceAccount the chart's - // hook pods run as. Today this is always the chart's default - // "ingestor". Customers who set `ingestionAuthz.serviceAccountName` - // to a non-default name in the parent client chart need to - // override via `tracebloc cluster info --ingestor-sa=` - // (and similarly for the future `dataset push` command). Reading - // the name from the cluster's ingestionAuthz ConfigMap so this - // flag becomes unnecessary is a v0.2 follow-up (see #7). + // IngestorSAName is the name of the ServiceAccount the CLI mints a + // jobs-manager token for and runs its stage/teardown pods as. It is + // discovered from the chart's ingestionAuthz ConfigMap — the policy + // jobs-manager actually enforces (see discoverIngestorSAName, #7) — and + // defaults to the chart default "ingestor" when the ConfigMap is absent + // (older chart), unreadable, or ambiguous. IngestorSAName string // IngestorImageDigest is the canonical digest the cluster's @@ -171,17 +171,16 @@ func DiscoverParentRelease(ctx context.Context, cs kubernetes.Interface, namespa release.JobsManagerServiceName = svc release.JobsManagerPort = jobsManagerPort - // Read INGESTOR_IMAGE_DIGEST from jobs-manager's pod-spec env. - // The chart pipes images.ingestor.digest through to here. - // - // SA name is NOT discovered today — the chart doesn't surface - // `ingestionAuthz.serviceAccountName` through the jobs-manager - // env, and reading the ingestionAuthz ConfigMap to learn it is a - // v0.2 follow-up (see #7). We default to "ingestor" (the chart - // default); customers who renamed it pass --ingestor-sa from - // the CLI. Bugbot caught the earlier version that incorrectly - // claimed to read the SA name from env. + // The ingestor ServiceAccount name. Default to the chart default + // ("ingestor"), then let the ingestionAuthz ConfigMap override it — that + // ConfigMap IS the policy jobs-manager enforces, so it is the authoritative + // source for which SA may call submit-ingestion-run (#7). Best-effort: + // a missing ConfigMap (older chart), an RBAC denial, or an ambiguous + // policy all leave the default in place. release.IngestorSAName = "ingestor" + if sa := discoverIngestorSAName(ctx, cs, namespace, release.ReleaseName); sa != "" { + release.IngestorSAName = sa + } if len(d.Spec.Template.Spec.Containers) > 0 { for _, env := range d.Spec.Template.Spec.Containers[0].Env { if env.Name == "INGESTOR_IMAGE_DIGEST" { @@ -193,6 +192,61 @@ func DiscoverParentRelease(ctx context.Context, cs kubernetes.Interface, namespa return release, nil } +// discoverIngestorSAName reads the chart's ingestion-authz ConfigMap +// (`-ingestion-authz`, key `ingestion-authz.yaml`) and returns the +// ServiceAccount the CLI should mint a token for — the one jobs-manager will +// accept on POST /internal/submit-ingestion-run. It replaces the old hardcoded +// "ingestor" default + the manual --ingestor-sa flag (#7). +// +// The policy lists `allowed[]` entries of (service_account, namespace, +// table_prefixes). We keep only entries whose namespace matches the namespace +// we'll mint in (the rendered ConfigMap always populates namespace, defaulting +// to the release namespace), then require exactly one distinct non-empty +// service_account. Anything else — ConfigMap absent (older chart), RBAC denial, +// empty policy, or two different SAs — returns "" so the caller keeps the +// "ingestor" default rather than guessing an SA the server would reject. +// Best-effort by contract: never returns an error. +func discoverIngestorSAName(ctx context.Context, cs kubernetes.Interface, namespace, releaseName string) string { + if releaseName == "" { + return "" + } + cm, err := cs.CoreV1().ConfigMaps(namespace).Get( + ctx, releaseName+"-ingestion-authz", metav1.GetOptions{}) + if err != nil { + // NotFound (older chart / no ingestor subchart) or an RBAC/API error — + // both non-fatal; fall back to the default. + return "" + } + raw, ok := cm.Data["ingestion-authz.yaml"] + if !ok { + return "" + } + var policy struct { + Allowed []struct { + ServiceAccount string `yaml:"service_account"` + Namespace string `yaml:"namespace"` + } `yaml:"allowed"` + } + if err := yaml.Unmarshal([]byte(raw), &policy); err != nil { + return "" + } + sa := "" + for _, e := range policy.Allowed { + // Only entries that apply to the namespace we mint in — a token minted + // here is worthless against a policy entry scoped to a different ns. + if e.Namespace != namespace || e.ServiceAccount == "" { + continue + } + switch { + case sa == "": + sa = e.ServiceAccount + case sa != e.ServiceAccount: + return "" // ambiguous — more than one SA for this namespace; keep the default + } + } + return sa +} + // FindClientNamespaces scans every namespace the kubeconfig user may list for // jobs-manager Deployments (the same selector + name filter DiscoverParentRelease // uses) and returns the sorted, de-duplicated namespaces hosting one. It backs diff --git a/internal/cluster/discover_test.go b/internal/cluster/discover_test.go index 23e2d8e5..10466c58 100644 --- a/internal/cluster/discover_test.go +++ b/internal/cluster/discover_test.go @@ -330,3 +330,78 @@ func siblingDeployment(name, namespace string) *appsv1.Deployment { }, } } + +// ingestionAuthzCM builds the chart's `-ingestion-authz` ConfigMap +// with the given rendered policy body under the `ingestion-authz.yaml` key — +// the contract DiscoverParentRelease reads the ingestor SA name from (#7). +func ingestionAuthzCM(release, namespace, policyYAML string) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: release + "-ingestion-authz", + Namespace: namespace, + }, + Data: map[string]string{"ingestion-authz.yaml": policyYAML}, + } +} + +// A renamed ingestor SA in the ingestionAuthz policy is discovered and +// overrides the "ingestor" default — the whole point of #7 (no --ingestor-sa). +func TestDiscoverParentRelease_DiscoversRenamedSAFromConfigMap(t *testing.T) { + const ns = "tracebloc" + cs := fake.NewClientset( + jobsManagerDeployment("tracebloc", ns, "client-1.3.5", "1.3.5", "d"), + jobsManagerService("jobs-manager", ns), + ingestionAuthzCM("tracebloc", ns, + "allowed:\n - service_account: \"foo-ingestor\"\n namespace: \""+ns+"\"\n table_prefixes: [\"\"]\n"), + ) + release, err := DiscoverParentRelease(context.Background(), cs, ns) + if err != nil { + t.Fatalf("DiscoverParentRelease: %v", err) + } + if release.IngestorSAName != "foo-ingestor" { + t.Errorf("IngestorSAName = %q, want %q (discovered from the ingestionAuthz ConfigMap)", + release.IngestorSAName, "foo-ingestor") + } +} + +// Two entries naming different SAs for the same namespace is ambiguous — keep +// the "ingestor" default rather than guess one the server would reject. +func TestDiscoverParentRelease_AmbiguousSAKeepsDefault(t *testing.T) { + const ns = "tracebloc" + cs := fake.NewClientset( + jobsManagerDeployment("tracebloc", ns, "client-1.3.5", "1.3.5", "d"), + jobsManagerService("jobs-manager", ns), + ingestionAuthzCM("tracebloc", ns, + "allowed:\n - service_account: \"sa-one\"\n namespace: \""+ns+"\"\n"+ + " - service_account: \"sa-two\"\n namespace: \""+ns+"\"\n"), + ) + release, err := DiscoverParentRelease(context.Background(), cs, ns) + if err != nil { + t.Fatalf("DiscoverParentRelease: %v", err) + } + if release.IngestorSAName != "ingestor" { + t.Errorf("IngestorSAName = %q, want the default %q on an ambiguous policy", + release.IngestorSAName, "ingestor") + } +} + +// An entry scoped to a DIFFERENT namespace than the one we resolve/mint in must +// be ignored — a token minted in our namespace is worthless against it. Pins +// the namespace-gate (#7 stress-review refinement). +func TestDiscoverParentRelease_CrossNamespaceSAIgnored(t *testing.T) { + const ns = "tracebloc" + cs := fake.NewClientset( + jobsManagerDeployment("tracebloc", ns, "client-1.3.5", "1.3.5", "d"), + jobsManagerService("jobs-manager", ns), + ingestionAuthzCM("tracebloc", ns, + "allowed:\n - service_account: \"other-ns-sa\"\n namespace: \"somewhere-else\"\n table_prefixes: [\"\"]\n"), + ) + release, err := DiscoverParentRelease(context.Background(), cs, ns) + if err != nil { + t.Fatalf("DiscoverParentRelease: %v", err) + } + if release.IngestorSAName != "ingestor" { + t.Errorf("IngestorSAName = %q, want the default %q (cross-namespace entry must be ignored)", + release.IngestorSAName, "ingestor") + } +} diff --git a/internal/push/pod.go b/internal/push/pod.go index fb28e6d7..f952b1e7 100644 --- a/internal/push/pod.go +++ b/internal/push/pod.go @@ -123,9 +123,9 @@ type PodSpecOptions struct { // Image overrides DefaultStagePodImage. Empty = use default. Image string - // ServiceAccountName is the SA the Pod runs as. Phase 2's - // discovery surfaces this as `ingestor` (the chart default) or - // whatever the customer's `--ingestor-sa` flag overrides to. + // ServiceAccountName is the SA the Pod runs as. Discovery surfaces + // this from the ingestionAuthz ConfigMap (the chart default `ingestor` + // when the policy is absent/ambiguous — see cluster.discoverIngestorSAName). // Using the chart's existing SA means the Pod inherits any // imagePullSecrets and PSA exemptions the admin already // configured for it. diff --git a/internal/push/stage.go b/internal/push/stage.go index eed58850..369e6a49 100644 --- a/internal/push/stage.go +++ b/internal/push/stage.go @@ -25,8 +25,8 @@ type StageOptions struct { // Tests: a fakeExecutor (see stream_test.go). Executor Executor - // Namespace + IngestorSAName come from Phase 2's parent-release - // discovery (with optional --ingestor-sa override). + // Namespace + IngestorSAName come from parent-release discovery + // (IngestorSAName is read from the ingestionAuthz ConfigMap, #7). Namespace string IngestorSAName string diff --git a/internal/push/stage_test.go b/internal/push/stage_test.go index b46e0cbb..558f4adc 100644 --- a/internal/push/stage_test.go +++ b/internal/push/stage_test.go @@ -188,13 +188,12 @@ func TestStage_OrphanWarningSurfaces(t *testing.T) { } } -// TestStage_IngestorSANameFlowsToPod: the --ingestor-sa override -// MUST land on the stage Pod's ServiceAccountName — otherwise the -// flag is silently ignored and customers who renamed the chart's -// ingestor SA get pods running as the default SA (no PVC write -// access). Pin the integration here at the Stage layer since the -// flag wiring's effect is only observable end-to-end. Bugbot -// flagged the missing test coverage on PR-a. +// TestStage_IngestorSANameFlowsToPod: the discovered ingestor SA name +// MUST land on the stage Pod's ServiceAccountName — otherwise customers +// whose ingestionAuthz policy names a non-default SA (#7) get pods +// running as the wrong SA (no PVC write access). Pin the integration +// here at the Stage layer since the wiring's effect is only observable +// end-to-end. Bugbot flagged the missing test coverage on PR-a. func TestStage_IngestorSANameFlowsToPod(t *testing.T) { root := imgcDir(t) layout, err := Discover(root)