[mesos] 01/02: Introduced a function for validating a `FrameworkInfo` update.

[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 734be1e9582f516edd168fa562cce570572ecf5e
Author: Andrei Sekretenko <as...@mesosphere.io>
AuthorDate: Mon May 20 14:24:41 2019 -0400

    Introduced a function for validating a `FrameworkInfo` update.
    
    This patch moves the code validating a new `FrameworkInfo` against
    the current one into a separate function. Note that the function
    currently *does not* check `user` and `checkpoint`, and that this
    will be done in a subsequent patch as some refactoring is needed
    to make that possible.
    
    Review: https://reviews.apache.org/r/70666/
---
 src/master/master.cpp     | 29 +++++++++--------------------
 src/master/validation.cpp | 34 ++++++++++++++++++++++++++++++++++
 src/master/validation.hpp |  8 ++++++++
 3 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/src/master/master.cpp b/src/master/master.cpp
index be606e5..24bd957 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -2566,12 +2566,15 @@ Option<Error> Master::validateFrameworkSubscription(
   Option<Error> validationError =
     validation::framework::validate(frameworkInfo);
 
-  if (!validationError.isNone()){
+  if (validationError.isSome()) {
     return validationError;
   }
 
   // Validate that resubscribing framework does not attempt
-  // to change its principal.
+  // to change immutable fields of the FrameworkInfo.
+  //
+  // TODO(asekretenko): This currently does not check 'user' and 'checkpoint',
+  // which are both immutable! Update this to check these fields.
   if (frameworkInfo.has_id() && !frameworkInfo.id().value().empty()) {
     const Framework* framework = getFramework(frameworkInfo.id());
 
@@ -2580,25 +2583,11 @@ Option<Error> Master::validateFrameworkSubscription(
     // unknown at the time of re-registration. This has to be changed if we
     // decide to start storing `FrameworkInfo` messages.
     if (framework != nullptr) {
-      Option<string> oldPrincipal;
-      if (framework->info.has_principal()) {
-          oldPrincipal = framework->info.principal();
-      }
-
-      Option<string> newPrincipal;
-      if (frameworkInfo.has_principal()) {
-        newPrincipal = frameworkInfo.principal();
-      }
-
-      if (oldPrincipal != newPrincipal) {
-        LOG(WARNING)
-          << "Framework " << frameworkInfo.id() << " which had a principal '"
-          << (oldPrincipal.isSome() ? oldPrincipal.get() : "<NONE>")
-          << "' tried to (re)subscribe with a new principal '"
-          << (newPrincipal.isSome() ? newPrincipal.get() : "<NONE>")
-          << "'";
+      validationError = validation::framework::validateUpdate(
+          framework->info, frameworkInfo);
 
-        return Error("Changing framework's principal is not allowed.");
+      if (validationError.isSome()) {
+        return validationError;
       }
     }
   }
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index 764c846..39f5bfd 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -566,6 +566,40 @@ Option<Error> validate(const mesos::FrameworkInfo& frameworkInfo)
   return None();
 }
 
+
+Option<Error> validateUpdate(
+    const FrameworkInfo& oldInfo,
+    const FrameworkInfo& newInfo)
+{
+  Option<string> oldPrincipal = None();
+  if (oldInfo.has_principal()) {
+    oldPrincipal = oldInfo.principal();
+  }
+
+  Option<string> newPrincipal = None();
+  if (newInfo.has_principal()) {
+    newPrincipal = newInfo.principal();
+  }
+
+  if (oldPrincipal != newPrincipal) {
+    // We should not expose the old principal to the 'scheduler' which tries
+    // to subscribe with a known framework ID but another principal.
+    // However, it still should be possible for the people having access to
+    // the master to understand what is going on, hence the log message.
+    LOG(WARNING)
+      << "Framework " << oldInfo.id() << " which had a principal "
+      << " '" << oldPrincipal.getOrElse("<NONE>") << "'"
+      << " tried to (re)subscribe with a new principal "
+      << " '" << newPrincipal.getOrElse("<NONE>") << "'";
+
+    return Error("Changing framework's principal is not allowed.");
+  }
+
+  // TODO(asekretenko): Validate that 'user' and 'checkpoint' are not modified.
+
+  return None();
+}
+
 } // namespace framework {
 
 
diff --git a/src/master/validation.hpp b/src/master/validation.hpp
index 95638a1..97c2406 100644
--- a/src/master/validation.hpp
+++ b/src/master/validation.hpp
@@ -112,6 +112,14 @@ Option<Error> validateOfferFilters(const FrameworkInfo& frameworkInfo);
 // Validate a FrameworkInfo.
 Option<Error> validate(const mesos::FrameworkInfo& frameworkInfo);
 
+// Validate that the immutable fields of two FrameworkInfos are identical.
+//
+// TODO(asekretenko): This currently does not check 'user' and 'checkpoint',
+// which are both immutable! Update this to check these fields.
+Option<Error> validateUpdate(
+    const FrameworkInfo& oldInfo,
+    const FrameworkInfo& newInfo);
+
 } // namespace framework {