Add memberlist TLS configuration options #4046
Conversation
jtlisi
changed the title
| if config.TLSEnabled { | ||
| tcpLn, err = tls.Listen("tcp", tcpAddr.String(), t.tlsConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to start tls TCP listener on %q port %d: %v", addr, port, err) |
There was a problem hiding this comment.
Should these be %w for the error?
There was a problem hiding this comment.
Yea, I think this would be a good fit for that formatting directive.
There was a problem hiding this comment.
I agree on %w but, doing a step forward, what if we move to errors.Wrapf() like we're currently doing everywhere? This looks material for a follow up PR, not to be done in this one.
There was a problem hiding this comment.
%w/%v makes no difference, until we want to unwrap the errors and work with them. If we just log them, it doesn't matter. (Agree on using errors.Wrap, when we get to fixing this)
| cortex1 = newSingleBinary("cortex-1", networkName+"-cortex-1", "") | ||
| cortex2 = newSingleBinary("cortex-2", networkName+"-cortex-2", networkName+"-cortex-1:8000") | ||
| cortex3 = newSingleBinary("cortex-3", networkName+"-cortex-3", networkName+"-cortex-1:8000") |
There was a problem hiding this comment.
This maybe a bit misleading. The TLSServerName should be set to a name required on the certificate of the peer (you are setting the name to ourselves).
So in the case of cortex1 (line 66), I would expect it to be set to cortex-2 and cortex-3.
Probably the best is only have a single DNS name on the certificate as otherwise we would need to set it independently depending on which peer we are connecting to.
| if config.TLSEnabled { | ||
| tcpLn, err = tls.Listen("tcp", tcpAddr.String(), t.tlsConfig) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to start tls TCP listener on %q port %d: %v", addr, port, err) |
There was a problem hiding this comment.
I agree on %w but, doing a step forward, what if we move to errors.Wrapf() like we're currently doing everywhere? This looks material for a follow up PR, not to be done in this one.
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
Signed-off-by: Jacob Lisi <[email protected]>
jtlisi
force-pushed
the
2021w13_memberlist_tls
branch
from
511594f to
cdf53d4
Compare
What this PR does:
Which issue(s) this PR fixes:
Fixes #4045
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]