loki-canary: Add support for client-side TLS certs for Loki connection #6310
Conversation
|
|
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
1 similar comment
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
… clear, check that -tls is set if client certs are used
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. + ingester 0%
+ distributor 0%
+ querier 0%
+ querier/queryrange 0.1%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
- loki -0.6% |
|
@DylanGuedes and @trevorwhitney thanks for the quick review, I got the CLA signed (and updated all commits with same username) and the PR should now be ready to merge. |
pkg/canary/reader/reader.go
Outdated
| func (r *Reader) webSocketDialer() *websocket.Dialer { | ||
| dialer := websocket.DefaultDialer | ||
| if r.clientTLSConfig != nil { | ||
| dialer.TLSClientConfig = r.clientTLSConfig |
There was a problem hiding this comment.
We probably shouldn't be mutating websocket.DefaultDialer in this library package.
Instead, let's create a new one like:
var dialer = &Dialer{
Proxy: http.ProxyFromEnvironment,
HandshakeTimeout: 45 * time.Second,
TLSClientConfig = r.clientTLSConfig,
}
There was a problem hiding this comment.
My understanding is that dialer is a copy of websocket.DefaultDialer and there is no mutation happening outside this package. However, I like your way better as it keeps me from having to do local mutation.
| var tlsConfig *tls.Config | ||
| tc := config.TLSConfig{} | ||
| if *certFile != "" || *keyFile != "" || *caFile != "" { | ||
| if !*useTLS { | ||
| _, _ = fmt.Fprintf(os.Stderr, "Must set --tls when specifying client certs\n") | ||
| os.Exit(1) | ||
| } | ||
| tc.CAFile = *caFile | ||
| tc.CertFile = *certFile | ||
| tc.KeyFile = *keyFile | ||
| tc.InsecureSkipVerify = false | ||
|
|
||
| var err error | ||
| tlsConfig, err = config.NewTLSConfig(&tc) | ||
| if err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "TLS configuration error: %s\n", err.Error()) | ||
| os.Exit(1) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Correct me if I'm missing anything. We could simplify this initialization code bit.
the gist is
- to first use
if useTLSto do all this and skip everything completely (even before checking for*certFile, *keyFile, *caFile. - And trusting
config.NewTLSConfigfor checking if all the files are invalid (even if its empty)
Something simple like
if useTLS {
tls, err := config.NewTLSConfig(
&config.TLSConfig{
CAFile, *caFile,
CertFile, *certFile,
KeyFile, *keyFile,
InsecureSkipVerify: false,
},
)
if err != nil {
// print error
os.Exit(1)
}
}
}No need to check for any empty string on those cert files. NewTLSConfig() should return error in that case correct?
There was a problem hiding this comment.
The main reason for doing this was to ensure valid configuration. Setting the client certs requires -tls to be set otherwise it is a noop. I actually made this mistake in my own testing and was glad to have the error message. On the other hand, when setting -tls it does not require setting client certs.
There was a problem hiding this comment.
On the other hand, when setting -tls it does not require setting client certs.
Ah make sense. In that case we just use default client. thanks for the clarification.
| resp, err := http.DefaultClient.Do(req) | ||
| httpClient, err := r.httpClient() | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
I would highly recommend wrapping the error here. (and also on the QueryCountOverTime) with some context.
Rationale is I see we use it on both Query and QueryCountOverTime and it can confuse the consumer of the error, where exactly it is coming from.
|
./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell. - ingester -0.1%
- distributor -0.3%
+ querier 0%
+ querier/queryrange 0%
+ iter 0%
+ storage 0%
+ chunkenc 0%
+ logql 0%
+ loki 0% |
|
@chodges15 also addressed your suggestion @owen-d. I think it's good to go. Merging it. |
What this PR does / why we need it:
Support for loki-canary in environments using mTLS, in anticipation of deny-by-default Loki server authentication coming for #6283.
Which issue(s) this PR fixes:
Fixes #6295
Checklist
CHANGELOG.md.