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 <[email protected]> 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 {
