Skip to content
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

Merged
merged 11 commits into from
Mar 6, 2023

Conversation

fayzal-g
Copy link
Contributor

@fayzal-g fayzal-g commented Mar 2, 2023

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

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@fayzal-g fayzal-g changed the title Initial implementation Fetch certificates and keys for TLS Config from Vault Mar 3, 2023
@fayzal-g fayzal-g changed the title Fetch certificates and keys for TLS Config from Vault Fetch certificates and keys for TLS from Vault Mar 3, 2023
@fayzal-g fayzal-g changed the title Fetch certificates and keys for TLS from Vault Use generic Reader interface to fetch TLS certs and keys Mar 3, 2023
@fayzal-g fayzal-g changed the title Use generic Reader interface to fetch TLS certs and keys Use Reader interface to fetch TLS certs and keys Mar 6, 2023
@fayzal-g fayzal-g marked this pull request as ready for review March 6, 2023 10:40
Copy link
Contributor

@pracucci pracucci left a 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)

// read ca certificates
// If Reader interface not provided, default to reading from File
if cfg.Reader == nil {
cfg.Reader = &fileReader{}
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, updated

Copy link
Member

@pstibrany pstibrany left a 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.

@fayzal-g fayzal-g merged commit ecdd82c into main Mar 6, 2023
@fayzal-g fayzal-g deleted the secure-creds-from-vault branch March 6, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants