Consul TLS Checks for Diagnose [draft] #11467
Conversation
command/operator_diagnose.go
Outdated
|
|
||
| } | ||
|
|
||
| // Initialize the Service Discovery, if there is one |
There was a problem hiding this comment.
If storage setup above fails with an error, does it make sense proceed to the tls portion?
There was a problem hiding this comment.
I guess what we can do is, we can return individual fail errors, so essentially the operator can run diagnose, get a tree of errors, fix those issues, and then run diagnose again to get potentially different errors, as opposed to running diagnose once and getting a whole slew of errors.
command/server.go
Outdated
| @@ -1154,7 +1154,7 @@ func (c *ServerCommand) Run(args []string) int { | |||
| } | |||
|
|
|||
| // Prevent server startup if migration is active | |||
| // TODO: how to incorporate this check into Diagnose? | |||
| // TODO: Use logs with OpenTelemetry to Integrate this into Diagnose | |||
There was a problem hiding this comment.
Hopefully logs aren't needed, but just general otel instrumentation
| @@ -178,47 +213,33 @@ func NewServiceRegistration(conf map[string]string, logger log.Logger, state sr. | |||
| } | |||
|
|
|||
| if consulConf.Scheme == "https" { | |||
|
|
|||
There was a problem hiding this comment.
I think this operation is quick enough to do no matter what. Then if we like the otel approach you just call diagnose.Error if it fails.
There was a problem hiding this comment.
It's definitely quick enough, but if the user doesn't specify tls wouldn't they be surprised by the diagnose TLS error?
There was a problem hiding this comment.
That's a good point, it should definitely be conditional on having a tls setup in the first place.
| @@ -178,47 +213,33 @@ func NewServiceRegistration(conf map[string]string, logger log.Logger, state sr. | |||
| } | |||
|
|
|||
| if consulConf.Scheme == "https" { | |||
|
|
|||
There was a problem hiding this comment.
That's a good point, it should definitely be conditional on having a tls setup in the first place.
| // TODO: Run Diagnose checks on the actual net.Listeners | ||
|
|
||
| var warnings []string |
There was a problem hiding this comment.
One of the things that'll be nice with the otel version is you can just emit warnings in real time rather than having to build up a list of them, but I'm reviewing this one independently of that.
This has TLS checks for Storage, HAStorage, and ServiceRegistration.
The tests just ensure that the correct validation function is called in all scenarios by operator_diagnose.go. Detailed tests around TLS verification are in the diagnose package (previously merged PR).