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 93aca1eb0efcec941e19e976f683a35ecd9a840b Author: Andrei Sekretenko <[email protected]> AuthorDate: Tue Mar 19 18:55:55 2019 +0100 Validated the match between Type and *Infos in the ContainerInfo. To avoid situations like described in [MESOS-6874](https://issues.apache.org/jira/browse/MESOS-6874), I'm clarifying the validity criteria for the `ContainerInfo` protobuf and adding validation of this protobuf into the ComposingContainerizer. There basically has been three ways to validate this protobuf: 1. Hardcoded check 2. Labelling all the *Infos with protobuf options and use some protobuf reflection 3. Require an exact match between the Type enum members and the *Info fields, and use a bit more reflection. Hardcoded check will not "scale well" in case more container types are added. Protobuf options are not a good option in our case (mesos.proto constitutes a part of an external interface and adding an option there would require to register it globally). That's why I opted for the third option. I'm also adding a test for this validation: passing ContainerInfo with Type==MESOS and docker set should make the ComposingContainerizer fail. After adding this validation I discovered an invalid ContainerInfo used in the test DefaultExecutorWithDockerImageCommandHealthCheck. This PR contains a fix for this test. Testing done: - run make check after adding the test only: test expectedly fails - run make check after adding validation code and fixing the problem with the health check test: tests pass. Reviewed at https://github.com/apache/mesos/pull/326. This closes #326 --- include/mesos/mesos.proto | 6 ++-- src/common/protobuf_utils.cpp | 56 +++++++++++++++++++++++++++++ src/common/protobuf_utils.hpp | 45 ++++++++++++++++++++++++ src/common/validation.cpp | 7 ++++ src/master/validation.cpp | 7 ++++ src/tests/health_check_tests.cpp | 22 ++++++++++-- src/tests/master_validation_tests.cpp | 66 +++++++++++++++++++++++++++++++++++ 7 files changed, 204 insertions(+), 5 deletions(-) diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto index 5f6e2c4..35e10f1 100644 --- a/include/mesos/mesos.proto +++ b/include/mesos/mesos.proto @@ -3328,6 +3328,8 @@ message TTYInfo { */ message ContainerInfo { // All container implementation types. + // For each type there should be a field in the ContainerInfo itself + // with exactly matching name in lowercase. enum Type { DOCKER = 1; MESOS = 2; @@ -3381,8 +3383,8 @@ message ContainerInfo { repeated Volume volumes = 2; optional string hostname = 4; - // Only one of the following *Info messages should be set to match - // the type. + // At most one of the following *Info messages should be set to match + // the type, i.e. the "protobuf union" in ContainerInfo should be valid. optional DockerInfo docker = 3; optional MesosInfo mesos = 5; diff --git a/src/common/protobuf_utils.cpp b/src/common/protobuf_utils.cpp index a10d36e..8b252cb 100644 --- a/src/common/protobuf_utils.cpp +++ b/src/common/protobuf_utils.cpp @@ -77,6 +77,62 @@ namespace mesos { namespace internal { namespace protobuf { +// If `descriptor` is not a descriptor of a protobuf union, +// this constructor will abort the process. +UnionValidator::UnionValidator(const google::protobuf::Descriptor* descriptor) +{ + const auto* typeFieldDescriptor = descriptor->FindFieldByName("type"); + CHECK_NOTNULL(typeFieldDescriptor); + + typeDescriptor_ = typeFieldDescriptor->enum_type(); + CHECK_NOTNULL(typeDescriptor_); + + const auto* unknownTypeValueDescriptor = + typeDescriptor_->FindValueByNumber(0); + if (unknownTypeValueDescriptor != nullptr) { + CHECK_EQ(unknownTypeValueDescriptor->name(), "UNKNOWN"); + } + + for (int index = 0; index < typeDescriptor_->value_count(); index++) { + const auto* typeValueDescriptor = typeDescriptor_->value(index); + if (typeValueDescriptor->number() == 0) { + // We are skipping the "UNKNOWN" value of the enum. + continue; + } + + const auto* fieldDescriptor = + descriptor->FindFieldByName(strings::lower(typeValueDescriptor->name())); + + CHECK_NOTNULL(fieldDescriptor); + unionFieldDescriptors_.emplace_back( + typeValueDescriptor->number(), fieldDescriptor); + } +} + + +Option<Error> UnionValidator::validate( + const int messageTypeNumber, const google::protobuf::Message& message) const +{ + const auto* reflection = message.GetReflection(); + for (const auto& item : unionFieldDescriptors_) { + const auto typeNumber = item.first; + const auto* fieldDescriptor = item.second; + if ( + messageTypeNumber != typeNumber && + reflection->HasField(message, fieldDescriptor)) { + const auto* descr = typeDescriptor_->FindValueByNumber(messageTypeNumber); + return Error( + "Protobuf union `" + message.GetDescriptor()->full_name() + + "` with `Type == " + + (descr == nullptr ? string("<UNKNOWN>") : descr->name()) + + "` should not have the field `" + + fieldDescriptor->name() + "` set."); + } + } + return None(); +} + + bool frameworkHasCapability( const FrameworkInfo& framework, FrameworkInfo::Capability::Type capability) diff --git a/src/common/protobuf_utils.hpp b/src/common/protobuf_utils.hpp index 1d3dd0b..273ae27 100644 --- a/src/common/protobuf_utils.hpp +++ b/src/common/protobuf_utils.hpp @@ -64,6 +64,51 @@ struct Slave; namespace protobuf { +// Internal helper class for protobuf union validation. +class UnionValidator +{ +public: + UnionValidator(const google::protobuf::Descriptor*); + Option<Error> validate( + const int messageTypeNumber, const google::protobuf::Message&) const; + +private: + std::vector<std::pair<int, const google::protobuf::FieldDescriptor*>> + unionFieldDescriptors_; + const google::protobuf::EnumDescriptor* typeDescriptor_; +}; + +// +// A message is a "protobuf union" if, and only if, +// the following requirements are satisfied: +// 1. It has a required field named `type` of an enum type. +// 2. A member of this enum with a number (not index!) of 0 +// either is named "UNKNOWN" or does not exist. +// 3. For each other member of this enum there is an optional field +// in the message with an exactly matching name in lowercase. +// (Being or not being a protobuf uinion depends on a message declaration only.) +// +// A "protobuf union" is valid if, and only if, all the message fields +// which correspond to members of this enum that do not matching the value +// of the `type` field, are not set. +// (Validity of the protobuf union depends on the message contents. +// Note that it does not depend on whether the matching field is set or not.) +// +// NOTE: If possible, oneof should be used in the new messages instead +// of the "protobuf union". +// +// This function returns None if the protobuf union is valid +// and Error otherwise. +// In case the ProtobufUnion is not a protobuf union, +// this function will abort the process on the first use. +template <class ProtobufUnion> +Option<Error> validateProtobufUnion(const ProtobufUnion& message) +{ + static const UnionValidator validator(ProtobufUnion::descriptor()); + return validator.validate(message.type(), message); +} + + bool frameworkHasCapability( const FrameworkInfo& framework, FrameworkInfo::Capability::Type capability); diff --git a/src/common/validation.cpp b/src/common/validation.cpp index b42053e..458f225 100644 --- a/src/common/validation.cpp +++ b/src/common/validation.cpp @@ -32,6 +32,8 @@ #include <stout/os/constants.hpp> +#include "common/protobuf_utils.hpp" + using std::string; using google::protobuf::RepeatedPtrField; @@ -264,6 +266,11 @@ Option<Error> validateVolume(const Volume& volume) Option<Error> validateContainerInfo(const ContainerInfo& containerInfo) { + Option<Error> unionError = protobuf::validateProtobufUnion(containerInfo); + if (unionError.isSome()) { + return unionError; + } + foreach (const Volume& volume, containerInfo.volumes()) { Option<Error> error = validateVolume(volume); if (error.isSome()) { diff --git a/src/master/validation.cpp b/src/master/validation.cpp index 565d6d4..c5f0d78 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -979,6 +979,13 @@ Option<Error> validateExecutorID(const ExecutorInfo& executor) Option<Error> validateType(const ExecutorInfo& executor) { + if (executor.has_container()) { + Option<Error> unionError = + protobuf::validateProtobufUnion(executor.container()); + if (unionError.isSome()) { + return unionError; + } + } switch (executor.type()) { case ExecutorInfo::DEFAULT: if (executor.has_command()) { diff --git a/src/tests/health_check_tests.cpp b/src/tests/health_check_tests.cpp index 40ba33e..062357e 100644 --- a/src/tests/health_check_tests.cpp +++ b/src/tests/health_check_tests.cpp @@ -1819,11 +1819,11 @@ TEST_F_TEMP_DISABLED_ON_WINDOWS( } } - +#ifdef __linux__ // Tests a healthy docker task via CMD health checks using the // DefaultExecutor. TEST_F_TEMP_DISABLED_ON_WINDOWS( - HealthCheckTest, DefaultExecutorWithDockerImageCommandHealthCheck) + HealthCheckTest, ROOT_DefaultExecutorWithDockerImageCommandHealthCheck) { Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -1839,6 +1839,17 @@ TEST_F_TEMP_DISABLED_ON_WINDOWS( flags.acls = acls; #endif // USE_SSL_SOCKET + const string directory = path::join(os::getcwd(), "archives"); + + Future<Nothing> testImage = DockerArchive::create(directory, "alpine"); + AWAIT_READY(testImage); + ASSERT_TRUE(os::exists(path::join(directory, "alpine.tar"))); + + flags.isolation = "docker/runtime,filesystem/linux"; + flags.image_providers = "docker"; + flags.docker_registry = directory; + flags.docker_store_dir = path::join(os::getcwd(), "store"); + Fetcher fetcher(flags); // We have to explicitly create a `Containerizer` in non-local mode, @@ -1889,9 +1900,13 @@ TEST_F_TEMP_DISABLED_ON_WINDOWS( TaskInfo task = createTask(offers->front(), "sleep 120"); // TODO(tnachen): Use local image to test if possible. + Image image; + image.set_type(Image::DOCKER); + image.mutable_docker()->set_name("alpine"); + ContainerInfo containerInfo; containerInfo.set_type(ContainerInfo::MESOS); - containerInfo.mutable_docker()->set_image("alpine"); + containerInfo.mutable_mesos()->mutable_image()->CopyFrom(image); task.mutable_container()->CopyFrom(containerInfo); @@ -1954,6 +1969,7 @@ TEST_F_TEMP_DISABLED_ON_WINDOWS( AWAIT_READY(containerizer->wait(containerId)); } } +#endif // __linux__ // This test verifies that the debug container launched by the command health diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp index 19ec7a7..7f3751a 100644 --- a/src/tests/master_validation_tests.cpp +++ b/src/tests/master_validation_tests.cpp @@ -3231,6 +3231,57 @@ TEST_F(TaskValidationTest, TaskMissingDockerInfo) driver.join(); } +// 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) +{ + Try<Owned<cluster::Master>> master = StartMaster(); + ASSERT_SOME(master); + + Owned<MasterDetector> detector = master.get()->createDetector(); + Try<Owned<cluster::Slave>> slave = StartSlave(detector.get()); + ASSERT_SOME(slave); + + MockScheduler sched; + MesosSchedulerDriver driver( + &sched, DEFAULT_FRAMEWORK_INFO, master.get()->pid, DEFAULT_CREDENTIAL); + + EXPECT_CALL(sched, registered(&driver, _, _)); + + Future<vector<Offer>> offers; + EXPECT_CALL(sched, resourceOffers(&driver, _)) + .WillOnce(FutureArg<1>(&offers)) + .WillRepeatedly(Return()); // Ignore subsequent offers. + + driver.start(); + + AWAIT_READY(offers); + ASSERT_FALSE(offers->empty()); + + Future<TaskStatus> status; + EXPECT_CALL(sched, statusUpdate(&driver, _)) + .WillOnce(FutureArg<1>(&status)); + + // Create an invalid task that has `ContainerInfo` set + // as MESOS and has a `DockerInfo` set. + TaskInfo task = createTask(offers.get()[0], "exit 0"); + task.mutable_container()->set_type(ContainerInfo::MESOS); + task.mutable_container()->mutable_docker()->set_image("alpine"); + + driver.launchTasks(offers.get()[0].id(), {task}); + + AWAIT_READY(status); + EXPECT_EQ(TASK_ERROR, status->state()); + EXPECT_EQ( + "Task's `ContainerInfo` is invalid: " + "Protobuf union `mesos.ContainerInfo` with `Type == MESOS` " + "should not have the field `docker` set.", + status->message()); + + driver.stop(); + driver.join(); +} + // This test verifies that a task that has `name` parameter set // in `DockerInfo` is rejected during `TaskInfo` validation. @@ -3389,6 +3440,21 @@ 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.")); + } }
