-
Notifications
You must be signed in to change notification settings - Fork 92
Conversation
Signed-off-by: Steve Sloka <steves@heptio.com>
Signed-off-by: Steve Sloka <steves@heptio.com>
@@ -123,13 +124,16 @@ func main() { | |||
osClient.HTTPClient.Transport = httpTransportWithCA(log, openstackCertificateAuthorityFile) | |||
} | |||
|
|||
osClient.HTTPClient = newHTTPClient() |
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 believe this will break the ability to provide a CA cert for use in the HTTP transport (L123). We should also make sure to respect the timeout passed via flag and set in L121.
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.
Let me double check
Signed-off-by: Steve Sloka <steves@heptio.com>
Signed-off-by: Steve Sloka <steves@heptio.com>
Signed-off-by: Steve Sloka <steves@heptio.com>
@alexbrand I refactored the http client and also moved the |
if openstackCertificateAuthorityFile != "" { | ||
osClient.HTTPClient.Transport = httpTransportWithCA(log, openstackCertificateAuthorityFile) | ||
dClient.Transport = httpTransportWithCA(log, openstackCertificateAuthorityFile) |
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.
We will overwrite the openstack.LogRoundTripper
that we are configuring in L126.
I think something like this would work?
transport := &openstack.LogRoundTripper{
RoundTripper: http.DefaultTransport,
Log: log,
ClusterName: clusterName,
ClusterType: clusterType,
Metrics: &discovererMetrics,
}
if openstackCertificateAuthorityFile != "" {
transport.RoundTripper = httpTransportWithCA(log, openstackCertificateAuthorityFile)
}
osClient.HTTPClient = http.Client{
Transport: transport,
Timeout: httpClientTimeout,
}
Signed-off-by: Steve Sloka <steves@heptio.com>
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.
LGTM
Fixes #47