From 35284f66e1be369f0d0676a048be83bf47b92c29 Mon Sep 17 00:00:00 2001 From: Tristan Colgate Date: Thu, 20 Sep 2018 08:53:22 +0100 Subject: [PATCH] fix review comments --- cmd/thanos/flags.go | 4 ++-- cmd/thanos/main.go | 19 +++++++++++++------ cmd/thanos/query.go | 6 ++---- docs/components/query.md | 8 +++++--- docs/components/rule.md | 8 +++++--- docs/components/sidecar.md | 8 +++++--- docs/components/store.md | 8 +++++--- 7 files changed, 37 insertions(+), 24 deletions(-) diff --git a/cmd/thanos/flags.go b/cmd/thanos/flags.go index d325e24c3f..1d084e998e 100644 --- a/cmd/thanos/flags.go +++ b/cmd/thanos/flags.go @@ -30,9 +30,9 @@ func regCommonServerFlags(cmd *kingpin.CmdClause) ( grpcAdvertiseAddr := cmd.Flag("grpc-advertise-address", "Explicit (external) host:port address to advertise for gRPC StoreAPI in gossip cluster. If empty, 'grpc-address' will be used."). String() - grpcTLSSrvKey = cmd.Flag("grpc-server-tls-key", "TLS Key for the gRPC server, leave blank to disable TLS").Default("").String() grpcTLSSrvCert = cmd.Flag("grpc-server-tls-cert", "TLS Certificate for gRPC server, leave blank to disable TLS").Default("").String() - grpcTLSSrvClientCA = cmd.Flag("grpc-server-tls-client-ca", "TLS CA to verify clients against").Default("").String() + grpcTLSSrvKey = cmd.Flag("grpc-server-tls-key", "TLS Key for the gRPC server, leave blank to disable TLS").Default("").String() + grpcTLSSrvClientCA = cmd.Flag("grpc-server-tls-client-ca", "TLS CA to verify clients against. If no client CA is specified, there is no client verification on server side. (tls.NoClientCert)").Default("").String() httpBindAddr = regHTTPAddrFlag(cmd) diff --git a/cmd/thanos/main.go b/cmd/thanos/main.go index c530a7ba9b..b34b4828e4 100644 --- a/cmd/thanos/main.go +++ b/cmd/thanos/main.go @@ -232,12 +232,22 @@ func defaultGRPCServerOpts(logger log.Logger, reg *prometheus.Registry, tracer o ), } - if key == "" || cert == "" { + if key == "" && cert == "" { + if clientCA != "" { + return nil, errors.New("when a client CA is used a server key and certificate must also be provided") + } + level.Info(logger).Log("msg", "disabled TLS, key and cert must be set to enable") return opts, nil } - tlsCfg := &tls.Config{} + if key == "" || cert == "" { + return nil, errors.New("both server key and certificate must be provided") + } + + tlsCfg := &tls.Config{ + MinVersion: tls.VersionTLS12, + } tlsCert, err := tls.LoadX509KeyPair(cert, key) if err != nil { @@ -264,10 +274,7 @@ func defaultGRPCServerOpts(logger log.Logger, reg *prometheus.Registry, tracer o level.Info(logger).Log("msg", "gRPC server TLS client verification enabled") } - creds := credentials.NewTLS(tlsCfg) - opts = append(opts, grpc.Creds(creds)) - - return opts, nil + return append(opts, grpc.Creds(credentials.NewTLS(tlsCfg))), nil } // metricHTTPListenGroup is a run.Group that servers HTTP endpoint with only Prometheus metrics. diff --git a/cmd/thanos/query.go b/cmd/thanos/query.go index a33cdb6367..a692b960e0 100644 --- a/cmd/thanos/query.go +++ b/cmd/thanos/query.go @@ -143,8 +143,7 @@ func storeClientGRPCOpts(logger log.Logger, reg *prometheus.Registry, tracer ope } if !secure { - dialOpts = append(dialOpts, grpc.WithInsecure()) - return dialOpts, nil + return append(dialOpts, grpc.WithInsecure()), nil } level.Info(logger).Log("msg", "Enabling client to server TLS") @@ -185,9 +184,8 @@ func storeClientGRPCOpts(logger log.Logger, reg *prometheus.Registry, tracer ope } creds := credentials.NewTLS(tlsCfg) - dialOpts = append(dialOpts, grpc.WithTransportCredentials(creds)) - return dialOpts, nil + return append(dialOpts, grpc.WithTransportCredentials(creds)), nil } // runQuery starts a server that exposes PromQL Query API. It is responsible for querying configured diff --git a/docs/components/query.md b/docs/components/query.md index ff84d368aa..e2810101ec 100644 --- a/docs/components/query.md +++ b/docs/components/query.md @@ -58,12 +58,14 @@ Flags: Explicit (external) host:port address to advertise for gRPC StoreAPI in gossip cluster. If empty, 'grpc-address' will be used. - --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to - disable TLS --grpc-server-tls-cert="" TLS Certificate for gRPC server, leave blank to disable TLS + --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to + disable TLS --grpc-server-tls-client-ca="" - TLS CA to verify clients against + TLS CA to verify clients against. If no client + CA is specified, there is no client + verification on server side. (tls.NoClientCert) --http-address="0.0.0.0:10902" Listen host:port for HTTP endpoints. --cluster.address="0.0.0.0:10900" diff --git a/docs/components/rule.md b/docs/components/rule.md index 3063f696f2..eeef144677 100644 --- a/docs/components/rule.md +++ b/docs/components/rule.md @@ -64,12 +64,14 @@ Flags: Explicit (external) host:port address to advertise for gRPC StoreAPI in gossip cluster. If empty, 'grpc-address' will be used. - --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to - disable TLS --grpc-server-tls-cert="" TLS Certificate for gRPC server, leave blank to disable TLS + --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to + disable TLS --grpc-server-tls-client-ca="" - TLS CA to verify clients against + TLS CA to verify clients against. If no client + CA is specified, there is no client + verification on server side. (tls.NoClientCert) --http-address="0.0.0.0:10902" Listen host:port for HTTP endpoints. --cluster.address="0.0.0.0:10900" diff --git a/docs/components/sidecar.md b/docs/components/sidecar.md index eba9abc67c..ebd8216a41 100644 --- a/docs/components/sidecar.md +++ b/docs/components/sidecar.md @@ -60,12 +60,14 @@ Flags: Explicit (external) host:port address to advertise for gRPC StoreAPI in gossip cluster. If empty, 'grpc-address' will be used. - --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to - disable TLS --grpc-server-tls-cert="" TLS Certificate for gRPC server, leave blank to disable TLS + --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to + disable TLS --grpc-server-tls-client-ca="" - TLS CA to verify clients against + TLS CA to verify clients against. If no client + CA is specified, there is no client + verification on server side. (tls.NoClientCert) --http-address="0.0.0.0:10902" Listen host:port for HTTP endpoints. --cluster.address="0.0.0.0:10900" diff --git a/docs/components/store.md b/docs/components/store.md index fe50c834a3..804a21b23d 100644 --- a/docs/components/store.md +++ b/docs/components/store.md @@ -52,12 +52,14 @@ Flags: Explicit (external) host:port address to advertise for gRPC StoreAPI in gossip cluster. If empty, 'grpc-address' will be used. - --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to - disable TLS --grpc-server-tls-cert="" TLS Certificate for gRPC server, leave blank to disable TLS + --grpc-server-tls-key="" TLS Key for the gRPC server, leave blank to + disable TLS --grpc-server-tls-client-ca="" - TLS CA to verify clients against + TLS CA to verify clients against. If no client + CA is specified, there is no client + verification on server side. (tls.NoClientCert) --http-address="0.0.0.0:10902" Listen host:port for HTTP endpoints. --cluster.address="0.0.0.0:10900"