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

Add HTTP transport with settings to support multiple connections #2685

Merged
merged 6 commits into from
Jan 24, 2023
Merged

Add HTTP transport with settings to support multiple connections #2685

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1bcccf8
Add HTTP transport with settings to support multiple connections
Jan 19, 2023
9663ff4
Add full http transport configuration
Jan 19, 2023
9308b5d
Add comments for http.Transport values used
Jan 19, 2023
9a40ea0
Add TLS config
Jan 20, 2023
a880363
Make default http client generic
Jan 23, 2023
e1ef08c
Merge branch 'main' into multi-httpconns
super-harsh Jan 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions v2/internal/genericarmclient/generic_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ package genericarmclient

import (
"context"
"crypto/tls"
"net"
"net/http"
"strings"
"time"
Expand Down Expand Up @@ -84,6 +86,30 @@ func NewGenericClientFromHTTPClient(cloudCfg cloud.Configuration, creds azcore.T
// the value IN the interface IS nil).
if httpClient != nil {
opts.Transport = httpClient
} else {
// If httpClient is not provided, we use a HTTPClient with default Transport + settings
// to establish multiple TCP ARMClient connections to avoid throttling.
// TODO: Use https://github.com/Azure/go-armbalancer here once its prod ready.
matthchr marked this conversation as resolved.
Show resolved Hide resolved
httpTransport := &http.Transport{
Proxy: http.ProxyFromEnvironment,
Copy link
Member

Choose a reason for hiding this comment

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

Ahh one more thing -- we should have a single global one of these, not 1 per call to this function (that's how the SDK does it as well, see the transport_default_http_client link I gave below.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So rather than having a httpClient per GenericClient, we want to share a default httpClient across all GenericClients?

Copy link
Member

Choose a reason for hiding this comment

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

Share the transport, the HTTPClient can be constructed 1 for each.

Copy link
Member

Choose a reason for hiding this comment

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

The reason for that is because the transport == connection pool == expensive to create. Golang has a single default transport and so does the Azure SDK, so we want to mirror that experience otherwise it would be easy to accidentally end up creating a lot more connections than we expected (because we constructed different GenericClient's)

DialContext: (&net.Dialer{
Timeout: 30 * time.Second, // default transport value
KeepAlive: 30 * time.Second, // default transport value
}).DialContext,
ForceAttemptHTTP2: false, // default is true; since HTTP/2 multiplexes a single TCP connection. we'd want to use HTTP/1, which would use multiple TCP connections.
MaxIdleConns: 100, // default transport value
MaxIdleConnsPerHost: 10, // default is 2, so we want to increase the number to use establish more connections.
Copy link
Member

Choose a reason for hiding this comment

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

This is missing TLS min version, which the SDK has I think?

See https://github.com/Azure/azure-sdk-for-go/blob/main/sdk/azcore/runtime/transport_default_http_client.go#L18

	defaultTransport := &http.Transport{
		Proxy: http.ProxyFromEnvironment,
		DialContext: (&net.Dialer{
			Timeout:   30 * time.Second,
			KeepAlive: 30 * time.Second,
		}).DialContext,
		ForceAttemptHTTP2:     true,
		MaxIdleConns:          100,
		IdleConnTimeout:       90 * time.Second,
		TLSHandshakeTimeout:   10 * time.Second,
		ExpectContinueTimeout: 1 * time.Second,
		TLSClientConfig: &tls.Config{
			MinVersion: tls.VersionTLS12,
		},
	}

IdleConnTimeout: 90 * time.Second, // default transport value
TLSHandshakeTimeout: 10 * time.Second, // default transport value
ExpectContinueTimeout: 1 * time.Second, // default transport value
TLSClientConfig: &tls.Config{
MinVersion: tls.VersionTLS12, // default tls version
},
}

opts.Transport = &http.Client{
Transport: httpTransport,
}
}

opts.PerCallPolicies = append([]policy.Policy{rpRegistrationPolicy}, opts.PerCallPolicies...)
Expand All @@ -101,6 +127,7 @@ func NewGenericClientFromHTTPClient(cloudCfg cloud.Configuration, creds azcore.T
opts: opts,
metrics: metrics,
}, nil

}

// SubscriptionID returns the subscription the client is configured for
Expand Down