[mesos] 03/03: Validated provider ID use in some resource provider calls.

[bb...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

bbannier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git

commit ec2b6e0ce15a527c155cc7348dd21a338244b145
Author: Benjamin Bannier <bb...@apache.org>
AuthorDate: Sat Aug 24 22:39:07 2019 +0200

    Validated provider ID use in some resource provider calls.
    
    For some calls we expect resource providers to set provider IDs with the
    calls. While the resource provider manager has always asserted that the
    calls were correct we never validated this.
    
    With this patch we perform additional validation for calls taking a
    `ResourceProviderInfo` into account. We add both unit tests for the
    validation code and an integration test confirming that the validation
    is actually triggered.
    
    Review: https://reviews.apache.org/r/71341/
---
 src/resource_provider/manager.cpp                | 12 +++--
 src/resource_provider/validation.cpp             | 23 +++++++++
 src/tests/resource_provider_manager_tests.cpp    | 64 ++++++++++++++++++++++++
 src/tests/resource_provider_validation_tests.cpp | 45 +++++++++++++++++
 4 files changed, 141 insertions(+), 3 deletions(-)

diff --git a/src/resource_provider/manager.cpp b/src/resource_provider/manager.cpp
index 773ccd3..427ce70 100644
--- a/src/resource_provider/manager.cpp
+++ b/src/resource_provider/manager.cpp
@@ -405,9 +405,15 @@ Future<http::Response> ResourceProviderManagerProcess::api(
           return BadRequest("Resource provider is not subscribed");
         }
 
-        ResourceProvider* resourceProvider =
-          this->resourceProviders.subscribed.at(call.resource_provider_id())
-            .get();
+        ResourceProvider* resourceProvider = CHECK_NOTNULL(
+            this->resourceProviders.subscribed.at(call.resource_provider_id())
+              .get());
+
+        // Perform additional validation with the now known provider info.
+        error = validate(call, resourceProvider->info);
+        if (error.isSome()) {
+          return BadRequest("Inconsistent call: " + error->message);
+        }
 
         // This isn't a `SUBSCRIBE` call, so the request should include a stream
         // ID.
diff --git a/src/resource_provider/validation.cpp b/src/resource_provider/validation.cpp
index 3688113..bff601c 100644
--- a/src/resource_provider/validation.cpp
+++ b/src/resource_provider/validation.cpp
@@ -16,6 +16,9 @@
 
 #include "resource_provider/validation.hpp"
 
+#include <mesos/type_utils.hpp>
+
+#include <stout/foreach.hpp>
 #include <stout/none.hpp>
 #include <stout/unreachable.hpp>
 
@@ -69,6 +72,17 @@ Option<Error> validate(
         return Error("Expecting 'update_operation_status' to be present");
       }
 
+      if (resourceProviderInfo.isSome() && resourceProviderInfo->has_id()) {
+        const Call::UpdateOperationStatus& updateOperationStatus =
+          call.update_operation_status();
+        if (!updateOperationStatus.status().has_resource_provider_id() ||
+            updateOperationStatus.status().resource_provider_id() !=
+              resourceProviderInfo->id()) {
+          return Error(
+              "Inconsistent resource provider ID in 'update_operation_status'");
+        }
+      }
+
       return None();
     }
 
@@ -77,6 +91,15 @@ Option<Error> validate(
         return Error("Expecting 'update_state' to be present");
       }
 
+      if (resourceProviderInfo.isSome() && resourceProviderInfo->has_id()) {
+        foreach (const Resource& resource, call.update_state().resources()) {
+          if (!resource.has_provider_id() ||
+              resource.provider_id() != resourceProviderInfo->id()) {
+            return Error("Inconsistent resource provider ID in 'update_state'");
+          }
+        }
+      }
+
       return None();
     }
 
diff --git a/src/tests/resource_provider_manager_tests.cpp b/src/tests/resource_provider_manager_tests.cpp
index bcf6a03..84ec70b 100644
--- a/src/tests/resource_provider_manager_tests.cpp
+++ b/src/tests/resource_provider_manager_tests.cpp
@@ -46,6 +46,7 @@
 #include <stout/recordio.hpp>
 #include <stout/result.hpp>
 #include <stout/stringify.hpp>
+#include <stout/strings.hpp>
 #include <stout/try.hpp>
 
 #include "common/http.hpp"
@@ -198,6 +199,69 @@ TEST_P(ResourceProviderManagerHttpApiTest, MalformedContent)
 }
 
 
