Added a 'SECRET' type to the 'Environment' protobuf message. This patch adds a field of type `Secret` to the `Environment` protobuf message, enabling the passing of secrets into the environments of executors and tasks. Additional validation and test code is added as well.
Review: https://reviews.apache.org/r/56053/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/7b326549 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/7b326549 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/7b326549 Branch: refs/heads/master Commit: 7b326549f4695e9d7b5eb2b7e51ac5e721aaac30 Parents: c74ab59 Author: Greg Mann <[email protected]> Authored: Fri Feb 17 11:47:41 2017 -0800 Committer: Vinod Kone <[email protected]> Committed: Fri Feb 17 11:47:41 2017 -0800 ---------------------------------------------------------------------- include/mesos/mesos.proto | 22 +++++- include/mesos/v1/mesos.proto | 22 +++++- src/common/validation.cpp | 73 ++++++++++++++--- src/common/validation.hpp | 2 + src/tests/check_tests.cpp | 5 +- src/tests/health_check_tests.cpp | 3 +- src/tests/master_validation_tests.cpp | 59 +++++++------- src/tests/slave_validation_tests.cpp | 123 +++++++++++++++++++++++++++-- 8 files changed, 247 insertions(+), 62 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/include/mesos/mesos.proto ---------------------------------------------------------------------- diff --git a/include/mesos/mesos.proto b/include/mesos/mesos.proto index c2b2e6e..030e79c 100644 --- a/include/mesos/mesos.proto +++ b/include/mesos/mesos.proto @@ -1898,15 +1898,29 @@ message Filters { /** * Describes a collection of environment variables. This is used with * CommandInfo in order to set environment variables before running a -* command. +* command. The contents of each variable may be specified as a string +* or a Secret; only one of `value` and `secret` must be set. */ message Environment { message Variable { required string name = 1; - // NOTE: The `value` field was made optional in Mesos 1.2 but it - // is currently enforced to be set. This constraint will be - // removed in a future version. + + enum Type { + UNKNOWN = 0; + VALUE = 1; + SECRET = 2; + } + + // In Mesos 1.2, the `Environment.variables.value` message was made + // optional. The default type for `Environment.variables.type` is now VALUE, + // which requires `value` to be set, maintaining backward compatibility. + // + // TODO(greggomann): The default can be removed in Mesos 2.1 (MESOS-7134). + optional Type type = 3 [default = VALUE]; + + // Only one of `value` and `secret` must be set. optional string value = 2; + optional Secret secret = 4; } repeated Variable variables = 1; http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/include/mesos/v1/mesos.proto ---------------------------------------------------------------------- diff --git a/include/mesos/v1/mesos.proto b/include/mesos/v1/mesos.proto index c5b4897..7f6f858 100644 --- a/include/mesos/v1/mesos.proto +++ b/include/mesos/v1/mesos.proto @@ -1897,15 +1897,29 @@ message Filters { /** * Describes a collection of environment variables. This is used with * CommandInfo in order to set environment variables before running a -* command. +* command. The contents of each variable may be specified as a string +* or a Secret; only one of `value` and `secret` must be set. */ message Environment { message Variable { required string name = 1; - // NOTE: The `value` field was made optional in Mesos 1.2 but it - // is currently enforced to be set. This constraint will be - // removed in a future version. + + enum Type { + UNKNOWN = 0; + VALUE = 1; + SECRET = 2; + } + + // In Mesos 1.2, the `Environment.variables.value` message was made + // optional. The default type for `Environment.variables.type` is now VALUE, + // which requires `value` to be set, maintaining backward compatibility. + // + // TODO(greggomann): The default can be removed in Mesos 2.1 (MESOS-7134). + optional Type type = 3 [default = VALUE]; + + // Only one of `value` and `secret` must be set. optional string value = 2; + optional Secret secret = 4; } repeated Variable variables = 1; http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/src/common/validation.cpp ---------------------------------------------------------------------- diff --git a/src/common/validation.cpp b/src/common/validation.cpp index 2818261..028679d 100644 --- a/src/common/validation.cpp +++ b/src/common/validation.cpp @@ -118,25 +118,74 @@ Option<Error> validateSecret(const Secret& secret) } -// TODO(greggomann): Do more than just validate the `Environment`. -Option<Error> validateCommandInfo(const CommandInfo& command) +Option<Error> validateEnvironment(const Environment& environment) { - // In Mesos 1.2, the `Environment.Variable.Value` message was made - // optional, but it is currently required to be set for backward - // compatibility. This constraint will be removed in a future - // version. - foreach (const Environment::Variable& variable, - command.environment().variables()) { - if (!variable.has_value()) { - return Error( - "Environment variable '" + variable.name() + - "' must have a value set"); + foreach (const Environment::Variable& variable, environment.variables()) { + switch (variable.type()) { + case Environment::Variable::SECRET: { + if (!variable.has_secret()) { + return Error( + "Environment variable '" + variable.name() + + "' of type 'SECRET' must have a secret set"); + } + + if (variable.has_value()) { + return Error( + "Environment variable '" + variable.name() + + "' of type 'SECRET' must not have a value set"); + } + + Option<Error> error = validateSecret(variable.secret()); + if (error.isSome()) { + return Error( + "Environment variable '" + variable.name() + "' specifies an " + "invalid secret: " + error->message); + } + + if (variable.secret().value().data().find('\0') != string::npos) { + return Error( + "Environment variable '" + variable.name() + "' specifies a " + "secret containing null bytes, which is not allowed in the " + "environment"); + } + break; + } + + // NOTE: If new variable types are added in the future and an upgraded + // client/master sends a new type to an older master/agent, the older + // master/agent will see VALUE instead of the new type, since VALUE is set + // as the default type in the protobuf definition. + case Environment::Variable::VALUE: + if (!variable.has_value()) { + return Error( + "Environment variable '" + variable.name() + + "' of type 'VALUE' must have a value set"); + } + + if (variable.has_secret()) { + return Error( + "Environment variable '" + variable.name() + + "' of type 'VALUE' must not have a secret set"); + } + break; + + case Environment::Variable::UNKNOWN: + return Error("Environment variable of type 'UNKNOWN' is not allowed"); + + UNREACHABLE(); } } return None(); } + +// TODO(greggomann): Do more than just validate the `Environment`. +Option<Error> validateCommandInfo(const CommandInfo& command) +{ + return validateEnvironment(command.environment()); +} + } // namespace validation { } // namespace common { } // namespace internal { http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/src/common/validation.hpp ---------------------------------------------------------------------- diff --git a/src/common/validation.hpp b/src/common/validation.hpp index caba07b..8c92436 100644 --- a/src/common/validation.hpp +++ b/src/common/validation.hpp @@ -43,6 +43,8 @@ Option<Error> validateFrameworkID(const FrameworkID& frameworkId); Option<Error> validateSecret(const Secret& secret); +Option<Error> validateEnvironment(const Environment& environment); + Option<Error> validateCommandInfo(const CommandInfo& command); } // namespace validation { http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/src/tests/check_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/check_tests.cpp b/src/tests/check_tests.cpp index b5a5d8e..f035c16 100644 --- a/src/tests/check_tests.cpp +++ b/src/tests/check_tests.cpp @@ -81,9 +81,8 @@ TEST_F(CheckTest, CheckInfoValidation) EXPECT_EQ("Command check must contain 'shell command'", validate->message); } - // Command check must specify a command with a valid environment. Currently, - // `Environment.Variable.Value` must be set, but this constraint will be - // removed in a future version. + // Command check must specify a command with a valid environment. + // Environment variable's `value` field must be set in this case. { CheckInfo checkInfo; checkInfo.set_type(CheckInfo::COMMAND); http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/src/tests/health_check_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/health_check_tests.cpp b/src/tests/health_check_tests.cpp index 476e04a..56e9074 100644 --- a/src/tests/health_check_tests.cpp +++ b/src/tests/health_check_tests.cpp @@ -231,8 +231,7 @@ TEST_F(HealthCheckTest, HealthCheckProtobufValidation) } // Command health check must specify a command with a valid environment. - // Currently, `Environment.Variable.Value` must be set, but this constraint - // will be removed in a future version. + // Environment variable's `value` field must be set in this case. { HealthCheck healthCheckProto; healthCheckProto.set_type(HealthCheck::COMMAND); http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/src/tests/master_validation_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/master_validation_tests.cpp b/src/tests/master_validation_tests.cpp index 7176738..badf318 100644 --- a/src/tests/master_validation_tests.cpp +++ b/src/tests/master_validation_tests.cpp @@ -1018,10 +1018,11 @@ TEST_F(TaskValidationTest, ExecutorUsesInvalidFrameworkID) } -// Verifies that an environment variable in `ExecutorInfo.command.environment` -// without a value will be correctly rejected. This constraint will be removed -// in a future version. -TEST_F(TaskValidationTest, ExecutorEnvironmentVariableWithoutValue) +// Verifies that an invalid `ExecutorInfo.command.environment` will be rejected. +// This test ensures that the common validation code is being executed; +// comprehensive tests for the `Environment` message can be found in the agent +// validation tests. +TEST_F(TaskValidationTest, ExecutorEnvironmentInvalid) { Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -1036,7 +1037,6 @@ TEST_F(TaskValidationTest, ExecutorEnvironmentVariableWithoutValue) EXPECT_CALL(sched, registered(&driver, _, _)); - // Create an executor. ExecutorInfo executor; executor = DEFAULT_EXECUTOR_INFO; Environment::Variable* variable = @@ -1057,8 +1057,8 @@ TEST_F(TaskValidationTest, ExecutorEnvironmentVariableWithoutValue) AWAIT_READY(status); EXPECT_EQ(TASK_ERROR, status.get().state()); EXPECT_EQ( - "Executor's `CommandInfo` is invalid: " - "Environment variable 'ENV_VAR_KEY' must have a value set", + "Executor's `CommandInfo` is invalid: Environment variable 'ENV_VAR_KEY' " + "of type 'VALUE' must have a value set", status->message()); // Make sure the task is not known to master anymore. @@ -2022,10 +2022,11 @@ TEST_F(TaskValidationTest, KillPolicyGracePeriodIsNonNegative) } -// This test verifies that a task containing an environment variable -// with no value will be rejected correctly. Note that this constraint -// will be removed in a future version. -TEST_F(TaskValidationTest, TaskEnvironmentVariableWithoutValue) +// Verifies that an invalid `TaskInfo.command.environment` will be rejected. +// This test ensures that the common validation code is being executed; +// comprehensive tests for the `Environment` message can be found in the agent +// validation tests. +TEST_F(TaskValidationTest, TaskEnvironmentInvalid) { Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -2066,8 +2067,8 @@ TEST_F(TaskValidationTest, TaskEnvironmentVariableWithoutValue) AWAIT_READY(status); EXPECT_EQ(TASK_ERROR, status.get().state()); EXPECT_EQ( - "Task's `CommandInfo` is invalid: " - "Environment variable 'ENV_VAR_KEY' must have a value set", + "Task's `CommandInfo` is invalid: Environment variable 'ENV_VAR_KEY' " + "of type 'VALUE' must have a value set", status->message()); driver.stop(); @@ -2843,11 +2844,11 @@ TEST_F(TaskGroupValidationTest, TaskUsesDifferentExecutor) } -// Ensures that a task group which specifies an invalid environment in -// `ExecutorInfo` will be correctly rejected. Currently, -// `Environment.Variable.Value` must be set, but this constraint will be removed -// in a future version. -TEST_F(TaskGroupValidationTest, ExecutorEnvironmentVariableWithoutValue) +// Verifies that a task group which specifies an invalid environment in +// `ExecutorInfo` will be rejected. This test ensures that the common validation +// code is being executed; comprehensive tests for the `Environment` message can +// be found in the agent validation tests. +TEST_F(TaskGroupValidationTest, ExecutorEnvironmentInvalid) { Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -2919,16 +2920,16 @@ TEST_F(TaskGroupValidationTest, ExecutorEnvironmentVariableWithoutValue) EXPECT_EQ(TASK_ERROR, task1Status->state()); EXPECT_EQ(TaskStatus::REASON_TASK_GROUP_INVALID, task1Status->reason()); EXPECT_EQ( - "Executor's `CommandInfo` is invalid: " - "Environment variable 'ENV_VAR_KEY' must have a value set", + "Executor's `CommandInfo` is invalid: Environment variable 'ENV_VAR_KEY' " + "of type 'VALUE' must have a value set", task1Status->message()); AWAIT_READY(task2Status); EXPECT_EQ(TASK_ERROR, task2Status->state()); EXPECT_EQ(TaskStatus::REASON_TASK_GROUP_INVALID, task2Status->reason()); EXPECT_EQ( - "Executor's `CommandInfo` is invalid: " - "Environment variable 'ENV_VAR_KEY' must have a value set", + "Executor's `CommandInfo` is invalid: Environment variable 'ENV_VAR_KEY' " + "of type 'VALUE' must have a value set", task2Status->message()); // Make sure the tasks are not known to master anymore. @@ -2947,11 +2948,11 @@ TEST_F(TaskGroupValidationTest, ExecutorEnvironmentVariableWithoutValue) } -// Ensures that a task group which specifies an invalid environment in -// `TaskGroupInfo` will be correctly rejected. Currently, -// `Environment.Variable.Value` must be set, but this constraint will be removed -// in a future version. -TEST_F(TaskGroupValidationTest, TaskEnvironmentVariableWithoutValue) +// Verifies that a task group which specifies an invalid environment in +// `TaskGroupInfo` will be rejected. This test ensures that the common +// validation code is being executed; comprehensive tests for the `Environment` +// message can be found in the agent validation tests. +TEST_F(TaskGroupValidationTest, TaskEnvironmentInvalid) { Try<Owned<cluster::Master>> master = StartMaster(); ASSERT_SOME(master); @@ -3025,7 +3026,7 @@ TEST_F(TaskGroupValidationTest, TaskEnvironmentVariableWithoutValue) EXPECT_EQ(TaskStatus::REASON_TASK_GROUP_INVALID, task1Status->reason()); EXPECT_EQ( "Task '1' is invalid: Task's `CommandInfo` is invalid: Environment " - "variable 'ENV_VAR_KEY' must have a value set", + "variable 'ENV_VAR_KEY' of type 'VALUE' must have a value set", task1Status->message()); AWAIT_READY(task2Status); @@ -3033,7 +3034,7 @@ TEST_F(TaskGroupValidationTest, TaskEnvironmentVariableWithoutValue) EXPECT_EQ(TaskStatus::REASON_TASK_GROUP_INVALID, task2Status->reason()); EXPECT_EQ( "Task '1' is invalid: Task's `CommandInfo` is invalid: Environment " - "variable 'ENV_VAR_KEY' must have a value set", + "variable 'ENV_VAR_KEY' of type 'VALUE' must have a value set", task2Status->message()); // Allow status updates to arrive. http://git-wip-us.apache.org/repos/asf/mesos/blob/7b326549/src/tests/slave_validation_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/slave_validation_tests.cpp b/src/tests/slave_validation_tests.cpp index 96218e8..588c5b2 100644 --- a/src/tests/slave_validation_tests.cpp +++ b/src/tests/slave_validation_tests.cpp @@ -25,6 +25,8 @@ #include <stout/option.hpp> #include <stout/uuid.hpp> +#include "common/validation.hpp" + #include "slave/slave.hpp" #include "slave/validation.hpp" @@ -32,6 +34,7 @@ namespace validation = mesos::internal::slave::validation; +using mesos::internal::common::validation::validateEnvironment; using mesos::internal::common::validation::validateSecret; using mesos::internal::slave::Slave; @@ -147,6 +150,108 @@ TEST(AgentValidationTest, Secret) } +// Tests that the common validation code for the +// `Environment` message works as expected. +TEST(AgentValidationTest, Environment) +{ + // Validate a variable of SECRET type. + { + Environment environment; + Environment::Variable* variable = environment.mutable_variables()->Add(); + variable->set_type(mesos::Environment::Variable::SECRET); + variable->set_name("ENV_VAR_KEY"); + + Option<Error> error = validateEnvironment(environment); + EXPECT_SOME(error); + EXPECT_EQ( + "Environment variable 'ENV_VAR_KEY' of type " + "'SECRET' must have a secret set", + error->message); + + Secret secret; + secret.set_type(Secret::VALUE); + secret.mutable_value()->set_data("SECRET_VALUE"); + variable->mutable_secret()->CopyFrom(secret); + + variable->set_value("ENV_VAR_VALUE"); + + error = validateEnvironment(environment); + EXPECT_SOME(error); + EXPECT_EQ( + "Environment variable 'ENV_VAR_KEY' of type 'SECRET' " + "must not have a value set", + error->message); + + variable->clear_value(); + char invalid_secret[5] = {'a', 'b', '\0', 'c', 'd'}; + variable->mutable_secret()->mutable_value()->set_data( + std::string(invalid_secret, 5)); + + error = validateEnvironment(environment); + EXPECT_SOME(error); + EXPECT_EQ( + "Environment variable 'ENV_VAR_KEY' specifies a secret containing " + "null bytes, which is not allowed in the environment", + error->message); + + // Test the valid case. + variable->mutable_secret()->mutable_value()->set_data("SECRET_VALUE"); + error = validateEnvironment(environment); + EXPECT_NONE(error); + } + + // Validate a variable of VALUE type. + { + // The default type for an environment variable + // should be VALUE, so we do not set the type here. + Environment environment; + Environment::Variable* variable = environment.mutable_variables()->Add(); + variable->set_name("ENV_VAR_KEY"); + + Option<Error> error = validateEnvironment(environment); + EXPECT_SOME(error); + EXPECT_EQ( + "Environment variable 'ENV_VAR_KEY' of type 'VALUE' " + "must have a value set", + error->message); + + variable->set_value("ENV_VAR_VALUE"); + + Secret secret; + secret.set_type(Secret::VALUE); + secret.mutable_value()->set_data("SECRET_VALUE"); + variable->mutable_secret()->CopyFrom(secret); + + error = validateEnvironment(environment); + EXPECT_SOME(error); + EXPECT_EQ( + "Environment variable 'ENV_VAR_KEY' of type 'VALUE' " + "must not have a secret set", + error->message); + + // Test the valid case. + variable->clear_secret(); + error = validateEnvironment(environment); + EXPECT_NONE(error); + } + + // Validate a variable of UNKNOWN type. + { + Environment environment; + Environment::Variable* variable = environment.mutable_variables()->Add(); + variable->set_type(mesos::Environment::Variable::UNKNOWN); + variable->set_name("ENV_VAR_KEY"); + variable->set_value("ENV_VAR_VALUE"); + + Option<Error> error = validateEnvironment(environment); + EXPECT_SOME(error); + EXPECT_EQ( + "Environment variable of type 'UNKNOWN' is not allowed", + error->message); + } +} + + TEST(AgentCallValidationTest, LaunchNestedContainer) { // Missing `launch_nested_container`. @@ -177,9 +282,9 @@ TEST(AgentCallValidationTest, LaunchNestedContainer) error = validation::agent::call::validate(call); EXPECT_SOME(error); - // Valid `container_id.parent` but invalid `command.environment`. Currently, - // `Environment.Variable.Value` must be set, but this constraint will be - // removed in a future version. + // Valid `container_id.parent` but invalid `command.environment`. Set + // an invalid environment variable to check that the common validation + // code for the command's environment is being executed. ContainerID parentContainerId; parentContainerId.set_value(UUID::random().toString()); @@ -192,12 +297,13 @@ TEST(AgentCallValidationTest, LaunchNestedContainer) ->mutable_variables() ->Add(); variable->set_name("ENV_VAR_KEY"); + variable->set_type(mesos::Environment::Variable::VALUE); error = validation::agent::call::validate(call); EXPECT_SOME(error); EXPECT_EQ( "'launch_nested_container.command' is invalid: Environment variable " - "'ENV_VAR_KEY' must have a value set", + "'ENV_VAR_KEY' of type 'VALUE' must have a value set", error->message); // Test the valid case. @@ -311,9 +417,9 @@ TEST(AgentCallValidationTest, LaunchNestedContainerSession) error = validation::agent::call::validate(call); EXPECT_SOME(error); - // Valid `container_id.parent` but invalid `command.environment`. Currently, - // `Environment.Variable.Value` must be set, but this constraint will be - // removed in a future version. + // Valid `container_id.parent` but invalid `command.environment`. Set + // an invalid environment variable to check that the common validation + // code for the command's environment is being executed. ContainerID parentContainerId; parentContainerId.set_value(UUID::random().toString()); @@ -326,12 +432,13 @@ TEST(AgentCallValidationTest, LaunchNestedContainerSession) ->mutable_variables() ->Add(); variable->set_name("ENV_VAR_KEY"); + variable->set_type(mesos::Environment::Variable::VALUE); error = validation::agent::call::validate(call); EXPECT_SOME(error); EXPECT_EQ( "'launch_nested_container_session.command' is invalid: Environment " - "variable 'ENV_VAR_KEY' must have a value set", + "variable 'ENV_VAR_KEY' of type 'VALUE' must have a value set", error->message); // Test the valid case.
