sds: clusters and listeners read static secrets from Bootstrap.static_resources#3378
sds: clusters and listeners read static secrets from Bootstrap.static_resources#3378mangchiandjjoe wants to merge 21 commits intoenvoyproxy:masterfrom
Conversation
mangchiandjjoe
changed the title
|
Excited to see this being worked on. @lizan @PiotrSikora can you take a first pass? |
lizan
left a comment
lizan
left a comment
There was a problem hiding this comment.
A quick pass. (I already went over once before this PR)
unittests for SecretManagerImpl?
ci/do_circle_ci.sh
Outdated
| # bazel uses jgit internally and the default circle-ci .gitconfig says to | ||
| # convert https://github.com to ssh://git@github.com, which jgit does not support. | ||
| mv ~/.gitconfig ~/.gitconfig_save | ||
| # mv ~/.gitconfig ~/.gitconfig_save |
include/envoy/secret/secret.h
Outdated
| virtual const std::string& getPrivateKey() PURE; | ||
| }; | ||
|
|
||
| typedef std::shared_ptr<Secret> SecretPtr; |
source/common/json/config_schemas.cc
Outdated
| "required" : ["hosts"] | ||
| } | ||
| )EOF"); | ||
|
|
There was a problem hiding this comment.
This is not going to v1 so I don't think you need this.
source/common/secret/secret_impl.cc
Outdated
| certificate_chain_(readDataSource(config.tls_certificate().certificate_chain(), true)), | ||
| private_key_(readDataSource(config.tls_certificate().private_key(), true)) {} | ||
|
|
||
| const std::string SecretImpl::readDataSource(const envoy::api::v2::core::DataSource& source, |
There was a problem hiding this comment.
Just use Config::DataSource::read
source/common/secret/secret_impl.h
Outdated
| public: | ||
| SecretImpl(const envoy::api::v2::auth::Secret& config); | ||
|
|
||
| virtual ~SecretImpl() {} |
source/common/secret/secret_impl.h
Outdated
| private: | ||
| const std::string readDataSource(const envoy::api::v2::core::DataSource& source, | ||
| bool allow_empty); | ||
| const std::string getDataSourcePath(const envoy::api::v2::core::DataSource& source); |
include/envoy/secret/secret.h
Outdated
| /** | ||
| * @return a name of the SDS secret | ||
| */ | ||
| virtual const std::string& getName() PURE; |
There was a problem hiding this comment.
make these getters as const method?
Also I would prefer not having get prefix for those properties, so just name, certificateChain...
There was a problem hiding this comment.
still not const method. (i.e. virtual const std::string& name() const PURE;)
| } | ||
|
|
||
| SecretSharedPtr SecretManagerImpl::getStaticSecret(const std::string& name) { | ||
| return (static_secrets_.find(name) != static_secrets_.end()) ? static_secrets_[name] : nullptr; |
There was a problem hiding this comment.
You're looking up twice, prefer:
auto it = static_secrets_.find(name);
return it != static_secrets_.end() ? it->second : nullptr;
| @@ -0,0 +1,30 @@ | |||
| #pragma once | |||
|
|
|||
| #include <shared_mutex> | |||
There was a problem hiding this comment.
Removed. It will be added later for dynamic secret support.
include/envoy/secret/secret.h
Outdated
|
|
||
| typedef std::shared_ptr<Secret> SecretSharedPtr; | ||
|
|
||
| typedef std::unordered_map<std::string, SecretSharedPtr> SecretSharedPtrMap; |
There was a problem hiding this comment.
The map and vector are not used in interfaces, so move them to impl.h?
mangchiandjjoe
changed the title
|
@mangchiandjjoe Can you try merge master? |
|
FIrst pass done, @mattklein123 can you take a look? |
|
@mangchiandjjoe since we are going to have to fix DCO eventually, can you potentially just do it now before we start more reviews? Feel free to squash/rebase/force push. |
Signed-off-by: jae Kim <jaebong.kim@gmail.com>
|
Presumably #3465 replaces this so closing. |
4 participants
Description:
Clusters and listeners read secrets from the static resources in the bootstrap configuration.
Reading secrets from the Secret Discovery Service will follow.
Risk Level: Low
Fixes #1194
Signed-off-by: Jae Kim jaebong.kim@gmail.com