Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cleanup usages of resolver.Target.Endpoint #5796

Closed
easwars opened this issue Nov 15, 2022 · 5 comments · Fixed by #5852
Closed

Cleanup usages of resolver.Target.Endpoint #5796

easwars opened this issue Nov 15, 2022 · 5 comments · Fixed by #5852
Assignees
@kylejb
Labels
fixit P2 Type: Bug

Comments

@easwars
Copy link
Contributor

easwars commented Nov 15, 2022 •
edited
Loading

resolver.Target.Endpoint is marked as deprecated, and we added resolver.Target.URL which is the more preferred way of getting to the details of the parsed target.

We should add a helper function to retrieve the endpoint from a given resolver.Target. Something like:

func (t resolver.Target) ToEndpoint() string {
	if target.URL.Path == "" {
		return target.URL.Opaque
	}
	return target.URL.Path
}

Codebase needs to be inspected for usages of resolver.Target.Endpoint and replaced with the above helper function.
@easwars easwars added the fixit label Nov 28, 2022
@Smityz Smityz mentioned this issue Dec 7, 2022
Closed
@kylejb
Copy link
Contributor

kylejb commented Dec 8, 2022

@easwars - any preference regarding how you'd like to modify tests to decouple reliance on deprecated resolver.Target.Endpoint (example in dns_resolver_test.go)?

Although I'm getting errors from dns_resolver, I feel like I've correctly cleaned up the usage locally. Since I'm new to Go, I'd like to double check that I'm going down the right path here.

dns_resolver_test: logs

    dns_resolver_test.go:740: dns resolver: missing address
--- FAIL: TestDNSResolverExponentialBackoff (0.00s)
    --- FAIL: TestDNSResolverExponentialBackoff/happy_case_default_port (0.00s)
        dns_resolver_test.go:812: Error building resolver for target foo.bar.com: dns resolver: missing address
    --- FAIL: TestDNSResolverExponentialBackoff/happy_case_specified_port (0.00s)
        dns_resolver_test.go:812: Error building resolver for target foo.bar.com:1234: dns resolver: missing address
    --- FAIL: TestDNSResolverExponentialBackoff/happy_case_another_default_port (0.00s)
        dns_resolver_test.go:812: Error building resolver for target srv.ipv4.single.fake: dns resolver: missing address
