Made base transport layer force ipv4 usage if ipv6 is not supported #10485

melinath · 2024-04-18T23:16:40Z

Fixes hashicorp/terraform-provider-google#6782

Note that while I was able to reproduce the Go issue, I was not able to reproduce the TF issue - there seems to be some amount of randomness in terms of when it presents. However, this seems to be the best option for working around the issue.

This PR also removes a lot of complexity that was previously implicitly part of our transport setup via transport.NewHTTPClient and that was mostly unused. (I traced through the code to boil it down to the ~~10 or so~~ 6 lines we were actually using.)

yaqs/47302089738551296

Release Note Template for Downstream PRs (will be copied)

provider: Forced HTTP requests to use ipv4 on systems with ipv6 disabled, such as Cloud Shell.

Fixes hashicorp/terraform-provider-google#6782 Workaround for golang/go#25321

modular-magician · 2024-04-18T23:28:49Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

google provider: Diff ( 1 file changed, 47 insertions(+), 5 deletions(-))
google-beta provider: Diff ( 1 file changed, 47 insertions(+), 5 deletions(-))

modular-magician · 2024-04-19T00:35:51Z

Tests analytics

Total tests: 3606
Passed tests: 3242
Skipped tests: 364
Affected tests: 0

Click here to see the affected service packages

all service packages are affected

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log

melinath · 2024-04-19T15:44:28Z

Some real test runs, in case VCR bypasses this change: https://hashicorp.teamcity.com/buildConfiguration/TerraformProviders_GoogleCloud_GOOGLE_MMUPSTREAMTESTS_GOOGLE_PACKAGE_PUBSUB/135875?hideTestsFromDependencies=false&hideProblemsFromDependencies=false&expandBuildChangesSection=true

rileykarson · 2024-04-19T18:15:13Z

Adding @ScottSuarez as a mandatory reviewer- we should ensure that MTLS is not broken as a result of this change.

c2thorn · 2024-04-19T20:06:32Z

mmv1/third_party/terraform/transport/config.go.erb

+	trans := http.DefaultTransport.(*http.Transport).Clone()
+	// Increase MaxIdleConnsPerHost due to reported performance issues under load in the
+	// GCS client.
+	trans.MaxIdleConnsPerHost = 100


If I'm understanding correctly, these tweaks are from here?

Great, you did mention it in the comment, but I'll leave the link here just for posterity

c2thorn · 2024-04-19T20:10:36Z

mmv1/third_party/terraform/transport/config.go.erb

