Use the ECS RunTask API to run attached processes #1043

ejholmes · 2017-03-01T01:06:43Z

Not ready to merge.

This is a prototype for switching emp run to use the ECS RunTask API, instead of calling out to Docker directly. The primary motivation behind this is to allow containers started with emp run to use AWS Roles for ECS tasks. This is described in https://github.com/remind101/empire/wiki/RFC:-Use-RunTask-API-for-attached-runs.

There's some serious caveats with this implementation at the moment:

Because we use AttachToContainer, the container needs to be started with the Tty and OpenStdin flags set to true. Unfortunately, the ECS API and the ECS agent don't currently provide a method to pass these down. We would currently need to fork the agent.
I haven't yet addressed backwards compatibility. It may be a good idea to provide a flag to use the old method for running attached runs entirely through Docker.

On the plus side, this sets up some infrastructure for adding an emp exec/attach command for starting up a process in an existing container, or attaching to a running process.

TODO

Figure out how to pass down Tty/OpenStdin flags.
Add the ability to limit attached runs to a group of hosts.
Enable this feature with a flag.
~~Add ability to limit normal containers to a group of hosts.~~
Docs
- Expiremental
- Document Docker TLS requirements
- Document need for ECS agent patch.

ejholmes · 2017-03-01T01:09:59Z

To expand a bit on the idea of adding a method to provide the Tty and OpenStdin flags to CreateContainer in the ECS agent, now that Go 1.8 is released and has support for plugins, we could add a feature in the ECS agent for "task plugins", to manipulate the task before starting. I'll open an issue on the amazon-ecs-agent repo to see if there would be any interest in supporting something like that.

phobologic · 2017-03-01T04:32:26Z

Oh that would be awesome if they were down for it.

ejholmes · 2017-03-07T03:45:55Z

cmd/empire/main.go

