refactor(networkpolicy): consolidate egress destination parsing

[tianfeng92]

Description

ParseExternalDestination and ExternalDestinationEntityRule already turn an endpoint into the tightest egress rule available. Only the OTel Collector used them. Dex and Guardian carried their own copies; both now go through the shared pair.

Dex's parseHostPortFromURL was a duplicate of pkg/url.ParseHostPortFromHTTPProxyURL, which Guardian already used, so it is deleted rather than reimplemented.

Preserved: Dex's 0.0.0.0/0 and ::/0 any-destination rules, and its unconditional domain rule — it has no license input.

Changed: a <svc>.<ns>.svc destination now renders a Services match instead of a Domains rule, so it follows the Service's own ports. A Domains rule never worked for a ClusterIP. Only the domain branch needs egress-access-control; a Services match does not.

Testing

• lint clean; render, enterprise, clusterconnection and authentication suites pass
• new test covers an unlicensed in-cluster destination
• draft until verified on a cluster: Dex reaching its IdP, Guardian's tunnel, with and without a proxy

Release Note

None

EV-6963

[Copilot]

Pull request overview

This PR refactors egress-destination parsing and rule construction to use shared helpers, removing duplicate logic across components while preserving existing licensing and behavior constraints for Dex and Guardian.

Changes:

• Dex: replaces local host/port extraction and destination parsing with pkg/url.ParseHostPortFromHTTPProxyURL and networkpolicy.ParseExternalDestination + ExternalDestinationEntityRule.
• Guardian: replaces inline tunnel-destination rule building with the shared parsing/rule helpers while preserving the “skip port-only allow when unlicensed” behavior.
• Tests: adds coverage ensuring an in-cluster <svc>.<ns>.svc tunnel destination renders as a Service match even without the license feature.

Reviewed changes

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

File	Description
pkg/render/dex.go	Removes duplicated URL/egress destination parsing and uses shared helpers for consistent rule rendering.
pkg/enterprise/clusterconnection/guardian.go	Consolidates tunnel destination rule generation via shared parsing/entity-rule helpers while preserving licensing behavior.
pkg/enterprise/clusterconnection/guardian_render_test.go	Adds a regression test for Service-match rendering of in-cluster tunnel destinations without the license feature.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (1)

pkg/render/common/networkpolicy/networkpolicy.go:353

• ParseHostPort can successfully parse destinations like ":443" (net.SplitHostPort allows an empty host) and return an empty host. That can later produce an invalid EntityRule (e.g., Domains: [""]). Reject empty-host destinations explicitly so misconfigurations fail loudly.

func ParseHostPort(destination string) (string, numorstring.Port, error) {
	host, portStr, err := net.SplitHostPort(destination)
	if err != nil {
		return "", numorstring.Port{}, err
	}
	port, err := numorstring.PortFromString(portStr)
	if err != nil {
		return "", numorstring.Port{}, err
	}
	return host, port, nil

[Copilot]

Pull request overview

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

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

pkg/render/otelcollector/component.go:853

• exporterDestination claims to default the port from the URL scheme, but when the endpoint has no explicit port it always defaults to 443. If a stale CRD ever allows non-https endpoints, this will silently pick the wrong port (and the comment is inaccurate). Consider defaulting based on u.Scheme and erroring on unknown schemes so the caller can safely skip the rule.

	portStr := u.Port()
	if portStr == "" {
		// https is the only scheme the API accepts.
		portStr = "443"
	}

pkg/render/otelcollector/component.go:859

• EntityRuleForHostPort always returns a Domains match for hostnames. The OpenTelemetry collector rendering no longer carries a license/feature flag (EgressAccessControl) to decide whether Domains rules are allowed, so an otel-collector-only license could cause the operator to render NetworkPolicy objects that are rejected/unsupported (previously this was handled via DomainEgressAllowed). Consider reintroducing a domain-egress feature gate (or dropping/errored rules when dest.Domains is set but the feature is inactive).

		return v3.EntityRule{}, err
	}
	return networkpolicy.EntityRuleForHostPort(host, clusterDomain, port), nil
}

pkg/render/common/networkpolicy/networkpolicy_test.go:112

• ParseHostPort has unit tests for URL and bare-host inputs, but it doesn't currently cover the empty-host case (e.g. ":9449"), which net.SplitHostPort accepts. Adding a test will prevent regressions around accidentally producing empty-domain rules.

		It("rejects a bare host", func() {
			_, _, err := networkpolicy.ParseHostPort("mgmt.example.com")
			Expect(err).To(HaveOccurred())
		})

pkg/render/common/networkpolicy/networkpolicy.go:349

• ParseHostPort currently accepts destinations like ":9449" (empty host) because net.SplitHostPort allows it. That can flow into EntityRuleForHostPort and produce a Domains rule containing an empty string, which is likely invalid and can cause confusing failures later. Reject empty hosts explicitly here.

	host, portStr, err := net.SplitHostPort(destination)
	if err != nil {
		return "", numorstring.Port{}, err
	}
	port, err := numorstring.PortFromString(portStr)

[Copilot]

Pull request overview

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

Suppressed comments (2)

pkg/render/common/networkpolicy/networkpolicy.go:353

• ParseHostPort accepts ":" (empty host) because net.SplitHostPort returns host=="" without error. That flows into EntityRuleForHostPort and produces Domains: [""] (or other invalid rules), which can later cause render/apply failures or unintended behavior. Reject empty hosts explicitly.

