feat(aws-pcs): cluster-scoped Name tag + PCS-API login lookup in docs

[DaisukeMiyamoto]

What

Two coupled changes so multi-cluster deployments in one account/VPC stop
colliding at the tag layer.

1. Cluster-scoped Name tag on every CNG instance

PCS-${CngName} → ${ClusterName}-${CngName} in all four hand-maintained
CNG UserData templates (add-cng, add-cng-p5, add-cng-p6-b200,
add-cng-p6-b300). Two deploy-all stacks in the same VPC no longer both
show PCS-login / PCS-cpu1 in the EC2 console; each is prefixed by
the stack name.

2. Docs derive LOGIN_INSTANCE_ID from the stack — by purpose

• §6 Accessing the Cluster + docs/IAM.md (open a shell on the login
  CNG): resolve via the PCS API —
  CFN Outputs.ClusterId → pcs list-compute-node-groups (name==login)
  → ec2 describe-instances by aws:pcs:compute-node-group-id.
  Tag-naming conventions no longer matter.
• §8.2 Monitoring (reach the monitoring stack): filter EC2 by
  tag:monitoring-role=login + tag:aws:pcs:cluster-id=<the cluster>.
  Same instance today but a different intent — the tag is what the
  monitoring stack itself uses to identify its host.

Every code block is self-contained so it can be copy-pasted as one unit
from GitHub — no cross-block variable threading.

Ancillary changes (in support of the above)

• cluster-user-iam.yaml SSM scope updated in lock-step with (1) —
  ssm:resourceTag/Name: PCS-login* → *-login, so cluster-user's
  login-only SSM scope still resolves under the new Name tag. Same
  intent, adjusted for the new naming.
• Two new tests/lint-docs.sh banned patterns to prevent regression
  of (2) — aws:pcs:compute-node-group-name (a tag PCS never emits;
  a doc filter using it returns zero rows silently) and
  Name=tag:Name,Values=PCS- (the old flat naming, superseded).
• tests/readme-walkthrough-test.md (Test 15) — a pre-merge test
  that follows README §3 → §6 → §8.2 as written; every command must
  return a real value, Grafana must load and accept admin +
  SSM-retrieved password. Runs on every PR touching those sections or
  the tags/policies they depend on. Formalises the walkthrough
  performed for this PR's Testing section so the same class of
  copy-paste-first-step regression can't slip through again.

