Improve proxy logging and add TLS-specific testing #180
Conversation
…ary APIs # Conflicts: # proxy/adminservice.go
| @@ -0,0 +1,75 @@ | |||
| inbound: | |||
There was a problem hiding this comment.
instead of calling it old, I prefer adding version. maybe call the new format version 2?
There was a problem hiding this comment.
I can rename the file, but I don't want to version the config at this point: All S2S proxies should be deployed for ~1-2months, tops, so we shouldn't be messing around with config versions
| // CAServerName must match against the remote host's CA cert | ||
| CAServerName string `yaml:"caServerName"` | ||
| // If set to false, VerifyCA will skip the CA authentication step | ||
| VerifyCA bool `yaml:"verifyCA"` |
There was a problem hiding this comment.
default would be false, should we make the default to be always Verify?
There was a problem hiding this comment.
I kept this matching the previous, but yeah setting default-verify does seem a lot safer
| logger = log.NewThrottledLogger(log.With(logger, common.ServiceTag(serviceName)), | ||
| // Replication streams / APIs will run many hundreds of times per second. Throttle their output | ||
| // to 3 / min | ||
| replicationLogger := log.NewThrottledLogger(log.With(logger, common.ServiceTag(serviceName)), |
There was a problem hiding this comment.
call it throttledLog instead?
I assume log replication message is for debugging purpose. can we have a way to config it (from no throttle to 100% throttle (i.e., no logging)?
There was a problem hiding this comment.
Yeah we probably should. Will do in a new PR
| if s.lifetime.Err() != nil { | ||
| // Cluster is closing, just exit. | ||
| return | ||
| break |
There was a problem hiding this comment.
maybe use a defer function here?
There was a problem hiding this comment.
Oh, yeah defer is nicer, will update in followup PR
What was changed
Why?
This change improves on the TLS and logging setup, which should help users more rapidly diagnose setup issues in the future.