You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by vi...@apache.org on 2017/02/17 19:48:18 UTC
[2/5] mesos git commit: Added a 'SECRET' type to the 'Environment'
protobuf message.
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 <gr...@mesosphere.io>
Authored: Fri Feb 17 11:47:41 2017 -0800
Committer: Vinod Kone <vi...@gmail.com>
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.