[mesos] 02/05: Validated static framework offer filters.

[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 a74cf0765c30659a71e0e7a431ddc0d1ecedb8d0
Author: Benjamin Bannier <be...@mesosphere.io>
AuthorDate: Wed Feb 6 15:05:02 2019 +0100

    Validated static framework offer filters.
    
    This patch adds validation to framework-specified static offer filters.
    In particular we enforce that minimal resource quantities cannot be
    negative.
    
    Review: https://reviews.apache.org/r/69862/
---
 src/common/validation.cpp             |  38 ++++++++++++
 src/common/validation.hpp             |   2 +
 src/master/validation.cpp             |  28 +++++++++
 src/master/validation.hpp             |   5 +-
 src/tests/common_validation_tests.cpp | 105 ++++++++++++++++++++++++++++++++++
 src/tests/master_validation_tests.cpp |  26 +++++++++
 6 files changed, 201 insertions(+), 3 deletions(-)

diff --git a/src/common/validation.cpp b/src/common/validation.cpp
index 4a64fc8..b622a32 100644
--- a/src/common/validation.cpp
+++ b/src/common/validation.cpp
@@ -20,6 +20,7 @@
 
 #include <algorithm>
 #include <cctype>
+#include <cmath>
 
 #include <mesos/executor/executor.hpp>
 
@@ -614,6 +615,43 @@ Option<Error> validateExecutorCall(const mesos::executor::Call& call)
   UNREACHABLE();
 }
 
+
+Option<Error> validateOfferFilters(const OfferFilters& offerFilters)
+{
+  if (offerFilters.has_min_allocatable_resources()) {
+    foreach (
+        const OfferFilters::ResourceQuantities& quantities,
+        offerFilters.min_allocatable_resources().quantities()) {
+      if (quantities.quantities().empty()) {
+        return Error("Resource quantities must contain at least one quantity");
+      }
+
+      // Use `auto` instead of `protobuf::MapPair<strinf, Value::>` since
+      // `foreach` is a macro and does not allow angle brackets.
+      foreach (auto&& quantity, quantities.quantities()) {
+        const Value::Scalar& scalar = quantity.second;
+
+        // We do not allow negative quantities in offer filters.
+        if (scalar.value() < 0) {
+          return Error("Negative resource quantities are not allowed");
+        }
+
+        // Resource quantities cannot have NaN values.
+        if (std::isnan(scalar.value())) {
+          return Error("Resource quantities cannot be NaN");
+        }
+
+        // We do not allow infinite quantities in offer filters.
+        if (std::isinf(scalar.value())) {
+          return Error("Infinite resource quantities are not allowed");
+        }
+      }
+    }
+  }
+
+  return None();
+}
+
 } // namespace validation {
 } // namespace common {
 } // namespace internal {
diff --git a/src/common/validation.hpp b/src/common/validation.hpp
index 5929ac2..d09528f 100644
--- a/src/common/validation.hpp
+++ b/src/common/validation.hpp
@@ -69,6 +69,8 @@ Option<Error> validateCheckStatusInfo(const CheckStatusInfo& checkStatusInfo);
 
 Option<Error> validateExecutorCall(const mesos::executor::Call& call);
 
+Option<Error> validateOfferFilters(const OfferFilters& offerFilters);
+
 } // namespace validation {
 } // namespace common {
 } // namespace internal {
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index 4a699f0..565d6d4 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -518,10 +518,32 @@ Option<Error> validateFrameworkId(const FrameworkInfo& frameworkInfo)
   return common::validation::validateID(frameworkInfo.id().value());
 }
 
+
+Option<Error> validateOfferFilters(const FrameworkInfo& frameworkInfo)
+{
+  // Use `auto` in place of `protobuf::MapPair<string, Value::Scalar>`
+  // below since `foreach` is a macro and cannot contain angle brackets.
+  foreach (auto&& filter, frameworkInfo.offer_filters()) {
+    const OfferFilters& offerFilters = filter.second;
+
+    Option<Error> error =
+      common::validation::validateOfferFilters(offerFilters);
+
+    if (error.isSome()) {
+      return error;
+    }
+  }
+
+  return None();
+}
+
 } // namespace internal {
 
+
 Option<Error> validate(const mesos::FrameworkInfo& frameworkInfo)
 {
+  // TODO(jay_guo): This currently only validates the role(s),
+  // framework ID and offer filters, validate more fields!
   Option<Error> error = internal::validateRoles(frameworkInfo);
 
   if (error.isSome()) {
@@ -534,6 +556,12 @@ Option<Error> validate(const mesos::FrameworkInfo& frameworkInfo)
     return error;
   }
 
+  error = internal::validateOfferFilters(frameworkInfo);
+
+  if (error.isSome()) {
+    return error;
+  }
+
   return None();
 }
 
diff --git a/src/master/validation.hpp b/src/master/validation.hpp
index a5bdff6..71748c1 100644
--- a/src/master/validation.hpp
+++ b/src/master/validation.hpp
@@ -105,12 +105,11 @@ Option<Error> validateRoles(const mesos::FrameworkInfo& frameworkInfo);
 
 Option<Error> validateFrameworkId(const mesos::FrameworkInfo& frameworkInfo);
 
+Option<Error> validateOfferFilters(const FrameworkInfo& frameworkInfo);
+
 } // namespace internal {
 
 // Validate a FrameworkInfo.
