[mesos] 02/02: Fixed the race between validating and applying FrameworkInfo updates.

[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 421728fbe4b509a5660f961873605f803ab0174e
Author: Andrei Sekretenko <as...@mesosphere.io>
AuthorDate: Tue May 21 21:16:40 2019 -0400

    Fixed the race between validating and applying FrameworkInfo updates.
    
    This patch moves the FrameworkInfo update validation into the
    continuation of the SUBSCRIBE calls (after authorization). This fixes
    the race between two concurrent SUBSCRIBE calls attempting to change
    principal (see MESOS-9763) and prevents the future changes of the update
    validation from exacerbating this race.
    
    Review: https://reviews.apache.org/r/70668/
---
 src/master/master.cpp | 51 +++++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/src/master/master.cpp b/src/master/master.cpp
index 72d7453..7c9d9c3 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -2570,28 +2570,6 @@ Option<Error> Master::validateFrameworkSubscription(
     return validationError;
   }
 
-  // Validate that resubscribing framework does not attempt
-  // 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());
-
-    // TODO(asekretenko): Masters do not store `FrameworkInfo` messages in the
-    // replicated log, so it is possible that the previous principal is still
-    // unknown at the time of re-registration. This has to be changed if we
-    // decide to start storing `FrameworkInfo` messages.
-    if (framework != nullptr) {
-      validationError = validation::framework::validateUpdate(
-          framework->info, frameworkInfo);
-
-      if (validationError.isSome()) {
-        return validationError;
-      }
-    }
-  }
-
   // Check the framework's role(s) against the whitelist.
   set<string> invalidRoles;
 
@@ -2789,6 +2767,21 @@ void Master::_subscribe(
 
   CHECK_NOTNULL(framework);
 
+  // The new framework info cannot be validated against the current one
+  // before authorization because the current framework info might have been
+  // modified by another concurrent SUBSCRIBE call.
+  // Therefore, we have to do this validation here.
+  Option<Error> updateValidationError = validation::framework::validateUpdate(
+      framework->info, frameworkInfo);
+
+  if (updateValidationError.isSome()) {
+    FrameworkErrorMessage message;
+    message.set_message(updateValidationError->message);
+    http.send(message);
+    http.close();
+    return;
+  }
+
   framework->metrics.incrementCall(scheduler::Call::SUBSCRIBE);
 
   if (!framework->recovered()) {
@@ -3046,6 +3039,20 @@ void Master::_subscribe(
 
   CHECK_NOTNULL(framework);
 
+  // The new framework info cannot be validated against the current one
+  // before authorization because the current framework info might have been
+  // modified by another concurrent SUBSCRIBE call.
+  // Therefore, we have to do this validation here.
+  Option<Error> updateValidationError = validation::framework::validateUpdate(
+      framework->info, frameworkInfo);
+
+  if (updateValidationError.isSome()) {
+    FrameworkErrorMessage message;
+    message.set_message(updateValidationError->message);
+    send(from, message);
+    return;
+  }
+
   if (!framework->recovered()) {
     // The framework has previously been registered with this master;
     // it may or may not currently be connected.