-
Notifications
You must be signed in to change notification settings - Fork 188
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
Add support for custom upstream CAs #34
Conversation
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.
@josefkarasek looking good so far, i have a few comments. Do you also mind to add a small unit test for the init method?
main.go
Outdated
|
||
rootPEM, err := ioutil.ReadFile(upstreamCAFile) | ||
if err != nil { | ||
return nil, err |
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's add some more context here, i.e. fmt.Errorf("error reading upstream CA file: %v", err)
.
main.go
Outdated
MaxIdleConns: 100, | ||
IdleConnTimeout: 90 * time.Second, | ||
TLSHandshakeTimeout: 10 * time.Second, | ||
ExpectContinueTimeout: 1 * time.Second, |
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.
do you mind add a small comment from what Go version these default values are sourced from?
main.go
Outdated
|
||
roots := x509.NewCertPool() | ||
if ok := roots.AppendCertsFromPEM([]byte(rootPEM)); !ok { | ||
return nil, err |
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.
this block doesn't have an err
instance at hand, just an ok == false
, if any certificates were not parsed successfully. I think this block needs an explicit return nil, errors.New("...")
.
main.go
Outdated
|
||
func initTransport(upstreamCAFile string) (http.RoundTripper, error) { | ||
if upstreamCAFile == "" { | ||
return nil, 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.
if there is no upstream CA file here, we are returning nil
here, which effectively uses the default transport later on. For brevity, and because this is an init method, let's return the default transport here to make it explicit.
@s-urbaniak Let's move it to a utils package first? Am thinking about unit testing |
@josefkarasek it is not quite idiomatic to have general |
main.go
Outdated
return nil, err | ||
} | ||
|
||
transport := &http.Transport{ |
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.
Do we care about ALL of these settings or can we use the default transport and simply set the TLSClientConfig field?
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.
Yes we care, they're set for a reason. You might run into troubles, eg run out of file descriptors etc.
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.
@squat I misunderstood you.
We can use http.DefaultTransport direclty. I just wanted to be explicit, instead of changing types between http.RoundTripper
and http.Transport
.
But my understanding is that this would work:
transport := http.DefaultTransport.(*http.Transport)
transport.TLSClientConfig = &tls.Config{RootCAs: roots}
return transport, nil
transport_test.go
Outdated
func TestInitTransportWithDefault(t *testing.T) { | ||
roundTripper, err := initTransport("") | ||
if err != nil { | ||
t.Errorf("initTransport(\"\") is legitimate. Should return http.DefaultTransport. But returned %v", err) |
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.
nit, a more idiomatic test failure would be:
t.Errorf("want err to be nil, but got %v", err)
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.
If I understand the logic correctly, here we should return
too?
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.
yes 👍
transport_test.go
Outdated
if err != nil { | ||
t.Errorf("initTransport(\"\") is legitimate. Should return http.DefaultTransport. But returned %v", err) | ||
} | ||
if eq := reflect.DeepEqual(http.DefaultTransport, roundTripper); !eq { |
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 wouldn't deep equal with the stdlib one, I'd say
if roundTripper == nil {
t.Errorf("expected roundtripper, got nil")
}
transport_test.go
Outdated
func TestInitTransportWithCustomCA(t *testing.T) { | ||
roundTripper, err := initTransport("test/ca.pem") | ||
if err != nil { | ||
t.Errorf("initTransport failed with: %v", err) |
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.
see above, also this should return
from the test.
transport_test.go
Outdated
} | ||
transport := roundTripper.(*http.Transport) | ||
if transport.TLSClientConfig.RootCAs == nil { | ||
t.Error("custom CA cert want't applied") |
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.
t.Error("expected root CA to be set, got nil")
49df7ed
to
a55a85a
Compare
transport.go
Outdated
} | ||
|
||
transport := http.DefaultTransport.(*http.Transport) | ||
transport.TLSClientConfig = &tls.Config{RootCAs: roots} |
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.
the previous version was better IMO. Here, you are modifying the runtime wide DefaultTransport
which might have side effects.
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.
oh crap it's a pointer... you're absolutely right
a55a85a
to
808ed5b
Compare
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 👍
Update OWNERS file
No description provided.