fix(kickstart): prefer az acr build and safeguards validation in cloudshell - #2376
Conversation
…safeguards Kickstart runs in Azure Cloud Shell, which has no Docker daemon, but the phase skills assumed a local one. Generate listed `docker build` first with ACR as a fallback, and its image-content check (`docker run ... ls`) was local-only while the exit criteria still demanded the entry point be confirmed present. Review then made that same unrunnable check a hard FAIL, so in Cloud Shell the agent would either stall or fabricate a pass. Images are now built with `az acr build` unconditionally — server-side on the ACR remote task builders, never `docker build`. One build path keeps the validated image and the deployed image identical. Entry-point verification is a build-time `RUN test -f <entrypoint>` assertion in the Dockerfile, which fails the ACR build directly and works before the cluster exists. Also documents the Cloud Shell specifics that still matter: clouddrive clone target, AcrPush requirement, context upload cost, idle-session handling. Separately, the safeguard checklist was a pod-security rule set, not the AKS Deployment Safeguards policy set — 9 of the 10 policies were unrepresented, including the mutating ones (resource requests, anti-affinity/topology spread, reserved system pool taints). Omitting those doesn't block admission; the cluster rewrites the object, so the deployed state silently stops matching the generated YAML. The checklist is now split into Part A (the 10 policies, with mutation outcomes) and Part B (pod security), Generate emits compliant manifests up front, and Deploy gains a `safeguard` failure class plus a server-side dry-run preview.
8234fe9 to
b782630
Compare
Three corrections after checking the AKS Deployment Safeguards docs against what the checklist claimed: - The anti-affinity safeguard is replica-conditional. Its error names two replicas, so single-replica workloads aren't flagged. Also documents what the mutator actually injects (preferred anti-affinity weight 100, maxSkew 1 on kubernetes.io/hostname) and its selector-label priority. - The resource-requests mutator defaults are much larger than assumed: 500m CPU and 2048Mi memory per container, not a token value. Minimums are 100m / 100Mi. Worth stating plainly, since omitting requests on a multi-container pod is a real scheduling and cost surprise. - The CSI safeguard evaluates the StorageClass provisioner, not its name. In-tree kubernetes.io/azure-disk and azure-file are rejected in favour of disk.csi.azure.com and file.csi.azure.com. Also records that Gatekeeper runs fail-open, so safeguards are a backstop rather than a guarantee, and that the policy set is all-or-nothing. Adds provenance markers to generated artifacts. Nothing kickstart produced was identifiable as kickstart-generated, while sibling features in this extension already stamp theirs. Kubernetes objects now get an app.kubernetes.io/managed-by label plus a kickstart.aks.azure.com/version annotation, Dockerfiles get com.azure.aks.kickstart.* LABELs, and Bicep gets tags. The label/annotation split is deliberate. managed-by is selectable identity, so it belongs in a label; version is read rather than selected, and label values reject '+' so a semver with build metadata could not be stored in one. Both go on the object's own metadata rather than the pod template, since anything on the template changes the pod-template hash and would force a rollout on every version bump, and neither may enter a selector. The restricted-labels safeguard only inspects labels, so the version annotation is outside its scope, but the reserved kubernetes.azure.com prefix is still avoided on both fields. Version is omitted rather than guessed when it can't be determined; the skills are static markdown with no interpolation, so there is not yet a path for injecting the extension version.
Uses the literal string v1 rather than the extension version. The marker now versions the kickstart artifact contract — the label/annotation scheme and the shape of what kickstart emits — instead of tracking releases, so it only changes when that contract changes. This also removes the dependency on injecting the extension version into agent context, which has no path today: the skills are static markdown with no interpolation. The previous "omit the version if you can't determine it" fallback is gone, so generated resources are now consistently stamped. Keeps the version as an annotation rather than a label. It is read, never selected on, and label values reject '+', so it could not hold a semver with build metadata if this later tracks a real release.
There was a problem hiding this comment.
Approving, (sharing some thoughts for your eyes please) the direction is right and the Part A rewrite is genuinely good work. I checked the numbers against MS Learn and they're exact: A2's 500m/2048Mi defaults and 100m/100Mi minimums, the QoS capping, A3's weight-100 anti-affinity and maxSkew: 1/ScheduleAnyway, the app -> app.kubernetes.io/name -> default-antiaffinity-applabel priority, A8 checking provisioner rather than name, fail-open Gatekeeper, all-or-nothing enforcement. Splitting policy-enforced (Part A) from best-practice (Part B) is a real improvement over what was there.
Caveat on everything below: I'm not 100% sure on these, and I don't have full context on where kickstart is headed. They're things I couldn't reconcile while reading, not blockers — please treat them as "worth a second look" rather than findings, and push back freely where I've got it wrong. Not gating the merge on any of them.
One that has no line to attach to: AGENTS.md:17-18 and kickstart-guide.md:77-78 still say "build & inspect the image" / "verify image builds", which reads like the docker build + docker run ... ls loop this PR replaces. Since AGENTS.md gets loaded as instructions it might be worth pulling those two files in — though possibly that's already queued behind this.
| All container images are built with **`az acr build`** — server-side on the ACR remote task builders. Never `docker build`. Kickstart runs in Azure Cloud Shell, which has no Docker daemon, and a single remote build path keeps behavior identical everywhere. | ||
|
|
||
| Cloud Shell constraints to honor: | ||
|
|
||
| - Clone into `~/clouddrive/` (persistent, ~5 GB) rather than the ephemeral home dir. | ||
| - `az acr build` needs the Phase 2 ACR to exist and the caller to hold `AcrPush` (or `Container Registry Tasks Contributor`) — see `/kickstart-handoff`. |
There was a problem hiding this comment.
Purely as some thoughts 💭 am I missing a planned Cloud Shell mode here? I went looking and couldn't find one — rg -in "cloudshell|clouddrive" comes back empty across the repo before this PR, and the launch path reads as local to me: extension.ts:117 -> kickstartLaunch.ts:27 calls workbench.action.closePanel, kickstartChat.ts:39 opens local Copilot chat, and kickstart-handoff:52 suggests brew install Azure/kubelogin/kubelogin.
If it does run locally, the ~/clouddrive/ line on 35 might send people somewhere that doesn't exist — Discover also covers the "use my current workspace" path, so it isn't only the sample-clone flow that sees it.
Worth noting the existing text at kickstart-generate:48 already justified preferring ACR on "no local daemon available" grounds, which gets you the same outcome without depending on where it runs. But entirely possible there's a direction here I don't have visibility into.
| ### A6: reserved-system-pool-taints *(mutating)* | ||
| - **Check**: No app workload declares a `CriticalAddonsOnly` toleration, and no user node pool config sets that taint. AKS uses it to keep customer pods off the system pool. |
There was a problem hiding this comment.
I went digging on this one specifically because the rest of Part A is sourced so tightly, and I think A6 may be node-pool-only rather than covering pod tolerations.
The constraint template (Azure/azure-policy, samples/KubernetesService/restricted-taints/template.yaml) binds:
node := input.review.object
taints := {x | x = node.spec.taints[_].key}
not is_system_pool(node)with is_system_pool keyed off the node's kubernetes.azure.com/mode label. Both fixtures in that directory are kind: Node with spec.taints, and the mutation policy's description says the taint "should not be applied to user pools". I couldn't find anywhere that pod tolerations get evaluated.
If that reading is right, the first half of this check ("No app workload declares a CriticalAddonsOnly toleration") wouldn't have a policy behind it, and A6 would be effectively N/A for generated manifests — which would mean A2 and A3 are the two carrying the mutating weight.
That said, the public constraint template may not be the whole story on an Automatic cluster. If you've seen it actually strip a toleration in practice I'd genuinely like the pointer, and I'll happily be wrong here.
| - **Readiness *and* liveness probes** on every container — use the real health path and port from the structure map, never a guessed `/healthz`. | ||
| - **Unique Service selectors** — each Service must select exactly one workload. In a monorepo do **not** reuse `app: <appName>` across services; use `app: <serviceName>` (or `app.kubernetes.io/name` + `app.kubernetes.io/component`) so no two Services overlap. | ||
| - **No AKS-specific labels** — never set `kubernetes.azure.com/*` labels on your own objects. (`azure.workload.identity/*` labels and annotations are fine and required.) | ||
| - **No reserved system-pool taints/tolerations** — do not add a `CriticalAddonsOnly` toleration to app pods; it would place them on the system pool and AKS strips it anyway. |
There was a problem hiding this comment.
Small one, related to the A6 thread: I don't think "it would place them on the system pool" holds — a toleration only permits scheduling onto a tainted node, it doesn't attract pods there. You'd still need a nodeSelector or node affinity for them to actually land on the system pool. So a stray toleration on its own probably wouldn't move anything.
Not a big deal in practice since you don't want app pods tolerating it either way, but since this is an instruction the agent acts on, the reasoning it's given might be worth tightening.
| ``` | ||
| 2. **Assert the entry point at build time.** Add to the Dockerfile's final stage so a wrong path fails the ACR build itself: | ||
| ```dockerfile | ||
| RUN test -f <entrypointPath> |
There was a problem hiding this comment.
Does this hold for a distroless or FROM scratch final stage? Neither has a shell, so RUN would fail the build outright — and those are common finals for Go and Rust, both of which show up in the Store Demo sample this skill recommends. (The sample's own Dockerfiles land on alpine:3.22 and debian:bookworm-slim, so the sample itself is fine; generated ones might not be.)
Also worth flagging that test -f proves the path exists, whereas the docker run --rm ... ls -la <workdir> it replaces was catching the case the old text explicitly called out — build succeeds but files land in the wrong directory. I'm not sure there's a clean server-side equivalent, so this may just be a tradeoff worth naming rather than something to fix.
| 1. **Build and push**: `az acr build --registry <acr> --image <image>:<tag> -f <dockerfilePath> <buildContext>` | ||
| `az acr build` runs **server-side on the ACR remote task builders** — always use it, never `docker build` + `docker push`. Kickstart runs in Azure Cloud Shell, which has no Docker daemon, and a single remote build keeps the pushed digest and the deployed image identical. | ||
| Use the build context and Dockerfile path from the structure map — never assume repo root (`.`). For monorepos, build each service from its own context. Tag with a version (e.g. v1.0.0), never `:latest`. | ||
| Requires `AcrPush` or `Container Registry Tasks Contributor` (verified in Pre-Deploy Check). Keep the command in the foreground so its streamed log holds an idle Cloud Shell session open, and make sure `.dockerignore` excludes bulk (the whole context is uploaded on each build). |
There was a problem hiding this comment.
"(verified in Pre-Deploy Check)" is accurate here in Phase 7 — but Phase 4 now runs az acr build for validation too, and per kickstart-handoff:89 that wants Container Registry Tasks Contributor, which isn't probed until Phase 6.
As far as I can tell it's never granted either: kickstartProvision.ts:337,740 grant only AcrPull to the cluster's kubelet identity, and kickstartAzureBackend.ts:815 is a stage.warn(...) rather than a gate. So someone without that role might hit a 403 at Phase 4, two phases before the remediation command and /kickstart-pim-activation escalation are loaded.
I realise dropping the local fallback was deliberate, so I'm not suggesting bringing docker build back — would moving just that one ACR permission probe earlier be easier than keeping a fallback path?
4 participants
kickstart agent should always prefer az acr build command to use remote builds instead of docker daemon local builds
add deployment safeguards validation to skills for manifest generation in kickstart