-
Notifications
You must be signed in to change notification settings - Fork 68
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use Reader interface to fetch TLS certs and keys #274
Conversation
eff5160
to
7a38715
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (modulo a nit)
crypto/tls/tls.go
Outdated
// read ca certificates | ||
// If Reader interface not provided, default to reading from File | ||
if cfg.Reader == nil { | ||
cfg.Reader = &fileReader{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to not overwrite cfg.Reader
. You can just store the reader in a local variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change lgtm. Shall we update description for CertPath
, KeyPath
and other fields to not refer to "file"? We can just talk about "paths" without specifying that they refer to a file in the filesystem.
What this PR does:
Uses a Reader interface to fetch secrets (certs/keys) used to configure TLS. Now allows reading not just from a file, but for anything that implements
ReadSecret
Fixes #
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]