Preliminary support for @SUMMONDOCKERARGS #186

doodlesbykumbi · 2020-12-30T15:23:59Z

Closes #194

@DOCKEROPTS is imagined to make it easier to lift summon secrets into docker. With usage like this in mind:

summon docker run @SUMMONDOCKERARGS -it alpine

~~Using the !file tag assumes that the file system where summon creates temporary files can be volume mounted to Docker.~~
If you use the !file tag you're on your own for now.

What does this PR do?

What's changed? Why were these changes made?
How should the reviewer approach this PR, especially if manual tests are required?
Are there relevant screenshots you can add to the PR description?

What ticket does this PR close?

Resolves #[relevant GitHub issues, eg 76]

Checklists

Change log

The CHANGELOG has been updated, or
This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

This PR includes new unit and integration tests to go with the code changes, or
The changes in this PR do not require tests

Documentation

Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs, or
This PR does not require updating any documentation

sgnn7 · 2020-12-30T17:58:14Z

404 on the tests :)

izgeri

I left some feedback on the docs updates. You should also include updates to the README with this change, since many people will see the README before they see the website (if they ever do).

I'm curious also how this change can help address some of the recent requests we've received in this project to better support multi-line secrets, in particular #184.

docs/_includes/docker.md

izgeri · 2021-01-12T17:07:38Z

docs/_includes/docker.md

+1. `summon` is
+invoking docker as the child process.


Suggested change

1. `summon` is

invoking docker as the child process.

1. `summon` is invoking `docker` as the child process.

Do you mean the docker CLI or something else?

Yup the docker CLI. I wonder, would it help to say:
summon is invoking the docker CLI as the child process.

izgeri · 2021-01-12T17:08:06Z

docs/_includes/docker.md

+1. `summon` is
+invoking docker as the child process.
+2. `@SUMMONDOCKERARGS` is replaced with a combination of `--env` and `--volume`
+arguments.


I suspect if this line isn't indented, it's going to display weird when the MD is rendered

It looks like markdown will assume that a new line is a continuation of the previous in this case, unless broken by an empty newline.

docs/_includes/docker.md

BradleyBoutcher · 2021-01-13T17:57:12Z

internal/command/action.go

+				dockerArgs = append(dockerArgs, "--volume", v+":"+v)
+			}
+			dockerArgs = append(dockerArgs, "--env", k)
+
 			results <- Result{k, v, nil}


Should we be zeroing results?

Probably a good idea, I don't know if Summon does any zeroization at all. But I think that piece is out of scope of this PR.

Isn't results being initialized as an empty channel (line 129 above)? So I think we shouldn't need to zero anything, unless I'm missing something?

BradleyBoutcher · 2021-01-13T18:41:29Z

internal/command/action.go

+		// 2. summon ... sh -c "docker run @SUMMONDOCKERARGS [...]", also replace substrings
+		// inside args but the replacement is as a single string.
+		//
+		// The code below should support (2). There'll be some ambiguity though...


I think we should support replacing substrings. I think lots of users wrap their subcommands with bin/bash -ec '...', and it would be better for us to continue to support that syntax. I personally can't think of a use case where someone would benefit from using that syntax over Summon directly calling docker, but it may be easier to ensure all docker subcommands are processed correctly.

I agree, we should include this support. Maybe there's some weird docker-in-docker use case where we're running summon inside a container that requires a bash/sh -c ...???

Uncomment, added feature and test case.

BradleyBoutcher · 2021-01-13T18:49:06Z

internal/command/action_test.go

@@ -120,6 +127,121 @@ func TestRunAction(t *testing.T) {

 		So(string(content), ShouldEqual, expectedValue)
 	})
+
+	Convey("Docker options correctly injected", t, func() {


So, from my understanding of how things currently work, we only support the following syntax:

summon docker @SUMMONDOCKERARGS someImage someCommand

That being said, we should also have tests for the following situations:

summon /bin/bash -c "docker @SUMMONDOCKERARGS someImage someCommand"

summon --yaml "VAR: !var some/path" /bin/bash -ec " docker @SUMMONDOCKERARGS someImage someCommand echo $VAR "

Although these may not be actual use cases, we should remove as much ambiguity as we can from possible situations.

We should also support the case where someone also uses @SUMMONENVFILE and @SUMMONDOCKEROPTS at the same time

I think it's supported by default. The 2 approaches aren't coupled in any way, and so shouldn't interact in anyway (at least on the Summon side of things).

@SUMMONENVFILE is just a string that points to an env file containing all the resolved values.

@SUMMONDOCKEROPTS output the Docker CLI options, which are combinations of --env and --volume, such that the environment variables injected by summon to the child process can become available inside a container created with these options.

That's understandable, but my concern would be changes we might make in the future surrounding how we parse these keywords prefixed with '@' and how we interact with docker args. I personally think it's safer to have tests in place now that ensure that changes don't have unintended conflicts in the future, and the features have enough overlap that it may be good to have a test, just in case. What do you think @diverdane ?

I thought about this a bit. I think a good way to proceed is to

Acknowledge that the 2 approaches are actually entirely unrelated, particularly in terms of implementation so their implementations need not be tested in unison.

Since they both result in modification of the arguments of the child process we can have a simple test that ensures that replacing doesn't get messed up.

diverdane

I'm liking it! A few nits, and a question on the path of volume mounted files in containers.

docs/_includes/docker.md

diverdane · 2021-01-13T20:17:44Z

docs/_includes/docker.md

+arguments.
+3. Variable `D` uses the `!file` tag and therefore is the only one that
+results in a `--volume` argument. The path to this variable inside the container
+is as it is on the host.


I'm assuming that a temp file is being created for these !file tagged variables. Will the location of the temp file on the host be host-OS dependent, or are we depending this to always be in /tmp? If it's OS dependent, then the path inside the container will be host-OS dependent, and therefore not 100% predictable.
Maybe we should recommend setting TMPDIR to some path on the host so that we can rely on this path inside the container?
Or maybe there's some Summon config, or something we can add to secrets.yml to indicate the destination location in the container?

Good point. This might get tricky if for host to container you're going from Windows to Linux or vice-versa. I'm wondering now if I should just altogether disregard the case for files since it adds complication. Having --env would already be a massive improvement.

I wonder if we can add some new secrets.yml syntax to allow users to specify a
destination path for files that Summon creates. This could be similar to what is being proposed in this Issue: #190.

The syntax proposed in Issue #190 is something like this:

summon --yaml 'FOO: !file=<path> contents' summon --yaml 'FOO: !file=<path> $env/aws/ec2/private_key'

For Issue #190, the <path> would specify a fixed, local location for Summon to create a file. (I don't think this syntax exists and/or is used for some other purpose.)

For @SUMMONDOCKERARGS, the <path> could potentially specify the location of the file as volume mounted in the container. So the resulting volume option for Docker could logically be:

--volume <local-temp-file-path>:<path>

I think this would be easy for users to understand.

Maybe this can be added as a followup... along with changes to the secretsyml to add this syntax?

WDYT?

I like the idea of allowing the user to specify the location that Summon creates files. Even though not every platform supports /dev/shm, the benefit of summon creating files is that it is also responsible for cleaning them up... as much as possible we should retain this for the users in the new features we add.

For now I got rid of the implicit --volume arguments. We'll let the user do that themselves. Some things to note

Summon does have a way to configure the tmp directory but it's difficult for the user to predict which one is being used. See https://github.com/cyberark/summon/blob/master/internal/command/temp_factory.go#L29. We try /dev/shm then os home directory then os tmp directory. It's not great!

I suppose we should just give the user a way to more explicitly specify this directory, then they can volume mount themselves. However there'd still be an issue of going from say Windows to Linux Containers, and vice versa. I suppose we'll need to look into this more.

docs/_includes/docker.md

internal/command/action.go

diverdane · 2021-01-13T21:51:55Z

internal/command/action.go

+		// 2. summon ... sh -c "docker run @SUMMONDOCKERARGS [...]", also replace substrings
+		// inside args but the replacement is as a single string.
+		//
+		// The code below should support (2). There'll be some ambiguity though...


I agree, we should include this support. Maybe there's some weird docker-in-docker use case where we're running summon inside a container that requires a bash/sh -c ...???

internal/command/action.go

diverdane

This LGTM, I really like the Docker mocking tests! We should probably figure out how we're going to handle the Docker volume mounts and where the temp files are mapped to inside the container. I think this feature can really be enhanced if we can incorporate new secrets.yml syntax !file=<path> to allow specification of the target file path. Maybe we can keep this change as is, and as a followup create a new PR to incorporate new syntax? Or skip the volume mounting altogether for now, and add this support later.

diverdane · 2021-01-21T17:39:28Z

Dockerfile.acceptance

@@ -5,7 +5,8 @@ RUN apk add --no-cache bash \
                       git \
                       libffi-dev \
                       ruby-bundler \
-                       ruby-dev
+                       ruby-dev \
+                       docker-cli


Should be moved up to retain alphabetical listing?

diverdane · 2021-01-21T17:51:23Z

docs/_includes/docker.md

+arguments.
+3. Variable `D` uses the `!file` tag and therefore is the only one that
+results in a `--volume` argument. The path to this variable inside the container
+is as it is on the host.


I wonder if we can add some new secrets.yml syntax to allow users to specify a
destination path for files that Summon creates. This could be similar to what is being proposed in this Issue: #190.

The syntax proposed in Issue #190 is something like this:

summon --yaml 'FOO: !file=<path> contents' summon --yaml 'FOO: !file=<path> $env/aws/ec2/private_key'

For Issue #190, the <path> would specify a fixed, local location for Summon to create a file. (I don't think this syntax exists and/or is used for some other purpose.)

For @SUMMONDOCKERARGS, the <path> could potentially specify the location of the file as volume mounted in the container. So the resulting volume option for Docker could logically be:

--volume <local-temp-file-path>:<path>

I think this would be easy for users to understand.

Maybe this can be added as a followup... along with changes to the secretsyml to add this syntax?

WDYT?

diverdane · 2021-01-22T12:37:59Z

internal/command/action.go

+				dockerArgs = append(dockerArgs, "--volume", v+":"+v)
+			}
+			dockerArgs = append(dockerArgs, "--env", k)
+
 			results <- Result{k, v, nil}


Isn't results being initialized as an empty channel (line 129 above)? So I think we shouldn't need to zero anything, unless I'm missing something?

diverdane · 2021-01-22T13:26:20Z

internal/command/action_test.go

+	}{}
+	envvars := map[string]string{}
+
+	// Mock server for handling API calls by `docker run`


