secret manager: refactoring to use statusor (envoyproxy#31166)

Signed-off-by: Alyssa Wilk <[email protected]> envoyproxy/envoy-mobile#176
fredyw · Dec 5, 2023 · dc32d4f · dc32d4f
1 parent 1ac3b9c
commit dc32d4f
Show file tree

Hide file tree

Showing 7 changed files with 50 additions and 45 deletions.
diff --git a/envoy/secret/secret_manager.h b/envoy/secret/secret_manager.h
@@ -25,9 +25,9 @@ class SecretManager {
 
   /**
    * @param add a static secret from envoy::extensions::transport_sockets::tls::v3::Secret.
-   * @throw an EnvoyException if the secret is invalid or not supported, or there is duplicate.
+   * @return a status indicating if the function completed successfully.
    */
-  virtual void
+  virtual absl::Status
   addStaticSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) PURE;
 
   /**

diff --git a/source/common/secret/secret_manager_impl.cc b/source/common/secret/secret_manager_impl.cc
@@ -26,15 +26,15 @@ SecretManagerImpl::SecretManagerImpl(OptRef<Server::ConfigTracker> config_tracke
   }
 }
 
-void SecretManagerImpl::addStaticSecret(
+absl::Status SecretManagerImpl::addStaticSecret(
     const envoy::extensions::transport_sockets::tls::v3::Secret& secret) {
   switch (secret.type_case()) {
   case envoy::extensions::transport_sockets::tls::v3::Secret::TypeCase::kTlsCertificate: {
     auto secret_provider =
         std::make_shared<TlsCertificateConfigProviderImpl>(secret.tls_certificate());
     if (!static_tls_certificate_providers_.insert(std::make_pair(secret.name(), secret_provider))
              .second) {
-      throwEnvoyExceptionOrPanic(
+      return absl::InvalidArgumentError(
           absl::StrCat("Duplicate static TlsCertificate secret name ", secret.name()));
     }
     break;
@@ -45,7 +45,7 @@ void SecretManagerImpl::addStaticSecret(
     if (!static_certificate_validation_context_providers_
              .insert(std::make_pair(secret.name(), secret_provider))
              .second) {
-      throwEnvoyExceptionOrPanic(absl::StrCat(
+      return absl::InvalidArgumentError(absl::StrCat(
           "Duplicate static CertificateValidationContext secret name ", secret.name()));
     }
     break;
@@ -56,7 +56,7 @@ void SecretManagerImpl::addStaticSecret(
     if (!static_session_ticket_keys_providers_
              .insert(std::make_pair(secret.name(), secret_provider))
              .second) {
-      throwEnvoyExceptionOrPanic(
+      return absl::InvalidArgumentError(
           absl::StrCat("Duplicate static TlsSessionTicketKeys secret name ", secret.name()));
     }
     break;
@@ -66,14 +66,15 @@ void SecretManagerImpl::addStaticSecret(
         std::make_shared<GenericSecretConfigProviderImpl>(secret.generic_secret());
     if (!static_generic_secret_providers_.insert(std::make_pair(secret.name(), secret_provider))
              .second) {
-      throwEnvoyExceptionOrPanic(
+      return absl::InvalidArgumentError(
           absl::StrCat("Duplicate static GenericSecret secret name ", secret.name()));
     }
     break;
   }
   default:
-    throwEnvoyExceptionOrPanic("Secret type not implemented");
+    return absl::InvalidArgumentError("Secret type not implemented");
   }
+  return absl::OkStatus();
 }
 
 TlsCertificateConfigProviderSharedPtr

diff --git a/source/common/secret/secret_manager_impl.h b/source/common/secret/secret_manager_impl.h
@@ -19,7 +19,7 @@ namespace Secret {
 class SecretManagerImpl : public SecretManager {
 public:
   SecretManagerImpl(OptRef<Server::ConfigTracker> config_tracker);
-  void
+  absl::Status
   addStaticSecret(const envoy::extensions::transport_sockets::tls::v3::Secret& secret) override;
 
   TlsCertificateConfigProviderSharedPtr

diff --git a/source/server/configuration_impl.cc b/source/server/configuration_impl.cc
@@ -119,7 +119,7 @@ void MainImpl::initialize(const envoy::config::bootstrap::v3::Bootstrap& bootstr
   ENVOY_LOG(info, "loading {} static secret(s)", secrets.size());
   for (ssize_t i = 0; i < secrets.size(); i++) {
     ENVOY_LOG(debug, "static secret #{}: {}", i, secrets[i].name());
-    server.secretManager().addStaticSecret(secrets[i]);
+    THROW_IF_NOT_OK(server.secretManager().addStaticSecret(secrets[i]));
   }
 
   ENVOY_LOG(info, "loading {} cluster(s)", bootstrap.static_resources().clusters().size());

diff --git a/test/common/secret/secret_manager_impl_test.cc b/test/common/secret/secret_manager_impl_test.cc
@@ -77,7 +77,7 @@ name: "abc.com"
 )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config);
   SecretManagerPtr secret_manager(new SecretManagerImpl(config_tracker_));
-  secret_manager->addStaticSecret(secret_config);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret_config).ok());
 
   ASSERT_EQ(secret_manager->findStaticTlsCertificateProvider("undefined"), nullptr);
   ASSERT_NE(secret_manager->findStaticTlsCertificateProvider("abc.com"), nullptr);
@@ -111,11 +111,11 @@ TEST_F(SecretManagerImplTest, DuplicateStaticTlsCertificateSecret) {
     )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config);
   SecretManagerPtr secret_manager(new SecretManagerImpl(config_tracker_));
-  secret_manager->addStaticSecret(secret_config);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret_config).ok());
 
   ASSERT_NE(secret_manager->findStaticTlsCertificateProvider("abc.com"), nullptr);
-  EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret_config), EnvoyException,
-                            "Duplicate static TlsCertificate secret name abc.com");
+  EXPECT_EQ(secret_manager->addStaticSecret(secret_config).message(),
+            "Duplicate static TlsCertificate secret name abc.com");
 }
 
 // Validate that secret manager adds static certificate validation context secret successfully.
@@ -130,7 +130,7 @@ TEST_F(SecretManagerImplTest, CertificateValidationContextSecretLoadSuccess) {
       )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config);
   std::unique_ptr<SecretManager> secret_manager(new SecretManagerImpl(config_tracker_));
-  secret_manager->addStaticSecret(secret_config);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret_config).ok());
 
   ASSERT_EQ(secret_manager->findStaticCertificateValidationContextProvider("undefined"), nullptr);
   ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr);
@@ -158,11 +158,11 @@ TEST_F(SecretManagerImplTest, DuplicateStaticCertificateValidationContextSecret)
     )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config);
   SecretManagerPtr secret_manager(new SecretManagerImpl(config_tracker_));
-  secret_manager->addStaticSecret(secret_config);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret_config).ok());
 
   ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr);
-  EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret_config), EnvoyException,
-                            "Duplicate static CertificateValidationContext secret name abc.com");
+  EXPECT_EQ(secret_manager->addStaticSecret(secret_config).message(),
+            "Duplicate static CertificateValidationContext secret name abc.com");
 }
 
 // Validate that secret manager adds static STKs secret successfully.
@@ -181,7 +181,7 @@ name: "abc.com"
 
   SecretManagerPtr secret_manager(new SecretManagerImpl(config_tracker_));
 
-  secret_manager->addStaticSecret(secret_config);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret_config).ok());
 
   ASSERT_EQ(secret_manager->findStaticTlsSessionTicketKeysContextProvider("undefined"), nullptr);
   ASSERT_NE(secret_manager->findStaticTlsSessionTicketKeysContextProvider("abc.com"), nullptr);
@@ -210,11 +210,11 @@ name: "abc.com"
 
   SecretManagerPtr secret_manager(new SecretManagerImpl(config_tracker_));
 
-  secret_manager->addStaticSecret(secret_config);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret_config).ok());
 
   ASSERT_NE(secret_manager->findStaticTlsSessionTicketKeysContextProvider("abc.com"), nullptr);
-  EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret_config), EnvoyException,
-                            "Duplicate static TlsSessionTicketKeys secret name abc.com");
+  EXPECT_EQ(secret_manager->addStaticSecret(secret_config).message(),
+            "Duplicate static TlsSessionTicketKeys secret name abc.com");
 }
 
 // Validate that secret manager adds static generic secret successfully.
@@ -230,7 +230,7 @@ name: "encryption_key"
     filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/aes_128_key"
 )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret);
-  secret_manager->addStaticSecret(secret);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret).ok());
 
   ASSERT_EQ(secret_manager->findStaticGenericSecretProvider("undefined"), nullptr);
   ASSERT_NE(secret_manager->findStaticGenericSecretProvider("encryption_key"), nullptr);
@@ -255,11 +255,11 @@ name: "encryption_key"
     filename: "{{ test_rundir }}/test/extensions/transport_sockets/tls/test_data/aes_128_key"
 )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret);
-  secret_manager->addStaticSecret(secret);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret).ok());
 
   ASSERT_NE(secret_manager->findStaticGenericSecretProvider("encryption_key"), nullptr);
-  EXPECT_THROW_WITH_MESSAGE(secret_manager->addStaticSecret(secret), EnvoyException,
-                            "Duplicate static GenericSecret secret name encryption_key");
+  EXPECT_EQ(secret_manager->addStaticSecret(secret).message(),
+            "Duplicate static GenericSecret secret name encryption_key");
 }
 
 // Validate that secret manager deduplicates dynamic TLS certificate secret provider.
@@ -900,7 +900,7 @@ name: "abc.com"
 )EOF";
   envoy::extensions::transport_sockets::tls::v3::Secret tls_cert_secret;
   TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate), tls_cert_secret);
-  secret_manager->addStaticSecret(tls_cert_secret);
+  EXPECT_TRUE(secret_manager->addStaticSecret(tls_cert_secret).ok());
   TestUtility::loadFromYaml(TestEnvironment::substitute(R"EOF(
 name: "abc.com.nopassword"
 tls_certificate:
@@ -910,7 +910,7 @@ name: "abc.com.nopassword"
     inline_string: "DUMMY_INLINE_BYTES_FOR_PRIVATE_KEY"
 )EOF"),
                             tls_cert_secret);
-  secret_manager->addStaticSecret(tls_cert_secret);
+  EXPECT_TRUE(secret_manager->addStaticSecret(tls_cert_secret).ok());
   const std::string expected_config_dump = R"EOF(
 static_secrets:
 - name: "abc.com.nopassword"
@@ -972,7 +972,7 @@ name: "abc.com.validation"
 )EOF";
   envoy::extensions::transport_sockets::tls::v3::Secret validation_secret;
   TestUtility::loadFromYaml(TestEnvironment::substitute(validation_context), validation_secret);
-  secret_manager->addStaticSecret(validation_secret);
+  EXPECT_TRUE(secret_manager->addStaticSecret(validation_secret).ok());
   const std::string expected_config_dump = R"EOF(
 static_secrets:
 - name: "abc.com.validation"
@@ -1021,7 +1021,7 @@ name: "abc.com.stek"
 )EOF";
   envoy::extensions::transport_sockets::tls::v3::Secret stek_secret;
   TestUtility::loadFromYaml(TestEnvironment::substitute(stek_context), stek_secret);
-  secret_manager->addStaticSecret(stek_secret);
+  EXPECT_TRUE(secret_manager->addStaticSecret(stek_secret).ok());
   const std::string expected_config_dump = R"EOF(
 static_secrets:
 - name: "abc.com.stek"
@@ -1051,7 +1051,7 @@ name: "signing_key"
 )EOF";
   envoy::extensions::transport_sockets::tls::v3::Secret typed_secret;
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), typed_secret);
-  secret_manager->addStaticSecret(typed_secret);
+  EXPECT_TRUE(secret_manager->addStaticSecret(typed_secret).ok());
 
   const std::string expected_config_dump = R"EOF(
 static_secrets:
@@ -1147,7 +1147,7 @@ TEST_F(SecretManagerImplTest, DeprecatedSanMatcher) {
       )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config);
   std::unique_ptr<SecretManager> secret_manager(new SecretManagerImpl(config_tracker_));
-  secret_manager->addStaticSecret(secret_config);
+  EXPECT_TRUE(secret_manager->addStaticSecret(secret_config).ok());
 
   ASSERT_EQ(secret_manager->findStaticCertificateValidationContextProvider("undefined"), nullptr);
   ASSERT_NE(secret_manager->findStaticCertificateValidationContextProvider("abc.com"), nullptr);

diff --git a/test/extensions/transport_sockets/tls/context_impl_test.cc b/test/extensions/transport_sockets/tls/context_impl_test.cc
@@ -989,7 +989,7 @@ name: "abc.com"
 )EOF";
 
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config);
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
 
   envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context;
   envoy::extensions::transport_sockets::tls::v3::TlsCertificate* server_cert =
@@ -1478,7 +1478,7 @@ name: "abc.com"
       ->Add()
       ->set_name("abc.com");
 
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
   ClientContextConfigImpl client_context_config(tls_context, factory_context_);
 
   const std::string cert_pem =
@@ -1514,7 +1514,7 @@ TEST_F(ClientContextConfigImplTest, PasswordProtectedTlsCertificates) {
       ->Add()
       ->set_name("abc.com");
 
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
   ClientContextConfigImpl client_context_config(tls_context, factory_context_);
 
   const std::string cert_pem =
@@ -1554,7 +1554,7 @@ TEST_F(ClientContextConfigImplTest, PasswordProtectedPkcs12) {
       ->Add()
       ->set_name("abc.com");
 
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
   ClientContextConfigImpl client_context_config(tls_context, factory_context_);
 
   const std::string cert_p12 =
@@ -1588,7 +1588,7 @@ TEST_F(ClientContextConfigImplTest, PasswordWrongPkcs12) {
       ->Add()
       ->set_name("abc.com");
 
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
   ClientContextConfigImpl client_context_config(tls_context, factory_context_);
 
   Stats::IsolatedStoreImpl store;
@@ -1616,7 +1616,7 @@ TEST_F(ClientContextConfigImplTest, PasswordNotSuppliedPkcs12) {
       ->Add()
       ->set_name("abc.com");
 
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
   ClientContextConfigImpl client_context_config(tls_context, factory_context_);
 
   Stats::IsolatedStoreImpl store;
@@ -1647,7 +1647,7 @@ TEST_F(ClientContextConfigImplTest, PasswordNotSuppliedTlsCertificates) {
       ->Add()
       ->set_name("abc.com");
 
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
   ClientContextConfigImpl client_context_config(tls_context, factory_context_);
 
   Stats::IsolatedStoreImpl store;
@@ -1670,7 +1670,7 @@ TEST_F(ClientContextConfigImplTest, StaticCertificateValidationContext) {
   )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml),
                             tls_certificate_secret_config);
-  factory_context_.secretManager().addStaticSecret(tls_certificate_secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(tls_certificate_secret_config).ok());
   envoy::extensions::transport_sockets::tls::v3::Secret
       certificate_validation_context_secret_config;
   const std::string certificate_validation_context_yaml = R"EOF(
@@ -1681,7 +1681,9 @@ TEST_F(ClientContextConfigImplTest, StaticCertificateValidationContext) {
   )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml),
                             certificate_validation_context_secret_config);
-  factory_context_.secretManager().addStaticSecret(certificate_validation_context_secret_config);
+  EXPECT_TRUE(factory_context_.secretManager()
+                  .addStaticSecret(certificate_validation_context_secret_config)
+                  .ok());
 
   envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context;
   tls_context.mutable_common_tls_context()
@@ -1715,7 +1717,7 @@ name: "abc.com"
 
   TestUtility::loadFromYaml(TestEnvironment::substitute(yaml), secret_config);
 
-  factory_context_.secretManager().addStaticSecret(secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(secret_config).ok());
 
   envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context;
   tls_context.mutable_common_tls_context()
@@ -1742,7 +1744,7 @@ TEST_F(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) {
     )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(tls_certificate_yaml),
                             tls_certificate_secret_config);
-  factory_context_.secretManager().addStaticSecret(tls_certificate_secret_config);
+  EXPECT_TRUE(factory_context_.secretManager().addStaticSecret(tls_certificate_secret_config).ok());
   envoy::extensions::transport_sockets::tls::v3::Secret
       certificate_validation_context_secret_config;
   const std::string certificate_validation_context_yaml = R"EOF(
@@ -1753,7 +1755,9 @@ TEST_F(ClientContextConfigImplTest, MissingStaticCertificateValidationContext) {
     )EOF";
   TestUtility::loadFromYaml(TestEnvironment::substitute(certificate_validation_context_yaml),
                             certificate_validation_context_secret_config);
-  factory_context_.secretManager().addStaticSecret(certificate_validation_context_secret_config);
+  EXPECT_TRUE(factory_context_.secretManager()
+                  .addStaticSecret(certificate_validation_context_secret_config)
+                  .ok());
 
   envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext tls_context;
   tls_context.mutable_common_tls_context()

diff --git a/test/mocks/secret/mocks.h b/test/mocks/secret/mocks.h
@@ -18,7 +18,7 @@ class MockSecretManager : public SecretManager {
   MockSecretManager();
   ~MockSecretManager() override;
 
-  MOCK_METHOD(void, addStaticSecret,
+  MOCK_METHOD(absl::Status, addStaticSecret,
               (const envoy::extensions::transport_sockets::tls::v3::Secret& secret));
   MOCK_METHOD(TlsCertificateConfigProviderSharedPtr, findStaticTlsCertificateProvider,
               (const std::string& name), (const));