resolver: replace resolver.Target.Endpoint field with Endpoint() method #5852

kylejb · 2022-12-09T03:25:06Z

Fixes #5796

Summary of Changes

Remove deprecated field: resolver.Target.Endpoint. (BREAKING CHANGE)
Create receiver function for resolver.Target called Endpoint() that returns either URL.Path or URL.Opaque (latter is used as endpoint, if former is empty).
Replace usage of resolver.Target.Endpoint to resolver.Target.Endpoint().
Fix tests like "internal/dns_resolver_test" by setting values for resolver.Target.URL with testutils.MustParseURL to support removal of resolver.Target.Endpoint.

RELEASE NOTES:

resolver: deprecated field resolver.Target.Endpoint has been removed and replaced by resolver.Target.Endpoint().

linux-foundation-easycla · 2022-12-09T03:25:09Z

The committers listed above are authorized under a signed CLA.

✅ login: kylejb / name: Kyle J. Burda (00aad9b)

easwars

Thank you very much for your changes.

resolver/resolver.go

internal/resolver/dns/dns_resolver_test.go

internal/testutils/parse_url.go

internal/resolver/dns/dns_resolver_test.go

internal/testutils/parse_url.go

easwars

Changes look good to me except the one nit.

internal/testutils/parse_url.go

Approach has changed now.

kylejb · 2023-01-21T20:06:09Z

xds/internal/balancer/clusterresolver/resource_resolver_test.go

@@ -614,7 +615,7 @@ func (s) TestResourceResolverEDSAndDNS(t *testing.T) {
 	}
 	select {
 	case target := <-dnsTargetCh:
-		if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}); diff != "" {
+		if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL(testDNSTarget)}); diff != "" {


@easwars, do we need to manually prepend scheme here (e.g., "dns:///" + testDNSTarget)? Any guidance as to when it would be necessary to do so and why it must be done manually?

FYI - there are a handful of lines in this file and another where I should potentially prepend scheme to target. Although not doing so does not seem to cause any errors in testing, I wanted to flag this inconsistency.

I think url.Parse() doesn't like a URL without a scheme, because there is no default scheme as we have in grpc. So, wherever we need to pass a value directly to url.Parse(), the value would need to have a scheme.

Ah, thanks! I went ahead and added scheme to remain consistent.

Let me know if you'd like me to revert that. Interestingly, tests do not appear to be affected by that decision.

easwars · 2023-01-23T19:06:13Z

resolver/resolver.go

 	// URL contains the parsed dial target with an optional default scheme added
 	// to it if the original dial target contained no scheme or contained an
 	// unregistered scheme. Any query params specified in the original dial
 	// target can be accessed from here.
 	URL url.URL
 }

+// Endpoint retrieves endpoint without leading "/" from either `URL.Path`
+// or `URL.Opaque`. The latter is set when the former is empty.


Nit: s/The latter is set when the former is empty/The latter is used when the former is empty/

easwars · 2023-01-23T19:07:57Z

xds/internal/balancer/clusterresolver/resource_resolver_test.go

 	"google.golang.org/grpc/resolver"
 	"google.golang.org/grpc/resolver/manual"
-	"google.golang.org/grpc/xds/internal/testutils"
+	testutilsXDS "google.golang.org/grpc/xds/internal/testutils"


Could you please rename the import as xdstestutils instead. We have done that in other places, and doing the same here wold stay consistent with existing usages. Thanks.

Done!

Thanks for the explanation. Makes sense.

easwars · 2023-01-23T19:42:51Z

xds/internal/balancer/clusterresolver/resource_resolver_test.go