--- FAIL: TestResolveFunc (0.00s)
    dns_resolver_test.go:1195: Build("www.google.com", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("foo.bar:12345", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("127.0.0.1", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("::", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("127.0.0.1:12345", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[::1]:80", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[2001:db8:a0b:12f0::1]:21", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build(":80", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("127.0.0...1:12345", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[fe80::1%lo0]:80", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("golang.org:http", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[2001:db8::1]:http", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[2001:db8::1]:", cc, _) = dns resolver: missing address, want dns resolver: missing port after port-separator colon
    dns_resolver_test.go:1195: Build(":", cc, _) = dns resolver: missing address, want dns resolver: missing port after port-separator colon
    dns_resolver_test.go:1195: Build("[2001:db8:a0b:12f0::1", cc, _) = dns resolver: missing address, want invalid target address [2001:db8:a0b:12f0::1, error info: address [2001:db8:a0b:12f0::1:443: missing ']' in address
--- FAIL: TestDisableServiceConfig (0.00s)
    dns_resolver_test.go:1231: dns resolver: missing address
--- FAIL: TestTXTError (0.00s)
    dns_resolver_test.go:1269: dns resolver: missing address
--- FAIL: TestDNSResolverRetry (0.00s)
    dns_resolver_test.go:1305: dns resolver: missing address
--- FAIL: TestCustomAuthority (0.00s)
    dns_resolver_test.go:1462: unexpected error using custom authority 4.3.2.1:53: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority 4.3.2.1:123: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority 4.3.2.1: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority ::1: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority [::1]: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority [::1]:123: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority dnsserver.com: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority :123: dns resolver: missing address
--- FAIL: TestRateLimitedResolve (0.00s)
    dns_resolver_test.go:1506: resolver.Build() returned error: dns resolver: missing address
--- FAIL: TestReportError (0.00s)
    dns_resolver_test.go:1615: Error building resolver for target notfoundaddress: dns resolver: missing address
--- FAIL: TestResolve (0.00s)
    dns_resolver_test.go:740: dns resolver: missing address
--- FAIL: TestDNSResolverExponentialBackoff (0.00s)
    --- FAIL: TestDNSResolverExponentialBackoff/happy_case_default_port (0.00s)
        dns_resolver_test.go:812: Error building resolver for target foo.bar.com: dns resolver: missing address
    --- FAIL: TestDNSResolverExponentialBackoff/happy_case_specified_port (0.00s)
        dns_resolver_test.go:812: Error building resolver for target foo.bar.com:1234: dns resolver: missing address
    --- FAIL: TestDNSResolverExponentialBackoff/happy_case_another_default_port (0.00s)
        dns_resolver_test.go:812: Error building resolver for target srv.ipv4.single.fake: dns resolver: missing address
--- FAIL: TestResolveFunc (0.00s)
    dns_resolver_test.go:1195: Build("www.google.com", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("foo.bar:12345", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("127.0.0.1", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("::", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("127.0.0.1:12345", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[::1]:80", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[2001:db8:a0b:12f0::1]:21", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build(":80", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("127.0.0...1:12345", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[fe80::1%lo0]:80", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("golang.org:http", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[2001:db8::1]:http", cc, _) = dns resolver: missing address, want <nil>
    dns_resolver_test.go:1195: Build("[2001:db8::1]:", cc, _) = dns resolver: missing address, want dns resolver: missing port after port-separator colon
    dns_resolver_test.go:1195: Build(":", cc, _) = dns resolver: missing address, want dns resolver: missing port after port-separator colon
    dns_resolver_test.go:1195: Build("[2001:db8:a0b:12f0::1", cc, _) = dns resolver: missing address, want invalid target address [2001:db8:a0b:12f0::1, error info: address [2001:db8:a0b:12f0::1:443: missing ']' in address
--- FAIL: TestDisableServiceConfig (0.00s)
    dns_resolver_test.go:1231: dns resolver: missing address
--- FAIL: TestTXTError (0.00s)
    dns_resolver_test.go:1269: dns resolver: missing address
--- FAIL: TestDNSResolverRetry (0.00s)
    dns_resolver_test.go:1305: dns resolver: missing address
--- FAIL: TestCustomAuthority (0.00s)
    dns_resolver_test.go:1462: unexpected error using custom authority 4.3.2.1:53: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority 4.3.2.1:123: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority 4.3.2.1: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority ::1: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority [::1]: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority [::1]:123: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority dnsserver.com: dns resolver: missing address
    dns_resolver_test.go:1462: unexpected error using custom authority :123: dns resolver: missing address
--- FAIL: TestRateLimitedResolve (0.00s)
    dns_resolver_test.go:1506: resolver.Build() returned error: dns resolver: missing address
--- FAIL: TestReportError (0.00s)
    dns_resolver_test.go:1615: Error building resolver for target notfoundaddress: dns resolver: missing address```

@easwars
Copy link
Contributor Author

easwars commented Dec 8, 2022

In tests, we could do something like this:

grpc-go/xds/internal/resolver/xds_resolver_test.go

Line 65 in 638141f

var target = resolver.Target{Endpoint: targetStr, URL: url.URL{Scheme: "xds", Path: "/" + targetStr}}

We can even consider writing a helper function in internal/testutils:

package testutils

func MustParseURL(u string) *url.URL {
  // Call url.Parse on the argument and panic if it returns an error
}

and have tests do something like this:
target := resolver.Target{URL: testutils.MustParselURL("scheme:///targetString")}

If you do a change like this for all tests, you will end up touching a whole bunch of files. If you prefer, you can even consider splitting the change into a handful of PRs instead of one huge one. Thanks.

@kylejb kylejb mentioned this issue Dec 9, 2022
Merged
4 tasks
@kylejb
Copy link
Contributor

kylejb commented Dec 15, 2022

If you do a change like this for all tests, you will end up touching a whole bunch of files. If you prefer, you can even consider splitting the change into a handful of PRs instead of one huge one. Thanks.

Will do.

Out of curiosity, how do you manage PRs that share the same dependency? Since this repo relies on rebasing, would cherry picking the commit that introduces the MustParseURL function be the way to go for every branch?

@easwars
Copy link
Contributor Author

easwars commented Dec 15, 2022

Out of curiosity, how do you manage PRs that share the same dependency? Since this repo relies on rebasing, would cherry picking the commit that introduces the MustParseURL function be the way to go for every branch?

I haven't found a good way to work on set of PRs where each one depends on a previous one. I simply end up serializing them, and in parallel work on something unrelated :)

@kylejb
Copy link
Contributor

kylejb commented Jan 21, 2023

If you do a change like this for all tests, you will end up touching a whole bunch of files. If you prefer, you can even consider splitting the change into a handful of PRs instead of one huge one. Thanks.

FYI - since we'll be removing this deprecated field, I made these changes as part of this issue/PR.

easwars pushed a commit that referenced this issue Jan 24, 2023
@kylejb
resolver: replace resolver.Target.Endpoint field with Endpoint() meth…
3930549 
…od (#5852)

Fixes #5796
@wyattanderson wyattanderson mentioned this issue Feb 28, 2023
Merged
@alanprot alanprot mentioned this issue Mar 17, 2023
Merged
bboreham added a commit to weaveworks/common that referenced this issue Apr 11, 2023
@bboreham
Merge pull request #281 from alanprot/update-grpc
f7d83a0 
Upgrades to a newer version (v1.53.0) of google.golang.org/grpc that includes breaking changes to the resolver.Target type (see Cleanup usages of resolver.Target.Endpoint grpc/grpc-go#5796)
Upgrade from v2.4.0 to v.4.0.0 of https://github.com/sercand/kuberesolver which is compatible to grpc to v1.53.0.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@kylejb kylejb

Labels
fixit P2 Type: Bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

resolver: replace resolver.Target.Endpoint field with Endpoint() method kylejb-forks/grpc-go
2 participants
@easwars @kylejb