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
		}