Testing (Test 15 executed on this PR's tree)

Deployed a fresh pcs-walkthrough cluster in us-east-2 from the modified
templates. Every README code block executed verbatim as one paste:

• §6 login lookup returned i-06f3486a5eefaf3b0
• New Name tag confirmed: pcs-walkthrough-login (was PCS-login)
• §8.2 admin password retrieval returned a 32-char value from SSM
• §8.2 Option A monitoring-role=login + aws:pcs:cluster-id lookup
  returned the same instance
• §8.2 port-forward opened localhost:8443 → /grafana/ = 302,
  /grafana/api/health = 200 with {"database":"ok","version":"13.0.2"}
• Basic auth with the SSM-retrieved password returned the admin user JSON
• All 12 provisioned dashboards listed by the Grafana API (Cluster
  Summary, Slurm Detail, GPU Node List, GPU Health, Cluster Costs,
  Storage, …)

Also passes locally:

• bash tests/lint-docs.sh
• aws cloudformation validate-template on all four CNG templates plus
  cluster-user-iam.yaml

[KeitaW]

Review Batch 1/3 — IAM & Access Scoping

[KeitaW]

ssm:resourceTag/Name: *-login is a leading-wildcard grant — it doesn't scope to the cluster, and it can match compute nodes — 🔴 (security) [confirmed]

Observation: The cluster-user SSM condition goes from PCS-login* (anchored prefix) to *-login (leading wildcard), while Resource stays arn:aws:ec2:*:*:instance/*. A leading * in StringLike matches any instance Name ending in -login, and cluster-user-iam.yaml has no ClusterName/ClusterId parameter to narrow it. So a cluster-user can ssm:StartSession (shell / port-forward / SSH-over-SSM) on:

1. Every other cluster's login node — this PR names them <ClusterName>-login, so all of them match *-login. The change makes the Name cluster-unique but the policy discards that uniqueness.
2. Any unrelated account instance ending in -login (bastion-login, vpn-login, …) — strictly broader than the old PCS-login*, which at least required the PCS- prefix.
3. A compute CNG whose name ends in login (e.g. CNG data-login → Name=<cluster>-data-login) — this breaks the template's own stated guarantee ("cannot open shells on compute nodes"); the old PCS-login* prefix would not have matched it.

The IAM.md prose ("so any login CNG matches; compute/GPU CNGs do not", "the most stable signal available") reassures where it should warn — it omits the account-wide breadth.
Impact: In the very multi-cluster/one-VPC scenario this PR targets, the IAM layer provides no per-cluster isolation and can reach unrelated instances. I confirmed this with iam simulate-custom-policy on the exact statement — under *-login, ssm:StartSession is allowed on myclusterB-login (another cluster), bastion-login / vpn-login (unrelated), and myclusterA-data-login (a compute CNG ending in login), and correctly denied only on myclusterA-cpu1 / -gpu-p5. Every one of those four unintended allows was implicitDeny under the old PCS-login* — so this is a demonstrated widening, not a theoretical one. (Today's real blast radius is small — 1 *-login instance live in the test region — but the glob is unbounded by design.)
Suggestion: Scope by the AWS-managed, operator-immutable aws:pcs:cluster-id tag — the same lever the §8.2 monitoring path in this PR already uses. Add a ClusterId parameter to this template and AND both keys in one StringLike (keys AND together), so it's PCS-instances-of-this-cluster-whose-role-is-login:

"StringLike": {
  "ssm:resourceTag/aws:pcs:cluster-id": "<ClusterId param>",
  "ssm:resourceTag/Name": "*login*"
}

(cluster-id alone would expose compute nodes; Name alone is account-wide — you need both.) If *-login stays for now, please make the IAM.md / description prose state plainly that it's an account-wide leading-wildcard match, so an operator tightening for production isn't misled. — verified live 2026-07-11: cluster-user-iam.yaml params are only GroupName/AttachUsers; instances carry an immutable aws:pcs:cluster-id tag.

[KeitaW]

Review Batch 2/3 — Docs, Tags & Maintainability

The 4-way byte-identical UserData duplication keeps forcing lock-step edits — 🟢 (observation)

Observation: This is the third change in recent memory that had to be applied identically to the four hand-maintained CNG templates (the needrestart guard, the cross-region-S3 --region fix, and now the Name/CngName retag) — I diffed the four added blocks here and they're byte-identical (good, no drift this time).
Impact: None today; it's a standing fragility — the next edit that lands in three of four is where a real divergence appears.
Suggestion: The new tests/lint-docs.sh is a natural home for a cross-file assertion that the four Tags: blocks stay byte-identical, so drift fails CI. Or factor the shared UserData into one source. (Not a blocker for this PR.)

[KeitaW]

The nested LOGIN_INSTANCE_ID=$(…) one-liner fails silently to None, and --cluster-identifier is unquoted — 🟢 [confirmed]

Observation: The triple-nested substitution (this line in §6, and the §8.2 copy) is functionally correct — the JMESPath backticks are protected by single quotes, and nested $(…) parses fine. But there's no error path: a wrong STACK_NAME/AWS_REGION or a missing ClusterId output makes the inner describe-stacks return empty → list-compute-node-groups runs with an empty identifier → the Values= filter matches nothing → --output text prints literal None → aws ssm start-session --target None fails opaquely. Separately, --cluster-identifier $(…) is unquoted, so on an empty ClusterId the token vanishes and the following --region is consumed as the identifier value — a different, more confusing error than a clean "empty identifier."
Impact: A first-time copy-paste into the wrong stack/region gets None with no hint why. (Test 15's "no None/empty" gate is exactly the right guard — but the snippet itself gives the user no diagnostic.)
Suggestion: Split into named steps with a guard, and quote the identifier — IAM.md already splits out CLUSTER_ID for the Grafana step, so the pattern exists:

CLUSTER_ID=$(aws cloudformation describe-stacks --stack-name "$STACK_NAME" --region "$AWS_REGION" \
  --query 'Stacks[0].Outputs[?OutputKey==`ClusterId`].OutputValue' --output text)
[ -n "$CLUSTER_ID" ] && [ "$CLUSTER_ID" != "None" ] || { echo "No ClusterId — check STACK_NAME/AWS_REGION"; return 1; }
CNG_ID=$(aws pcs list-compute-node-groups --cluster-identifier "$CLUSTER_ID" --region "$AWS_REGION" \
  --query 'computeNodeGroups[?name==`login`].id' --output text)
LOGIN_INSTANCE_ID=$(aws ec2 describe-instances --region "$AWS_REGION" \
  --filters "Name=tag:aws:pcs:compute-node-group-id,Values=$CNG_ID" "Name=instance-state-name,Values=running" \
  --query 'Reservations[0].Instances[0].InstanceId' --output text)

[KeitaW]

The CngName tag no longer holds the CNG name — 🟢 [confirmed]

Observation: The custom tag Key: CngName now gets Value: !Sub '${ClusterName}-${CngName}' — identical to Name. A tag keyed CngName whose value is <cluster>-<cng> is misleading; the bare CNG name is !Ref CngName. (It was already mildly off as PCS-${CngName}, but this PR edits the line and could set it right.)
Impact: Redundant with Name; anything grouping/filtering by tag:CngName (dashboards, cost allocation) now sees a cluster-prefixed value, not the CNG name.
Suggestion:

Suggested change

	Value: !Sub '${ClusterName}-${CngName}'
	Value: !Ref CngName

(or drop the duplicate tag, since Name already carries the console-readable value). Applies to the same line in all four add-cng*.yaml.

[KeitaW]

Review Batch 3/3 — Positives & Sources

Things That Look Great

• The PCS-API login lookup is the right approach, and I verified it works live. On a real cluster (pcs_3duasoz99i), list-compute-node-groups → computeNodeGroups[?name=='login'].id → describe-instances by aws:pcs:compute-node-group-id resolves the login node correctly. Moving off the human-facing Name tag to the PCS API (and the immutable aws:pcs:* tags) is exactly the robust, naming-independent path.
• The new walkthrough test (Test 15) is excellent. A pre-merge test that follows README §3 → §6 → §8.2 as written, gating on "every command returns a real value (no None/Not found)", is precisely the guard for the copy-paste-first-step regressions this doc set has hit before. The failure-classification section (None → stale snippet; AccessDenied → IAM scope drift; Grafana 403 → monitoring) is genuinely useful.
• The lint bans are well-formed — aws:pcs:compute-node-group-name (bare, no tag: prefix, so it catches both the CLI-filter and console-hint forms) plus Name=tag:Name,Values=PCS- to retire the old flat naming. This is the correct, complete guard.
• The monitoring path (§8.2) is properly per-cluster scoped (aws:pcs:cluster-id + monitoring-role=login) — that same immutable-tag pattern is the fix the SSM IAM policy above wants.
• Applied consistently — the Name/CngName change is byte-identical across all four CNG templates, and the README/IAM.md/lint changes move in lock-step.
• Self-contained copy-paste blocks with STACK_NAME/AWS_REGION set once per block — a real usability improvement over cross-block variable threading.

---

Sources

• Live verification (2026-07-11, acct 159553542841): PCS cluster pcs_3duasoz99i (us-east-1) — aws pcs list-compute-node-groups returns a login CNG pcs_y20b00q2o6, and describe-instances by tag:aws:pcs:compute-node-group-id resolves login node i-0ca17ac97d8d60571. cluster-user-iam.yaml parameters confirmed to be only GroupName/AttachUsers (no cluster identifier).
• IAM StringLike leading-wildcard semantics: a *-prefixed pattern matches any suffix — IAM policy element: condition operators. Demonstrated live (aws iam simulate-custom-policy, 2026-07-11) on the PR's exact ssm:StartSession statement: *-login allows myclusterB-login, bastion-login, vpn-login, myclusterA-data-login (all implicitDeny under PCS-login*); both correctly deny myclusterA-cpu1 / -gpu-p5.
• PCS instance tags (aws:pcs:cluster-id, aws:pcs:compute-node-group-id) are AWS-applied/immutable — AWS PCS docs.

[KeitaW]

Few comments

[DaisukeMiyamoto]

Thanks for the security catch — Item 1 is exactly the kind of thing this PR should have gotten right the first time. All three should-fixes are addressed in 4b52e3e / 9ecd426 / b9ee204 (three separate commits, one per Item).

Item 1 — cluster-user-iam.yaml scope (4b52e3e)

Adopted your recommendation to key on an operator-immutable cluster identifier, via a required ClusterStackName parameter — the stack name is what the operator typed at deploy time and is already the ClusterName the templates pass through (add-cng*.yaml tags Name=<ClusterName>-<CngName>; deploy-all passes ${AWS::StackName} as ClusterName). So the StringLike becomes an exact-match StringEquals against !Sub "${ClusterStackName}-login" — which is the actual Name tag the login CNG carries.

Verified with aws iam simulate-custom-policy on the deployed policy — all four unintended-allow cases in your report flip to implicitDeny, and the legitimate target-cluster login stays allowed:

ssm:resourceTag/Name context	Result
pcs-um-acct-login (target)	allowed
other-cluster-login	implicitDeny
bastion-login	implicitDeny
pcs-um-acct-cpu1 (compute)	implicitDeny
pcs-um-acct-data-login (compute w/ login suffix)	implicitDeny

Trade-off: one deploy of cluster-user-iam.yaml now grants access to exactly one cluster — deploy it once per target cluster. The template Description, IAM.md's scope wording, the deploy example, and iam-test.md Test B expectations + a simulate-custom-policy recipe are all updated to reflect this. I also rewrote the PolicyDocument from a JSON-string literal to native YAML so the !Sub reference is legible.

Item 2 — split login-lookup snippet (9ecd426)

Adopted your suggestion. All three copies (README §6, README §8.2, IAM.md "What the cluster user can do") now split into three named steps, quote every substitution, and add an early guard:

```bash
CLUSTER_ID=$(aws cloudformation describe-stacks ... --query 'Stacks[0].Outputs[?OutputKey==`ClusterId`].OutputValue' --output text)
[ -n "$CLUSTER_ID" ] && [ "$CLUSTER_ID" != "None" ] || { echo "No ClusterId — check STACK_NAME/AWS_REGION"; return 1; }
LOGIN_CNG_ID=$(aws pcs list-compute-node-groups --cluster-identifier "$CLUSTER_ID" ...)
LOGIN_INSTANCE_ID=$(aws ec2 describe-instances ... "Name=tag:aws:pcs:compute-node-group-id,Values=$LOGIN_CNG_ID" ...)
```

While in there I also fixed the same fail-silent + Name=*login glob in the already-merged docs/JUPYTER.md from #1172 — it was the same pattern, and Item 1's exact-tag argument applies to it too. So JUPYTER.md Step 3 now uses the same PCS-API resolver instead of "Name=tag:Name,Values=*login".

Item 3 — bare CngName tag (b9ee204)

Taken as-is: the CngName custom tag now holds !Ref CngName in all four add-cng*.yaml. As you noted, prefixed values were already off-spec pre-PR (commit 10613e3, #1120, meant the tag to "preserve the per-node-group name" but coded PCS-${CngName}); this PR was widening the drift, and the fix restores the original intent.

[DaisukeMiyamoto]

Correction on my previous comment. The JUPYTER.md change I bundled into 9ecd426 was scope-creep on my part — I saw the same Name=*login glob and reflexively rewrote it to match the README/IAM.md fix, but:

1. JUPYTER.md's filter is a client-side describe-instances search for the end user's own SSM session, not an IAM Condition. Item 1's exact-tag argument is about who a policy grants session-start rights to; a shell filter that resolves the wrong instance just fails against the IAM scope the principal actually has. It isn't a security issue for JUPYTER.md.
2. The replacement pipeline in JUPYTER.md was not verbatim-run on a live cluster; only the README / IAM.md shape was. The Jupyter connect step is the load-bearing step of that guide, and feat(aws-pcs): add Jupyter-on-compute-node guide with verification procedure #1172 shipped its current form with your live-cluster verification.

Reverted in 43a54fc. The README §6, README §8.2, and IAM.md parts of 9ecd426 — the ones you actually asked for in Item 2 — stay.

[KeitaW]

Approving with nit. Kindly resolve conflict before merge!

[KeitaW]

Re-review (round 2) — resolution scoreboard

Thorough response round — all three round-1 findings are addressed with dedicated commits, and I verified the fixes rather than trusting the messages. Headline: the login-node IAM scope is now correct and I re-verified it live — the old *-login leading-wildcard is gone, replaced by StringEquals ${ClusterStackName}-login, and running the PR's own iam-test.md matrix against the resolved policy gives exactly allowed for <stack>-login and implicitDeny for other-cluster-login / bastion-login / <stack>-cpu1 / <stack>-data-login (round 1 those were allowed).

#	Round-1 finding	Status
1	ssm:resourceTag/Name: *-login leading-wildcard SSM grant	✅ Resolved (4b52e3e) — ClusterStackName param + StringEquals ${ClusterStackName}-login; verified live via simulate-custom-policy (allowed + implicitDeny×4). deploy-all wiring (ClusterName=${AWS::StackName}, login CngName: login) confirms the real login node matches.
2	nested LOGIN_INSTANCE_ID=$(…) fails silently to None	✅ Resolved (mostly) (9ecd426) — split, quoted, guarded on CLUSTER_ID. One residual last-hop, inline below.
3	CngName tag held PCS-<name> not the CNG name	✅ Resolved (b9ee204) — !Ref CngName, byte-identical across all four templates.
—	(obs) 4-way byte-identical UserData duplication	➖ Unchanged / informational — still 4 copies; the new lint doesn't assert Tags:-block identity (non-blocking).

Three round-2 comments inline: one 🟡 (the new lint gives a false all-clear) and two 🟢 doc-accuracy notes. Details on each line.

[KeitaW]

The new lint gives a false all-clear — two Jupyter docs still carry the patterns it's meant to eradicate

This PR replaces the mutable-Name-tag login discovery and the PCS-login* scope across README / IAM.md / iam-test.md, and adds these two BANNED patterns to stop them creeping back. The gap: two sibling docs describing the same login-node flow still carry the pre-PR forms, and both sit inside the lint's own DOC_GLOBS (docs/*.md tests/*.md):

• docs/JUPYTER.md:120 and tests/jupyter-notebook-test.md:119 — login discovery via --filters "Name=tag:Name,Values=*login" + Reservations[0].Instances[0] (un-cluster-scoped, arbitrary first match — the exact shared-VPC "wrong cluster" hazard §6/§8/IAM.md now avoid).
• docs/JUPYTER.md:144 — describes the SSM condition as ssm:resourceTag/Name = PCS-login* (the old StringLike). The claim it supports is still true, but the named mechanism is stale and misleads anyone auditing a restricted role from that doc.

I checked live that these two new patterns (aws:pcs:compute-node-group-name, Name=tag:Name,Values=PCS-) match none of those forms, so the lint passes green while the drift survives in-tree — it reads as "no stale lookups" when three remain. I see you deliberately reverted the JUPYTER.md scope change (43a54fc, "out of scope, untested") — deferring the Jupyter rewrite is a reasonable call for a PR already carrying a security fix. The narrower ask is to keep the lint honest about it: either widen the two patterns so they actually catch Values=*login / PCS-login* (which forces the small Jupyter fix now), or add an explicit "Jupyter docs deferred to #NNNN" note so a green lint isn't read as full coverage. I'd lean toward widening the patterns — making this class fail CI is the whole point of adding them.

[KeitaW]

The split login-lookup guards only the first of three hops — a renamed login CNG still lands on --target None

The split + CLUSTER_ID guard is a solid fix for the round-1 silent-None. The residual: the guard covers CLUSTER_ID, but LOGIN_CNG_ID and LOGIN_INSTANCE_ID are unguarded. If list-compute-node-groups finds no CNG named login (renamed, or not yet running), LOGIN_CNG_ID comes back empty → the describe-instances filter becomes Values= → LOGIN_INSTANCE_ID=None → aws ssm start-session --target None fails with an opaque SSM error — the round-1 class, one hop down. A single guard right before the session covers all three hops:

[ -n "$LOGIN_INSTANCE_ID" ] && [ "$LOGIN_INSTANCE_ID" != "None" ] || { echo "No running login node for cluster $CLUSTER_ID"; return 1; }

Two asides: the existing CLUSTER_ID guard uses return 1, which only halts inside a function or a sourced script — pasted at an interactive prompt (which the README instructs) return prints an error and execution continues anyway, so the guard's "stop here" intent isn't enforced interactively. And credit due — the new Test 15 gate (echo $LOGIN_INSTANCE_ID must print an i-…) already catches this before a customer would, so this is polish, not a hole. docs/IAM.md's equivalent block has the same shape and would take the same one-line guard.

[KeitaW]

§6 rationale is slightly self-contradictory — the resilience is against the Name tag, not the CNG name

Small wording thing: the §6 intro says the lookup "works no matter what your CNG is named because the lookup goes through the PCS API." But the lookup hardcodes computeNodeGroups[?name==\login`], so it *does* depend on the login CNG being named login(which deploy-all guarantees). What the PCS-API path actually buys you is independence from the mutable EC2Name` tag — that's the real win worth stating.

Suggested change

	instance ID — it works no matter what your CNG is named because the
	instance ID — it works no matter how instances are Name-tagged because the

[KeitaW]

Walkthrough Test 15, Step 4 — "the same way" conflates two lookup mechanisms

Minor nit in the new (excellent) walkthrough test: Step 4 says §8.2 Option A "produces LOGIN_INSTANCE_ID the same way" as Step 2 (§6). They're different paths — §6 uses the PCS API + aws:pcs:compute-node-group-id, while §8.2 filters on monitoring-role=login + aws:pcs:cluster-id. Both valid, but a reader following the gates literally will notice the commands differ. Dropping "the same way" (or noting the different tag path) keeps the "as written" contract precise.

---

Things that look great (round 2)

• The IAM scope fix is exactly right and I verified it live. ClusterStackName + StringEquals (not StringLike) fails closed — a wrong stack name yields AccessDenied, not an over-grant — and the inline comment enumerating what's excluded (bastion-login, other clusters, *login-suffixed compute CNGs) matches the simulated reality one-for-one.
• The JSON→YAML policy rewrite is faithful. All seven statements preserved, and — the easy CFN foot-gun here — ${aws:username} in SSMSessionTerminateOwn is correctly left outside !Sub, so CloudFormation doesn't try to resolve it as a template parameter (a clean validate-template confirms). The only !Sub is the intended ${ClusterStackName}-login.
• You turned my round-1 demonstration into a durable test. iam-test.md now scopes the check with simulate-custom-policy across the five representative names with the exact expected allowed / implicitDeny×4 — regression guard institutionalized, and its expectations match my live run precisely.
• ClusterStackName is properly constrained — AllowedPattern + a clear ConstraintDescription, so a bad value fails at deploy with a readable message rather than silently mis-scoping.
• Scope discipline — the self-revert of the out-of-scope JUPYTER.md change (43a54fc) is the right call for a PR already carrying a security fix.

---

Sources

• Verified live (2026-07-13, acct 159553542841): validate-template on the rewritten cluster-user-iam.yaml → valid (GroupName/AttachUsers/ClusterStackName, CAPABILITY_NAMED_IAM); simulate-custom-policy on the resolved ssm:StartSession statement → pcs-ml-cluster-login=allowed, other-cluster-login/bastion-login/pcs-ml-cluster-cpu1/pcs-ml-cluster-data-login=implicitDeny; bash tests/lint-docs.sh → PASS.
• deploy-all wiring: pcs-ml-cluster-deploy-all.yaml passes ClusterName: !Sub '${AWS::StackName}' to every CNG stack and sets login CngName: login, so the login node's Name = <stack>-login.
• PCS-emitted instance tags (docs + live EC2, 3 regions): aws:pcs:cluster-id and aws:pcs:compute-node-group-id exist; aws:pcs:compute-node-group-name does not (0 instances) — confirming the lint ban and the id-based lookups. Finding CNG instances, Connect to your cluster.
• IAM StringEquals exact-match (no wildcards): IAM condition operators.