@@ -57,6 +57,8 @@ const (
 	FlagECSServiceRole       = "ecs.service.role"
 	FlagECSLogDriver         = "ecs.logdriver"
 	FlagECSLogOpts           = "ecs.logopt"
+	FlagECSAttachedEnabled   = "ecs.attached.enabled"
+	FlagECSAttachedPlacement = "ecs.attached.placement"


This should probably also allow you to set placement constraints for normal procs and detached procs. If not (and assuming you want to restrict attached processes to a group of hosts), normal tasks could get scheduled onto the hosts that you've designated for attached runs.

ejholmes · 2017-03-09T04:18:04Z

scheduler/cloudformation/cloudformation.go

+
+	// Wait for the task to start running. It will stay in the
+	// PENDING state while the container is being pulled.
+	if err := m.ecs.WaitUntilTasksRunning(&ecs.DescribeTasksInput{


This should actually wait until it's RUNNING or STOPPED. If you run something that returns quickly (e.g. emp run echo hello world), the task could hit STOPPED before this method had a change to actually see it was running. It should be fine to AttachToContainer on a stopped task, since the ECS agent waits 3 hours to cleanup containers.

ejholmes · 2017-03-10T01:12:17Z

Ok, @phobologic. If you wanna give this another look, I think this is probably good to go. We've been running this on staging, without any hitches so far.

Primary changes since the last review:

Added docs/caveats about the new method, and requirements for patching the ECS agent.
Fixed some bugs with attaching to tasks that enter RUNNING for a brief period of time.
Stripped out support for placement constraints. This requires a more thoughtful implementation, and isn't necessary for most use cases. I'll follow up on this later.
Added an Attaching to task/<task id>... message, since we no longer see the Docker pull output (yay!).

The first commit can just be ignored, since it's just an update to the AWS SDK.

ejholmes · 2017-03-10T03:37:17Z

Noticed some zombie procs lying around. This should probably try a StopTask after detaching.

phobologic

Looks good so far!

phobologic · 2017-03-10T04:25:45Z

CHANGELOG.md

@@ -8,6 +8,7 @@
 * An ALIAS record is now created for `<process>.<app>.<zone>` [#1005](https://github.com/remind101/empire/pull/1005)
 * You can now provide a `-p` flag to the `emp cert-attach` command to attach a certificate to a specific process (instead of just `web`). [#1014](https://github.com/remind101/empire/pull/1014)
 * Empire now supports a SAML authentication backend. [#1017](https://github.com/remind101/empire/pull/1017)
+* Empire supports a new (experimental) feature to enabled attached processes to be ran with ECS. [#1043](https://github.com/remind101/empire/pull/1043)


to enabled -> to enable

Good catch. I'll update.

phobologic · 2017-03-10T04:27:35Z

cmd/empire/main.go

@@ -310,6 +311,11 @@ var EmpireFlags = []cli.Flag{
 		Usage:  "Log driver to options. Maps to the --log-opt docker cli arg",
 		EnvVar: "EMPIRE_ECS_LOG_OPT",
 	},
+	cli.BoolFlag{
+		Name:   FlagECSAttachedEnabled,
+		Usage:  "When enabled, indicates that ECS tasks can be attached to, using `docker attach`. When provided, this will also use ECS to run attached processes.",


With this requiring the patched agent, it might be worth pointing at the doc that details what this means.

Good call. I'll update.

phobologic · 2017-03-10T04:29:06Z

scheduler/cloudformation/clients.go

+	*ecs.ECS
+}
+
+func (c *ECS) WaitUntilTasksNotPending(input *ecs.DescribeTasksInput) error {


my linter is going to complain about no docstring here :)

phobologic · 2017-03-10T04:30:47Z

scheduler/cloudformation/cloudformation.go

+		// Amazon ECS Container Agent, since the official agent doesn't
+		// provide a method to pass these down to the `CreateContainer`
+		// call.
+		containerDefinition.DockerLabels["docker.config.Tty"] = aws.String("true")


Curious why both?

The Tty flag allocates a psuedo TTY for the container, and OpenStdin opens the stdin stream.

Without OpenStdin, running something like emp run bash would just exit as soon as bash hit the prompt. Without Tty, I think most commands that expect a TTY/terminal wouldn't work (If I try docker run -i ubuntu bash (missing -t flag), my terminal just hangs).

👍 ah, right - not sure why I didn't tie this to docker run --it

phobologic · 2017-03-10T04:37:53Z

scheduler/cloudformation/cloudformation.go

+
+	// Wait for the task to start running. It will stay in the
+	// PENDING state while the container is being pulled.
+	if err := m.ecs.WaitUntilTasksNotPending(&ecs.DescribeTasksInput{


Wondering if we need some sort of timeout here?

This will timeout after 10 minutes (6 second delay between refreshes, and 100 retries).

phobologic · 2017-03-10T04:38:55Z

scheduler/cloudformation/cloudformation.go

+	// instance where the task is running.
+	d, err := m.NewDockerClient(ec2Instance)
+	if err != nil {
+		return fmt.Errorf("error establishing docker client connection to %s", aws.StringValue(ec2Instance.InstanceId))


Will this bubble up with the task id, etc? Might want to put some more info in this so it's easier to figure out what happened.

Not atm. I'll add that information to the error.

Actually, it does already show this, since it's output above the error (Attaching to task/<task id>...).

Oh, but this is totally missing the err.

ejholmes · 2017-03-14T04:31:51Z

This has been working out pretty well for us in our staging environment without any major issues/bugs. Gonna go ahead and merge.

ejholmes force-pushed the ecs-run branch 2 times, most recently from a66a9f2 to 390fb78 Compare March 7, 2017 01:57

ejholmes commented Mar 7, 2017

View reviewed changes

ejholmes force-pushed the ecs-run branch from c807aa9 to 8bdf700 Compare March 9, 2017 03:30

ejholmes commented Mar 9, 2017

View reviewed changes

ejholmes force-pushed the ecs-run branch from 7f134ce to 7628249 Compare March 10, 2017 00:55

phobologic suggested changes Mar 10, 2017

View reviewed changes

ejholmes force-pushed the ecs-run branch from 037a575 to f463e5f Compare March 10, 2017 09:54

phobologic approved these changes Mar 13, 2017

View reviewed changes

ejholmes force-pushed the ecs-run branch 3 times, most recently from aef931d to 2f792d9 Compare March 14, 2017 03:36

Uses the ECS RunTask API to run attached processes.

06c8066

ejholmes force-pushed the ecs-run branch from 2f792d9 to 06c8066 Compare March 14, 2017 04:25

ejholmes merged commit 5bcfd21 into master Mar 14, 2017

ejholmes deleted the ecs-run branch March 14, 2017 04:31

ejholmes mentioned this pull request Sep 19, 2017

v0.13.0 #1107

Merged

tyrken mentioned this pull request Jul 13, 2018

ECS Task roles not working in emp run without patched ECS agent #1128

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the ECS RunTask API to run attached processes #1043

Use the ECS RunTask API to run attached processes #1043

ejholmes commented Mar 1, 2017 •

edited

Loading

ejholmes commented Mar 1, 2017

phobologic commented Mar 1, 2017

ejholmes Mar 7, 2017

ejholmes Mar 9, 2017

ejholmes commented Mar 10, 2017

ejholmes commented Mar 10, 2017

phobologic left a comment

phobologic Mar 10, 2017

ejholmes Mar 10, 2017

phobologic Mar 10, 2017

ejholmes Mar 10, 2017

phobologic Mar 10, 2017

phobologic Mar 10, 2017

ejholmes Mar 10, 2017

phobologic Mar 13, 2017 •

edited

Loading

phobologic Mar 10, 2017

ejholmes Mar 10, 2017

phobologic Mar 13, 2017

phobologic Mar 10, 2017

ejholmes Mar 10, 2017

ejholmes Mar 10, 2017

ejholmes Mar 10, 2017

ejholmes commented Mar 14, 2017

Use the ECS RunTask API to run attached processes #1043

Use the ECS RunTask API to run attached processes #1043

Conversation

ejholmes commented Mar 1, 2017 • edited Loading

ejholmes commented Mar 1, 2017

phobologic commented Mar 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejholmes commented Mar 10, 2017

ejholmes commented Mar 10, 2017

phobologic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phobologic Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejholmes commented Mar 14, 2017

ejholmes commented Mar 1, 2017 •

edited

Loading

phobologic Mar 13, 2017 •

edited

Loading