[mesos] 03/04: Added validation of QuotaRequest.

[bm...@apache.org]

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

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

commit 4189143ab0ea778bf59b85fc883ec1cda7482e95
Author: Benjamin Mahler <bm...@apache.org>
AuthorDate: Mon Feb 5 18:56:43 2018 -0800

    Added validation of QuotaRequest.
    
    This obviates the need for the old-style validation of quotaInfo,
    but leaves the latter in order to keep the v0 code path untouched.
    A TODO has been added to remove the old validation and replace it
    with the one in this patch.
    
    Review: https://reviews.apache.org/r/65784
---
 src/master/quota.cpp | 104 +++++++++++++++++++++++++++++++++++++++++++++++++++
 src/master/quota.hpp |  26 +++++++++++++
 2 files changed, 130 insertions(+)

diff --git a/src/master/quota.cpp b/src/master/quota.cpp
index bef18c0..671626c 100644
--- a/src/master/quota.cpp
+++ b/src/master/quota.cpp
@@ -21,9 +21,11 @@
 #include <mesos/mesos.hpp>
 #include <mesos/resources.hpp>
 #include <mesos/roles.hpp>
+#include <mesos/values.hpp>
 
 #include <stout/error.hpp>
 #include <stout/option.hpp>
+#include <stout/set.hpp>
 
 #include "common/resources_utils.hpp"
 
@@ -188,6 +190,108 @@ Option<Error> quotaInfo(const QuotaInfo& quotaInfo)
 
 } // namespace validation {
 
+Option<Error> validate(const QuotaRequest& request)
+{
+  if (!request.has_role()) {
+    return Error("'QuotaRequest.role' must be set");
+  }
+
+  // Check the provided role is valid.
+  Option<Error> error = roles::validate(request.role());
+  if (error.isSome()) {
+    return Error("Invalid 'QuotaRequest.role': " + error->message);
+  }
+
+  // Disallow quota for '*' role.
+  //
+  // TODO(alexr): Consider allowing setting quota for '*' role,
+  // see MESOS-3938.
+  if (request.role() == "*") {
+    return Error("Invalid 'QuotaRequest.role': setting quota for the"
+                 " default '*' role is not supported");
+  }
+
+  // Define a helper for use against guarantee and limit.
+  auto validateQuotaResources = [](
+      const RepeatedPtrField<Resource>& resources) -> Option<Error> {
+    hashset<string> names;
+
+    // Check that each resource does not contain fields
+    // that are disallowed for quota resources.
+    foreach (const Resource& resource, resources) {
+      if (resource.has_reservation()) {
+        return Error("'Resource.reservation' must not be set");
+      }
+
+      if (resource.reservations_size() > 0) {
+        return Error("'Resource.reservations' must not be set");
+      }
+
+      if (resource.has_disk()) {
+        return Error("'Resource.disk' must not be set");
+      }
+
+      if (resource.has_revocable()) {
+        return Error("'Resource.revocable' must not be set");
+      }
+
+      if (resource.type() != Value::SCALAR) {
+        return Error("'Resource.type' must be 'SCALAR'");
+      }
+
+      if (resource.has_shared()) {
+        return Error("'Resource.shared' must not be set");
+      }
+
+      if (resource.has_provider_id()) {
+        return Error("'Resource.provider_id' must not be set");
+      }
+
+      // Check that resource names do not repeat.
+      if (names.contains(resource.name())) {
+        return Error("Duplicate '" + resource.name() + "'; only a single"
+                     " entry for each resource is supported");
+      }
+
+      names.insert(resource.name());
+    }
+
+    // Finally, ensure the resources are valid.
+    return Resources::validate(resources);
+  };
+
+  error = validateQuotaResources(request.guarantee());
+  if (error.isSome()) {
+    return Error("Invalid 'QuotaRequest.guarantee': " + error->message);
+  }
+
+  error = validateQuotaResources(request.limit());
+  if (error.isSome()) {
+    return Error("Invalid 'QuotaRequest.limit': " + error->message);
+  }
+
+  // Validate that guarantee <= limit.
+  Resources guarantee = request.guarantee();
+  Resources limit = request.limit();
+
+  // This needs to be checked on a per-resource basis for those
+  // resources that are specified in both the guarantee and limit.
+  foreach (const string& name, guarantee.names() & limit.names()) {
+    Value::Scalar guaranteeScalar =
+      CHECK_NOTNONE(guarantee.get<Value::Scalar>(name));
+    Value::Scalar limitScalar =
+      CHECK_NOTNONE(limit.get<Value::Scalar>(name));
+
+    if (!(guaranteeScalar <= limitScalar)) {
+      return Error("'QuotaRequest.guarantee' (" + stringify(guarantee) + ")"
+                     " is not contained within the 'QuotaRequest.limit'"
+                     " (" + stringify(limit) + ")");
+    }
+  }
+
+  return None();
+}
+
 } // namespace quota {
 } // namespace master {
 } // namespace internal {
diff --git a/src/master/quota.hpp b/src/master/quota.hpp
index 40447bc..5cd2bb0 100644
--- a/src/master/quota.hpp
+++ b/src/master/quota.hpp
@@ -103,10 +103,36 @@ namespace validation {
 //   - Irrelevant fields in `Resources` are not set
 //     (e.g. `ReservationInfo`).
 //   - Request only contains scalar `Resources`.
+//
+// TODO(bmahler): Remove this in favor of `validate` below. This
+// requires some new logic outside of this function to prevent
+// users from setting `limit` explicitly in the old API and
+// setting the `limit` implicitly for users of the old API before
+// calling into this.
 Option<Error> quotaInfo(const mesos::quota::QuotaInfo& quotaInfo);
 
 } // namespace validation {
 
+/**
+ * A `QuotaRequest` is valid if the following conditions are met:
+ *
+ *   (1) The request has a valid non-"*" role.
+ *
+ *   (2) The guarantee and limit contain only valid non-revocable
+ *       scalar resources without reservations, disk info, and
+ *       only 1 entry for each resource name.
+ *
+ *   (3) If both guarantee and limit are set for a particular
+ *       resource, then guarantee <= limit for that resource.
+ *
+ * TODO(bmahler): Remove the old validation function in favor of
+ * this one. This requires some new logic outside of this function
+ * to prevent users from setting `limit` explicitly in the old API
+ * and setting the `limit` implicitly for users of the old API before
+ * calling into this.
+ */
+Option<Error> validate(const mesos::quota::QuotaRequest& request);
+
 } // namespace quota {
 } // namespace master {
 } // namespace internal {