-
Notifications
You must be signed in to change notification settings - Fork 5.5k
secret: add secret provider interface and use it for TlsCertificates #4086
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/common/pure.h" | ||
| #include "envoy/ssl/tls_certificate_config.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| /** | ||
| * A secret provider for each kind of secrets. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: s/secrets/secret |
||
| */ | ||
| template <class SecretType> class SecretProvider { | ||
| public: | ||
| virtual ~SecretProvider() {} | ||
|
|
||
| /** | ||
| * @return the secret. Returns nullptr if the secret is not ready. | ||
| */ | ||
| virtual const SecretType* secret() const PURE; | ||
|
|
||
| // TODO(lizan): Add more methods for dynamic secret provider. | ||
| }; | ||
|
|
||
| typedef SecretProvider<Ssl::TlsCertificateConfig> TlsCertificateConfigProvider; | ||
| typedef std::shared_ptr<TlsCertificateConfigProvider> TlsCertificateConfigProviderSharedPtr; | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,7 @@ class ContextConfig { | |
| */ | ||
| virtual const std::string& certificateRevocationListPath() const PURE; | ||
|
|
||
| // TODO(lizan): consider refactor 4 methods below to Ssl::TlsCertificateConfig | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 for this change.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW: lizan/envoy@static_secret_provider...tls_certificate_config , I prefer to have this change in a follow up PRs, but if reviewers prefer in same PR, I will add a commit. |
||
| /** | ||
| * @return The certificate chain used to identify the local side. | ||
| */ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ | |
|
|
||
| #include "envoy/api/v2/core/base.pb.h" | ||
|
|
||
| #include "absl/types/optional.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Config { | ||
| namespace DataSource { | ||
|
|
@@ -19,7 +21,7 @@ std::string read(const envoy::api::v2::core::DataSource& source, bool allow_empt | |
| * @param source data source. | ||
| * @return std::string path to DataSource if a filename, otherwise an empty string. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. update |
||
| */ | ||
| std::string getPath(const envoy::api::v2::core::DataSource& source); | ||
| absl::optional<std::string> getPath(const envoy::api::v2::core::DataSource& source); | ||
|
|
||
| } // namespace DataSource | ||
| } // namespace Config | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| #include "common/secret/secret_provider_impl.h" | ||
|
|
||
| #include "common/common/assert.h" | ||
| #include "common/ssl/tls_certificate_config_impl.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| TlsCertificateConfigProviderImpl::TlsCertificateConfigProviderImpl( | ||
| const envoy::api::v2::auth::TlsCertificate& tls_certificate) | ||
| : tls_certificate_(std::make_unique<Ssl::TlsCertificateConfigImpl>(tls_certificate)) {} | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/api/v2/auth/cert.pb.h" | ||
| #include "envoy/secret/secret_provider.h" | ||
| #include "envoy/ssl/tls_certificate_config.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Secret { | ||
|
|
||
| class TlsCertificateConfigProviderImpl : public TlsCertificateConfigProvider { | ||
| public: | ||
| TlsCertificateConfigProviderImpl(const envoy::api::v2::auth::TlsCertificate& tls_certificate); | ||
|
|
||
| const Ssl::TlsCertificateConfig* secret() const override { return tls_certificate_.get(); } | ||
|
|
||
| private: | ||
| Ssl::TlsCertificateConfigPtr tls_certificate_; | ||
| }; | ||
|
|
||
| } // namespace Secret | ||
| } // namespace Envoy |
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.
Why do we need this? And couldn't this be static? It doesn't seem to be modifying or requiring SecretManager at all.
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.
It could be static with current implementation since I wanted to keep this PR as a refactor. Though secret_manager might do more in future, (e.g. returning same provider if config is exactly same), and/or track all secrets for dump config like rds does.
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.
Sorry what is the different between an inline secret and a static secret? I'm confused.
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.
inline secret is the secret directly embedded in the config. static secrets are specified in the static_resources, the config only has a name to point to it.
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.
OK then I guess I have the same question as @PiotrSikora. Why isn't this static?
Uh oh!
There was an error while loading. Please reload this page.
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.
My understanding is: inline is the legacy way of configuring ssl cert. statis secrets are only added when SDS works started to share secrets used in many places. By removing inline, it may break existing users.
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'm not saying to remove inline, I think that will always be there. My question is why isn't inline just a static secret?
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.
@mattklein123 IIUC @PiotrSikora asks why not making the C++ method static, your question is to merge inline secret and static secret.
Static secrets can be only registered via static_resources in bootstrap.proto, looked up by name, owned by secret manager. Inline secrets are in each tls_context, have no name, in future, secret manager may track more than current implementation. They have different lifecycles.
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.
Ah I see. OK.