+// default transport and makes some tweaks to match best practices
+// from google-api-go-client, as well as ensuring that IPv6 does
+// not get used in environments that don't support it.
+func baseTransport() (http.RoundTripper, error) {


I may be missing something here, but why can't we reuse the google-api-go-client's NewClient method and override the given transport's dial context after?

http.Client's Transport is a RoundTripper rather than a Transport, so we would need to cast it back to a Transport in order to do that. I don't see a reason that wouldn't work, and that would've been a lot easier to implement.

I think, having dug through the code, I would slightly prefer this way because we know exactly what we're getting and don't have to worry about the intricacies of NewClient.

I think that's reasonable now that we've gone through the code. My only concerns are about missing something, but I've gone through it myself now and everything seems in order.

c2thorn · 2024-04-19T20:12:44Z

Do we want a provider-wide changelog note?

melinath · 2024-04-19T20:21:07Z

Do we want a provider-wide changelog note?

added

c2thorn

LGTM for non-MTLS stuff

c2thorn · 2024-04-19T21:23:13Z

Do we want a provider-wide changelog note?

added

We are expecting some user-facing impact right? Specifically user's facing hashicorp/terraform-provider-google#6782 will now have their ipv4 settings honored?

melinath · 2024-04-19T21:38:48Z

Do we want a provider-wide changelog note?

added

We are expecting some user-facing impact right? Specifically user's facing hashicorp/terraform-provider-google#6782 will now have their ipv4 settings honored?

🤦 yep. yep. we sure are. fixed.

ScottSuarez

As is, this pr will likely break mtls.

Mtls endpoint switch is detected here:
https://github.com/modular-magician/terraform-provider-google-beta/blob/5c35eba9eb80027ba7ca98ebc3dddbf3229500af/google-beta/provider/mtls_util.go#L19

https://github.com/googleapis/google-api-go-client/blob/81ccc7e5305f4913388c254b39144c9a144abb20/internal/cba.go#L289
https://github.com/google/s2a-go/blob/fcfd1dacc64be11cc3532ab6a7e1cdd1a3e4b69e/s2a.go#L361

The long term solution was the switch to the DCL (at the time).

They have a testing playbook for mtls here:
https://docs.google.com/document/d/1MeAoY-XQ2O1DmP_PZDYWS1PFx9vkZ4qWwnhSV0_kfPU/edit?tab=t.0#heading=h.r7dggrs9pw6r

ScottSuarez · 2024-04-22T17:50:53Z

mmv1/third_party/terraform/transport/config.go.erb

+// from google-api-go-client, as well as ensuring that IPv6 does
+// not get used in environments that don't support it.
+func baseTransport() (http.RoundTripper, error) {
+	trans := http.DefaultTransport.(*http.Transport).Clone()


We switched to using the goapis transport since it has the capability of auto discovering the client certificate apart of for mtls handshake.

https://docs.google.com/document/d/10tFeBhvTgtKl9XwMXbuKOPzq9mGdV-RSyLuXOQtRZos/edit?tab=t.0
https://docs.google.com/document/d/1Ns3gnY94yV07-mMFaISY3CZi5DJwCKb_6A2OGseRT60/edit?tab=t.0
https://docs.google.com/document/d/14Y2PyzXJS1yvvPY9yYK54jsA-V8o6Vdvbom9GBeP2FY/edit?tab=t.0

The clientCertSource contains the auto loaded mtls credentials.
https://github.com/googleapis/google-api-go-client/blob/707b9b287dc37fb07c2553c0391f83995f1420a6/transport/http/dial.go#L37

rileykarson

Marking request changes so that we can't click the big green button accidentally

melinath · 2024-04-26T16:52:46Z

Unfortunately the "just override the transport's dialcontext" approach is not feasible because the base transport is wrapped a few times and two of them use private fields to store the wrapped RoundTripper.

melinath · 2024-04-26T17:01:14Z

yaqs/5034757202074664960

melinath · 2024-04-26T20:16:27Z

I'm going to attempt to get this change made in google-api-go-client

melinath · 2024-04-26T20:41:44Z

Opened googleapis/google-api-go-client#2549

Made base transport layer force ipv4 usage if ipv6 is not supported

82d8a8f

Fixes hashicorp/terraform-provider-google#6782 Workaround for golang/go#25321

melinath requested a review from c2thorn April 19, 2024 15:39

rileykarson requested a review from ScottSuarez April 19, 2024 18:14

c2thorn reviewed Apr 19, 2024

View reviewed changes

c2thorn approved these changes Apr 19, 2024

View reviewed changes

ScottSuarez reviewed Apr 22, 2024

View reviewed changes

rileykarson requested changes Apr 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Made base transport layer force ipv4 usage if ipv6 is not supported #10485

Made base transport layer force ipv4 usage if ipv6 is not supported #10485

melinath commented Apr 18, 2024 •

edited

modular-magician commented Apr 18, 2024

modular-magician commented Apr 19, 2024

melinath commented Apr 19, 2024

rileykarson commented Apr 19, 2024

c2thorn Apr 19, 2024

melinath Apr 19, 2024

c2thorn Apr 19, 2024

c2thorn Apr 19, 2024 •

edited

melinath Apr 19, 2024

c2thorn Apr 19, 2024

c2thorn commented Apr 19, 2024

melinath commented Apr 19, 2024

c2thorn left a comment

c2thorn commented Apr 19, 2024

melinath commented Apr 19, 2024 •

edited

ScottSuarez left a comment

ScottSuarez Apr 22, 2024

rileykarson left a comment •

edited

melinath commented Apr 26, 2024

melinath commented Apr 26, 2024

melinath commented Apr 26, 2024

melinath commented Apr 26, 2024

Made base transport layer force ipv4 usage if ipv6 is not supported #10485

Are you sure you want to change the base?

Made base transport layer force ipv4 usage if ipv6 is not supported #10485

Conversation

melinath commented Apr 18, 2024 • edited

modular-magician commented Apr 18, 2024

Diff report

modular-magician commented Apr 19, 2024

Tests analytics

melinath commented Apr 19, 2024

rileykarson commented Apr 19, 2024

c2thorn Apr 19, 2024

Choose a reason for hiding this comment

melinath Apr 19, 2024

Choose a reason for hiding this comment

c2thorn Apr 19, 2024

Choose a reason for hiding this comment

c2thorn Apr 19, 2024 • edited

Choose a reason for hiding this comment

melinath Apr 19, 2024

Choose a reason for hiding this comment

c2thorn Apr 19, 2024

Choose a reason for hiding this comment

c2thorn commented Apr 19, 2024

melinath commented Apr 19, 2024

c2thorn left a comment

Choose a reason for hiding this comment

c2thorn commented Apr 19, 2024

melinath commented Apr 19, 2024 • edited

ScottSuarez left a comment

Choose a reason for hiding this comment

ScottSuarez Apr 22, 2024

Choose a reason for hiding this comment

rileykarson left a comment • edited

Choose a reason for hiding this comment

melinath commented Apr 26, 2024

melinath commented Apr 26, 2024

melinath commented Apr 26, 2024

melinath commented Apr 26, 2024

melinath commented Apr 18, 2024 •

edited

c2thorn Apr 19, 2024 •

edited

melinath commented Apr 19, 2024 •

edited

rileykarson left a comment •

edited