-//
-// TODO(jay_guo): This currently only validates
-// the role(s), validate more fields!
 Option<Error> validate(const mesos::FrameworkInfo& frameworkInfo);
 
 } // namespace framework {
diff --git a/src/tests/common_validation_tests.cpp b/src/tests/common_validation_tests.cpp
index 97466ab..214b993 100644
--- a/src/tests/common_validation_tests.cpp
+++ b/src/tests/common_validation_tests.cpp
@@ -14,6 +14,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <limits>
+
 #include <stout/gtest.hpp>
 
 #include "common/validation.hpp"
@@ -81,6 +83,109 @@ TEST(CommonValidationTest, Volume)
   }
 }
 
+
+TEST(CommonValidationTest, OfferFilters)
+{
+  // Offer filters can be empty.
+  EXPECT_NONE(common::validation::validateOfferFilters(OfferFilters()));
+
+  {
+    // Offer filters can have empty minimal allocatable resources to
+    // communicate that any resource should be considered allocatable.
+    OfferFilters offerFilters;
+    offerFilters.mutable_min_allocatable_resources();
+    EXPECT_NONE(common::validation::validateOfferFilters(offerFilters));
+  }
+
+  {
+    // Minimal allocatable resources cannot have empty quantities.
+    OfferFilters offerFilters;
+    offerFilters.mutable_min_allocatable_resources()->add_quantities();
+    EXPECT_SOME_EQ(
+        Error("Resource quantities must contain at least one quantity"),
+        common::validation::validateOfferFilters(offerFilters));
+  }
+
+  {
+    // A resource quantity cannot be negative.
+    Value::Scalar scalar;
+    scalar.set_value(-2);
+
+    OfferFilters offerFilters;
+    offerFilters.mutable_min_allocatable_resources()
+      ->add_quantities()
+      ->mutable_quantities()
+      ->insert({"cpus", scalar});
+    EXPECT_SOME_EQ(
+        Error("Negative resource quantities are not allowed"),
+        common::validation::validateOfferFilters(offerFilters));
+  }
+
+  {
+    // A resource quantity must be a number.
+    static_assert(
+        std::numeric_limits<double>::has_quiet_NaN,
+        "Expected double to have a quiet NaN representation");
+
+    Value::Scalar scalar;
+    scalar.set_value(std::numeric_limits<double>::quiet_NaN());
+
+    OfferFilters offerFilters;
+    offerFilters.mutable_min_allocatable_resources()
+      ->add_quantities()
+      ->mutable_quantities()
+      ->insert({"cpus", scalar});
+    EXPECT_SOME_EQ(
+        Error("Resource quantities cannot be NaN"),
+        common::validation::validateOfferFilters(offerFilters));
+  }
+
+  {
+    // A resource quantity cannot be (positive) infinite.
+    static_assert(
+        std::numeric_limits<double>::has_infinity,
+        "Expected double to have a representation of infinity");
+
+    Value::Scalar scalar;
+    scalar.set_value(std::numeric_limits<double>::infinity());
+
+    OfferFilters offerFilters;
+    offerFilters.mutable_min_allocatable_resources()
+      ->add_quantities()
+      ->mutable_quantities()
+      ->insert({"cpus", scalar});
+    EXPECT_SOME_EQ(
+        Error("Infinite resource quantities are not allowed"),
+        common::validation::validateOfferFilters(offerFilters));
+  }
+
+  {
+    // A resource quantity can be zero.
+    Value::Scalar scalar;
+    scalar.set_value(0);
+
+    OfferFilters offerFilters;
+    offerFilters.mutable_min_allocatable_resources()
+      ->add_quantities()
+      ->mutable_quantities()
+      ->insert({"cpus", scalar});
+    EXPECT_NONE(common::validation::validateOfferFilters(offerFilters));
+  }
+
+  {
+    // A resource quantity can be positive.
+    Value::Scalar scalar;
+    scalar.set_value(2);
+
+    OfferFilters offerFilters;
+    offerFilters.mutable_min_allocatable_resources()
+      ->add_quantities()
+      ->mutable_quantities()
+      ->insert({"cpus", scalar});
+    EXPECT_NONE(common::validation::validateOfferFilters(offerFilters));
+  }
+}
+
 } // namespace tests {
 } // namespace internal {
 } // namespace mesos {
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index b34e88f..9568d39 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -4718,6 +4718,32 @@ TEST_F(FrameworkInfoValidationTest, ValidateFrameworkID)
 }
 
 
+// This test validates that framework cannot configure negative
+// resources in their minimal allocatable resources offer filters.
+TEST_F(FrameworkInfoValidationTest, ValidateOfferFilters)
+{
+  Value::Scalar scalar;
+  scalar.set_value(-2);
+
+  OfferFilters offerFilters;
+  offerFilters.mutable_min_allocatable_resources()
+    ->add_quantities()
+    ->mutable_quantities()
+    ->insert({"cpus", scalar});
+
+  FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
+
+  ASSERT_FALSE(frameworkInfo.roles().empty());
+
+  frameworkInfo.mutable_offer_filters()->insert(
+      {frameworkInfo.roles(0), offerFilters});
+
+  EXPECT_SOME_EQ(
+      Error("Negative resource quantities are not allowed"),
+      framework::validate(frameworkInfo));
+}
+
+
 // This test ensures that ia framework cannot use the
 // `FrameworkInfo.roles` field without providing the
 // MULTI_ROLE capability.