This is some slick docker daemon mocking. I haven't used http mocks in a while, this is a good refresher.

jonahx · 2021-01-26T02:13:39Z

internal/command/action_test.go

+	// This is a test case for @SUMMONDOCKERARGS. It exercises Docker CLI pointed to a mock
+	// server. It asserts on the request payload received on the container creation
+	// endpoint, the volume mounts and environment variables injected by summon are
+	// expected to be present.


Super fly by comment, so ignore if this is missing something, but I wouldn't have expected this kind of test here. I would have thought that all we're doing is a pre-process, where the command is updated so that @SUMMONDOCKERARGS is replaced with the --env A --env B... start or whatever. Then after that, there is nothing new to test, because you're just running summon with a regular docker command.

doodlesbykumbi · 2021-02-02T14:11:35Z

Closing this in favour of #199

doodlesbykumbi requested a review from a team as a code owner December 30, 2020 15:23

doodlesbykumbi force-pushed the dockeropts branch 4 times, most recently from a52fa25 to 31f52b1 Compare December 30, 2020 20:25

doodlesbykumbi force-pushed the dockeropts branch 4 times, most recently from 8e0f550 to 7cf5a34 Compare January 11, 2021 17:53

doodlesbykumbi changed the title ~~Preliminary support for @DOCKEROPTS~~ Preliminary support for @SUMMONDOCKERARGS Jan 11, 2021

