[mashkovd]

What

wft-create-tenant now fails when the tenant already exists, unless the re-run is asked for explicitly.

• validate-tenant-name:
  • clones the repository (alpine/git, mctl-platform-config, the same way tpl-validate-tenant does);
  • fails if platform-gitops/tenants/<name> exists. The error names the way to re-run.
  • A failed clone fails the step: "could not check" is not "absent".
• New parameter reprovision (default "false"):
  • "true" lets an existing tenant through, for re-running its provisioning (Vault policies and roles, ApplicationSet refresh) without writing its files;
  • "true" refuses a tenant that does not exist;
  • any value other than true or false is refused.
• commit-tenant-to-git refuses an existing tenant directory without reprovision=true. This covers a tenant created between validate and the step's own clone. With reprovision=true, the existing skip-and-continue path (SKIP_GIT) runs as before.
• Hardening in validate:
  • parameters are read from env instead of being interpolated into the script, like commit-tenant-to-git's;
  • a multi-line tenant name is refused, because the per-line grep -E would otherwise accept it on one good line.

Why

Until now, the commit step treated an existing tenant directory as "skip the commit, continue provisioning". A request for a taken name therefore ended Succeeded without creating anything, and the template's own header ("Validates tenant name (DNS-safe, not already exists)") was not true.

That skip was added on purpose in 935f0bb, as the way to re-run a half-provisioned tenant. Nothing else in the repository depends on it: the Backstage template and mctl-api only submit new tenants, and no doc or runbook re-runs create-tenant. So the path stays, but behind an explicit opt-in instead of being the silent default.

Re-running a tenant is done by hand:
argo submit --from clusterworkflowtemplate/create-tenant -p tenant_name=<t> -p reprovision=true.
mctl-api does not declare reprovision, so API and MCP submissions always take the default.

Tests

tests/test_wft_create_tenant_existing.py runs the template's own extracted validate source and commit-step check against a fixture checkout, with git clone stubbed. It is wired into validate-manifests. It runs 12 checks:

• validate:
  • a new tenant passes;
  • an existing tenant fails, with the re-run hint;
  • reprovision=true on an existing tenant passes;
  • reprovision=true on a missing tenant fails;
  • a bad reprovision value fails;
  • a failed clone fails;
  • a multi-line name fails;
  • a reserved name still fails;
• commit step:
  • a new tenant writes its files;
  • an existing tenant without the flag fails;
  • an existing tenant with the flag skips its files and continues.

The test also asserts that reprovision defaults to false and is passed to both steps.

On main's template the test fails: validate gets no reprovision, and its existence check is missing. 8 of 8 mutations are caught:

• validate existence check removed;
• commit guard removed (the old behaviour);
• clone failure ignored;
• multi-line guard removed;
• default flipped to true;
• reprovision of a missing tenant allowed;
• value check removed;
• parameter not passed to validate.

validate-shell-param-interpolation.py, validate-tenant-workflow-shared-writes.py and validate-argocd-tenant-rbac.py pass.

Note

#1648 also edits validate-tenant-name's reserved-name list. git merge-tree against its head merges cleanly.

Type of change

• Bug fix

Checklist

• Tests pass (python3 tests/test_wft_create_tenant_existing.py, plus the validators above)
• Linter passes
• Documentation updated (template header and parameter comment)
• Commit messages follow conventional commits

🤖 Generated with Claude Code

https://claude.ai/code/session_01D1kGUJMWeA8hyhQuxKuHd2

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @mashkovd's task in 4m 25s —— View job

---

Review complete ✅

• Gather context (PR diff, CLAUDE.md conventions)
• Review wft-create-tenant.yaml changes (validate-tenant-name, commit-tenant-to-git)
• Review new test file tests/test_wft_create_tenant_existing.py
• Review .github/workflows/validate-manifests.yml wiring
• Post findings / submit verdict

Verdict: APPROVE (no P1/P2, 1 P3 nit posted inline)

Checked:

