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

transport/http: use ipv4 explicitly for dialers if ipv6 is not supported #2549

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

transport/http: use ipv4 explicitly for dialers if ipv6 is not supported #2549

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 17 additions & 1 deletion transport/http/dial.go
View file
Open in desktop
Expand Up @@ -22,6 +22,7 @@ import (
"go.opencensus.io/plugin/ochttp"
"go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp"
"golang.org/x/net/http2"
"golang.org/x/net/nettest"
"golang.org/x/oauth2"
"google.golang.org/api/googleapi/transport"
"google.golang.org/api/internal"
Expand Down Expand Up @@ -266,6 +267,14 @@ func defaultBaseTransport(ctx context.Context, clientCertSource cert.Source, dia
if trans == nil {
trans = fallbackBaseTransport()
}
trans.DialContext = func(ctx context.Context, network string, addr string) (net.Conn, error) {
// Don't try IPv6 if it's not supported.
// https://github.com/golang/go/issues/25321
if !nettest.SupportsIPv6() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit hesitant to use a package meant for testing in a production context. Is this what the Go team recommends? Is there any other package we can use that is meant for critical path usage?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this package would be fine to use as it is meant to test the network and not necessary unit tests. This does feel like code that should happen in terraform though maybe and not here. I think the reason for putting it here is that our transport function is not handling some of the mTLS bits that our NewHTTPClient function does. Is that correct @melinath? If so maybe we can move that logic ad NewHTTPClient calls into NewTransport.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and not necessary unit tests.

Some of the APIs in that package take testing.T which gave me pause.

meant to test the network

The package description is Package nettest provides utilities for network testing. which is sufficiently vague..."testing aspects of the network setup" vs. "network testing" vs. "for us in tests relating to the network" are all reasonable interpretations of this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the APIs in that package take testing.T which gave me pause.

I had not noticed that at first glance. That does give me more pause.

return baseDialContext(ctx, "tcp4", addr)
}
return baseDialContext(ctx, network, addr)
}
trans.MaxIdleConnsPerHost = 100

if clientCertSource != nil {
Expand All @@ -275,7 +284,14 @@ func defaultBaseTransport(ctx context.Context, clientCertSource cert.Source, dia
}
if dialTLSContext != nil {
// If DialTLSContext is set, TLSClientConfig wil be ignored
trans.DialTLSContext = dialTLSContext
trans.DialTLSContext = func(ctx context.Context, network string, addr string) (net.Conn, error) {
// Don't try IPv6 if it's not supported.
// https://github.com/golang/go/issues/25321
if !nettest.SupportsIPv6() {
return dialTLSContext(ctx, "tcp4", addr)
}
return dialTLSContext(ctx, network, addr)
}
}

configureHTTP2(trans)
Expand Down