rgw: Address review comments, fix test cases for rgw_kms

Addressed earlier review comments on having const class members.
Resolved the make check problem.

Signed-off-by: Priya Sehgal <priya.sehgal@flipkart.com>
(cherry picked from commit 38e8f63baa73be04b0e844df2b33620ea7ee669f)
[ merge conflict in src/rgw/rgw_crypt.cc -- will address separately... -mdw ]

diff --git a/src/rgw/rgw_crypt.cc b/src/rgw/rgw_crypt.cc
index fb6ea9956b279a5b6f597c72a9ce516d99b0388d..0d75749472fbbbd5ed2cf2d94578c6a16df41f8a 100644 (file)
--- a/src/rgw/rgw_crypt.cc
+++ b/src/rgw/rgw_crypt.cc
@@ -1103,7 +1103,7 @@ int rgw_s3_prepare_encrypt(struct req_state* s,
     }
 
     /* Checking bucket attributes if SSE is enabled. Currently only supporting SSE-S3 */
-    rgw::sal::Attrs buck_attrs(s->bucket_attrs);
+    const auto& buck_attrs = s->bucket_attrs;
     auto aiter = buck_attrs.find(RGW_ATTR_BUCKET_ENCRYPTION_POLICY);
     if (aiter != buck_attrs.end()) {
       ldpp_dout(s, 5) << "Found RGW_ATTR_BUCKET_ENCRYPTION_POLICY on "

diff --git a/src/rgw/rgw_kms.cc b/src/rgw/rgw_kms.cc
index 86975c79449414118d9b4b8ae1f89e3ce30eb791..53ea9c82c5b567a5e5d5dc6f91f158c936ccadae 100644 (file)
--- a/src/rgw/rgw_kms.cc
+++ b/src/rgw/rgw_kms.cc
@@ -185,17 +185,17 @@ class SSEContext {
 protected:
   virtual ~SSEContext(){};
 public:
-  virtual std::string & backend() = 0;
-  virtual std::string & addr() = 0;
-  virtual std::string & auth() = 0;
-  virtual std::string & k_namespace() = 0;
-  virtual std::string & prefix() = 0;
+  virtual const std::string & backend() = 0;
+  virtual const std::string & addr() = 0;
+  virtual const std::string & auth() = 0;
+  virtual const std::string & k_namespace() = 0;
+  virtual const std::string & prefix() = 0;
   virtual const std::string & secret_engine() = 0;
-  virtual std::string & ssl_cacert() = 0;
-  virtual std::string & ssl_clientcert() = 0;
-  virtual std::string & ssl_clientkey() = 0;
-  virtual std::string & token_file() = 0;
-  virtual bool verify_ssl() = 0;
+  virtual const std::string & ssl_cacert() = 0;
+  virtual const std::string & ssl_clientcert() = 0;
+  virtual const std::string & ssl_clientkey() = 0;
+  virtual const std::string & token_file() = 0;
+  virtual const bool verify_ssl() = 0;
 };
 
 class VaultSecretEngine: public SecretEngine {
@@ -1085,37 +1085,37 @@ class KMSContext : public SSEContext {
 public:
   KMSContext(CephContext*_cct) : cct{_cct} {};
   ~KMSContext() override {};
-  std::string & backend() override {
+  const std::string & backend() override {
     return cct->_conf->rgw_crypt_s3_kms_backend;
   };
-  std::string & addr() override {
+  const std::string & addr() override {
     return cct->_conf->rgw_crypt_vault_addr;
   };
-  std::string & auth() override {
+  const std::string & auth() override {
     return cct->_conf->rgw_crypt_vault_auth;
   };
-  std::string & k_namespace() override {
+  const std::string & k_namespace() override {
     return cct->_conf->rgw_crypt_vault_namespace;
   };
-  std::string & prefix() override {
+  const std::string & prefix() override {
     return cct->_conf->rgw_crypt_vault_prefix;
   };
   const std::string & secret_engine() override {
     return cct->_conf->rgw_crypt_vault_secret_engine;
   };
-  std::string & ssl_cacert() override {
+  const std::string & ssl_cacert() override {
     return cct->_conf->rgw_crypt_vault_ssl_cacert;
   };
-  std::string & ssl_clientcert() override {
+  const std::string & ssl_clientcert() override {
     return cct->_conf->rgw_crypt_vault_ssl_clientcert;
   };
-  std::string & ssl_clientkey() override {
+  const std::string & ssl_clientkey() override {
     return cct->_conf->rgw_crypt_vault_ssl_clientkey;
   };
-  std::string & token_file() override {
+  const std::string & token_file() override {
     return cct->_conf->rgw_crypt_vault_token_file;
   };
-  bool verify_ssl() override {
+  const bool verify_ssl() override {
     return cct->_conf->rgw_crypt_vault_verify_ssl;
   };
 };
@@ -1126,37 +1126,37 @@ public:
   static const std::string sse_s3_secret_engine;
   SseS3Context(CephContext*_cct) : cct{_cct} {};
   ~SseS3Context(){};
-  std::string & backend() override {
+  const std::string & backend() override {
    return cct->_conf->rgw_crypt_sse_s3_backend;
   };
-  std::string & addr() override {
+  const std::string & addr() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_addr;
   };
-  std::string & auth() override {
+  const std::string & auth() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_auth;
   };
-  std::string & k_namespace() override {
+  const std::string & k_namespace() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_namespace;
   };
-  std::string & prefix() override {
+  const std::string & prefix() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_prefix;
   };
   const std::string & secret_engine() override {
     return sse_s3_secret_engine;
   };
-  std::string & ssl_cacert() override {
+  const std::string & ssl_cacert() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_ssl_cacert;
   };
-  std::string & ssl_clientcert() override {
+  const std::string & ssl_clientcert() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_ssl_clientcert;
   };
-  std::string & ssl_clientkey() override {
+  const std::string & ssl_clientkey() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_ssl_clientkey;
   };
-  std::string & token_file() override {
+  const std::string & token_file() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_token_file;
   };
-  bool verify_ssl() override {
+  const bool verify_ssl() override {
     return cct->_conf->rgw_crypt_sse_s3_vault_verify_ssl;
   };
 };
@@ -1168,7 +1168,7 @@ int reconstitute_actual_key_from_kms(const DoutPrefixProvider *dpp, CephContext
 {
   std::string key_id = get_str_attribute(attrs, RGW_ATTR_CRYPT_KEYID);
   KMSContext kctx { cct };
-  std::string &kms_backend { kctx.backend() };
+  const std::string &kms_backend { kctx.backend() };
 
   ldpp_dout(dpp, 20) << "Getting KMS encryption key for key " << key_id << dendl;
   ldpp_dout(dpp, 20) << "SSE-KMS backend is " << kms_backend << dendl;
@@ -1199,7 +1199,7 @@ int make_actual_key_from_kms(const DoutPrefixProvider *dpp, CephContext *cct,
                             std::string& actual_key)
 {
   KMSContext kctx { cct };
-  std::string &kms_backend { kctx.backend() };
+  const std::string &kms_backend { kctx.backend() };
   if (RGW_SSE_KMS_BACKEND_VAULT == kms_backend)
     return make_actual_key_from_vault(dpp, cct, kctx, attrs, actual_key);
   return reconstitute_actual_key_from_kms(dpp, cct, attrs, actual_key);
@@ -1212,7 +1212,7 @@ int reconstitute_actual_key_from_sse_s3(const DoutPrefixProvider *dpp,
 {
   std::string key_id = get_str_attribute(attrs, RGW_ATTR_CRYPT_KEYID);
   SseS3Context kctx { cct };
-  std::string &kms_backend { kctx.backend() };
+  const std::string &kms_backend { kctx.backend() };
 
   ldpp_dout(dpp, 20) << "Getting SSE-S3  encryption key for key " << key_id << dendl;
   ldpp_dout(dpp, 20) << "SSE-KMS backend is " << kms_backend << dendl;
@@ -1231,7 +1231,7 @@ int make_actual_key_from_sse_s3(const DoutPrefixProvider *dpp,
                             std::string& actual_key)
 {
   SseS3Context kctx { cct };
-  std::string kms_backend { kctx.backend() };
+  const std::string kms_backend { kctx.backend() };
   if (RGW_SSE_KMS_BACKEND_VAULT != kms_backend) {
     ldpp_dout(dpp, 0) << "ERROR: Unsupported rgw_crypt_sse_s3_backend: " << kms_backend << dendl;
     return -EINVAL;

diff --git a/src/test/rgw/test_rgw_kms.cc b/src/test/rgw/test_rgw_kms.cc
index 148f0bd32653dd6a8ef8c12f76d164cd15f260af..14319d7396706c0bc39896d617c3d94ef96ea891 100644 (file)
--- a/src/test/rgw/test_rgw_kms.cc
+++ b/src/test/rgw/test_rgw_kms.cc
@@ -18,7 +18,7 @@ using ::testing::StrEq;
 class MockTransitSecretEngine : public TransitSecretEngine {
 
 public:
-  MockTransitSecretEngine(CephContext *cct, EngineParmMap parms) : TransitSecretEngine(cct, parms){}
+  MockTransitSecretEngine(CephContext *cct, SSEContext & kctx, EngineParmMap parms) : TransitSecretEngine(cct, kctx, parms){}
 
   MOCK_METHOD(int, send_request, (const DoutPrefixProvider *dpp, const char *method, std::string_view infix, std::string_view key_id, const std::string& postdata, bufferlist &bl), (override));
 
@@ -27,7 +27,7 @@ public:
 class MockKvSecretEngine : public KvSecretEngine {
 
 public:
-  MockKvSecretEngine(CephContext *cct, EngineParmMap parms) : KvSecretEngine(cct, parms){}
+  MockKvSecretEngine(CephContext *cct, SSEContext & kctx, EngineParmMap parms) : KvSecretEngine(cct, kctx, parms){}
 
   MOCK_METHOD(int, send_request, (const DoutPrefixProvider *dpp, const char *method, std::string_view infix, std::string_view key_id, const std::string& postdata, bufferlist &bl), (override));
 
@@ -44,11 +44,12 @@ protected:
   void SetUp() override {
     EngineParmMap old_parms, kv_parms, new_parms;
     cct = (new CephContext(CEPH_ENTITY_TYPE_ANY))->get();
+    KMSContext kctx { cct };
     old_parms["compat"] = "2";
-    old_engine = new MockTransitSecretEngine(cct, std::move(old_parms));
-    kv_engine = new MockKvSecretEngine(cct, std::move(kv_parms));
+    old_engine = new MockTransitSecretEngine(cct, kctx, std::move(old_parms));
+    kv_engine = new MockKvSecretEngine(cct, kctx, std::move(kv_parms));
     new_parms["compat"] = "1";
-    transit_engine = new MockTransitSecretEngine(cct, std::move(new_parms));
+    transit_engine = new MockTransitSecretEngine(cct, kctx, std::move(new_parms));
   }
 
   void TearDown() {
@@ -64,8 +65,9 @@ TEST_F(TestSSEKMS, vault_token_file_unset)
 {
   cct->_conf.set_val("rgw_crypt_vault_auth", "token");
   EngineParmMap old_parms, kv_parms;
-  TransitSecretEngine te(cct, std::move(old_parms));
-  KvSecretEngine kv(cct, std::move(kv_parms));
+  KMSContext kctx { cct };
+  TransitSecretEngine te(cct, kctx, std::move(old_parms));
+  KvSecretEngine kv(cct, kctx, std::move(kv_parms));
   const NoDoutPrefix no_dpp(cct, 1);
 
   std::string_view key_id("my_key");
@@ -81,8 +83,9 @@ TEST_F(TestSSEKMS, non_existent_vault_token_file)
   cct->_conf.set_val("rgw_crypt_vault_auth", "token");
   cct->_conf.set_val("rgw_crypt_vault_token_file", "/nonexistent/file");
   EngineParmMap old_parms, kv_parms;
-  TransitSecretEngine te(cct, std::move(old_parms));
-  KvSecretEngine kv(cct, std::move(kv_parms));
+  KMSContext kctx { cct };
+  TransitSecretEngine te(cct, kctx, std::move(old_parms));
+  KvSecretEngine kv(cct, kctx, std::move(kv_parms));
   const NoDoutPrefix no_dpp(cct, 1);
 
   std::string_view key_id("my_key/1");