Skip to content

fix(k8s): custom-stack VPA targets a non-existent (double-suffixed) deployment#330

Merged
Cre-eD merged 3 commits into
mainfrom
fix/custom-stack-vpa-targetref
Jun 21, 2026
Merged

fix(k8s): custom-stack VPA targets a non-existent (double-suffixed) deployment#330
Cre-eD merged 3 commits into
mainfrom
fix/custom-stack-vpa-targetref

Conversation

@Cre-eD

@Cre-eD Cre-eD commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Bug

For custom stacks (parentEnv != stackEnv), the VPA never right-sizes the pods because it targets a deployment that doesn't exist.

DeploySimpleContainer already applies the parentEnv suffix to Service/Deployment (e.g. myappmyapp-tenant-a). NewSimpleContainer then re-derived baseResourceName via generateDeploymentName on the already-suffixed name → myapp-tenant-a-tenant-a. Since baseResourceName names the VPA (resource name and spec.targetRef.name) and the HPA, the VPA's targetRef pointed at a non-existent myapp-tenant-a-tenant-a Deployment. The Deployment itself is correct (it uses sanitizedDeployment), so the mismatch was silent — the VPA simply applied no recommendation and pods stayed pinned at the base request.

Non-custom stacks are unaffected (no suffix is applied upstream, so the names already matched).

Fix

-	baseResourceName := generateDeploymentName(sanitizedService, args.ScEnv, parentEnv)
+	baseResourceName := sanitizedDeployment

The only caller (DeploySimpleContainer) already made the name parentEnv-aware, so re-deriving it was the bug.

Blast radius

On the next deploy of a custom stack, Pulumi replaces the VPA and HPA (old double-suffixed logical names removed; correctly-named ones created). The HPA's scaleTargetRef already used the real Deployment object, so scaling is unaffected by the rename. This fixes previously-orphaned VPAs on all custom stacks.

Scoped intentionally: the configmap/secret/image-pull-secret names stay double-suffixed for custom stacks — they're self-consistent (the pod references the same generated names), so they keep working. Renaming them is a separate migration with a pod-rollout cost and isn't needed for this fix.

Test

Test-first regression (TestNewSimpleContainer_CustomStackBaseNameNotDoubleSuffixed): replicates the production caller contract (pre-suffixed Service/Deployment for a custom stack) and asserts baseResourceName is single-suffixed via the HPA (which shares it with the VPA). Confirmed it fails on the bug and passes with the fix; full pkg/clouds/pulumi/kubernetes suite green.

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

Semgrep Scan Results

Repository: api | Commit: a54b80b

Check Status Details
⚠️ Semgrep Warning 1 warning(s), 1 total

Scanned at 2026-06-21 14:06 UTC

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown

Security Scan Results

Repository: api | Commit: a54b80b

Check Status Details
✅ Secret Scan Pass No secrets detected
✅ Dependencies (Trivy) Pass 0 total (no critical/high)
✅ Dependencies (Grype) Pass 0 total (no critical/high)
📦 SBOM Generated 522 components (CycloneDX)

Scanned at 2026-06-21 14:06 UTC

DeploySimpleContainer already applies the parentEnv suffix to Service/Deployment
for custom stacks (parentEnv != stackEnv), e.g. myapp -> myapp-tenant-a. But
NewSimpleContainer re-derived baseResourceName via generateDeploymentName on the
already-suffixed name, producing myapp-tenant-a-tenant-a. baseResourceName names
the VPA (resource name AND spec.targetRef) and the HPA, so for every custom stack
the VPA targeted a non-existent deployment and never right-sized the pods (they
stayed pinned at the base request). The Deployment itself was always correct (it
uses sanitizedDeployment directly), which is why the mismatch went unnoticed.

Fix: baseResourceName := sanitizedDeployment — the caller already made it
parentEnv-aware. No-op for non-custom stacks; fixes the VPA targetRef and tidies
the HPA name for custom stacks. Documented the caller pre-suffix contract on
NewSimpleContainer so it can't regress.

On the next deploy of a custom stack, Pulumi replaces the VPA and HPA (old
double-suffixed logical names removed, correctly-named ones created); the HPA's
scaleTargetRef already pointed at the real Deployment object, so scaling is
unaffected. VPA/HPA carry no RetainOnDelete/aliases, so the orphans are cleanly
deleted (not stranded).

The configmap/secret/image-pull-secret names remain double-suffixed for custom
stacks (self-consistent: the pod references the same generated names), so they
keep working; renaming them is a separate migration with pod-rollout cost.

Test: TestNewSimpleContainer_CustomStackVPATargetsDeployment replicates the
production caller contract (pre-suffixed Service/Deployment for a custom stack)
and asserts the rendered VPA's spec.targetRef.name + name are single-suffixed.
Adds the VPA's GVK type token to the test mock so the CustomResource is captured.
Confirmed it fails on the bug and passes with the fix; full package green.

Signed-off-by: Dmitrii Creed <creeed22@gmail.com>
@Cre-eD

Cre-eD commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

Hardened after a 4-way review (2 subagents + codex + gemini):

  • Strengthened the test to capture the VPA directly (the VPA registers under kubernetes:autoscaling.k8s.io/v1:VerticalPodAutoscaler, not the generic CustomResource token — added that case to the test mock) and assert its spec.targetRef.name is single-suffixed, instead of inferring it via the HPA.
  • Documented the caller pre-suffix contract on NewSimpleContainer so this can't regress.
  • Reviewers confirmed: orphan double-suffixed VPA/HPA are cleanly deleted on redeploy (no RetainOnDelete/aliases on VPA/HPA — only the Namespace has them), the left-double-suffixed secrets/configmaps are self-consistent (safe to leave), and there's no P0. Rollout note: this churns VPA/HPA for every custom stack on its next deploy, so it'll be staged (one stack first, verify the plan is exactly delete-old/create-new).

@Cre-eD
Cre-eD force-pushed the fix/custom-stack-vpa-targetref branch from 1b41804 to 8e52f56 Compare June 20, 2026 11:10
@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown

📊 Statement coverage

Measured on the documented included set (see docs/TESTING.md → Coverage scope). Observe-only — no regression gate is enforced yet.

Scope This PR main baseline Δ
Included set (Gold-tier denominator) 90.3% 90.3% +0.0 pp
Full set (whole repo, transparency) 27.6% 27.7% -0.1 pp

Baseline: main @ 6d0d0cb

@Cre-eD
Cre-eD merged commit a3b1896 into main Jun 21, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants