fix HTTPS frontend name mismatch breaking backend pathID matching

[abh]

When ssl-passthrough is enabled, the HTTPS frontend is rendered as _front_https__local in haproxy.tmpl, but createPathsMaps() used Frontend.Name (_front_https without the suffix). This caused the var(req.fe) string match to fail, silently skipping all path-conditional rules (rewrite-target, HSTS, ssl-redirect, cors, etc.).

Add Frontend.RenderedName() that includes the __local suffix when appropriate, and use it in both createPathsMaps() and the template.

Fixes #1432.

[abh]

I couldn't get the integration tests working on my laptop, so 🤞 that the automated CI ones will work (but I'm going to sleep and might not be able to check for a few days).

[Copilot]

Pull request overview

Fixes a mismatch between the rendered HAProxy HTTPS frontend name when SSL passthrough is enabled (e.g. _front_https__local) and the name used to build backend path maps / frontend ACL matching, which previously caused var(req.fe) checks to miss and path-conditional rules to be skipped.

Changes:

• Introduce Frontend.RenderedName() to centralize the “__local when SSL passthrough” naming logic.
• Use RenderedName() in both haproxy.tmpl and backend createPathsMaps() so map filenames + req.fe matching align with the actual frontend name.
• Add a regression test covering mixed HTTP/HTTPS paths on the same backend plus an SSL-passthrough host forcing the __local frontend name.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
rootfs/etc/templates/haproxy/haproxy.tmpl	Switch HTTPS frontend naming to RenderedName() so the config frontend name matches the model.
pkg/haproxy/types/frontend.go	Add RenderedName() helper for consistent frontend naming when SSL passthrough is active.
pkg/haproxy/types/backend.go	Use RenderedName() when grouping/creating per-frontend backend path maps.
pkg/haproxy/instance_test.go	Add regression test asserting correct req.fe matching with _front_https__local.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[jcmoraisjr]

Awesome, thanks for the report and PR!

I think that the method could be a field instead, populated via config.go/SyncConfig(), since it is being used late in the process. This should also make Copilot happy since there is no performance penalty.

However, my guess is that the __local suffix is being used only for backward compatibility now, mainly on the metrics field. I cannot see a conflict happening by removing it, and I'd rather include a new line in the backward compatibility changes instead of adding indirection in the frontend name.

@copilot do you see any caveat or hidden misbehavior if we fix #1432 by simply patching the haproxy.tmpl file, removing __local suffix like in the diff below:

-{{- $httpsFrontendName := printf "%s%s" $frontend.Name (iif $hasSSLPassthrough "__local" "") }}
-frontend {{ $httpsFrontendName }}
+frontend {{ $frontend.Name }}

[abh]

Awesome, thanks for the report and PR!

Thank you for all the work over the last many years!

I think that the method could be a field instead, populated via config.go/SyncConfig(), since it is being used late in the process. This should also make Copilot happy since there is no performance penalty.

That makes sense.

However, my guess is that the __local suffix is being used only for backward compatibility now, mainly on the metrics field. I cannot see a conflict happening by removing it, and I'd rather include a new line in the backward compatibility changes instead of adding indirection in the frontend name.

If I remember right from my limited understanding yesterday, in the scenario where a frontend is used for passthrough then we need two "frontends", the one listening to incoming connections and another for dispatching the actual HTTP requests that aren't passthrough (or vice versa?).

__local might not be the best name though!

[jcmoraisjr]

Thank you for all the work over the last many years!

<3

If I remember right from my limited understanding yesterday, in the scenario where a frontend is used for passthrough then we need two "frontends", the one listening to incoming connections and another for dispatching the actual HTTP requests that aren't passthrough (or vice versa?).

The one for the SNI inspection is already named differently:

Define:

haproxy-ingress/pkg/haproxy/config.go

Lines 103 to 112 in 1ab99c2

	if f.HasSSLPassthrough() {
	// using ssl-passthrough config, so need a `mode tcp`
	// frontend with `inspect-delay` and `req.ssl_sni`
	if f.Name == "_front_https" {
	f.TLSProxyName = "_front__tls" // backward compatible name
	} else {
	f.TLSProxyName = fmt.Sprintf("_front__tls_%d", f.Port())
	}
	f.HTTPSSocket = fmt.Sprintf("unix@%s/var/run/haproxy/%s_socket.sock", c.global.LocalFSPrefix, f.Name)
	f.HTTPSProxy = true

Use:

haproxy-ingress/rootfs/etc/templates/haproxy/haproxy.tmpl

Lines 1174 to 1180 in 1ab99c2

	# # # # # # # # # # # # # # # # # # #
	# #
	# TCP/TLS frontend
	#
	listen {{ $frontend.TLSProxyName }}
	mode tcp
	bind {{ $frontend.Bind }}{{ if $frontend.AcceptProxy }} accept-proxy{{ end }}

IIRC the _local variation was used to distinguish a single _https frontend binding the TCP port and ssl-offloading from the _tcp and _https_local pair used when SNI inspection is needed, mostly (if not exclusively) for metrics.

I prefer not having another indirection, but also don't want to break things in the metrics side. We are not in a hurry, this patch can wait the next week, so I'll invest some time revisiting the metrics dashboard and will let you know if we are good removing the _local suffix. TBC.

[jcmoraisjr]

I revisited metrics and in fact it depends on the frontends named the way they are now, otherwise some counters should overlap each other, providing non accurate values. So any changes on this land should have a metrics counterpart, and probably adding more backward incompatibility.

That said, lets go to the render name approach which fixes the issue for now. However, I think we can use instead a field in the frontend type, updated via SyncConfig(), since all the calls happen after that point. This way we have a better performance in the hot path and make copilot happy.

[abh]

Thanks for digging into the metrics side. Went with the field approach as you suggested — 1b377a2 replaces the method with a RenderedName string field on Frontend, populated in syncFrontend(). Default is f.Name, overridden to f.Name + "__local" when HTTPS + ssl-passthrough. Call sites in createPathsMaps() and the template read the field directly, so the hot path is O(1). All unit tests pass; the integration tests still fail locally the same way (missing etcd binary on darwin/arm64).

[Copilot]

Pull request overview

Fixes a mismatch between the rendered HTTPS frontend name in haproxy.cfg (notably _front_https__local when ssl-passthrough is active) and the frontend name used when building backend path maps, which previously caused txn.pathID-dependent features (rewrite-target, HSTS, ssl-redirect, CORS, etc.) to be skipped.

Changes:

• Introduces Frontend.RenderedName and populates it during config.SyncConfig() (adding __local for HTTPS frontends when ssl-passthrough is enabled).
• Uses RenderedName for HTTPS frontend naming in haproxy.tmpl and when grouping backend path maps (createPathsMaps()), aligning var(req.fe) matching with the actual frontend name.
• Adds an instance-level regression test asserting correct var(req.fe) matching for _front_https__local.

Reviewed changes

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

Show a summary per file

File	Description
rootfs/etc/templates/haproxy/haproxy.tmpl	Renders HTTPS frontend using Frontend.RenderedName to match actual HAProxy frontend name under ssl-passthrough.
pkg/haproxy/types/types.go	Adds RenderedName field to Frontend.
pkg/haproxy/types/frontend.go	Initializes RenderedName when acquiring a frontend.
pkg/haproxy/types/backend.go	Uses Frontend.RenderedName when building backend PathsMaps() groups/keys.
pkg/haproxy/config.go	Sets Frontend.RenderedName during SyncConfig() based on ssl-passthrough state.
pkg/haproxy/instance_test.go	Adds a regression test covering _front_https__local frontend ACL/pathID matching.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Backend.PathsMaps() caches the derived Frontends list (now based on Frontend.RenderedName). Because RenderedName is mutated during config.SyncConfig() (e.g., when any host enables/disables ssl-passthrough) but b.pathsMaps is never invalidated, a subsequent update can keep stale frontend names and keep generating var(req.fe) -m str _front_https / old map filenames for backends that were not rebuilt in a partial sync. This can reintroduce the pathID mismatch for unchanged backends when ssl-passthrough is toggled elsewhere.

Suggested fix: make PathsMaps()/PathConfigs() caches self-invalidating when any referenced path.Host.Frontend.RenderedName changes (e.g., track a hash/version), or explicitly clear these caches on backends whenever RenderedName changes during sync (and ensure map generation considers those backends dirty as needed). Adding a regression test that performs two consecutive updates (first without ssl-passthrough, then enabling it on a different host) would catch this.

[jcmoraisjr]

I understand Copilot's worry here, but this is not something that should impact the expected behavior. There are two premises used in haproxy model that makes our life easier: fields are never changed once populated during a single reconciliation, and caches are invalidated and rebuilt on every reconciliation. That said, it's safe the way it is, further safeguards would only make code more complex.

[jcmoraisjr]

Looks good, thanks!