@@ -614,7 +615,7 @@ func (s) TestResourceResolverEDSAndDNS(t *testing.T) {
 	}
 	select {
 	case target := <-dnsTargetCh:
-		if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", Endpoint: testDNSTarget}); diff != "" {
+		if diff := cmp.Diff(target, resolver.Target{Scheme: "dns", URL: *testutils.MustParseURL(testDNSTarget)}); diff != "" {


I think url.Parse() doesn't like a URL without a scheme, because there is no default scheme as we have in grpc. So, wherever we need to pass a value directly to url.Parse(), the value would need to have a scheme.

* feat(resolver): replace with Endpoint() * chore(resolver): update documentation for Endpoint() * fix(dns_resolver_test): add escape for percent char

This is based on the upstream breaking-change in v1.53 grpc/grpc-go#5852

Upgrade grpc to v1.53.0, as preparation for introducing OpenTelemetry, which depends on that grpc version. Two changes to our own code were necessitated by upstream changes: 1. Add a stub implementation of GetOrBuildProducer: this was added to the balancer.SubConn interface by grpc v1.51.0 2. Change use of Endpoint field to Endpoint() method: the field was removed and replaced by a method in grpc/grpc-go#5852. This also means that our tests can't set the .Endpoint field, so the tests are updated to use the .URL field instead, and a helper has been added to make that easy. Part of #6361

kylejb force-pushed the refactor/5796-resolver-target-endpoint branch from 00aad9b to 345111e Compare December 9, 2022 03:45

Smityz mentioned this pull request Dec 9, 2022

example: change name resolve example to URL style #5816

Closed

kylejb force-pushed the refactor/5796-resolver-target-endpoint branch 2 times, most recently from 6f04842 to c87606b Compare December 9, 2022 04:47

easwars self-assigned this Dec 13, 2022

easwars added the Type: Internal Cleanup Refactors, etc label Dec 13, 2022

easwars added this to the 1.52 Release milestone Dec 13, 2022

easwars requested changes Dec 15, 2022

View reviewed changes

resolver/resolver.go Outdated Show resolved Hide resolved

internal/resolver/dns/dns_resolver_test.go Outdated Show resolved Hide resolved

internal/testutils/parse_url.go Outdated Show resolved Hide resolved

easwars added the Status: Requires Reporter Clarification label Dec 15, 2022

easwars assigned kylejb and unassigned easwars Dec 15, 2022

kylejb force-pushed the refactor/5796-resolver-target-endpoint branch from c87606b to 43c8130 Compare December 15, 2022 04:20

kylejb requested a review from easwars December 15, 2022 04:48

kylejb removed their assignment Dec 15, 2022

easwars removed the Status: Requires Reporter Clarification label Dec 15, 2022

easwars self-assigned this Dec 15, 2022

easwars reviewed Dec 15, 2022

View reviewed changes

internal/resolver/dns/dns_resolver_test.go Outdated Show resolved Hide resolved

easwars assigned kylejb and unassigned easwars Dec 15, 2022

easwars added the Status: Requires Reporter Clarification label Dec 15, 2022

This was referenced Dec 15, 2022

cleanup: usage of deprecated resolver.Target fields #5858

Closed

balancer: cleanup of deprecated ClientConn.Target() #5855

Closed

kylejb requested a review from easwars December 15, 2022 22:35

easwars reviewed Dec 15, 2022

View reviewed changes

internal/testutils/parse_url.go Outdated Show resolved Hide resolved

kylejb requested a review from easwars December 15, 2022 23:39

easwars previously approved these changes Dec 15, 2022

View reviewed changes

internal/testutils/parse_url.go Outdated Show resolved Hide resolved

kylejb force-pushed the refactor/5796-resolver-target-endpoint branch from 676cfc8 to 978441f Compare December 16, 2022 00:14

easwars removed the Status: Requires Reporter Clarification label Dec 16, 2022

easwars assigned dfawley Dec 16, 2022

easwars self-requested a review January 19, 2023 23:03

easwars removed their assignment Jan 19, 2023

easwars removed their request for review January 19, 2023 23:04

kylejb force-pushed the refactor/5796-resolver-target-endpoint branch 3 times, most recently from 5234364 to 685552f Compare January 21, 2023 19:58

kylejb commented Jan 21, 2023

View reviewed changes

easwars removed the Status: Requires Reporter Clarification label Jan 23, 2023

easwars reviewed Jan 23, 2023

View reviewed changes

easwars added the Status: Requires Reporter Clarification label Jan 23, 2023

cleanup: remove resolver.Target.Endpoint

779f9c5

* feat(resolver): replace with Endpoint() * chore(resolver): update documentation for Endpoint() * fix(dns_resolver_test): add escape for percent char

kylejb force-pushed the refactor/5796-resolver-target-endpoint branch from 685552f to 779f9c5 Compare January 23, 2023 22:26

kylejb requested a review from easwars January 23, 2023 22:29

dfawley approved these changes Jan 23, 2023

View reviewed changes

dfawley changed the title ~~cleanup: usages of resolver.Target.Endpoint~~ resolver: replace resolver.Target.Endpoint field with Endpoint() method Jan 23, 2023

dfawley added Type: API Change Breaking API changes (experimental APIs only!) and removed Type: Internal Cleanup Refactors, etc labels Jan 23, 2023

arvindbr8 assigned easwars and unassigned kylejb Jan 24, 2023

arvindbr8 removed the Status: Requires Reporter Clarification label Jan 24, 2023

easwars approved these changes Jan 24, 2023

View reviewed changes

easwars merged commit 3930549 into grpc:master Jan 24, 2023
1 check passed

kylejb deleted the refactor/5796-resolver-target-endpoint branch January 24, 2023 20:24

kylejb mentioned this pull request Jan 24, 2023

tests: fix spelling of variable #5966

Merged

1 task

aaronbee mentioned this pull request Feb 10, 2023

google.golang.org/grpc/xds has dependency on "testing" #6015

Closed

mcpherrinm added a commit to mcpherrinm/boulder that referenced this pull request Mar 9, 2023

Change use of Endpoint field to Endpoint() method

29a1c20

This is based on the upstream breaking-change in v1.53 grpc/grpc-go#5852

mcpherrinm mentioned this pull request Mar 9, 2023

Upgrade GRPC to v1.53.0 letsencrypt/boulder#6734

Merged

github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

resolver: replace resolver.Target.Endpoint field with Endpoint() method #5852

resolver: replace resolver.Target.Endpoint field with Endpoint() method #5852

kylejb commented Dec 9, 2022 •

edited

linux-foundation-easycla bot commented Dec 9, 2022 •

edited

easwars left a comment

easwars left a comment

kylejb Jan 21, 2023

easwars Jan 23, 2023

kylejb Jan 23, 2023

easwars Jan 23, 2023

kylejb Jan 23, 2023

easwars Jan 23, 2023

kylejb Jan 23, 2023

easwars Jan 23, 2023

resolver: replace resolver.Target.Endpoint field with Endpoint() method #5852

resolver: replace resolver.Target.Endpoint field with Endpoint() method #5852

Conversation

kylejb commented Dec 9, 2022 • edited

linux-foundation-easycla bot commented Dec 9, 2022 • edited

easwars left a comment

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylejb commented Dec 9, 2022 •

edited

linux-foundation-easycla bot commented Dec 9, 2022 •

edited