Add a mTLS gRPC server to veneur-proxy. #1029
Conversation
arnavdugar-stripe
force-pushed
the
arnavdugar/mtls
branch
2 times, most recently
from
608d5ad to
b05f832
Compare
arnavdugar-stripe
force-pushed
the
arnavdugar/mtls
branch
2 times, most recently
from
271b0cf to
7dec690
Compare
arnavdugar-stripe
force-pushed
the
arnavdugar/mtls
branch
from
7dec690 to
32437fc
Compare
| @@ -72,6 +72,7 @@ func main() { | |||
| if err != nil { | |||
| logger.WithError(err).Fatal("failed to create statsd client") | |||
| } | |||
| defer statsClient.Flush() | |||
There was a problem hiding this comment.
I'm missing this on the 'left side'. Is this newly needed for mTLS?
There was a problem hiding this comment.
Nope, this is just missing in general.
| @@ -72,6 +81,21 @@ func Create(params *CreateParams) *Proxy { | |||
| IsClient: false, | |||
| Statsd: params.Statsd, | |||
| })), | |||
| grpcTlsAddress: params.Config.GrpcTlsAddress, | |||
| grpcTlsServer: grpc.NewServer( | |||
There was a problem hiding this comment.
If TLS is specified, the service will still also open a non-TLS, is that correct?
There was a problem hiding this comment.
Yes. Both will be there.
| @@ -39,6 +40,9 @@ type Proxy struct { | |||
| grpcAddress string | |||
| grpcListener net.Listener | |||
| grpcServer *grpc.Server | |||
| grpcTlsAddress string | |||
There was a problem hiding this comment.
I believe that these should have updates included?
| @@ -7,18 +7,18 @@ import ( | |||
| "os" | |||
| ) | |||
|
|
|||
| type config struct { | |||
| type Config struct { | |||
There was a problem hiding this comment.
Why is config now being exposed? (it appears the original intent was for it to be hidden & an implementation detail. Exposing it, I believe, would imply we need to reevaluate the API it entails (In this case, specifically the GetTlsConfig() method)
There was a problem hiding this comment.
I needed it in the unit test here, couldn't find a way to do it without making it public:
https://github.com/stripe/veneur/pull/1029/files#diff-8d5bed61a3c0b7b0bfdd65dd4c93e647568b1a402840f5e302c0c922f6c58ea3R38
There was a problem hiding this comment.
Yeah the need for type Config struct seems redundant if the fields are exposed anyways. Could prolly just use type Tls struct
There was a problem hiding this comment.
Could prolly just use type Tls struct
Doing this causes infinite recursion when unmarshalling yaml, because then
err := unmarshal(&config.Config)
will call
func (config *Tls) UnmarshalYAML(unmarshal func(interface{}) error) error {
The type passed into unmarshal must be different than the receiver type of UnmarshalYAML.
proxy/proxy.go
Outdated
| // Start gRPC TLS server | ||
| grpcTlsExit := make(chan error, 1) | ||
| proxy.logger.WithFields(logrus.Fields{ | ||
| "address": proxy.GetGrpcTlsAddress(), | ||
| "tls": "true", | ||
| }).Debug("serving grpc") | ||
| waitGroup.Add(1) | ||
| go func() { | ||
| defer waitGroup.Done() | ||
|
|
There was a problem hiding this comment.
The grpcTLS server seems like a dupe of grpc Server above. I would at least want to see a separate function for the deep nesting of goroutines like go start(server))
There was a problem hiding this comment.
It's subtlety different enough that I figured it would be more readable to keep it separate. Wdyt?
There was a problem hiding this comment.
I think the anonymous function at line 292 should at least be a separate function as right now it handles ~a lot of different tasks.
go handleShutdown(proxy.grpcServer) seems easy enough. (may add a logger field so that the logger outputs "server_type: grpcTls" or "server_type: grpc" to distinguish the logs)
There was a problem hiding this comment.
Actually I was able to refactor this cleanly.
proxy/proxy.go
Outdated
| go func() { | ||
| defer func() { | ||
| proxy.grpcTlsListener.Close() | ||
| close(grpcTlsError) | ||
| waitGroup.Done() | ||
| }() | ||
|
|
There was a problem hiding this comment.
Can this be a separate function rather than anonymous?
arnavdugar-stripe
force-pushed
the
arnavdugar/mtls
branch
from
bfcb176 to
6e6435d
Compare
Summary
Add a mTLS gRPC server to veneur-proxy.