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.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1b48d1e8-85f6-4575-b25e-4b022d62eea7
Tatsat (Tats) Mishra 🐉 (Tatsinnit)
left a comment
Tatsat (Tats) Mishra 🐉 (Tatsinnit)
left a comment
There was a problem hiding this comment.
Re-reviewed after a809fc8e. You've addressed all of it, and in a couple of places more thoroughly than I'd suggested.
Cloud Shell framing — gone from kickstart-discover, kickstart-deploy, kickstart-generate and the reviewer agent, and the ~/clouddrive/ instruction with it. "Avoids depending on a local Docker daemon" carries the change without asserting where kickstart runs, which is exactly right.
A6 — the correction is more precise than my comment was:
Applies to: Node and node-pool configuration, not Pod tolerations. The policy evaluates node
spec.taintsand excludes system pools by theirkubernetes.azure.com/modelabel.
That matches the constraint template exactly, including the kubernetes.azure.com/mode exclusion I hadn't mentioned. The table row, the reviewer agent's mutating-set, and the deploy "toleration is stripped" line are all consistent now.
Toleration semantics — "permits, but does not force, scheduling onto a matching tainted node" is the correct statement.
RUN test -f — removed, and the replacement picks up both halves of what I raised: no shell in distroless/scratch, and that path-existence never proved the entry point landed in the right directory. Reconciling final-stage COPY/ADD against WORKDIR and CMD is a better check than the one it replaces.
Phase ordering — the ACR permission preflight as step 1 of Generate, wired to aks.checkDeploymentPermissions with the acr-push/acr-tasks probes and PIM escalation, resolves the 403-two-phases-early problem properly. Better than the fallback I'd floated.
AGENTS.md / kickstart-guide.md — both updated, so nothing contradicts the new flow.
One left, and it's the one I flagged as lowest-confidence, so genuinely fine to leave:
kickstart-generate/SKILL.md:50 still reads "app.kubernetes.io/managed-by is the convention already used elsewhere in this extension". As far as I can find there's one authored use — resources/yaml/argocd-application.template.yaml:30, value argocd — plus read-only selectors for Microsoft.ArgoCD. The extension's documented tool-provenance schema is aks-project/deployed-by: vscode (container-assist-integration.md:226,245), described as shared with aks-desktop. Not blocking, and I have no strong view on which prefix wins — just that the justification sentence claims more precedent than exists. Happy for it to land as-is.
Approving.
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