discovery: add insecureTls flag to skip TLS checks #551
Conversation
alban
force-pushed
the
alban/discovery-insecure
branch
from
7600119 to
fe1d9d1
Compare
|
Should we use bit flags rather than separate bools or are we pretty confident these are the only two there'll ever be? |
|
@jonboulle: As stated in #545 by @alban - we will likely need three flags - one for allowing connections over unencrypted HTTP, one for skipping TLS certificates verification and one for allowing credentials passing over insecure connections (either HTTP or HTTPS with unverified TLS certificates). So I guess it would be better to have bit flags. Another solution, quicker and less dirty than 3 bools, would be separate bool-like, self-documenting types (like in https://github.com/coreos/rkt/blob/master/rkt/pubkey/pubkey.go#L42) |
| @@ -49,13 +52,24 @@ func init() { | |||
| return net.DialTimeout(n, a, defaultDialTimeout) | |||
| }, | |||
| } | |||
| tInsecureTls := &http.Transport{ | |||
There was a problem hiding this comment.
You could probably do a shallow copy of http.DefaultTransport and modify the TLSClientConfig field.
tInsecureTls := *http.DefaultTransport
tInsecureTls.TLSClientConfig = &tls.Config{InsecureSkipVerify: true}
ClientInsecureTls = &http.Client{
Transport: &tInsecureTls,
}
|
This PR doesn't allow to skip sending credentials over HTTP. |
alban
force-pushed
the
alban/discovery-insecure
branch
from
fe1d9d1 to
29bafaa
Compare
|
Updated with a bit field for
Can this be done separately? Now that it is a bitfield, it should be possible to add after without changing the function prototype. |
| @@ -58,7 +58,11 @@ func runDiscover(args []string) (exit int) { | |||
| stderr("%s: %s", name, err) | |||
| return 1 | |||
| } | |||
| eps, attempts, err := discovery.DiscoverEndpoints(*app, nil, transportFlags.Insecure) | |||
| var insecure discovery.InsecureOption | |||
There was a problem hiding this comment.
How about insecure := discovery.InsecureNone?
|
Looks fine, but the testing is not sufficient. |
alban
force-pushed
the
alban/discovery-insecure
branch
from
29bafaa to
296ac04
Compare
The "insecure" bool was previously used to allow http. This is now changed to a bit field with "InsecureHttp" and "InsecureTls". Related to appc#545
alban
force-pushed
the
alban/discovery-insecure
branch
from
296ac04 to
dec3590
Compare
|
@krnowak ready for another review :) |
| if err != nil && !tt.expectDiscoverySuccess { | ||
| continue | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Should we handle the case when err == nil and tt.expectDiscoverySuccess == false?
There was a problem hiding this comment.
ok, fixed in a separate commit.
|
One question, but otherwise LFAD. But won't merge it anyway, I have no power here. |
The test cases where a failure was expected was not tested correctly.
|
Branch updated. |
|
@jonboulle can you review this? :) |
jonboulle
added
component/discovery
area/developer tooling
component/tooling
and removed
area/developer tooling
labels
|
thanks! |
discovery: add insecureTls flag to skip TLS checks
|
For the record, this is in v0.7.4 |
The "insecure" was previously used to allow http. This is now split into
two flags "insecureHttp" and "insecureTls".
Related to #545
/cc @krnowak
I will need this for tests in rkt, see rkt/rkt#1822.