refactor(networkpolicy): consolidate egress destination parsing - #5223
refactor(networkpolicy): consolidate egress destination parsing#5223tianfeng92 wants to merge 2 commits into
Conversation
Dex, Guardian and the OTel Collector each turned an endpoint into an egress rule. ParseExternalDestination and ExternalDestinationEntityRule already do that, and only the collector used them. Dex loses parseHostPortFromURL, a duplicate of pkg/url's ParseHostPortFromHTTPProxyURL that Guardian already used, and builds its specific-destination rules through the shared pair. Guardian does the same for the tunnel destination. Its copy had moved to pkg/enterprise/clusterconnection since the original review. Two behaviours are kept rather than folded into the helper's defaults. Guardian skips a named destination when the EgressAccessControl feature is absent instead of falling back to port-only: the trailing Pass rule already governs the tunnel, and a port-only allow would be wider than what ships today. Dex renders the domain rule unconditionally because it has no license input, and narrowing it would change IdP egress. Dex's 0.0.0.0/0 and ::/0 any-destination rules are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review catch. The consolidation gated the whole rule on EgressAccessControl whenever the destination was not a literal IP, but ExternalDestinationEntityRule has three branches and only the domain one needs the feature. An in-cluster management address renders a service match, which does not, so an unlicensed cluster lost the rule for its own tunnel. Ask for the rule with the feature we actually have and skip it only when that left nothing to match on. Covered by a test that fails against the previous gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tianfeng92
changed the title
There was a problem hiding this comment.
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.ParseHostPortFromHTTPProxyURLandnetworkpolicy.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>.svctunnel 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.
tianfeng92
added
docs-not-required
release-note-not-required
and removed
release-note-required
docs-pr-required
labels
There was a problem hiding this comment.
A large portion of my feedback is about the ParseExternalDestination implementation, which I should have given to you in the previous PR - apologies that I missed that.
I'll give that feedback here:
clusterServicelogic doesn't look quite right - if it's passed something like proxy.corp.svc.example.com it will treat it as a kubernetes service. I think this function should be strict and only parse service name and namespace if the host string ends with.svc,.svc.<cluster-domain>, or.svc.<cluster-domain>.. This scheme doesn't support if a customer specifies their service without a .svc based suffix, but I couldn't think of a clean way to support that - we can brainstorm on that if you'd likeExternalDestinationEntityRuleresults in a fail-open style of policy, where if we aren't able to restrict the host in the egress rule (with a Domains or Nets field), we return an egress rule that allows all hosts. I don't think a general policy helper should be making a decision to open broad access by default - that kind of decision should be made by the component. (And I don't think components should be choosing broad access in such a scenario, including otel collector)- Guardian, Dex, and other components all utilized the flow of
SplitHostPort>PortFromString>ParseIP. Looks like we replacedPortFromStringwithAtoi- could we revert that?Atoidoesn't support all of the different scenarios thatPortFromStringdoes and in general I think it's better to not change longstanding logic - I think it would be nicer for components to provide their destination string to one function and get back an
EntityRule. This way it's only one function call and no new concept (ExternalDestination) needs to be introduced. I'd recommend structuring it as such:func X(destination string) v3.EntityRule, which itself callsfunc Y(destination string) (host string, port string)and thenfunc Z(host string, port string) v3.EntityRuleto do its work- OTel, Guardian, Dex would use
Xdirectly to get their entity rule - Manager and ServiceEndpoint would use
Zto get their entity rule
There was a problem hiding this comment.
General comment on this PR: looks like we are missing the opportunity for manager.go (managerComponent) and k8s-endpoint.go (ServiceEndpoint) to utilize our shared logic. They could use the function Z, as mentioned above
| // the Pass rule below already governs the tunnel, and a port-only allow | ||
| // here would be wider than what ships today. | ||
| dest := networkpolicy.ExternalDestinationEntityRule(parsed, gpc.IncludeEgressNetworkPolicy) | ||
| if dest.Services == nil && len(dest.Nets) == 0 && len(dest.Domains) == 0 { |
There was a problem hiding this comment.
Seems like this is a large guard trying to detect if the fail-open I mentioned in the larger comment occurred? If so, I would hope that we could remove it as a result of removing the fail-open behaviour
| host, port, err := net.SplitHostPort(tunnelDestinationHostPort) | ||
| if err != nil { | ||
| return v3.NetworkPolicySpec{}, err | ||
| parsed, ok := networkpolicy.ParseExternalDestination(tunnelDestinationHostPort) |
There was a problem hiding this comment.
Claude: This replaced net.SplitHostPort, which rejected anything that was not host:port. ParseExternalDestination's url.Parse fallback now silently accepts a URL-shaped gpc.URL — e.g. https://mgmt.example.com parses as host mgmt.example.com port 443 (the scheme default), not the tunnel port. So a misconfig that used to fail the render loudly now ships a rule for the wrong port. Note the // gpc.URL has host:port form comment just above is no longer enforced. Since — unlike OTel's endpoint — this field is not meant to be a URL, Guardian should keep rejecting non-host:port here (a strict entry point, or validate before parsing).
| } | ||
|
|
||
| httpProxyDestination, err := parseHostPortFromURL(httpProxyURL) | ||
| httpProxyDestination, err := operatorurl.ParseHostPortFromHTTPProxyURL(httpProxyURL) |
There was a problem hiding this comment.
Claude: Robustness note — pre-existing, not introduced here, low priority. Swapping to ParseHostPortFromHTTPProxyURL (here and at line 579) widens the set of proxy values that error out: it rejects any non-http/https scheme up front, whereas the old parseHostPortFromURL accepted anything that carried a port. The problem is what an error here costs: resolveEgressDestinationsForPod returns the error before the 0.0.0.0/0/::/0 catch-alls are appended, and the caller (resolveEgressRulesByDestination) then continues the whole pod — so a single unparseable proxy value silently drops that pod's entire egress, IdP catch-alls included (log line only, status stays healthy). The ordering and the whole-pod skip both predate this PR, and the newly-rejected schemes (e.g. socks5://, scheme-relative //host:port) aren't a supported proxy type anyway (Guardian's tunnel dialer is HTTP-CONNECT-only), so this is hardening, not a regression. If we want to harden it: append the catch-alls unconditionally, and handle a proxy-parse failure locally (skip just that one destination) so a bad proxy value can't wipe the broad allows.
Description
ParseExternalDestinationandExternalDestinationEntityRulealready 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
parseHostPortFromURLwas a duplicate ofpkg/url.ParseHostPortFromHTTPProxyURL, which Guardian already used, so it is deleted rather than reimplemented.Preserved: Dex's
0.0.0.0/0and::/0any-destination rules, and its unconditional domain rule — it has no license input.Changed: a
<svc>.<ns>.svcdestination 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 needsegress-access-control; a Services match does not.Testing
Release Note
EV-6963