You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by bm...@apache.org on 2019/05/20 19:18:16 UTC
[mesos] branch master updated (5bb4f28 -> 1f83bfc)
This is an automated email from the ASF dual-hosted git repository.
bmahler pushed a change to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git.
from 5bb4f28 Made activateRecoveredFramework() return void instead of Try.
new 734be1e Introduced a function for validating a `FrameworkInfo` update.
new 1f83bfc Added unit tests for 'framework::validateUpdate()'.
The 2 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
Summary of changes:
src/master/master.cpp | 29 ++++++++-----------------
src/master/validation.cpp | 34 +++++++++++++++++++++++++++++
src/master/validation.hpp | 8 +++++++
src/tests/master_validation_tests.cpp | 41 +++++++++++++++++++++++++++++++++++
4 files changed, 92 insertions(+), 20 deletions(-)
[mesos] 02/02: Added unit tests for 'framework::validateUpdate()'.
Posted by 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 1f83bfc2877004e0e3ddd1e7a15902a6b471d7ee
Author: Andrei Sekretenko <as...@mesosphere.io>
AuthorDate: Mon May 20 14:57:57 2019 -0400
Added unit tests for 'framework::validateUpdate()'.
Review: https://reviews.apache.org/r/70667/
---
src/tests/master_validation_tests.cpp | 41 +++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp
index 1b7a827..f102906 100644
--- a/src/tests/master_validation_tests.cpp
+++ b/src/tests/master_validation_tests.cpp
@@ -4763,6 +4763,47 @@ TEST_F(FrameworkInfoValidationTest, ValidateOfferFilters)
}
+// This tests validation of FrameworkInfo updates.
+TEST_F(FrameworkInfoValidationTest, ValidateUpdate)
+{
+ {
+ FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
+ frameworkInfo.add_roles("bar");
+
+ EXPECT_NONE(framework::validateUpdate(
+ DEFAULT_FRAMEWORK_INFO, frameworkInfo));
+ }
+
+ {
+ FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
+ *frameworkInfo.mutable_principal() += "_foo";
+
+ EXPECT_SOME(framework::validateUpdate(
+ DEFAULT_FRAMEWORK_INFO, frameworkInfo));
+ }
+
+
+ // TODO(asekretenko): The validate function currently does not check
+ // 'user' and 'checkpoint', which are both immutable! Update this to
+ // test that these fields are also validated.
+ {
+ FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
+ *frameworkInfo.mutable_user() += "_foo";
+
+ EXPECT_NONE(framework::validateUpdate(
+ DEFAULT_FRAMEWORK_INFO, frameworkInfo));
+ }
+
+ {
+ FrameworkInfo frameworkInfo = DEFAULT_FRAMEWORK_INFO;
+ frameworkInfo.set_checkpoint(!frameworkInfo.checkpoint());
+
+ EXPECT_NONE(framework::validateUpdate(
+ DEFAULT_FRAMEWORK_INFO, frameworkInfo));
+ }
+}
+
+
// This test ensures that ia framework cannot use the
// `FrameworkInfo.roles` field without providing the
// MULTI_ROLE capability.
[mesos] 01/02: Introduced a function for validating a
`FrameworkInfo` update.
Posted by 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 734be1e9582f516edd168fa562cce570572ecf5e
Author: Andrei Sekretenko <as...@mesosphere.io>
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 {