[mesos] 01/02: Relaxed protobuf union validation strictness.

[be...@apache.org]

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

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

commit 799a1e7415ebad9c010398fb4fd407e1066fdd31
Author: Joseph Wu <jo...@mesosphere.io>
AuthorDate: Fri Apr 26 13:51:21 2019 +0200

    Relaxed protobuf union validation strictness.
    
    As part of MESOS-6874, the master validates protobuf unions passed as
    part of an ExecutorInfo::ContainerInfo.  This prevents a task from
    specifying, for example, a ContainerInfo::MESOS, but filling
    out the docker field (which is then ignored by the agent).
    
    This validation change is actually an API change, because previously
    runnable ExecutorInfo's and TaskInfo's will now fail validation.
    This has two visible effects on clusters:
      * Agents running containers with invalid protobuf unions will not
        be able to reregister with the master.
      * Existing frameworks will not be able to re-launch the same tasks
        that were working before a Mesos master upgrade.
    
    This changes the validation to print a warning instead.  Where possible,
    the warning will provide some information to indicate which task
    or executor is sending the invalid protobuf.
    
    Review: https://reviews.apache.org/r/70546/
---
 src/common/validation.cpp             |  9 ++++++++-
 src/master/validation.cpp             |  8 +++++++-
 src/tests/master_validation_tests.cpp | 19 +++----------------
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/common/validation.cpp b/src/common/validation.cpp
index 458f225..14a8c7b 100644
--- a/src/common/validation.cpp
+++ b/src/common/validation.cpp
@@ -268,7 +268,14 @@ Option<Error> validateContainerInfo(const ContainerInfo& containerInfo)
 {
   Option<Error> unionError = protobuf::validateProtobufUnion(containerInfo);
   if (unionError.isSome()) {
-    return unionError;
+    // TODO(josephw): There is not enough information in this function to
+    // print an actionable warning. Readers of this warning will not know
+    // which container (task or executor) has this problem. Printing
+    // the entire ContainerInfo is the best we can do.
+    LOG(WARNING)
+      << "Invalid protobuf union detected in the given ContainerInfo ("
+      << containerInfo.DebugString()
+      << "): " << unionError.get();
   }
 
   foreach (const Volume& volume, containerInfo.volumes()) {
diff --git a/src/master/validation.cpp b/src/master/validation.cpp
index d7f210f..9fb0850 100644
--- a/src/master/validation.cpp
+++ b/src/master/validation.cpp
@@ -1001,7 +1001,13 @@ Option<Error> validateType(const ExecutorInfo& executor)
     Option<Error> unionError =
       protobuf::validateProtobufUnion(executor.container());
     if (unionError.isSome()) {
-      return unionError;
+      LOG(WARNING)
+        << "Executor " << executor.executor_id()
+        // NOTE: Validation of FrameworkID is done after this validation.
+        << " of framework '"
+        << (executor.has_framework_id() ? executor.framework_id().value() : "")
+        << "' has an invalid protobuf union: "
+        << unionError.get();
     }
   }
   switch (executor.type()) {
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index c98f751..1b7a827 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -3341,7 +3341,9 @@ TEST_F(TaskValidationTest, TaskMissingDockerInfo)
 
 // This test verifies that a task that has `ContainerInfo` set as MESOS
 // but has a `DockerInfo` is rejected during `TaskInfo` validation.
-TEST_F(TaskValidationTest, TaskMesosTypeWithDockerInfo)
+// TODO(josephw): Enable this regression test when we officially deprecate
+// this invalid protobuf. See MESOS-6874 and MESOS-9740.
+TEST_F(TaskValidationTest, DISABLED_TaskMesosTypeWithDockerInfo)
 {
   Try<Owned<cluster::Master>> master = StartMaster();
   ASSERT_SOME(master);
@@ -3548,21 +3550,6 @@ TEST_F(ExecutorValidationTest, ExecutorType)
         "'ExecutorInfo.container.mesos.image' must not be set for "
         "'DEFAULT' executor"));
   }
-  {
-    // Invalid protobuf union in ContainerInfo.
-    executorInfo.set_type(ExecutorInfo::CUSTOM);
-    executorInfo.mutable_command();
-    executorInfo.mutable_container()->set_type(ContainerInfo::DOCKER);
-    executorInfo.mutable_container()->mutable_mesos();
-
-    Option<Error> error = ::executor::internal::validateType(executorInfo);
-
-    EXPECT_SOME(error);
-    EXPECT_TRUE(strings::contains(
-      error->message,
-      "Protobuf union `mesos.ContainerInfo` with `Type == DOCKER` "
-      "should not have the field `mesos` set."));
-  }
 }