From 050c2ba7d456e481fe00e02edcd08c7dba16ae8e Mon Sep 17 00:00:00 2001 From: riccardom Date: Tue, 8 Sep 2026 16:37:08 +0200 Subject: [PATCH] [client] Reuse util's service-URL comparison instead of a second copy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The endpoint-comparison rules this branch introduced now live in util (PR #7472 moved them there so the MDM conflict check could stop comparing URLs as strings). Keeping a copy here is what produced that bug in the first place: two implementations of "is this the same endpoint?" drift, and the one that drifts starts refusing a URL that addresses the very server it already points at. So SameServiceURL delegates the port normalization to util.ServiceURLPort and drops the local one, and SameServiceURLIncludingPath — endpoint plus path, for the admin panel URL, which is opened rather than dialed — is util.SameServiceURL plus the query, fragment and userinfo it adds on top, so the local path normalization goes too. What stays here is the distinction util does not make: SameServiceURL is endpoint-only, because a management URL is dialed and only its host and port are, while util.SameServiceURL includes the path. Pure refactor. Verified as one: all 198 pairs of a 14-spelling matrix (default and zero-padded ports, host case, trailing slash, path, query, fragment, userinfo, both schemes, nil operands) answer identically for both functions before and after. --- client/internal/profilemanager/config.go | 36 ++++--------------- .../config_would_change_test.go | 5 +-- 2 files changed, 9 insertions(+), 32 deletions(-) diff --git a/client/internal/profilemanager/config.go b/client/internal/profilemanager/config.go index 61e606cad..3ae9ea852 100644 --- a/client/internal/profilemanager/config.go +++ b/client/internal/profilemanager/config.go @@ -13,7 +13,6 @@ import ( "reflect" "runtime" "slices" - "strconv" "strings" "time" @@ -1001,6 +1000,10 @@ func ParseServiceURL(serviceName, serviceURL string) (*url.URL, error) { // and reading them as three values makes a client that restates its own // management URL look like a client asking to be repointed. A nil operand // matches only another nil one. +// +// The path plays no part: a management URL is dialed, and only its host and +// port are. util.SameServiceURL is this comparison plus the path, which is +// what SameServiceURLIncludingPath needs and delegates to. func SameServiceURL(a, b *url.URL) bool { if a == nil || b == nil { return a == b @@ -1008,29 +1011,7 @@ func SameServiceURL(a, b *url.URL) bool { return a.Scheme == b.Scheme && strings.EqualFold(a.Hostname(), b.Hostname()) && - ServiceURLPort(a) == ServiceURLPort(b) -} - -// ServiceURLPort returns the port a service URL addresses, resolving an absent -// one to the default of its scheme. The port is normalized numerically, so a -// zero-padded ":0443" is the same port as ":443". -func ServiceURLPort(u *url.URL) string { - port := u.Port() - if port == "" { - switch u.Scheme { - case "https": - return "443" - case "http": - return "80" - default: - return "" - } - } - - if n, err := strconv.Atoi(port); err == nil { - return strconv.Itoa(n) - } - return port + util.ServiceURLPort(a) == util.ServiceURLPort(b) } // SameServiceURLIncludingPath is SameServiceURL plus everything a URL carries @@ -1046,17 +1027,12 @@ func SameServiceURLIncludingPath(a, b *url.URL) bool { return a == b } - return SameServiceURL(a, b) && - normalizedURLPath(a) == normalizedURLPath(b) && + return util.SameServiceURL(a, b) && a.RawQuery == b.RawQuery && a.Fragment == b.Fragment && a.User.String() == b.User.String() } -func normalizedURLPath(u *url.URL) string { - return strings.TrimSuffix(u.Path, "/") -} - func parseURL(serviceName, serviceURL string) (*url.URL, error) { parsedMgmtURL, err := url.ParseRequestURI(serviceURL) if err != nil { diff --git a/client/internal/profilemanager/config_would_change_test.go b/client/internal/profilemanager/config_would_change_test.go index cfbc2fbad..2387cf1b5 100644 --- a/client/internal/profilemanager/config_would_change_test.go +++ b/client/internal/profilemanager/config_would_change_test.go @@ -481,13 +481,14 @@ func TestWouldChangeNormalizesBeforeMeasuring(t *testing.T) { require.True(t, changed, "a non-default value is still a change") } -// A zero-padded port addresses the same port. +// A zero-padded port addresses the same port. The normalization itself belongs +// to util.ServiceURLPort and is tested there; this asserts that the comparison +// this package hands its callers inherits it. func TestServiceURLPortIsNormalizedNumerically(t *testing.T) { padded, err := ParseServiceURL("padded", "https://mgmt.example.com:0443") require.NoError(t, err) plain, err := ParseServiceURL("plain", "https://mgmt.example.com:443") require.NoError(t, err) - require.Equal(t, "443", ServiceURLPort(padded)) require.True(t, SameServiceURL(padded, plain)) }