Isolate concurrent snapshot agent runs in Kubernetes

[mchmarny]

Bug description

Client.CollectSnapshot documents concurrent calls as safe and independent, but snapshot agent runs share several Kubernetes resources and selectors:

• Every run creates or updates the cluster-scoped aicr-node-reader ClusterRole and ClusterRoleBinding.
• Cleanup from either run deletes those shared objects.
• Non-ConfigMap outputs stage through the same aicr-snapshot ConfigMap in the namespace.
• Pod discovery uses only app.kubernetes.io/name=aicr and selects the youngest matching pod, rather than a pod owned by the current Job.
• Callers commonly use the same default Job and ServiceAccount names.

Impact

Overlapping SDK or CLI calls can overwrite RBAC subjects/rules, delete permissions while another Job is running, delete/recreate another run's Job, stream logs from the wrong pod, or retrieve another run's snapshot. DiscoverNetwork makes the shared ClusterRole rules configuration-dependent, increasing the race surface.

Evidence

• Concurrent-safety contract:

  aicr/pkg/client/v1/aicr.go

  Lines 1440 to 1442 in 6cb3ab9

  	// Concurrent CollectSnapshot calls are safe; each call constructs an
  	// independent run.
  	func (c *Client) CollectSnapshot(ctx context.Context, cfg *AgentConfig) (*Snapshot, error) {

• Fixed cluster-scoped resource name:

  aicr/pkg/k8s/agent/types.go

  Lines 22 to 23 in 6cb3ab9

  	// clusterRoleName is the name used for the ClusterRole and ClusterRoleBinding.
  	const clusterRoleName = "aicr-node-reader"

• Shared ClusterRole and binding are updated:

  aicr/pkg/k8s/agent/rbac.go

  Lines 205 to 264 in 6cb3ab9

  	// nodes, and patches mellanox.com NicClusterPolicy via server-side
  	// apply. Grant the extra cluster-scoped rules only when the snapshot
  	// opted into discovery so non-network snapshots stay minimal-priv.
  	if d.config.DiscoverNetwork {
  	rules = append(rules, discoverNetworkClusterRules()...)
  	}
  	cr := &rbacv1.ClusterRole{
  	ObjectMeta: metav1.ObjectMeta{
  	Name: clusterRoleName,
  	},
  	Rules: rules,
  	}
  	_, err := d.clientset.RbacV1().ClusterRoles().Create(ctx, cr, metav1.CreateOptions{})
  	if apierrors.IsAlreadyExists(err) {
  	_, err = d.clientset.RbacV1().ClusterRoles().Update(ctx, cr, metav1.UpdateOptions{})
  	if err != nil {
  	return errors.Wrap(errors.ErrCodeInternal, "failed to update ClusterRole", err)
  	}
  	return nil
  	}
  	if err != nil {
  	return errors.Wrap(errors.ErrCodeInternal, "failed to create ClusterRole", err)
  	}
  	return nil
  	}
  	// ensureClusterRoleBinding creates or updates the ClusterRoleBinding to bind the ClusterRole to the ServiceAccount.
  	func (d *Deployer) ensureClusterRoleBinding(ctx context.Context) error {
  	crb := &rbacv1.ClusterRoleBinding{
  	ObjectMeta: metav1.ObjectMeta{
  	Name: clusterRoleName,
  	},
  	Subjects: []rbacv1.Subject{
  	{
  	Kind: "ServiceAccount",
  	Name: d.config.ServiceAccountName,
  	Namespace: d.config.Namespace,
  	},
  	},
  	RoleRef: rbacv1.RoleRef{
  	APIGroup: rbacAPIGroup,
  	Kind: "ClusterRole",
  	Name: clusterRoleName,
  	},
  	}
  	_, err := d.clientset.RbacV1().ClusterRoleBindings().Create(ctx, crb, metav1.CreateOptions{})
  	if apierrors.IsAlreadyExists(err) {
  	_, err = d.clientset.RbacV1().ClusterRoleBindings().Update(ctx, crb, metav1.UpdateOptions{})
  	if err != nil {
  	return errors.Wrap(errors.ErrCodeInternal, "failed to update ClusterRoleBinding", err)
  	}
  	return nil
  	}
  	if err != nil {
  	return errors.Wrap(errors.ErrCodeInternal, "failed to create ClusterRoleBinding", err)
  	}
  	return nil

• Cleanup deletes shared RBAC:

  aicr/pkg/k8s/agent/deployer.go

  Lines 100 to 121 in 6cb3ab9

  	// Deletions are fanned out concurrently so a slow apiserver does not serialize the wall clock.
  	func (d *Deployer) Cleanup(ctx context.Context, opts CleanupOptions) error {
  	if !opts.Enabled {
  	return nil
  	}
  	type result struct {
  	label string
  	err error
  	}
  	tasks := []struct {
  	label string
  	op func(context.Context) error
  	}{
  	{fmt.Sprintf("Job %q", d.config.JobName), d.deleteJob},
  	{fmt.Sprintf("ServiceAccount %q", d.config.ServiceAccountName), d.deleteServiceAccount},
  	{fmt.Sprintf("Role %q", d.config.ServiceAccountName), d.deleteRole},
  	{fmt.Sprintf("RoleBinding %q", d.config.ServiceAccountName), d.deleteRoleBinding},
  	{fmt.Sprintf("ClusterRole %q", clusterRoleName), d.deleteClusterRole},
  	{fmt.Sprintf("ClusterRoleBinding %q", clusterRoleName), d.deleteClusterRoleBinding},
  	}

• Fixed staging ConfigMap:

  aicr/pkg/snapshotter/agent.go

  Lines 501 to 529 in 6cb3ab9

  	// agentConfigMapTarget resolves where the agent Job stages its result and
  	// whether that ConfigMap is the user's delivery vehicle.
  	//
  	// The Job always writes to a ConfigMap. When config.Output is a cm:// URI the
  	// user asked for that exact ConfigMap, so the Job targets it directly and
  	// deliverViaConfigMap is true — which makes a failed AKS-pool-merge rewrite
  	// fatal rather than a warning, because the bytes the user will read live
  	// there. Any other Output (file, stdout, template, or unset) stages to an
  	// internal ConfigMap in config.Namespace that the caller never sees.
  	//
  	// A cm:// Output is fully parsed here, not merely prefix-matched. The
  	// namespace/name only has to be well-formed for the in-pod writer much later,
  	// so a typo like "cm://aicr-snapshot" (no namespace) would otherwise surface
  	// as a Job failure — after RBAC and the Job exist, and with Cleanup false
  	// (the zero value) they stay behind. Returns ErrCodeInvalidRequest instead.
  	func agentConfigMapTarget(config *AgentConfig) (uri string, deliverViaConfigMap bool, err error) {
  	if strings.HasPrefix(config.Output, serializer.ConfigMapURIScheme) {
  	if _, _, parseErr := pod.ParseConfigMapURI(config.Output); parseErr != nil {
  	// Wrap with the same code rather than PropagateOrWrap: the inner
  	// error says "invalid configmap URI", which does not tell the
  	// caller WHICH input was wrong. Naming the field is the point.
  	return "", false, errors.Wrap(errors.ErrCodeInvalidRequest,
  	fmt.Sprintf("invalid ConfigMap output URI %q (expected cm://namespace/name)", config.Output),
  	parseErr)
  	}
  	return config.Output, true, nil
  	}
  	return fmt.Sprintf("%s%s/aicr-snapshot", serializer.ConfigMapURIScheme, config.Namespace), false, nil
  	}

• Global pod selector:

  aicr/pkg/k8s/agent/wait.go

  Lines 111 to 149 in 6cb3ab9

  	// findPodName finds the pod name by label selector for this Job.
  	// One-shot: returns ErrCodeNotFound if no pod is currently labeled.
  	// Skips pods that are being deleted or have already failed so an
  	// orphaned pod from a prior run is not selected.
  	func (d *Deployer) findPodName(ctx context.Context) (string, error) {
  	pods, err := d.clientset.CoreV1().Pods(d.config.Namespace).List(ctx, metav1.ListOptions{
  	LabelSelector: agentLabelSelector,
  	})
  	if err != nil {
  	return "", errors.Wrap(errors.ErrCodeInternal, "failed to list Pods", err)
  	}
  	name := pickLivePod(pods.Items)
  	if name == "" {
  	return "", errors.New(errors.ErrCodeNotFound, fmt.Sprintf("no Pods found for Job %s", d.config.JobName))
  	}
  	return name, nil
  	}
  	// pickLivePod returns the name of the youngest pod that is neither being
  	// deleted nor in a Failed phase. Returns "" if no usable pod exists.
  	func pickLivePod(pods []corev1.Pod) string {
  	var best *corev1.Pod
  	for i := range pods {
  	p := &pods[i]
  	if p.DeletionTimestamp != nil {
  	continue
  	}
  	if p.Status.Phase == corev1.PodFailed {
  	continue
  	}
  	if best == nil || p.CreationTimestamp.After(best.CreationTimestamp.Time) {
  	best = p
  	}
  	}
  	if best == nil {
  	return ""
  	}
  	return best.Name

Reproduction

1. Start two CollectSnapshot calls against the same cluster and namespace.
2. Use different DiscoverNetwork values or let one run complete cleanup while the other is active.
3. Observe shared RBAC updates/deletion and ambiguous pod/ConfigMap selection.

Expected behavior

Each documented independent run must own and select only its Kubernetes resources and result data.

Acceptance criteria

• Generate a run ID for every snapshot operation and apply it to Job, ServiceAccount, namespaced RBAC, cluster-scoped RBAC, staging ConfigMap, labels, and selectors.
• Select pods by current Job ownership (controller-uid or equivalent), never the global application label alone.
• Cleanup deletes only resources owned by that run.
• Concurrent runs with different network-discovery settings cannot change one another's permissions.
• A concurrency test overlaps two runs and proves correct logs, result bytes, and cleanup ownership.

[mchmarny]

Triaged: First-time classification. Ready and P1 because this violates the public concurrent-safety contract and can return another run's cluster result.

[ayuskauskas]

Proposed design, let me know if you would prefer an ADR.

⚠️ Breaking changes

Fixing this issue means the snapshot agent can no longer use fixed resource names, and can no longer silently overwrite what it finds. Three user-visible behaviors change:

1. --service-account-name changes meaning: select, not create.
It now names an existing ServiceAccount that aicr binds to and never mutates or deletes. If it does not exist, the run fails fast with ErrCodeNotFound before creating anything. Today it names an SA that aicr creates and deletes.
Why: a supplied name is a user-managed identity. Creating it made aicr the owner of an object the user named, and deleting it at cleanup is what lets one run revoke a concurrent run's token mid-flight. Requiring pre-existence is also what makes the whole model exhaustive — see the invariant below.
Impact: anyone passing --service-account-name aicr (today's CLI default, the shipped config examples, and the SDK example) fails on the first run after upgrade. Escapes are named in the error: use the new --service-account-prefix, or omit the flag.

2. Resource names are no longer fixed.
The Job, ServiceAccount, Role, RoleBinding, ClusterRole, ClusterRoleBinding, and internal staging ConfigMap all carry a run ID. kubectl logs job/aicr no longer resolves.
Why: fixed names are the direct cause of every failure mode in this issue. Two runs cannot own the same object.
Impact: the app.kubernetes.io/name=aicr label still works, but selectors keyed on job-name / batch.kubernetes.io/job-name silently match zero pods — including the aicr-agent egress NetworkPolicy documented in docs/integrator/automation.md, which would quietly stop fencing the agent with no diagnostic. A podSelector matching nothing is not an error in Kubernetes.

3. A taken --job-name is now a hard failure.
Returns ErrCodeConflict (exit 2) instead of deleting the existing Job and recreating. The new --force replaces a Job that is not running.
Why: the current delete-and-recreate is how one run kills another's in-flight Job.
Impact: --job-name foo --no-cleanup run twice now fails on the second run; --force is the fix and is named in the error.

The public Go signature does not change, so api-diff will not flag any of this. It needs a release-note callout by hand.

---

Approach

The governing invariant: aicr creates exactly the objects it deletes, and deletes exactly the objects it creates. Every object is either run-owned — named with the run ID, created and deleted by this run — or external: named by the user, required to pre-exist, only referenced. Nothing is created and left behind, and nothing pre-existing is ever deleted.

Object	Name	Lifecycle
ClusterRole / ClusterRoleBinding	aicr-node-reader-<runID>	run-owned
staging ConfigMap	aicr-snapshot-<runID>	run-owned
Role / RoleBinding	<prefix>-<runID> — never user-named	run-owned
ServiceAccount, default	<prefix>-<runID>	run-owned
ServiceAccount, --service-account-prefix p	p-<runID>	run-owned
ServiceAccount, --service-account-name n	n	external — must exist
Job, default or --job-name n	<prefix>-<runID> or n	run-owned; a supplied name is reserved

Three ServiceAccount modes. Let aicr own it, own it under your prefix, or point at one you manage. The last is what serves pre-provisioned workload-identity ServiceAccounts, which today only work by accident. The two flags are mutually exclusive.

Role and RoleBinding are never user-named, even when the ServiceAccount is external — they are aicr's grants, not the user's identity. Otherwise using your own SA would mean hand-authoring aicr's exact RBAC rules. Side benefit: an external SA now receives a per-run grant that is revoked at cleanup instead of sharing a long-lived Role.

The Job reserves rather than requiring pre-existence, because a Job is a workload, not an identity — it is created fresh every run by definition. Collision detection is Create + IsAlreadyExists, which is atomic at the apiserver; a Get-then-Create would have a TOCTOU window open at exactly the moment two runs overlap.

Cleanup is ownership-scoped. The deployer records (kind, name, UID) for everything it creates and deletes only that set, UID-pinned. This matters more than it looks: Cleanup is currently registered as a defer before Deploy, so it runs on the failure path too — without ownership scoping, a run that loses a name conflict would delete the winner's RBAC on its way out, making the fail-closed path more destructive than today's silent overwrite.

Reuses the run-isolation machinery already in this repo. pkg/validator already generates a per-run ID, names its ServiceAccount and ClusterRoleBinding from it, and labels resources with labels.RunID — which is already aicr.run/run-id. The snapshot agent predates that work and never adopted it. Rather than invent a second, incompatible ID format under the identical label key, the shared primitives move to a neutral package and both subsystems use them.

Also fixed along the way: aicr validate currently races aicr snapshot — both default --service-account-name to aicr and both create aicr-node-reader, so the two commands collide even though their Job names differ.

Acceptance criteria

Maps to the criteria in the issue description, with one deviation:
A user-supplied ServiceAccount name is deliberately not run-ID-suffixed. Silently renaming a resource the user explicitly named is worse than the alternative; the three-mode split above covers the same ground without doing that.

[mchmarny]

@ayuskauskas, the core direction aligns with the issue, but I think the design needs revision before implementation. I would prefer an ADR because this changes public SDK/CLI semantics and establishes a durable Kubernetes ownership model.

Three material gaps:

1. An external ServiceAccount does not provide permission isolation. Kubernetes RBAC permissions are additive by subject. If two runs use the same external SA and only one enables DiscoverNetwork, both pods receive the mutating permissions while that run's binding exists. Unique Role/Binding names prevent cleanup races, but they do not satisfy the acceptance criterion that runs with different discovery settings cannot change one another's permissions. This needs an explicit choice: external SAs bring pre-provisioned RBAC and AICR never binds them; external-SA mode explicitly waives permission isolation; or isolated runs always use unique SAs.

2. The ownership invariant is not exhaustive yet. The Namespace is currently created or patched but is absent from the lifecycle table. Explicit cm:// output ConfigMaps are intentionally written and retained. --force also deletes a Job the current run did not create. Please define those lifecycle classes explicitly. If --force remains, restrict it to terminal, recognizably AICR-managed Jobs and delete with a UID precondition; "not running" is too ambiguous.

3. The public contract and security migration need to be part of the design. CollectSnapshot currently promises that every concurrent call is an independent run, while fixed Job names, shared external SAs, and explicit ConfigMaps introduce conflicts or shared effects. The exported AgentConfig and config schema need explicit omitted/prefix/external semantics even if the method signature itself does not change. Also add a stable snapshot-agent label and update the existing NetworkPolicy selector in the same change; documenting that the current selector silently stops matching is not sufficient.

The remaining choices make sense and align well: unique default resources, controller-UID pod selection, atomic Job reservation, UID-pinned cleanup, reuse of the run-ID machinery, and an overlapping-run test that proves correct logs, result bytes, permission behavior, and cleanup ownership.

[ayuskauskas]

Do we need the ability for users to set the SA or Job name? What if those just become prefixes. Then there is never a possibility of overlapped and everything cleanly becomes 1:1 of objects to acir snapshot invoke. The namespace can similarly become a aicr-{run-id} pattern

[ayuskauskas]

ADR is up: ADR-021: Snapshot Agent Run Isolation — #2334 (draft, docs only). Numbered 021 since docs/adr-020-resolution-policy is in flight.

The design changed materially in response to the review. Every user-supplied resource name is now a prefix, never an exact name. --job-name and --service-account-name keep working and keep meaning what they have always meant — a name — but the run ID is always appended. Nothing AICR creates can collide with another run's.

That collapses the three gaps rather than mitigating them:

1. Permission isolation — there is no external-ServiceAccount mode. Every run gets its own ServiceAccount, so RBAC additivity across concurrent runs is structurally impossible; a DiscoverNetwork run's mutating grants bind to an identity no other run uses. This is the third option you listed.

2. Exhaustive ownership — three named lifecycle classes, with every object assigned to one: run-owned (run-ID-suffixed, created and deleted by this run), ensured (the Namespace — created if absent, never deleted), and delivered (an explicit cm:// output ConfigMap — the user's artifact, written on purpose, never deleted). AICR deletes an object if and only if it is run-owned. --force is gone entirely, so there is no path that deletes a Job the run did not create. Cleanup is still UID-pinned.

3. Public contract and migration — covered in decision 8: the exported AgentConfig, the config schema, and the CollectSnapshot godoc all get explicit prefix semantics, including a statement of the one shared effect that remains (two runs targeting the same explicit cm:// URI still overwrite each other, by design). A stable app.kubernetes.io/component: snapshot-agent label is added, and the aicr-agent NetworkPolicy selector is changed in the implementation PR, not just documented.

On the per-run namespace I floated earlier — the ADR argues against it (decision 4). It would need cross-namespace ConfigMap write permission for the output artifact, which reintroduces the broad grant this removes, and a generated namespace inherits none of the operator's PSS labels, quotas, or NetworkPolicy. The Namespace stays user-specified and is classified as ensured.

Sharpest remaining edge, called out under Consequences → Negative: object names stop being predictable, so job-name-keyed selectors silently match zero pods — and a podSelector matching nothing is not an error in Kubernetes. In-tree call sites are fixed in the same change; out-of-tree ones need a release-note callout.

Review on the ADR rather than here, and I will hold implementation until it is accepted.

[ayuskauskas]

ADR renumbered to 020 — the prior ADR-020 PR (#2286) was closed without merging, so the slot is free. Now at docs/design/020-snapshot-agent-run-isolation.md on #2334.

Also folded in review feedback. Two of the changes are substantive rather than editorial:

• One run ID per invocation. aicr validate collects a snapshot and deploys validation Jobs, and today those generate run IDs independently (pkg/cli/validate.go:191 collects, :265 generates). Left as-is that would split names, labels, logs, and cleanup across two IDs for a single aicr validate. The ADR now requires the ID be generated once at the top of the command and passed into collection.
• Pod ownership is established from the ownerReference, not a label. Pod labels — batch.kubernetes.io/controller-uid included — are writable by anything that can update pods in the namespace, so they narrow the candidate set but cannot authorize it. The authority is a controller: true ownerReference of kind Job carrying the recorded Job UID.

The rest were consistency fixes: the collision guarantee is now scoped to run-owned objects (the Namespace and an explicit output ConfigMap are shared by design), the delete-only-what-you-own invariant is scoped to runtime with tools/cleanup's legacy removal named as a migration step, and the staging ConfigMap's UID is called out as needing capture at read time since the in-pod agent creates it.

Note the branch is still named adr-021 — renaming it needs admin on a protected branch, and the name does not reach main since squash-merge composes the commit from the PR title.