This is an automated email from the ASF dual-hosted git repository. bbannier pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 40210dbcfb218d6ef3f647b047afbdda24133f4d Author: Benjamin Bannier <[email protected]> AuthorDate: Thu Dec 20 10:24:29 2018 +0100 Added validation for framework IDs. During scheduler subscription we currently cannot distinguish self-assigned from master-assigned framework IDs, so we might allow frameworks to subscribe even if the used framework ID was never assigned by any master, see MESOS-1719. This patch adds some validation of used framework IDs so that they can e.g., savely be used in agents when persisting information. We also clarify the API to call out that self-assigned framework IDs are not explicitly supported. Review: https://reviews.apache.org/r/69398/ --- include/mesos/mesos.proto | 12 +++++++----- include/mesos/v1/mesos.proto | 12 +++++++----- src/master/validation.cpp | 24 +++++++++++++++++++++++- src/master/validation.hpp | 2 ++ src/tests/master_validation_tests.cpp | 28 ++++++++++++++++++++++++++++ 5 files changed, 67 insertions(+), 11 deletions(-) diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto index 137df1f..2ef6ba3 100644 --- a/include/mesos/mesos.proto +++ b/include/mesos/mesos.proto @@ -239,11 +239,13 @@ message FrameworkInfo { // Name of the framework that shows up in the Mesos Web UI. required string name = 2; - // Note that 'id' is only available after a framework has - // registered, however, it is included here in order to facilitate - // scheduler failover (i.e., if it is set then the - // MesosSchedulerDriver expects the scheduler is performing - // failover). + // Used to uniquely identify the framework. + // + // This field must be unset when the framework subscribes for the + // first time upon which the master will assign a new ID. To + // resubscribe after scheduler failover the framework should set + // 'id' to the ID assigned by the master. Setting 'id' to values + // not assigned by Mesos masters is unsupported. optional FrameworkID id = 3; // The amount of time (in seconds) that the master will wait for the diff --git a/include/mesos/v1/mesos.proto b/include/mesos/v1/mesos.proto index f121709..1a701da 100644 --- a/include/mesos/v1/mesos.proto +++ b/include/mesos/v1/mesos.proto @@ -237,11 +237,13 @@ message FrameworkInfo { // Name of the framework that shows up in the Mesos Web UI. required string name = 2; - // Note that 'id' is only available after a framework has - // registered, however, it is included here in order to facilitate - // scheduler failover (i.e., if it is set then the - // MesosSchedulerDriver expects the scheduler is performing - // failover). + // Used to uniquely identify the framework. + // + // This field must be unset when the framework subscribes for the + // first time upon which the master will assign a new ID. To + // resubscribe after scheduler failover the framework should set + // 'id' to the ID assigned by the master. Setting 'id' to values + // not assigned by Mesos masters is unsupported. optional FrameworkID id = 3; // The amount of time (in seconds) that the master will wait for the diff --git a/src/master/validation.cpp b/src/master/validation.cpp index 5fccc9f..a71edeb 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -508,11 +508,33 @@ Option<Error> validateRoles(const FrameworkInfo& frameworkInfo) return None(); } + +Option<Error> validateFrameworkId(const FrameworkInfo& frameworkInfo) +{ + if (!frameworkInfo.has_id() || frameworkInfo.id().value().empty()) { + return None(); + } + + return common::validation::validateID(frameworkInfo.id().value()); +} + } // namespace internal { Option<Error> validate(const mesos::FrameworkInfo& frameworkInfo) { - return internal::validateRoles(frameworkInfo); + Option<Error> error = internal::validateRoles(frameworkInfo); + + if (error.isSome()) { + return error; + } + + error = internal::validateFrameworkId(frameworkInfo); + + if (error.isSome()) { + return error; + } + + return None(); } } // namespace framework { diff --git a/src/master/validation.hpp b/src/master/validation.hpp index 9af9039..a5bdff6 100644 --- a/src/master/validation.hpp +++ b/src/master/validation.hpp @@ -103,6 +103,8 @@ namespace internal { // +-------+-------+---------+ Option<Error> validateRoles(const mesos::FrameworkInfo& frameworkInfo); +Option<Error> validateFrameworkId(const mesos::FrameworkInfo& frameworkInfo); + } // namespace internal { // Validate a FrameworkInfo. diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp index 61c3d2a..c00e8bb 100644 --- a/src/tests/master_validation_tests.cpp +++ b/src/tests/master_validation_tests.cpp @@ -4678,6 +4678,34 @@ TEST_F(FrameworkInfoValidationTest, ValidateRoles) } +// This tests the validation of the `FrameworkID`. +TEST_F(FrameworkInfoValidationTest, ValidateFrameworkID) +{ + FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO; + + // Unset framework IDs are used in an initial subscription and are valid. + frameworkInfo.clear_id(); + EXPECT_NONE(::framework::validate(frameworkInfo)); + + // We allow set but empty framework IDs, see MESOS-9481. + frameworkInfo.mutable_id()->set_value(""); + EXPECT_NONE(::framework::validate(frameworkInfo)); + + // Typical IDs the master would assign are valid. + frameworkInfo.mutable_id()->set_value(id::UUID::random().toString()); + frameworkInfo.mutable_id()->set_value( + strings::format("%s-4711", id::UUID::random().toString()).get()); + EXPECT_NONE(::framework::validate(frameworkInfo)); + + // Framework IDs containing typical path separators are invalid. + frameworkInfo.mutable_id()->set_value("foo/bar"); + EXPECT_SOME(::framework::validate(frameworkInfo)); + + frameworkInfo.mutable_id()->set_value("foo/.."); + EXPECT_SOME(::framework::validate(frameworkInfo)); +} + + // This test ensures that ia framework cannot use the // `FrameworkInfo.roles` field without providing the // MULTI_ROLE capability.
