cli: add --cert-principal-map to cert list command
#48013
Conversation
|
This change is |
|
This is an intermediate step towards moving |
There was a problem hiding this comment.
LGTM modulo the default initialization
Reviewed 4 of 4 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @petermattis)
pkg/cli/context.go, line 400 at r1 (raw file):
// Defaults set by InitCLIDefaults() above. var certCtx struct { certPrincipalMap []string
you should reset this at the top in initCLIDefaults()
pkg/server/server.go, line 206 at r2 (raw file):
// Attempt to load TLS configs right away, failures are permanent. if !cfg.Insecure {
I think this change makes this PR fix #17194 - WDYT?
Remove the use of `base.Config.GetCertificateManager()` from the `cert list` implementation as a first step in limiting use of that method, and possibly removing it in the future. Fixes cockroachdb#48011 Release note (cli change): Support `list cert` with certificates which require `--cert-principal-map` to pass validation.
Remove `base.Config.InitializeNodeTLSConfigs` and inline its functionality in `server.NewServer`. This is another step in an effort to isolate usage of `CertificateManager` to server related code. Unexport some methods of `base.Config` that were not used externally. Release note: None
petermattis
force-pushed
the
pmattis/list-certs
branch
from
40d5cc1
to
d098fcc
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/cli/context.go, line 400 at r1 (raw file):
Previously, knz (kena) wrote…
you should reset this at the top in
initCLIDefaults()
Done.
pkg/server/server.go, line 206 at r2 (raw file):
Previously, knz (kena) wrote…
I think this change makes this PR fix #17194 - WDYT?
I'm not seeing why this PR would fix #17194. The code here is intended to be identical to what was previously being done. Note that InitializeNodeTLSConfigs already had a if cfg.Insecure check. Am I missing something? I'm not actually sure what the problem is in #17194.
There was a problem hiding this comment.
Reviewed 1 of 3 files at r3, 2 of 2 files at r4.
Reviewable status:
pkg/server/server.go, line 206 at r2 (raw file):
Previously, petermattis (Peter Mattis) wrote…
I'm not seeing why this PR would fix #17194. The code here is intended to be identical to what was previously being done. Note that
InitializeNodeTLSConfigsalready had aif cfg.Insecurecheck. Am I missing something? I'm not actually sure what the problem is in #17194.
Yes sorry I misread the call point.
The problem in #17194 is that the node certs should be checked to include the listen addr upon startup, not merely when another node tries to join. But I agree it's unrelated here.
|
TFTR! bors r+ |
Build succeeded |
48013: cli: add --cert-principal-map to `cert list` command r=petermattis a=petermattis Remove the use of `base.Config.GetCertificateManager()` from the `cert list` implementation as a first step in limiting use of that method, and possibly removing it in the future. Fixes cockroachdb#48011 Release note (cli change): Support `list cert` with certificates which require `--cert-principal-map` to pass validation. Co-authored-by: Peter Mattis <petermattis@gmail.com>
Remove the use of
base.Config.GetCertificateManager()from thecert listimplementation as a first step in limiting use of that method, andpossibly removing it in the future.
Fixes #48011
Release note (cli change): Support
list certwith certificates whichrequire
--cert-principal-mapto pass validation.