+// Confirm that the resource provider manager performs call validation
+// taking into account the resource provider info of the caller. The
+// validation is tested in detail in `resource_provider_validation_tests.cpp`.
+TEST_F(ResourceProviderManagerHttpApiTest, CallProviderValidation)
+{
+  Clock::pause();
+
+  // Start master and agent.
+  Try<Owned<cluster::Master>> master = StartMaster();
+  ASSERT_SOME(master);
+
+  Owned<MasterDetector> detector = master.get()->createDetector();
+
+  Future<UpdateSlaveMessage> updateSlaveMessage =
+    FUTURE_PROTOBUF(UpdateSlaveMessage(), _, _);
+
+  slave::Flags slaveFlags = CreateSlaveFlags();
+
+  Try<Owned<cluster::Slave>> agent = StartSlave(detector.get(), slaveFlags);
+  ASSERT_SOME(agent);
+
+  Clock::advance(slaveFlags.registration_backoff_factor);
+  Clock::settle();
+  AWAIT_READY(updateSlaveMessage);
+
+  mesos::v1::ResourceProviderInfo resourceProviderInfo;
+  resourceProviderInfo.set_type("org.apache.mesos.rp.test");
+  resourceProviderInfo.set_name("test");
+
+  v1::TestResourceProvider resourceProvider(resourceProviderInfo);
+
+  // Start and register a resource provider.
+  Owned<EndpointDetector> endpointDetector(
+      resource_provider::createEndpointDetector(agent.get()->pid));
+
+  Future<Event::Subscribed> subscribed;
+  EXPECT_CALL(*resourceProvider.process, subscribed(_))
+    .WillOnce(FutureArg<0>(&subscribed));
+
+  resourceProvider.start(std::move(endpointDetector), ContentType::PROTOBUF);
+
+  AWAIT_READY(subscribed);
+
+  Call call;
+  call.set_type(Call::UPDATE_STATE);
+  call.mutable_resource_provider_id()->CopyFrom(subscribed->provider_id());
+
+  Call::UpdateState* updateState = call.mutable_update_state();
+  updateState->mutable_resource_version_uuid()->set_value(
+      id::UUID::random().toBytes());
+
+  // Add a single resource with unset `provider_id`. This will be
+  // caught by call validation in the resource provider manager.
+  v1::Resource* resource = updateState->add_resources();
+  resource->CopyFrom(*v1::Resources::parse("disk", "32", "*"));
+
+  Future<Nothing> send = resourceProvider.send(call);
+  AWAIT_FAILED(send);
+  EXPECT_TRUE(strings::contains(send.failure(), BadRequest().status))
+    << send.failure();
+}
+
+
 TEST_P(ResourceProviderManagerHttpApiTest, UnsupportedContentMediaType)
 {
   Call call;
diff --git a/src/tests/resource_provider_validation_tests.cpp b/src/tests/resource_provider_validation_tests.cpp
index 5ff7894..c719fc5 100644
--- a/src/tests/resource_provider_validation_tests.cpp
+++ b/src/tests/resource_provider_validation_tests.cpp
@@ -84,6 +84,23 @@ TEST(ResourceProviderCallValidationTest, UpdateOperationStatus)
 
   error = call::validate(call, None());
   EXPECT_NONE(error);
+
+  // If given some `ResourceProviderInfo` the call is rejected
+  // since above status had no resource provider ID set.
+  ResourceProviderInfo resourceProviderInfo;
+  resourceProviderInfo.set_type("org.apache.mesos.rp.test");
+  resourceProviderInfo.set_name("test");
+  resourceProviderInfo.mutable_id()->set_value("test_id");
+
+  error = call::validate(call, resourceProviderInfo);
+  EXPECT_SOME(error);
+
+  // If the provider of the operation state is consistent
+  // with the given provider info the call validates.
+  status->mutable_resource_provider_id()->CopyFrom(resourceProviderInfo.id());
+
+  error = call::validate(call, resourceProviderInfo);
+  EXPECT_NONE(error);
 }
 
 
@@ -109,6 +126,34 @@ TEST(ResourceProviderCallValidationTest, UpdateState)
 
   error = call::validate(call, None());
   EXPECT_NONE(error);
+
+  // If given some `ResourceProviderInfo` the call still validates.
+  ResourceProviderInfo resourceProviderInfo;
+  resourceProviderInfo.set_type("org.apache.mesos.rp.test");
+  resourceProviderInfo.set_name("test");
+  resourceProviderInfo.mutable_id()->set_value("test_id");
+
+  error = call::validate(call, resourceProviderInfo);
+  EXPECT_NONE(error);
+
+  // Adding resource without `provider_id` if given a provider is not rejected.
+  Resource* resource = updateState->add_resources();
+  resource->CopyFrom(*Resources::parse("disk", "1024", "*"));
+
+  error = call::validate(call, resourceProviderInfo);
+  EXPECT_SOME(error);
+
+  // A resource with a `provider_id` not matching the provider is rejected.
+  resource->mutable_provider_id()->set_value("another_id");
+
+  error = call::validate(call, resourceProviderInfo);
+  EXPECT_SOME(error);
+
+  // The call is accepted if the resource's `provider_id` matches the provider.
+  resource->mutable_provider_id()->CopyFrom(resourceProviderInfo.id());
+
+  error = call::validate(call, resourceProviderInfo);
+  EXPECT_NONE(error);
 }
 
 } // namespace tests {