• Ordering is correct: reprovision value validation happens before the network call, so a bad value fails fast regardless of clone success; a failed git clone fails the step under set -e (not silently treated as "doesn't exist").
• The multi-line-name guard (wc -l check before the per-line grep -E) correctly catches a value that would otherwise slip through the anchored regex.
• commit-tenant-to-git's existence check tightened from -d to -e (catches non-directory paths too), consistent with the new check added in validate-tenant-name.
• Confirmed via a sub-agent that mctl-platform-config does define GITOPS_ORG/GITOPS_REPO (so the new clone URL interpolates correctly), and that the argo-workflows namespace has no egress-restricting NetworkPolicy, so the new HTTPS clone in validate-tenant-name needs no network policy changes.
• Traced the SKIP_GIT=true path through to the closing fi # end SKIP_GIT — it correctly skips the whole write/commit/push block while create-vault-policy still runs unconditionally per the DAG, matching the PR's stated intent for re-provisioning.
• The new test file extracts both shell blocks directly from the live template (via regex/YAML parsing) rather than restating the logic, so it can't drift silently from the template it's testing — a good pattern given this repo's history of shell-interpolation bugs (gitops#992, gitops#993).

P3 (non-blocking): inline comment on wft-create-tenant.yaml:193 — validate-tenant-name now clones over anonymous HTTPS (no deploy-key mount), while tpl-validate-tenant and commit-tenant-to-git both clone via SSH with the deploy key. The PR description says this is "the same way tpl-validate-tenant does," but it's actually a different (arguably better, least-privilege) mechanism — worth confirming that's intentional rather than restating it as equivalent.

[github-actions]

Antigravity (agy) review — informational

Model: gemini-3.8-flash-medium · commit e1dbd17.

Findings

[P1] Unauthenticated HTTPS clone in validate-tenant-name will fail on private repository

• File: platform-gitops/argo-workflows/cluster-templates/wft-create-tenant.yaml:~192

• Anchor: + git clone --depth=1 "https://github.com/${GITOPS_ORG}/${GITOPS_REPO}.git" /tmp/mctl-gitops

• Concrete failure scenario:
  mctlhq/mctl-gitops is a private GitOps repository. In the same workflow template, commit-tenant-to-git requires mounting the mctl-gitops-deploy-key Secret and configuring SSH to clone git@github.com:${GITOPS_ORG}/${GITOPS_REPO}.git.

  In validate-tenant-name, the step attempts an unauthenticated clone over HTTPS (https://github.com/${GITOPS_ORG}/${GITOPS_REPO}.git) with no volume mounts for deploy keys and no GitHub token in its environment. In a non-interactive Kubernetes pod, git clone will fail to authenticate (exiting with status 128 due to disabled terminal prompts or 401/404). Because set -e is enabled, the script immediately exits on failure.

  Since validate is the entrypoint step of create-tenant-pipeline that blocks all subsequent steps, every execution of create-tenant in the cluster will fail at validation, breaking tenant provisioning completely. This was masked in CI because tests/test_wft_create_tenant_existing.py stubs git with a mock shell script that copies a local fixture directory instead of running git.

• Proposed fix:
  Mount the mctl-gitops-deploy-key secret in validate-tenant-name and clone over SSH (git@github.com:${GITOPS_ORG}/${GITOPS_REPO}.git) following the exact SSH setup used in commit-tenant-to-git, or inject authenticated credentials for HTTPS.

[claude]

P3: This clones over anonymous HTTPS (no deploy-key mount), whereas tpl-validate-tenant and commit-tenant-to-git both clone via SSH with the mctl-gitops-deploy-key deploy key. The PR description says this clones "the same way tpl-validate-tenant does" — functionally it's actually better (least-privilege: a read-only existence check doesn't need a write-capable key), but it does mean this step now depends on unauthenticated GitHub HTTPS access and is worth calling out explicitly as an intentional deviation, not an oversight.

[claude]

No P1/P2 findings (1 P3). Good to merge.

Verified: existence check order (reprovision value validated before clone, clone failure under set -e fails the step), the multi-line-name wc -l guard, the -e vs -d tightening in commit-tenant-to-git, GITOPS_ORG/GITOPS_REPO present in mctl-platform-config, the argo-workflows namespace has no egress NetworkPolicy so the new HTTPS clone needs no network changes, and that the SKIP_GIT=true path correctly skips the whole write/commit/push block via the closing fi # end SKIP_GIT while create-vault-policy still runs unconditionally per-step as intended. Test suite extracts both shell blocks directly from the template rather than restating them, so it can't drift silently.

[mashkovd]

Reply to agy P1 (unauthenticated HTTPS clone fails on a private repository): not applicable. mctlhq/mctl-gitops is public (gh api repos/mctlhq/mctl-gitops --jq .visibility returns public), so the anonymous clone succeeds. Claude's review on e1dbd17 also confirmed that the argo-workflows namespace has no egress NetworkPolicy blocking it. Least privilege is the reason the validate step does not mount the deploy key.