security: new sub-package for certificate/key paths (cli refactor sequence 1/n)#82068
security: new sub-package for certificate/key paths (cli refactor sequence 1/n)#82068craig[bot] merged 4 commits intomasterfrom
Conversation
|
This change is |
1732699 to
bfa8d93
Compare
catj-cockroach
left a comment
There was a problem hiding this comment.
Looks good to me!
Reviewed 2 of 2 files at r1, 9 of 9 files at r2, 8 of 8 files at r3, 15 of 15 files at r4, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @knz and @otan)
pkg/security/certnames/certnames.go line 25 at r3 (raw file):
// IsCertificateFile returns true if the file name looks like a certificate file. func IsCertificateFile(filename string) bool {
nit: I would argue this should be IsCertificateFilename rather than IsCertificateFile unless we're checking that the file has a PEM header.
pkg/security/certnames/locator.go line 22 at r4 (raw file):
// A Locator provides locations to certificates. type Locator struct {
This is more for my curiosity rather than a request, but would it be difficult to move the certificate fallback logic into Locator?
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/security/certnames/certnames.go line 25 at r3 (raw file):
Previously, catj-cockroach (Cat J) wrote…
nit: I would argue this should be
IsCertificateFilenamerather thanIsCertificateFileunless we're checking that the file has a PEM header.
Done.
pkg/security/certnames/locator.go line 22 at r4 (raw file):
Previously, catj-cockroach (Cat J) wrote…
This is more for my curiosity rather than a request, but would it be difficult to move the certificate fallback logic into Locator?
This new package is only interested in defining paths. In particular, it does not actually access the filesystem.
I think to do what you suggest we'll want to extract the fallback logic from CertificateManager into an actuall file access layer. It's a good idea (and on the way to solve #82075) but it's out of scope on the sequence of PRs that I'm currently working on (towards solving #81882).
|
(NB: Holding off on merging this despite the review approval, since we can't merge intermediate PRs in a sequence without dropping the entire sequence off github.) |
0f5ba4f to
9a2e831
Compare
The duplicated directive was making `dev generate bazel` unable to keep the list of deps in sync. Release note: None
We want to have the cert/key filenames in a package with minimal dependencies, suitable for embedding in the lightweight SQL executable. This commit starts the work by moving the file name constants in a new separate package. Release note: None
Release note: None
... since the function does not actually look at the contents of the file. Release note: None
catj-cockroach
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r5, 8 of 8 files at r6, all commit messages.
Reviewable status:
catj-cockroach
left a comment
There was a problem hiding this comment.
Reviewable status:
Informs 82075.
PR split from #82020 to ease review.