func ParseHostPort(destination string) (string, numorstring.Port, error) {
	host, portStr, err := net.SplitHostPort(destination)
	if err != nil {
		return "", numorstring.Port{}, err
	}
	port, err := numorstring.PortFromString(portStr)
	if err != nil {
		return "", numorstring.Port{}, err
	}
	return host, port, nil

pkg/render/otelcollector/component.go:875

• This comment claims Validate rejects endpoints that exporterDestination cannot parse, but OpenTelemetrySpec.Validate currently only enforces an https:// prefix (api/v1/logcollector_types.go:344-349) and does not validate URL parsing or port correctness. That means malformed endpoints (e.g., bad port) will hit this path and be silently skipped, making exports fail without a clear configuration error. At minimum, update the comment to reflect reality; ideally tighten Validate (or fail rendering) so users get an actionable error instead of a silent skip.

		dest, err := exporterDestination(exp, c.cfg.ClusterDomain)
		if err != nil {
			// Validate rejects an endpoint this cannot parse, so reaching here means
			// a stale CRD let one through. Skip the rule rather than widening it:
			// the default-deny then blocks that exporter, which is the safe failure.
			continue
		}

[Copilot]

Pull request overview

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

Suppressed comments (1)

pkg/render/otelcollector/component.go:868

• When exporterDestination fails to parse an exporter endpoint, the error is silently ignored (continue) and no egress rule is rendered. That can leave exports blocked by default-deny with no signal in operator logs about why the destination was skipped.

		dest, err := exporterDestination(exp, c.cfg.ClusterDomain)
		if err != nil {
			continue
		}

[pasanw]

Looks like we dropped the domain egress check, could we add it back into this function similar to how it is done in guardian (if len(dest.Domains) > 0 && !gpc.IncludeEgressNetworkPolicy...)?

[tianfeng92]

https://github.com/tigera/operator/pull/5223/changes#diff-4b08b3b19dde288685e682b4b7ab31da5775d3906bf8ead59f9f3a10812275a9R857 put it back here.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/render/manager.go:1374

• The LDAP/AD egress comment no longer matches the implementation: EntityRuleForHostPort can now render an in-cluster Service match (dropping ports) when Authentication.spec.ldap.host is ..svc[.]. Please update the comment so it accurately describes the possible destination rule types (service, exact net, or domain).

		// being configured on the Authentication CR. The destination is scoped to
		// Authentication.spec.ldap.host — a domain match for a hostname or a
		// /32/128 for a literal IP. Both standard ports stay open (the host may
		// specify a non-standard port; scoping the port too would risk denying a
		// valid config), so only the host is narrowed.

pkg/render/dex.go:614

• The PR description says Dex's parseHostPortFromURL was deleted in favor of pkg/url.ParseHostPortFromHTTPProxyURL, but this PR still keeps a local parseHostPortFromURL implementation. Please reconcile the PR description with the actual approach (either update the description, or switch to the shared helper if that’s the intended end state).

// parseHostPortFromURL is kept local to Dex rather than using the shared
// HTTP-proxy helper: Dex may be configured with a socks proxy, which the shared
// helper rejects outright but which works here when the URL carries a port.
func parseHostPortFromURL(url *url.URL) (string, error) {

[Copilot]

Pull request overview

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

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/render/dex.go:618

• parseHostPortFromURL returns host:port strings for proxy URLs. The default-port branch (the switch below) uses net.JoinHostPort(url.Host, ...) which can double-bracket IPv6 hosts because url.Host may already include brackets (e.g. "[2001:db8::1]"). Consider using url.Hostname() when joining so IPv6 proxy URLs without an explicit port are rendered correctly.

func parseHostPortFromURL(url *url.URL) (string, error) {
	if url.Port() != "" {
		return url.Host, nil
	}

pkg/render/dex.go:613

• PR description says Dex's parseHostPortFromURL was deleted in favor of a shared helper, but this change keeps a Dex-local parseHostPortFromURL (and even adds a comment explaining why). Consider updating the PR description so it matches the implementation and review/testing expectations.

// parseHostPortFromURL is kept local to Dex rather than using the shared
// HTTP-proxy helper: Dex may be configured with a socks proxy, which the shared
// helper rejects outright but which works here when the URL carries a port.

[Copilot]

Pull request overview

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

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

pkg/render/dex.go:617

• parseHostPortFromURL returns url.Host when a port is present, but for the no-port case it later uses net.JoinHostPort(url.Host, ...). For IPv6 literals, url.Host includes brackets, so JoinHostPort will produce an invalid "[[...]]:port". Use url.Hostname() (bracket-free) when constructing host:port strings so IPv6 proxy URLs are handled correctly.

func parseHostPortFromURL(url *url.URL) (string, error) {
	if url.Port() != "" {
		return url.Host, nil
	}

pkg/render/dex.go:613

• The PR description says Dex's parseHostPortFromURL was deleted in favor of a shared helper, but this PR keeps a local parseHostPortFromURL implementation. Please update the PR description to match the implementation, or switch Dex to the shared helper if that was the intent.

// parseHostPortFromURL is kept local to Dex rather than using the shared
// HTTP-proxy helper: Dex may be configured with a socks proxy, which the shared
// helper rejects outright but which works here when the URL carries a port.