doodlesbykumbi force-pushed the dockeropts branch from 7cf5a34 to 3b6d286 Compare January 12, 2021 16:42

Preliminary support for @SUMMONDOCKERARGS

71e4da1

doodlesbykumbi force-pushed the dockeropts branch from 3b6d286 to 71e4da1 Compare January 12, 2021 16:50

izgeri reviewed Jan 12, 2021

View reviewed changes

BradleyBoutcher reviewed Jan 13, 2021

View reviewed changes

diverdane reviewed Jan 13, 2021

View reviewed changes

Address PR feedback

ba7a5c2

diverdane reviewed Jan 22, 2021

View reviewed changes

Remove support for --volume arguments in SUMMONDOCKERARGS

4126897

jonahx reviewed Jan 26, 2021

View reviewed changes

doodlesbykumbi closed this Feb 2, 2021

doodlesbykumbi deleted the dockeropts branch February 2, 2021 14:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preliminary support for @SUMMONDOCKERARGS #186

Preliminary support for @SUMMONDOCKERARGS #186

doodlesbykumbi commented Dec 30, 2020 •

edited

Loading

sgnn7 commented Dec 30, 2020

izgeri left a comment

izgeri Jan 12, 2021

doodlesbykumbi Jan 20, 2021

izgeri Jan 12, 2021

doodlesbykumbi Jan 20, 2021

BradleyBoutcher Jan 13, 2021

doodlesbykumbi Jan 20, 2021

diverdane Jan 22, 2021

BradleyBoutcher Jan 13, 2021

diverdane Jan 13, 2021

doodlesbykumbi Jan 20, 2021

BradleyBoutcher Jan 13, 2021

BradleyBoutcher Jan 13, 2021

doodlesbykumbi Jan 20, 2021

BradleyBoutcher Jan 21, 2021 •

edited

Loading

doodlesbykumbi Jan 25, 2021

diverdane left a comment

diverdane Jan 13, 2021 •

edited

Loading

doodlesbykumbi Jan 20, 2021

diverdane Jan 21, 2021

doodlesbykumbi Jan 25, 2021

diverdane Jan 13, 2021

diverdane left a comment •

edited

Loading

diverdane Jan 21, 2021

doodlesbykumbi Jan 25, 2021

diverdane Jan 21, 2021

diverdane Jan 22, 2021

diverdane Jan 22, 2021

jonahx Jan 26, 2021

doodlesbykumbi commented Feb 2, 2021

	1. `summon` is
	invoking docker as the child process.
	1. `summon` is invoking `docker` as the child process.

Preliminary support for @SUMMONDOCKERARGS #186

Preliminary support for @SUMMONDOCKERARGS #186

Conversation

doodlesbykumbi commented Dec 30, 2020 • edited Loading

What does this PR do?

What ticket does this PR close?

Checklists

Change log

Test coverage

Documentation

sgnn7 commented Dec 30, 2020

izgeri 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

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

BradleyBoutcher Jan 21, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diverdane left a comment

Choose a reason for hiding this comment

diverdane Jan 13, 2021 • 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

diverdane left a comment • 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

doodlesbykumbi commented Feb 2, 2021

doodlesbykumbi commented Dec 30, 2020 •

edited

Loading

BradleyBoutcher Jan 21, 2021 •

edited

Loading

diverdane Jan 13, 2021 •

edited

Loading

diverdane left a comment •

edited

Loading