Added enforce_container_ports flag for `network/ports` isolation. To reduce deployment risk, a no-enforce mode is added for the `network/ports` isolator. When this flag is false (which is the default) the isolator will not raise any limitation, even if the task uses ports not in its container resources.
Added new test for this flag and updated the existing tests. Review: https://reviews.apache.org/r/67195/ Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/24359e64 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/24359e64 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/24359e64 Branch: refs/heads/master Commit: 24359e64323e65e90e07b485840b334968307f15 Parents: 4f95d75 Author: Xudong Ni <[email protected]> Authored: Thu May 24 17:06:40 2018 -0700 Committer: James Peach <[email protected]> Committed: Thu May 24 17:12:00 2018 -0700 ---------------------------------------------------------------------- docs/configuration/agent.md | 9 ++ docs/isolators/network-ports.md | 6 + .../mesos/isolators/network/ports.cpp | 25 +++- .../mesos/isolators/network/ports.hpp | 3 + src/slave/flags.cpp | 5 + src/slave/flags.hpp | 1 + .../containerizer/ports_isolator_tests.cpp | 126 +++++++++++++++++++ 7 files changed, 170 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/24359e64/docs/configuration/agent.md ---------------------------------------------------------------------- diff --git a/docs/configuration/agent.md b/docs/configuration/agent.md index 962211a..af0c040 100644 --- a/docs/configuration/agent.md +++ b/docs/configuration/agent.md @@ -662,6 +662,15 @@ is used by the <code>disk/du</code> and <code>disk/xfs</code> isolators. (defaul </tr> <tr> <td> + --[no-]enforce_container_ports + </td> + <td> +Whether to enable network port enforcement for containers. This flag +is used by the <code>network/ports</code> isolator. (default: false) + </td> +</tr> +<tr> + <td> --executor_environment_variables=VALUE </td> <td> http://git-wip-us.apache.org/repos/asf/mesos/blob/24359e64/docs/isolators/network-ports.md ---------------------------------------------------------------------- diff --git a/docs/isolators/network-ports.md b/docs/isolators/network-ports.md index ea63968..5d14fc2 100644 --- a/docs/isolators/network-ports.md +++ b/docs/isolators/network-ports.md @@ -41,6 +41,12 @@ to use the Linux launcher (i.e. the agent has the `--launcher=linux` flag), because it uses Linux cgroups to track the processes belonging to a Mesos task. +The `--enforce_container_ports` flag, specifies whether the network +ports isolator should terminate tasks that listen on ports they have +not been assigned. If enforcement is disabled, the isolator will log +violations but will not terminate tasks. By default, network port +enforcement is disabled. + The `--container_ports_watch_interval` flag specifies the interval between task port reconciliations. http://git-wip-us.apache.org/repos/asf/mesos/blob/24359e64/src/slave/containerizer/mesos/isolators/network/ports.cpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/network/ports.cpp b/src/slave/containerizer/mesos/isolators/network/ports.cpp index 53e894d..82c1441 100644 --- a/src/slave/containerizer/mesos/isolators/network/ports.cpp +++ b/src/slave/containerizer/mesos/isolators/network/ports.cpp @@ -343,6 +343,7 @@ Try<Isolator*> NetworkPortsIsolatorProcess::create(const Flags& flags) new NetworkPortsIsolatorProcess( strings::contains(flags.isolation, "network/cni"), flags.container_ports_watch_interval, + flags.enforce_container_ports, flags.cgroups_root, freezerHierarchy.get(), agentPorts))); @@ -352,12 +353,14 @@ Try<Isolator*> NetworkPortsIsolatorProcess::create(const Flags& flags) NetworkPortsIsolatorProcess::NetworkPortsIsolatorProcess( bool _cniIsolatorEnabled, const Duration& _watchInterval, + const bool& _enforceContainerPorts, const string& _cgroupsRoot, const string& _freezerHierarchy, const Option<IntervalSet<uint16_t>>& _agentPorts) : ProcessBase(process::ID::generate("network-ports-isolator")), cniIsolatorEnabled(_cniIsolatorEnabled), watchInterval(_watchInterval), + enforceContainerPorts(_enforceContainerPorts), cgroupsRoot(_cgroupsRoot), freezerHierarchy(_freezerHierarchy), agentPorts(_agentPorts) @@ -574,6 +577,16 @@ Future<Nothing> NetworkPortsIsolatorProcess::check( const IntervalSet<uint16_t> unallocatedPorts = ports - info->allocatedPorts.get(); + // Only log unallocated ports once to prevent excessive logging + // for the same unallocated ports while port enforcement is disabled. + if (info->activePorts.isSome() && info->activePorts == ports) { + continue; + } + + // Cache the last listeners port sample so that we will + // only log new ports resource violations. + info->activePorts = ports; + Resource resource; resource.set_name("ports"); resource.set_type(Value::RANGES); @@ -587,11 +600,13 @@ Future<Nothing> NetworkPortsIsolatorProcess::check( LOG(INFO) << message; - info->limitation.set( - protobuf::slave::createContainerLimitation( - Resources(resource), - message, - TaskStatus::REASON_CONTAINER_LIMITATION)); + if (enforceContainerPorts) { + info->limitation.set( + protobuf::slave::createContainerLimitation( + Resources(resource), + message, + TaskStatus::REASON_CONTAINER_LIMITATION)); + } } } http://git-wip-us.apache.org/repos/asf/mesos/blob/24359e64/src/slave/containerizer/mesos/isolators/network/ports.hpp ---------------------------------------------------------------------- diff --git a/src/slave/containerizer/mesos/isolators/network/ports.hpp b/src/slave/containerizer/mesos/isolators/network/ports.hpp index d9a97d1..8d467ce 100644 --- a/src/slave/containerizer/mesos/isolators/network/ports.hpp +++ b/src/slave/containerizer/mesos/isolators/network/ports.hpp @@ -85,6 +85,7 @@ private: NetworkPortsIsolatorProcess( bool _cniIsolatorEnabled, const Duration& _watchInterval, + const bool& _enforcePortsEnabled, const std::string& _cgroupsRoot, const std::string& _freezerHierarchy, const Option<IntervalSet<uint16_t>>& agentPorts); @@ -92,11 +93,13 @@ private: struct Info { Option<IntervalSet<uint16_t>> allocatedPorts; + Option<IntervalSet<uint16_t>> activePorts; process::Promise<mesos::slave::ContainerLimitation> limitation; }; const bool cniIsolatorEnabled; const Duration watchInterval; + const bool enforceContainerPorts; const std::string cgroupsRoot; const std::string freezerHierarchy; const Option<IntervalSet<uint16_t>> agentPorts; http://git-wip-us.apache.org/repos/asf/mesos/blob/24359e64/src/slave/flags.cpp ---------------------------------------------------------------------- diff --git a/src/slave/flags.cpp b/src/slave/flags.cpp index a319b5e..23d9bb1 100644 --- a/src/slave/flags.cpp +++ b/src/slave/flags.cpp @@ -1117,6 +1117,11 @@ mesos::internal::slave::Flags::Flags() "published by the agent's resources. Otherwise tasks are restricted\n" "to only listen on ports for which they have been assigned resources.", false); + add(&Flags::enforce_container_ports, + "enforce_container_ports", + "Whether to enable port enforcement for containers. This flag\n" + "is used by `network/ports` isolator.", + false); #endif // ENABLE_NETWORK_PORTS_ISOLATOR add(&Flags::network_cni_plugins_dir, http://git-wip-us.apache.org/repos/asf/mesos/blob/24359e64/src/slave/flags.hpp ---------------------------------------------------------------------- diff --git a/src/slave/flags.hpp b/src/slave/flags.hpp index a839591..ae09e19 100644 --- a/src/slave/flags.hpp +++ b/src/slave/flags.hpp @@ -152,6 +152,7 @@ public: #ifdef ENABLE_NETWORK_PORTS_ISOLATOR Duration container_ports_watch_interval; bool check_agent_port_range_only; + bool enforce_container_ports; #endif // ENABLE_NETWORK_PORTS_ISOLATOR Option<std::string> network_cni_plugins_dir; http://git-wip-us.apache.org/repos/asf/mesos/blob/24359e64/src/tests/containerizer/ports_isolator_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/containerizer/ports_isolator_tests.cpp b/src/tests/containerizer/ports_isolator_tests.cpp index c5b9f92..db080c4 100644 --- a/src/tests/containerizer/ports_isolator_tests.cpp +++ b/src/tests/containerizer/ports_isolator_tests.cpp @@ -250,6 +250,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_CommandExecutorPorts) flags.isolation = "network/ports"; flags.launcher = "linux"; flags.check_agent_port_range_only = false; + flags.enforce_container_ports = true; Owned<MasterDetector> detector = master.get()->createDetector(); @@ -343,6 +344,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_AllocatedPorts) // Watch only the agent ports resources range because we want this // test to trigger on the nc command, not on the command executor. flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; Owned<MasterDetector> detector = master.get()->createDetector(); @@ -496,6 +498,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_NoPortsResource) // Watch only the agent ports resources range because we want this // test to trigger on the nc command, not on the command executor. flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; Owned<MasterDetector> detector = master.get()->createDetector(); @@ -615,6 +618,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_DefaultPortsResource) // Watch only the agent ports resources range because we want this // test to trigger on the nc command, not on the command executor. flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; Owned<MasterDetector> detector = master.get()->createDetector(); @@ -726,6 +730,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_UnallocatedPorts) // Watch only the agent ports resources range because we want this // test to trigger on the nc command, not on the command executor. flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; Owned<MasterDetector> detector = master.get()->createDetector(); @@ -824,6 +829,122 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_UnallocatedPorts) } +// This test verifies that a task that listens on a port for which +// it has no resources is detected and will not be killed by +// a container limitation if enforce_container_ports is false. +TEST_F(NetworkPortsIsolatorTest, ROOT_NC_NoPortEnforcement) +{ + Try<Owned<cluster::Master>> master = StartMaster(); + ASSERT_SOME(master); + + slave::Flags flags = CreateSlaveFlags(); + flags.isolation = "network/ports"; + flags.launcher = "linux"; + + // Watch only the agent ports resources range because we want this + // test to trigger on the nc command, not on the command executor. + flags.check_agent_port_range_only = true; + flags.enforce_container_ports = false; + + Owned<MasterDetector> detector = master.get()->createDetector(); + + Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), flags); + ASSERT_SOME(slave); + + MockScheduler sched; + + MesosSchedulerDriver driver( + &sched, + DEFAULT_FRAMEWORK_INFO, + master.get()->pid, + DEFAULT_CREDENTIAL); + + EXPECT_CALL(sched, registered(&driver, _, _)); + + Future<vector<Offer>> offers; + EXPECT_CALL(sched, resourceOffers(&driver, _)) + .WillOnce(FutureArg<1>(&offers)) + .WillRepeatedly(Return()); // Ignore subsequent offers. + + driver.start(); + + AWAIT_READY(offers); + ASSERT_EQ(1u, offers->size()); + + const Offer& offer = offers.get()[0]; + + // Make sure we have a `ports` resource. + Resources resources(offer.resources()); + ASSERT_SOME(resources.ports()); + ASSERT_LE(1, resources.ports()->range().size()); + + uint16_t taskPort = selectRandomPort(resources); + uint16_t usedPort = selectOtherPort(resources, taskPort); + + resources = Resources::parse( + "cpus:1;mem:32;" + "ports:[" + stringify(taskPort) + "," + stringify(taskPort) + "]").get(); + + // Launch a task that uses a port that it hasn't been allocated. Use + // "nc -k" so nc keeps running after accepting the healthcheck connection. + TaskInfo task = createTask( + offer.slave_id(), + resources, + "nc -k -l " + stringify(usedPort)); + + addTcpHealthCheck(task, usedPort); + + Future<TaskStatus> startingStatus; + Future<TaskStatus> runningStatus; + Future<TaskStatus> healthStatus; + EXPECT_CALL(sched, statusUpdate(&driver, _)) + .WillOnce(FutureArg<1>(&startingStatus)) + .WillOnce(FutureArg<1>(&runningStatus)) + .WillOnce(FutureArg<1>(&healthStatus)); + + driver.launchTasks(offer.id(), {task}); + + awaitStatusUpdateAcked(startingStatus); + EXPECT_EQ(task.task_id(), startingStatus->task_id()); + EXPECT_EQ(TASK_STARTING, startingStatus->state()); + + awaitStatusUpdateAcked(runningStatus); + EXPECT_EQ(task.task_id(), runningStatus->task_id()); + EXPECT_EQ(TASK_RUNNING, runningStatus->state()); + + awaitStatusUpdateAcked(healthStatus); + ASSERT_EQ(task.task_id(), healthStatus->task_id()); + expectHealthyStatus(healthStatus.get()); + + Future<Nothing> check = + FUTURE_DISPATCH(_, &NetworkPortsIsolatorProcess::check); + + Clock::pause(); + Clock::advance(flags.container_ports_watch_interval); + + AWAIT_READY(check); + + Clock::settle(); + Clock::resume(); + + // Since container ports are not being enforced, we expect that the task + // should still be running after the check and that we should be able to + // explicitly kill it. + Future<TaskStatus> killedStatus; + EXPECT_CALL(sched, statusUpdate(&driver, _)) + .WillOnce(FutureArg<1>(&killedStatus)); + + driver.killTask(task.task_id()); + + AWAIT_READY(killedStatus); + EXPECT_EQ(task.task_id(), killedStatus->task_id()); + EXPECT_EQ(TASK_KILLED, killedStatus->state()); + + driver.stop(); + driver.join(); +} + + // Test that after we recover a task, the isolator notices that it // is using the wrong ports and kills it. TEST_F(NetworkPortsIsolatorTest, ROOT_NC_RecoverBadTask) @@ -920,6 +1041,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_RecoverBadTask) // and terminate it. flags.isolation = "network/ports"; flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; slave = StartSlave(detector.get(), flags); ASSERT_SOME(slave); @@ -1044,6 +1166,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_RecoverGoodTask) // listening and will let it continue running. flags.isolation = "network/ports"; flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; Future<SlaveReregisteredMessage> slaveReregisteredMessage = FUTURE_PROTOBUF(SlaveReregisteredMessage(), _, _); @@ -1100,6 +1223,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_TaskGroup) flags.isolation = "network/ports"; flags.launcher = "linux"; flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; Owned<MasterDetector> detector = master.get()->createDetector(); Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), flags); @@ -1359,6 +1483,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_RecoverNestedBadTask) // and terminate it. flags.isolation = "network/ports"; flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; slave = cluster::Slave::create(detector.get(), flags, slaveId); ASSERT_SOME(slave); @@ -1536,6 +1661,7 @@ TEST_F(NetworkPortsIsolatorTest, ROOT_NC_RecoverNestedGoodTask) // listening and will let it continue running. flags.isolation = "network/ports"; flags.check_agent_port_range_only = true; + flags.enforce_container_ports = true; slave = cluster::Slave::create(detector.get(), flags, slaveId); ASSERT_SOME(slave);
