grpcutil: add ValidateTargetURI function

[HueCodes]

Fixes #8747

Add an internal API to validate gRPC target URI strings. The function
checks that the target parses as a valid RFC 3986 URI and that a
resolver is registered for its scheme.

RELEASE NOTES: n/a

[codecov]

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.25%. Comparing base (2abe1f0) to head (9c8aa04).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
balancer/rls/config.go	50.00%	1 Missing ⚠️
clientconn.go	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8876      +/-   ##
==========================================
+ Coverage   83.17%   83.25%   +0.08%     
==========================================
  Files         414      415       +1     
  Lines       32751    32762      +11     
==========================================
+ Hits        27241    27277      +36     
+ Misses       4091     4078      -13     
+ Partials     1419     1407      -12     

Files with missing lines	Coverage Δ
internal/grpcutil/target.go	100.00% <100.00%> (ø)
balancer/rls/config.go	85.18% <50.00%> (-0.43%)	⬇️
clientconn.go	90.27% <85.71%> (-0.53%)	⬇️

... and 22 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[arjan-bal]

Hi @HueCodes, thanks for picking up this issue.

To ensure we have a reusable API, we should update existing call sites to use the new implementation. Specifically:

1. ClientConn:
   

   grpc-go/clientconn.go

   Lines 1802 to 1827 in b7b1cce

   	parsedTarget, err := parseTarget(cc.target)
   	if err == nil {
   	rb = cc.getResolver(parsedTarget.URL.Scheme)
   	if rb != nil {
   	cc.parsedTarget = parsedTarget
   	cc.resolverBuilder = rb
   	return nil
   	}
   	}
   	// We are here because the user's dial target did not contain a scheme or
   	// specified an unregistered scheme. We should fallback to the default
   	// scheme, except when a custom dialer is specified in which case, we should
   	// always use passthrough scheme. For either case, we need to respect any overridden
   	// global defaults set by the user.
   	defScheme := cc.dopts.defaultScheme
   	if internal.UserSetDefaultScheme {
   	defScheme = resolver.GetDefaultScheme()
   	}
   	canonicalTarget := defScheme + ":///" + cc.target
   	parsedTarget, err = parseTarget(canonicalTarget)
   	if err != nil {
   	return err
   	}

2. Route Lookup Service:
   

   grpc-go/balancer/rls/config.go

   Lines 198 to 205 in b7b1cce

   	parsedTarget, err := url.Parse(lookupService)
   	if err != nil {
   	// url.Parse() fails if scheme is missing. Retry with default scheme.
   	parsedTarget, err = url.Parse(resolver.GetDefaultScheme() + ":///" + lookupService)
   	if err != nil {
   	return nil, fmt.Errorf("rls: invalid target URI in lookup_service %s", lookupService)
   	}
   	}

Both of these locations share common behavior:

1. If parsing fails, they attempt to append the default scheme and re-parse.
2. They return a url.URL object.

To handle this, the new function would need to accept a default scheme param and return a url.Url as the result. I think we should design the API to handle these two operations internally. This would allow us to refactor the existing code to call the new API as well. @easwars, what do you think?

[HueCodes]

Thanks for the response! I will work on fixing this today when I am off work.

[easwars]

Agree with the comments by @arjan-bal.

Another place where we would have to use the new API is here:

grpc-go/internal/xds/bootstrap/bootstrap.go

Line 399 in b7b1cce

if sc.serverURI == "" {

Currently, that code only verifies that the server_uri is not empty. But we need to ensure that it is a valid URI. I think this will cause a whole bunch of unit tests to fail because we simply pass a non-empty string for this value in a lot of tests. So, this could also be done as a follow-up if it ends up touching too many test files. Thanks.

[HueCodes]

Thanks for taking the time to review this. I appreciate it. I addressed the issues:

• Added ParseTarget function that accepts a default scheme parameter and returns *url.URL
• Updated clientconn.go and balancer/rls/config.go to use the new function
• Left xds/bootstrap for a follow-up as suggested, since it may touch many test files

Let me know if there are any other changes you would like me to make.