Moved `CheckInfo` validation to common code. The master validation uses `validation::checkStatus` and `validation::checkStatusInfo`, so move them to the common validation library so that the master doesn't have a dependency on the checker library.
Review: https://reviews.apache.org/r/67795/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/b50f6c8a Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/b50f6c8a Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/b50f6c8a Branch: refs/heads/master Commit: b50f6c8afaa322e40ba07bb7bf9bf2f100163a68 Parents: 0fa3935 Author: James Peach <[email protected]> Authored: Tue Jul 3 20:54:42 2018 +1000 Committer: James Peach <[email protected]> Committed: Tue Jul 3 20:54:42 2018 +1000 ---------------------------------------------------------------------- src/checks/checker.cpp | 119 +---------------------------------------- src/checks/checker.hpp | 10 ---- src/common/validation.cpp | 113 ++++++++++++++++++++++++++++++++++++++ src/common/validation.hpp | 4 ++ src/master/validation.cpp | 2 +- src/slave/validation.cpp | 2 +- src/tests/check_tests.cpp | 45 +++++++++------- 7 files changed, 145 insertions(+), 150 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/b50f6c8a/src/checks/checker.cpp ---------------------------------------------------------------------- diff --git a/src/checks/checker.cpp b/src/checks/checker.cpp index 6d01349..e2b2baa 100644 --- a/src/checks/checker.cpp +++ b/src/checks/checker.cpp @@ -93,7 +93,7 @@ Try<Owned<Checker>> Checker::create( Variant<runtime::Plain, runtime::Docker, runtime::Nested> runtime) { // Validate the `CheckInfo` protobuf. - Option<Error> error = validation::checkInfo(check); + Option<Error> error = common::validation::validateCheckInfo(check); if (error.isSome()) { return error.get(); } @@ -180,123 +180,6 @@ void Checker::processCheckResult(const Try<CheckStatusInfo>& result) { } } - -namespace validation { - -Option<Error> checkInfo(const CheckInfo& checkInfo) -{ - if (!checkInfo.has_type()) { - return Error("CheckInfo must specify 'type'"); - } - - switch (checkInfo.type()) { - case CheckInfo::COMMAND: { - if (!checkInfo.has_command()) { - return Error("Expecting 'command' to be set for COMMAND check"); - } - - const CommandInfo& command = checkInfo.command().command(); - - if (!command.has_value()) { - string commandType = - (command.shell() ? "'shell command'" : "'executable path'"); - - return Error("Command check must contain " + commandType); - } - - Option<Error> error = - common::validation::validateCommandInfo(command); - if (error.isSome()) { - return Error( - "Check's `CommandInfo` is invalid: " + error->message); - } - - // TODO(alexr): Make sure irrelevant fields, e.g., `uris` are not set. - - break; - } - case CheckInfo::HTTP: { - if (!checkInfo.has_http()) { - return Error("Expecting 'http' to be set for HTTP check"); - } - - const CheckInfo::Http& http = checkInfo.http(); - - if (http.has_path() && !strings::startsWith(http.path(), '/')) { - return Error( - "The path '" + http.path() + "' of HTTP check must start with '/'"); - } - - break; - } - case CheckInfo::TCP: { - if (!checkInfo.has_tcp()) { - return Error("Expecting 'tcp' to be set for TCP check"); - } - - break; - } - case CheckInfo::UNKNOWN: { - return Error( - "'" + CheckInfo::Type_Name(checkInfo.type()) + "'" - " is not a valid check type"); - } - } - - if (checkInfo.has_delay_seconds() && checkInfo.delay_seconds() < 0.0) { - return Error("Expecting 'delay_seconds' to be non-negative"); - } - - if (checkInfo.has_interval_seconds() && checkInfo.interval_seconds() < 0.0) { - return Error("Expecting 'interval_seconds' to be non-negative"); - } - - if (checkInfo.has_timeout_seconds() && checkInfo.timeout_seconds() < 0.0) { - return Error("Expecting 'timeout_seconds' to be non-negative"); - } - - return None(); -} - - -Option<Error> checkStatusInfo(const CheckStatusInfo& checkStatusInfo) -{ - if (!checkStatusInfo.has_type()) { - return Error("CheckStatusInfo must specify 'type'"); - } - - switch (checkStatusInfo.type()) { - case CheckInfo::COMMAND: { - if (!checkStatusInfo.has_command()) { - return Error( - "Expecting 'command' to be set for COMMAND check's status"); - } - break; - } - case CheckInfo::HTTP: { - if (!checkStatusInfo.has_http()) { - return Error("Expecting 'http' to be set for HTTP check's status"); - } - break; - } - case CheckInfo::TCP: { - if (!checkStatusInfo.has_tcp()) { - return Error("Expecting 'tcp' to be set for TCP check's status"); - } - break; - } - case CheckInfo::UNKNOWN: { - return Error( - "'" + CheckInfo::Type_Name(checkStatusInfo.type()) + "'" - " is not a valid check's status type"); - } - } - - return None(); -} - -} // namespace validation { - } // namespace checks { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/b50f6c8a/src/checks/checker.hpp ---------------------------------------------------------------------- diff --git a/src/checks/checker.hpp b/src/checks/checker.hpp index 07507a5..884dc86 100644 --- a/src/checks/checker.hpp +++ b/src/checks/checker.hpp @@ -98,16 +98,6 @@ private: process::Owned<CheckerProcess> process; }; -namespace validation { - -// TODO(alexr): A better place for these functions would be something like -// "mesos_validation.cpp", since they validate API protobufs which are not -// solely related to this library. -Option<Error> checkInfo(const CheckInfo& checkInfo); -Option<Error> checkStatusInfo(const CheckStatusInfo& checkStatusInfo); - -} // namespace validation { - } // namespace checks { } // namespace internal { } // namespace mesos { http://git-wip-us.apache.org/repos/asf/mesos/blob/b50f6c8a/src/common/validation.cpp ---------------------------------------------------------------------- diff --git a/src/common/validation.cpp b/src/common/validation.cpp index e7988ce..f89b0ca 100644 --- a/src/common/validation.cpp +++ b/src/common/validation.cpp @@ -393,6 +393,119 @@ Option<Error> validateHealthCheck(const HealthCheck& healthCheck) return None(); } + +Option<Error> validateCheckInfo(const CheckInfo& checkInfo) +{ + if (!checkInfo.has_type()) { + return Error("CheckInfo must specify 'type'"); + } + + switch (checkInfo.type()) { + case CheckInfo::COMMAND: { + if (!checkInfo.has_command()) { + return Error("Expecting 'command' to be set for COMMAND check"); + } + + const CommandInfo& command = checkInfo.command().command(); + + if (!command.has_value()) { + string commandType = + (command.shell() ? "'shell command'" : "'executable path'"); + + return Error("Command check must contain " + commandType); + } + + Option<Error> error = + common::validation::validateCommandInfo(command); + if (error.isSome()) { + return Error( + "Check's `CommandInfo` is invalid: " + error->message); + } + + // TODO(alexr): Make sure irrelevant fields, e.g., `uris` are not set. + + break; + } + case CheckInfo::HTTP: { + if (!checkInfo.has_http()) { + return Error("Expecting 'http' to be set for HTTP check"); + } + + const CheckInfo::Http& http = checkInfo.http(); + + if (http.has_path() && !strings::startsWith(http.path(), '/')) { + return Error( + "The path '" + http.path() + "' of HTTP check must start with '/'"); + } + + break; + } + case CheckInfo::TCP: { + if (!checkInfo.has_tcp()) { + return Error("Expecting 'tcp' to be set for TCP check"); + } + + break; + } + case CheckInfo::UNKNOWN: { + return Error( + "'" + CheckInfo::Type_Name(checkInfo.type()) + "'" + " is not a valid check type"); + } + } + + if (checkInfo.has_delay_seconds() && checkInfo.delay_seconds() < 0.0) { + return Error("Expecting 'delay_seconds' to be non-negative"); + } + + if (checkInfo.has_interval_seconds() && checkInfo.interval_seconds() < 0.0) { + return Error("Expecting 'interval_seconds' to be non-negative"); + } + + if (checkInfo.has_timeout_seconds() && checkInfo.timeout_seconds() < 0.0) { + return Error("Expecting 'timeout_seconds' to be non-negative"); + } + + return None(); +} + + +Option<Error> validateCheckStatusInfo(const CheckStatusInfo& checkStatusInfo) +{ + if (!checkStatusInfo.has_type()) { + return Error("CheckStatusInfo must specify 'type'"); + } + + switch (checkStatusInfo.type()) { + case CheckInfo::COMMAND: { + if (!checkStatusInfo.has_command()) { + return Error( + "Expecting 'command' to be set for COMMAND check's status"); + } + break; + } + case CheckInfo::HTTP: { + if (!checkStatusInfo.has_http()) { + return Error("Expecting 'http' to be set for HTTP check's status"); + } + break; + } + case CheckInfo::TCP: { + if (!checkStatusInfo.has_tcp()) { + return Error("Expecting 'tcp' to be set for TCP check's status"); + } + break; + } + case CheckInfo::UNKNOWN: { + return Error( + "'" + CheckInfo::Type_Name(checkStatusInfo.type()) + "'" + " is not a valid check's status type"); + } + } + + return None(); +} + } // namespace validation { } // namespace common { } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/b50f6c8a/src/common/validation.hpp ---------------------------------------------------------------------- diff --git a/src/common/validation.hpp b/src/common/validation.hpp index 44259d6..fa7f4a7 100644 --- a/src/common/validation.hpp +++ b/src/common/validation.hpp @@ -56,6 +56,10 @@ Option<Error> validateGpus( Option<Error> validateHealthCheck(const HealthCheck& healthCheck); +Option<Error> validateCheckInfo(const CheckInfo& checkInfo); + +Option<Error> validateCheckStatusInfo(const CheckStatusInfo& checkStatusInfo); + } // namespace validation { } // namespace common { } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/b50f6c8a/src/master/validation.cpp ---------------------------------------------------------------------- diff --git a/src/master/validation.cpp b/src/master/validation.cpp index 51cfb74..79c67c3 100644 --- a/src/master/validation.cpp +++ b/src/master/validation.cpp @@ -1228,7 +1228,7 @@ Option<Error> validateMaxCompletionTime(const TaskInfo& task) Option<Error> validateCheck(const TaskInfo& task) { if (task.has_check()) { - Option<Error> error = checks::validation::checkInfo(task.check()); + Option<Error> error = common::validation::validateCheckInfo(task.check()); if (error.isSome()) { return Error("Task uses invalid check: " + error->message); } http://git-wip-us.apache.org/repos/asf/mesos/blob/b50f6c8a/src/slave/validation.cpp ---------------------------------------------------------------------- diff --git a/src/slave/validation.cpp b/src/slave/validation.cpp index 7b4c15a..3e333a7 100644 --- a/src/slave/validation.cpp +++ b/src/slave/validation.cpp @@ -627,7 +627,7 @@ Option<Error> validate(const mesos::executor::Call& call) if (status.has_check_status()) { Option<Error> validate = - checks::validation::checkStatusInfo(status.check_status()); + common::validation::validateCheckStatusInfo(status.check_status()); if (validate.isSome()) { return validate.get(); http://git-wip-us.apache.org/repos/asf/mesos/blob/b50f6c8a/src/tests/check_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/check_tests.cpp b/src/tests/check_tests.cpp index 73ea5a9..99367df 100644 --- a/src/tests/check_tests.cpp +++ b/src/tests/check_tests.cpp @@ -34,6 +34,8 @@ #include "checks/checker.hpp" +#include "common/validation.hpp" + #include "slave/containerizer/fetcher.hpp" #include "tests/flags.hpp" @@ -41,6 +43,9 @@ #include "tests/mesos.hpp" #include "tests/utils.hpp" +using mesos::internal::common::validation::validateCheckInfo; +using mesos::internal::common::validation::validateCheckStatusInfo; + using mesos::internal::slave::Fetcher; using mesos::internal::slave::MesosContainerizer; @@ -2865,12 +2870,12 @@ TEST_F(CheckTest, CheckInfoValidation) { CheckInfo checkInfo; - Option<Error> validate = validation::checkInfo(checkInfo); + Option<Error> validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ("CheckInfo must specify 'type'", validate->message); checkInfo.set_type(CheckInfo::UNKNOWN); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ("'UNKNOWN' is not a valid check type", validate->message); } @@ -2880,21 +2885,21 @@ TEST_F(CheckTest, CheckInfoValidation) CheckInfo checkInfo; checkInfo.set_type(CheckInfo::COMMAND); - Option<Error> validate = validation::checkInfo(checkInfo); + Option<Error> validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'command' to be set for COMMAND check", validate->message); checkInfo.set_type(CheckInfo::HTTP); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'http' to be set for HTTP check", validate->message); checkInfo.set_type(CheckInfo::TCP); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'tcp' to be set for TCP check", @@ -2907,12 +2912,12 @@ TEST_F(CheckTest, CheckInfoValidation) checkInfo.set_type(CheckInfo::COMMAND); checkInfo.mutable_command()->CopyFrom(CheckInfo::Command()); - Option<Error> validate = validation::checkInfo(checkInfo); + Option<Error> validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ("Command check must contain 'shell command'", validate->message); checkInfo.mutable_command()->mutable_command()->CopyFrom(CommandInfo()); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ("Command check must contain 'shell command'", validate->message); } @@ -2926,7 +2931,7 @@ TEST_F(CheckTest, CheckInfoValidation) checkInfo.mutable_command()->mutable_command()->CopyFrom( createCommandInfo("exit 0")); - Option<Error> validate = validation::checkInfo(checkInfo); + Option<Error> validate = validateCheckInfo(checkInfo); EXPECT_NONE(validate); Environment::Variable* variable = @@ -2934,7 +2939,7 @@ TEST_F(CheckTest, CheckInfoValidation) ->mutable_variables()->Add(); variable->set_name("ENV_VAR_KEY"); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); } @@ -2945,11 +2950,11 @@ TEST_F(CheckTest, CheckInfoValidation) checkInfo.set_type(CheckInfo::HTTP); checkInfo.mutable_http()->set_port(8080); - Option<Error> validate = validation::checkInfo(checkInfo); + Option<Error> validate = validateCheckInfo(checkInfo); EXPECT_NONE(validate); checkInfo.mutable_http()->set_path("healthz"); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ( "The path 'healthz' of HTTP check must start with '/'", @@ -2964,7 +2969,7 @@ TEST_F(CheckTest, CheckInfoValidation) checkInfo.mutable_http()->set_port(8080); checkInfo.set_delay_seconds(-1.0); - Option<Error> validate = validation::checkInfo(checkInfo); + Option<Error> validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'delay_seconds' to be non-negative", @@ -2972,7 +2977,7 @@ TEST_F(CheckTest, CheckInfoValidation) checkInfo.set_delay_seconds(0.0); checkInfo.set_interval_seconds(-1.0); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'interval_seconds' to be non-negative", @@ -2980,14 +2985,14 @@ TEST_F(CheckTest, CheckInfoValidation) checkInfo.set_interval_seconds(0.0); checkInfo.set_timeout_seconds(-1.0); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'timeout_seconds' to be non-negative", validate->message); checkInfo.set_timeout_seconds(0.0); - validate = validation::checkInfo(checkInfo); + validate = validateCheckInfo(checkInfo); EXPECT_NONE(validate); } } @@ -3002,12 +3007,12 @@ TEST_F(CheckTest, CheckStatusInfoValidation) { CheckStatusInfo checkStatusInfo; - Option<Error> validate = validation::checkStatusInfo(checkStatusInfo); + Option<Error> validate = validateCheckStatusInfo(checkStatusInfo); EXPECT_SOME(validate); EXPECT_EQ("CheckStatusInfo must specify 'type'", validate->message); checkStatusInfo.set_type(CheckInfo::UNKNOWN); - validate = validation::checkStatusInfo(checkStatusInfo); + validate = validateCheckStatusInfo(checkStatusInfo); EXPECT_SOME(validate); EXPECT_EQ( "'UNKNOWN' is not a valid check's status type", @@ -3019,21 +3024,21 @@ TEST_F(CheckTest, CheckStatusInfoValidation) CheckStatusInfo checkStatusInfo; checkStatusInfo.set_type(CheckInfo::COMMAND); - Option<Error> validate = validation::checkStatusInfo(checkStatusInfo); + Option<Error> validate = validateCheckStatusInfo(checkStatusInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'command' to be set for COMMAND check's status", validate->message); checkStatusInfo.set_type(CheckInfo::HTTP); - validate = validation::checkStatusInfo(checkStatusInfo); + validate = validateCheckStatusInfo(checkStatusInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'http' to be set for HTTP check's status", validate->message); checkStatusInfo.set_type(CheckInfo::TCP); - validate = validation::checkStatusInfo(checkStatusInfo); + validate = validateCheckStatusInfo(checkStatusInfo); EXPECT_SOME(validate); EXPECT_EQ( "Expecting 'tcp' to be set for TCP check's status",
