Added parameter validation to RegisterSlaveMessage. Add some simple validation checks to the RegisterSlaveMessage message to ensure that we have a minimally consistent registration request before proceeding.
Review: https://reviews.apache.org/r/58621/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/e83551d6 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/e83551d6 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/e83551d6 Branch: refs/heads/master Commit: e83551d6729c70846e83df774c4251701a161f98 Parents: b22c9e7 Author: James Peach <[email protected]> Authored: Wed Apr 26 15:36:17 2017 -0400 Committer: Neil Conway <[email protected]> Committed: Wed Apr 26 16:01:35 2017 -0400 ---------------------------------------------------------------------- src/master/master.cpp | 10 ++++++ src/master/validation.cpp | 50 ++++++++++++++++++++++++++++++ src/master/validation.hpp | 4 +++ src/tests/master_validation_tests.cpp | 40 ++++++++++++++++++++++++ 4 files changed, 104 insertions(+) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/e83551d6/src/master/master.cpp ---------------------------------------------------------------------- diff --git a/src/master/master.cpp b/src/master/master.cpp index 9814df2..a0cb505 100644 --- a/src/master/master.cpp +++ b/src/master/master.cpp @@ -5380,6 +5380,16 @@ void Master::registerSlave( return; } + Option<Error> error = validation::master::message::registerSlave( + slaveInfo, checkpointedResources); + + if (error.isSome()) { + LOG(WARNING) << "Dropping registration of agent at " << from + << " because it sent an invalid registration: " + << error->message; + return; + } + MachineID machineId; machineId.set_hostname(slaveInfo.hostname()); machineId.set_ip(stringify(from.address.ip)); http://git-wip-us.apache.org/repos/asf/mesos/blob/e83551d6/src/master/validation.cpp ---------------------------------------------------------------------- diff --git a/src/master/validation.cpp b/src/master/validation.cpp index 768fc35..8d104af 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -231,6 +231,51 @@ Option<Error> validate( namespace message { +static Option<Error> validateSlaveInfo(const SlaveInfo& slaveInfo) +{ + if (slaveInfo.has_id()) { + Option<Error> error = common::validation::validateSlaveID(slaveInfo.id()); + if (error.isSome()) { + return error.get(); + } + } + + Option<Error> error = Resources::validate(slaveInfo.resources()); + if (error.isSome()) { + return error.get(); + } + + return None(); +} + + +Option<Error> registerSlave( + const SlaveInfo& slaveInfo, + const vector<Resource>& checkpointedResources) +{ + Option<Error> error = validateSlaveInfo(slaveInfo); + if (error.isSome()) { + return error.get(); + } + + if (!checkpointedResources.empty()) { + if (!slaveInfo.has_checkpoint() || !slaveInfo.checkpoint()) { + return Error( + "Checkpointed resources provided when checkpointing is not enabled"); + } + } + + foreach (const Resource& resource, checkpointedResources) { + error = Resources::validate(resource); + if (error.isSome()) { + return error.get(); + } + } + + return None(); +} + + Option<Error> reregisterSlave( const SlaveInfo& slaveInfo, const vector<Task>& tasks, @@ -241,6 +286,11 @@ Option<Error> reregisterSlave( hashset<FrameworkID> frameworkIDs; hashset<ExecutorID> executorIDs; + Option<Error> error = validateSlaveInfo(slaveInfo); + if (error.isSome()) { + return error.get(); + } + foreach (const Resource& resource, resources) { Option<Error> error = Resources::validate(resource); if (error.isSome()) { http://git-wip-us.apache.org/repos/asf/mesos/blob/e83551d6/src/master/validation.hpp ---------------------------------------------------------------------- diff --git a/src/master/validation.hpp b/src/master/validation.hpp index 5f01a67..ad9d07c 100644 --- a/src/master/validation.hpp +++ b/src/master/validation.hpp @@ -57,6 +57,10 @@ Option<Error> validate( namespace message { +Option<Error> registerSlave( + const SlaveInfo& slaveInfo, + const std::vector<Resource>& checkpointedResources); + Option<Error> reregisterSlave( const SlaveInfo& slaveInfo, const std::vector<Task>& tasks, http://git-wip-us.apache.org/repos/asf/mesos/blob/e83551d6/src/tests/master_validation_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp index 1e0e764..3308803 100644 --- a/src/tests/master_validation_tests.cpp +++ b/src/tests/master_validation_tests.cpp @@ -3780,6 +3780,46 @@ TEST_F(RegisterSlaveValidationTest, DropInvalidReregistration) Clock::settle(); } + +TEST_F(RegisterSlaveValidationTest, DropInvalidRegistration) +{ + Try<Owned<cluster::Master>> master = StartMaster(); + ASSERT_SOME(master); + + // Drop and capture the slave's RegisterSlaveMessage. + Future<RegisterSlaveMessage> registerSlaveMessage = + DROP_PROTOBUF(RegisterSlaveMessage(), _, _); + + // We expect the master to drop the RegisterSlaveMessage, so it + // will never send any SlaveRegisteredMessage responses. + EXPECT_NO_FUTURE_PROTOBUFS(SlaveRegisteredMessage(), _, _); + + Owned<MasterDetector> detector = master.get()->createDetector(); + Try<Owned<cluster::Slave>> slave = StartSlave(detector.get()); + ASSERT_SOME(slave); + + AWAIT_READY(registerSlaveMessage); + + // Now that we have a valid RegisterSlaveMessage, tweak it to + // fail validation by setting an invalid slave ID, + RegisterSlaveMessage message = registerSlaveMessage.get(); + + SlaveInfo* slaveInfo = message.mutable_slave(); + slaveInfo->mutable_id()->set_value( + strings::join( + "/../", + UUID::random().toString(), + UUID::random().toString(), + UUID::random().toString())); + + // Send the modified message to the master. + process::post(slave.get()->pid, master->get()->pid, message); + + // Settle the clock to retire in-flight messages. + Clock::pause(); + Clock::settle(); +} + } // namespace tests { } // namespace internal { } // namespace mesos {
