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

[melinath]

This is a workaround for golang/go#25321, and is related to hashicorp/terraform-provider-google#6782

For additional context, see yaqs/5034757202074664960 and yaqs/47302089738551296.

[melinath]

The conventionalcommits linter conflicts with https://github.com/googleapis/google-api-go-client/blob/main/CONTRIBUTING.md - which should I follow?

[melinath]

I'll assume conventionalcommits for now.

[enocom]

Related to hashicorp/terraform-provider-google#6782

[noahdietz]

I'd like @codyoss to weigh in here too

[noahdietz]

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?

[codyoss]

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.

[noahdietz]

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.

[codyoss]

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.

[noahdietz]

The conventionalcommits linter conflicts with https://github.com/googleapis/google-api-go-client/blob/main/CONTRIBUTING.md - which should I follow?

I'll fix this, thanks for the shout out.

[codyoss]

I think because of how this is constructed in our new auth library it will not be an issue -- we allow passing a base roundtripper: https://pkg.go.dev/cloud.google.com/go/auth/httptransport#Options

I am less inclined to accept a change here that does this in our base transport. As a workaround you could likely still use NewClient and override http.DefaultTransport as we use that as our base if it is an http.Transport: https://github.com/googleapis/google-api-go-client/blob/4b79cc4e7d85f3a05e5ca99738cdf340b46da243/transport/http/dial.go#L265C11-L265C26

[melinath]

Just to clarify, are you suggesting that I should override the global http.DefaultTransport variable? That seems like it could have unintended side effects.

This bug impacts all users of this repository, so it would be great to get it fixed here rather than having to do a hacky workaround in every downstream project. (Better yet would be if Go core could fix the issue but they don't seem inclined to do so and I couldn't figure out how to make the fix there myself.)

That said, if you don't want to fix the bug, would you at least accept a contribution to allow passing in a new default transport rather than requiring an override of a global variable?

[melinath]

For some discussion of how modifying the global state of DefaultTransport can be problematic, see https://github.com/hashicorp/go-cleanhttp?tab=readme-ov-file#cleanhttp

[melinath]

@noahdietz @codyoss any updates on whether you will consider this contribution? It would be great to fix this issue here instead of downstream users of the libraries!

[codyoss]

@melinath a couple of thoughts.

1. I don't think I currently understand the problem well enough to know if doing this is enough to fix the issue. For instance this is not the same http client that is used in the auth layer. I assume you could have the same issues there talking to token endpoints and/or metadata service.
2. As discussed in the review, I don't think the dependency you are pulled in is recommended for use in non-test code as it exposes the testing package in its public signatures.
3. I am also curious how this was never reported for GKE environments. To my knowledge ipv6 support was only added in the last couple of years. So I wonder was we did not get reports of this in the past.

I will ask around to see if any other of our client library transport code is trying to do any special handling here. I am not against what this code is trying to do in general, but it is not clear to me that this is how we should do such a test and if it really solves all of the issues.

[codyoss]

cc @dneil do you have any thoughts here? If we were going to add something like this is there an easier way to probe for ipv6?