-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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.
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
[Elastic Agent] Use http2 to connect to Fleet Server. #26474
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -73,6 +73,63 @@ func TestTLSDialer( | |
}), nil | ||
} | ||
|
||
type DialerH2 interface { | ||
Dial(network, address string, cfg *tls.Config) (net.Conn, error) | ||
} | ||
|
||
type DialerFuncH2 func(network, address string, cfg *tls.Config) (net.Conn, error) | ||
|
||
func (d DialerFuncH2) Dial(network, address string, cfg *tls.Config) (net.Conn, error) { | ||
return d(network, address, cfg) | ||
} | ||
|
||
func TLSDialerH2(forward Dialer, config *tlscommon.TLSConfig, timeout time.Duration) (DialerH2, error) { | ||
return TestTLSDialerH2(testing.NullDriver, forward, config, timeout) | ||
} | ||
|
||
func TestTLSDialerH2( | ||
d testing.Driver, | ||
forward Dialer, | ||
config *tlscommon.TLSConfig, | ||
timeout time.Duration, | ||
) (DialerH2, error) { | ||
var lastTLSConfig *tls.Config | ||
var lastNetwork string | ||
var lastAddress string | ||
var m sync.Mutex | ||
|
||
return DialerFuncH2(func(network, address string, cfg *tls.Config) (net.Conn, error) { | ||
switch network { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can this be extracted as it is the same thing as regular http with some modifier passed in changing nextProtos There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sadly no because the function has different parameters, h2 passes in the |
||
case "tcp", "tcp4", "tcp6": | ||
default: | ||
return nil, fmt.Errorf("unsupported network type %v", network) | ||
} | ||
|
||
host, _, err := net.SplitHostPort(address) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
var tlsConfig *tls.Config | ||
m.Lock() | ||
if network == lastNetwork && address == lastAddress { | ||
tlsConfig = lastTLSConfig | ||
} | ||
if tlsConfig == nil { | ||
tlsConfig = config.BuildModuleClientConfig(host) | ||
lastNetwork = network | ||
lastAddress = address | ||
lastTLSConfig = tlsConfig | ||
} | ||
m.Unlock() | ||
|
||
// NextProtos must be set from the passed h2 connection or it will fail | ||
tlsConfig.NextProtos = cfg.NextProtos | ||
|
||
return tlsDialWith(d, forward, network, address, timeout, tlsConfig, config) | ||
}), nil | ||
} | ||
|
||
func tlsDialWith( | ||
d testing.Driver, | ||
dialer Dialer, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ import ( | |
"time" | ||
|
||
"github.com/pkg/errors" | ||
"golang.org/x/net/http2" | ||
|
||
"github.com/elastic/beats/v7/libbeat/common" | ||
"github.com/elastic/beats/v7/libbeat/common/transport" | ||
|
@@ -113,8 +114,16 @@ func NewWithConfig(log *logger.Logger, cfg Config, wrapper wrapperFunc) (*Client | |
hosts := cfg.GetHosts() | ||
clients := make([]*requestClient, len(hosts)) | ||
for i, host := range cfg.GetHosts() { | ||
var transport http.RoundTripper | ||
transport, err := makeTransport(cfg.Timeout, cfg.TLS) | ||
connStr, err := common.MakeURL(string(cfg.Protocol), p, host, 0) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "invalid fleet-server endpoint") | ||
} | ||
addr, err := url.Parse(connStr) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "invalid fleet-server endpoint") | ||
} | ||
|
||
transport, err := makeTransport(addr.Scheme, cfg.Timeout, cfg.TLS) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
@@ -136,12 +145,8 @@ func NewWithConfig(log *logger.Logger, cfg Config, wrapper wrapperFunc) (*Client | |
Timeout: cfg.Timeout, | ||
} | ||
|
||
url, err := common.MakeURL(string(cfg.Protocol), p, host, 0) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "invalid fleet-server endpoint") | ||
} | ||
clients[i] = &requestClient{ | ||
request: prefixRequestFactory(url), | ||
request: prefixRequestFactory(connStr), | ||
client: httpClient, | ||
} | ||
} | ||
|
@@ -272,17 +277,18 @@ func prefixRequestFactory(URL string) requestFunc { | |
} | ||
|
||
// makeTransport create a transport object based on the TLS configuration. | ||
func makeTransport(timeout time.Duration, tls *tlscommon.Config) (*http.Transport, error) { | ||
func makeTransport(scheme string, timeout time.Duration, tls *tlscommon.Config) (http.RoundTripper, error) { | ||
dialer := transport.NetDialer(timeout) | ||
if scheme == "http" { | ||
return &http.Transport{Dial: dialer.Dial}, nil | ||
} | ||
tlsConfig, err := tlscommon.LoadTLSConfig(tls) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this was first just so you dont end up instantiating dialer in case tls config is incorrect. |
||
if err != nil { | ||
return nil, errors.Wrap(err, "invalid TLS configuration") | ||
} | ||
dialer := transport.NetDialer(timeout) | ||
tlsDialer, err := transport.TLSDialer(dialer, tlsConfig, timeout) | ||
tlsDialer, err := transport.TLSDialerH2(dialer, tlsConfig, timeout) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "fail to create TLS dialer") | ||
} | ||
|
||
// TODO: Dial is deprecated we need to move to DialContext. | ||
return &http.Transport{Dial: dialer.Dial, DialTLS: tlsDialer.Dial}, nil | ||
return &http2.Transport{DialTLS: tlsDialer.Dial}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not an issue of this PR but it's pretty disturbing we don't provide Context Dialer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, I was trying to limit the change as much as possible. But we should look at switching all of it to context dailers.