🐛 (helm/v2-alpha): scope manager port templating to the manager container

[v47]

Description

The Helm v2-alpha plugin templated the manager Deployment's ports, health probes, and port-related arguments (--metrics-bind-address, --webhook-port, --health-probe-bind-address) by running regex replacements across the entire rendered Deployment. This change scopes all of that Deployment port, probe and argument templating to the manager container only.

Detection reuses the existing FindManagerContainerRange lookup — the same manager-container detection the values extractor relies on — through a new applyToManagerContainer helper, so the manager is correctly identified whether it is declared before or after other containers. Services and NetworkPolicies have no containers and continue to be matched by name, split out into a dedicated templateServicePorts for clarity.

While here, the port-templating regexes are hoisted to package-level variables so they are compiled once instead of on every call (matching the existing pattern in escape.go/helpers.go). A separate fix in AddCustomLabelsAndAnnotations is folded in: hand-ordered Deployment metadata (labels: before annotations:) used to emit a duplicate annotations: header before spec:; the state machine now flushes the pending labels merge on the annotations: line so both blocks stay under metadata:.

Test coverage added:

• unit tests covering both container orderings (sidecar before and after the manager);
• pipeline-level isolation tests through ApplyHelmSubstitutions asserting the manager container is fully templated while a sidecar's ports, probes, image, and arguments are left untouched.
• a pinning test for the hand-ordered labels:-then-annotations: metadata shape, asserting a single merged block instead of a duplicate header.

Motivation

Because the replacements matched across the whole Deployment, a sidecar that happened to reuse the same ports, probe paths, or bind-address flags was rewritten to use manager chart values. For example, a proxy sidecar with a health probe on port 9440 had it rewritten to {{ .Values.manager.healthProbe.port }}, so changing manager.healthProbe.port would unintentionally move the sidecar's port as well. The same class of bug affected the manager args: rewrite: when a sidecar declared its own args: before the manager, the whole-document search landed on the sidecar first and silently skipped the manager's args wrappers. Webhook detection had the same problem because it also inspected the fully rendered document. Only values belonging to the manager container should use manager chart values; sidecars must remain unchanged.

Partial Fixes #5901

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: v47
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubernetes-prow]

Hi @v47. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Tip

We noticed you've done this a few times! Consider joining the org to skip this step and gain /lgtm and other bot rights. We recommend asking approvers on your previous PRs to sponsor you.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[camilamacedo86]

Hi @v47

This one do not address fully #5901 right? It is partial.
Could you please change the PR desc for we do not close #5901 with this one? Just add partial close

[v47]

Hey @camilamacedo86 — heads up, this PR ended up a bit larger than I initially planned. I considered splitting it into a few smaller step-by-step PRs since each scoping fix and the labels/annotations fix are logically independent, but they all live in the same manager templating flow, and all fall under #5901, so keeping them together felt cleaner for review and history than a chain of three PRs stacked on the same file. Happy to split it if you would prefer separate PRs - just let me know.

One thing I intentionally left out for a follow-up: MakeContainerArgsConditional and the four Make*Volumes*Conditional functions still scan the whole Deployment for --webhook-cert-path, --metrics-cert-path, webhook-certs, metrics-certs, and the /tmp/k8s-webhook-server/serving-certs mount path. Folding them through applyToManagerContainer (same pattern this PR already uses for TemplatePorts and templateControllerManagerArgs) would complete the "manager scoping" story issue #5901 sketches at the design level.

I kept it out of this PR for two reasons:

• it would push the diff well past 500 lines
• the token collision is much more theoretical than the port/probe one - a real-world sidecar would essentially have to impersonate the kubebuilder manager's cert-manager wiring to trigger it, unlike the plain containerPort: 9440 proxy sidecar the issue describes.

Happy to open the follow-up after we're done with this one.

[Copilot]

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

[v47]

/retest

[kubernetes-prow]

@v47: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[v47]

@camilamacedo86 sorry, can you take a look?

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[camilamacedo86]

Just a nit otherwise, all shows fine.
@v47 , could you please address the doc comments on the tests?

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

pkg/plugins/optional/helm/v2alpha/scaffolds/internal/kustomize/templater/appliers/helpers.go:155

• The helper/comment says "manager container", but FindManagerContainerRange (per its implementation) uses GetDefaultContainerName(yamlContent) and then searches by "name: ". This mismatch makes the behavior easy to misunderstand and can lead to incorrect scoping if the default container annotation differs from the actual manager container. Consider either (a) updating naming/docs to "default container" everywhere (function name + comments), or (b) changing the range finder to explicitly locate the manager container by name ("manager") if that is the intended invariant for this templating pass.

// applyToManagerContainer runs transform on only the manager container's block within
// yamlContent, leaving sidecar containers and the rest of the Deployment untouched.
// When the manager container cannot be located, transform is applied to the whole
// document, preserving behavior for manifests without a detectable manager container.
func applyToManagerContainer(yamlContent string, transform func(string) string) string {
	start, end := FindManagerContainerRange(yamlContent)

[camilamacedo86]

@v47 could you check this one ??