Code review tweaks for InsecureSkipVerify support #2
Conversation
| config.ServerName = serverName | ||
| // Set the ServerName and rely on the usual logic in | ||
| // tls.Conn.Handshake() to do its verification | ||
| configCopy.ServerName = serverName |
There was a problem hiding this comment.
We want to modify the copy, not the original
There was a problem hiding this comment.
It just depends on which we pass where though right? If we want to pass the copy to the tls layer, we should modify the copy. Otherwise, we should modify the original. Similarly, in that case we would check the copy when deciding whether or not to verify the certs in that case.
There was a problem hiding this comment.
My requirement here is that the original remain unchanged, since it was passed in to us and we want to be a polite API. Also, your original change made a copy, but modified and passed the original, so the only effect of the copy was to remember the original InsecureSkipVerify setting.
There was a problem hiding this comment.
I should add that my original code failed to make a copy when it should have, and it's not clear that the copy it was making was made correctly :( Hence the added unit test condition :)
There was a problem hiding this comment.
Makes sense on not modifying what users pass in. I think in practice people rarely reuse that config, but I guess you never know.
Also, your original change made a copy, but modified and passed the original, so the only effect of the copy was to remember the original InsecureSkipVerify setting.
Sure, it could just as easily have pulled out that boolean.
Code review tweaks for InsecureSkipVerify support
@myleshorton Some suggested changes to your changes