From 0c8470d77442f35de6a153411f2e905fe166e291 Mon Sep 17 00:00:00 2001 From: Ollie Edwards Date: Thu, 21 Nov 2019 18:26:40 +0000 Subject: [PATCH 1/3] Fix native build test so that environment vars are passed to native builder as intended The existing test was passing DOCKER_BUILDKIT=1 as an env var but the test was passing without using buildkit Adds buildkit only features to the Dockerfile so that it will fail unless buildkit is invoked. It also ensures that the test environment is passed to the forked native build process Signed-off-by: Ollie Edwards --- compose/service.py | 13 ++++++++++--- tests/integration/service_test.py | 16 ++++++++-------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/compose/service.py b/compose/service.py index d329be97992..5a8b436da0f 100644 --- a/compose/service.py +++ b/compose/service.py @@ -1080,7 +1080,7 @@ def build(self, no_cache=False, pull=False, force_rm=False, memory=None, build_a 'Impossible to perform platform-targeted builds for API version < 1.35' ) - builder = self.client if not cli else _CLIBuilder(progress) + builder = self.client if not cli else _CLIBuilder(progress, self.options.get('environment')) build_output = builder.build( path=path, tag=self.image_name, @@ -1717,8 +1717,11 @@ def rewrite_build_path(path): class _CLIBuilder(object): - def __init__(self, progress): + def __init__(self, progress, environment=None): self._progress = progress + if environment is None: + environment = {} + self._environment = environment def build(self, path, tag=None, quiet=False, fileobj=None, nocache=False, rm=False, timeout=None, @@ -1801,7 +1804,11 @@ def build(self, path, tag=None, quiet=False, fileobj=None, magic_word = "Successfully built " appear = False - with subprocess.Popen(args, stdout=subprocess.PIPE, universal_newlines=True) as p: + sub_env = merge_environment( + os.environ.copy(), + self._environment + ) + with subprocess.Popen(args, stdout=subprocess.PIPE, universal_newlines=True, env=sub_env) as p: while True: line = p.stdout.readline() if not line: diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index c50aab08bb2..213a6441b57 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -7,6 +7,7 @@ import tempfile from distutils.spawn import find_executable from os import path +from textwrap import dedent import pytest from docker.errors import APIError @@ -973,12 +974,16 @@ def test_build_cli(self): self.addCleanup(shutil.rmtree, base_dir) with open(os.path.join(base_dir, 'Dockerfile'), 'w') as f: - f.write("FROM busybox\n") + # f.write("FROM busybox\n") + f.write(dedent("""\ + # syntax=docker/dockerfile:1.0.0-experimental + FROM busybox + RUN --mount=type=secret,target=/secret_1 true + """)) service = self.create_service('web', build={'context': base_dir}, environment={ - 'COMPOSE_DOCKER_CLI_BUILD': '1', 'DOCKER_BUILDKIT': '1', }) service.build(cli=True) @@ -992,12 +997,7 @@ def test_up_build_cli(self): with open(os.path.join(base_dir, 'Dockerfile'), 'w') as f: f.write("FROM busybox\n") - web = self.create_service('web', - build={'context': base_dir}, - environment={ - 'COMPOSE_DOCKER_CLI_BUILD': '1', - 'DOCKER_BUILDKIT': '1', - }) + web = self.create_service('web', build={'context': base_dir}) project = Project('composetest', [web], self.client) project.up(do_build=BuildAction.force) From 26fe5d7681784c4d5d89ca7ea418aca49e8e66f4 Mon Sep 17 00:00:00 2001 From: Ollie Edwards Date: Fri, 22 Nov 2019 08:03:06 +0000 Subject: [PATCH 2/3] adds build time secret support This is done using the build section in the yaml which may look like a strange decision given build secrets are almost by definition not in a well known location. It's intended that this feature be used with env substitution. Signed-off-by: Ollie Edwards --- compose/config/config_schema_v3.7.json | 3 +- compose/service.py | 9 +++-- tests/acceptance/cli_test.py | 21 ++++++++++++ tests/fixtures/build-secrets/Dockerfile | 8 +++++ .../fixtures/build-secrets/docker-compose.yml | 7 ++++ tests/fixtures/build-secrets/secret_1 | 1 + tests/fixtures/build-secrets/secret_2 | 1 + tests/integration/service_test.py | 33 +++++++++++++++++++ 8 files changed, 80 insertions(+), 3 deletions(-) create mode 100644 tests/fixtures/build-secrets/Dockerfile create mode 100644 tests/fixtures/build-secrets/docker-compose.yml create mode 100644 tests/fixtures/build-secrets/secret_1 create mode 100644 tests/fixtures/build-secrets/secret_2 diff --git a/compose/config/config_schema_v3.7.json b/compose/config/config_schema_v3.7.json index cd7882f5b24..dbdac57c3ed 100644 --- a/compose/config/config_schema_v3.7.json +++ b/compose/config/config_schema_v3.7.json @@ -88,7 +88,8 @@ "cache_from": {"$ref": "#/definitions/list_of_strings"}, "network": {"type": "string"}, "target": {"type": "string"}, - "shm_size": {"type": ["integer", "string"]} + "shm_size": {"type": ["integer", "string"]}, + "secrets": {"$ref": "#/definitions/list_of_strings"} }, "additionalProperties": false } diff --git a/compose/service.py b/compose/service.py index 5a8b436da0f..949da3ed6ef 100644 --- a/compose/service.py +++ b/compose/service.py @@ -1080,7 +1080,9 @@ def build(self, no_cache=False, pull=False, force_rm=False, memory=None, build_a 'Impossible to perform platform-targeted builds for API version < 1.35' ) - builder = self.client if not cli else _CLIBuilder(progress, self.options.get('environment')) + builder = self.client if not cli else _CLIBuilder(progress, + self.options.get('environment'), + build_opts.get('secrets')) build_output = builder.build( path=path, tag=self.image_name, @@ -1717,11 +1719,12 @@ def rewrite_build_path(path): class _CLIBuilder(object): - def __init__(self, progress, environment=None): + def __init__(self, progress, environment=None, secrets=None): self._progress = progress if environment is None: environment = {} self._environment = environment + self._secrets = secrets def build(self, path, tag=None, quiet=False, fileobj=None, nocache=False, rm=False, timeout=None, @@ -1800,6 +1803,8 @@ def build(self, path, tag=None, quiet=False, fileobj=None, command_builder.add_arg("--tag", tag) command_builder.add_arg("--target", target) command_builder.add_arg("--iidfile", iidfile) + if self._secrets is not None: + command_builder.add_list('--secret', self._secrets) args = command_builder.build([path]) magic_word = "Successfully built " diff --git a/tests/acceptance/cli_test.py b/tests/acceptance/cli_test.py index a03d56567c2..8d613725d09 100644 --- a/tests/acceptance/cli_test.py +++ b/tests/acceptance/cli_test.py @@ -877,6 +877,27 @@ def test_build_override_dir(self): assert 'Successfully built' in result.stdout + @mock.patch.dict(os.environ) + def test_build_with_secrets(self): + os.environ["COMPOSE_DOCKER_CLI_BUILD"] = "1" + os.environ["DOCKER_BUILDKIT"] = "1" + self.base_dir = 'tests/fixtures/build-secrets' + build_result = self.dispatch(['build', '--build-arg', 'CACHEBUST=1']) + assert 'Successfully built' in build_result.stdout + run_result = self.dispatch(['run', 'foo']) + assert 'secret 1' in run_result.stdout + + @mock.patch.dict(os.environ) + def test_build_with_secrets_substitution(self): + os.environ["COMPOSE_DOCKER_CLI_BUILD"] = "1" + os.environ["DOCKER_BUILDKIT"] = "1" + os.environ["FOO_SECRET"] = "secret_2" + self.base_dir = 'tests/fixtures/build-secrets' + build_result = self.dispatch(['build', '--build-arg', 'CACHEBUST=2']) + assert 'Successfully built' in build_result.stdout + run_result = self.dispatch(['run', 'foo']) + assert 'secret 2' in run_result.stdout + def test_build_override_dir_invalid_path(self): config_path = os.path.abspath('tests/fixtures/build-path-override-dir/docker-compose.yml') result = self.dispatch([ diff --git a/tests/fixtures/build-secrets/Dockerfile b/tests/fixtures/build-secrets/Dockerfile new file mode 100644 index 00000000000..6998735ec78 --- /dev/null +++ b/tests/fixtures/build-secrets/Dockerfile @@ -0,0 +1,8 @@ +# syntax=docker/dockerfile:1.0.0-experimental +FROM busybox + +# https://github.com/moby/moby/issues/1996#issuecomment-550020843 +ARG CACHEBUST + +RUN --mount=type=secret,target=/secret,required cp secret out +CMD cat out diff --git a/tests/fixtures/build-secrets/docker-compose.yml b/tests/fixtures/build-secrets/docker-compose.yml new file mode 100644 index 00000000000..29696a7b676 --- /dev/null +++ b/tests/fixtures/build-secrets/docker-compose.yml @@ -0,0 +1,7 @@ +version: '3.7' +services: + foo: + build: + context: . + secrets: + - "id=secret,src=${FOO_SECRET:-secret_1}" diff --git a/tests/fixtures/build-secrets/secret_1 b/tests/fixtures/build-secrets/secret_1 new file mode 100644 index 00000000000..a64bbd647ac --- /dev/null +++ b/tests/fixtures/build-secrets/secret_1 @@ -0,0 +1 @@ +secret 1 diff --git a/tests/fixtures/build-secrets/secret_2 b/tests/fixtures/build-secrets/secret_2 new file mode 100644 index 00000000000..b71ba7d0f9c --- /dev/null +++ b/tests/fixtures/build-secrets/secret_2 @@ -0,0 +1 @@ +secret 2 diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 213a6441b57..039cb424c1a 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -990,6 +990,39 @@ def test_build_cli(self): self.addCleanup(self.client.remove_image, service.image_name) assert self.client.inspect_image('composetest_web') + def test_build_cli_with_secrets(self): + base_dir = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, base_dir) + secret_path_1 = os.path.join(base_dir, 'secret_1') + secret_path_2 = os.path.join(base_dir, 'secret_2') + + with open(os.path.join(base_dir, 'Dockerfile'), 'w') as f: + # f.write("FROM busybox\n") + f.write(dedent("""\ + # syntax=docker/dockerfile:1.0.0-experimental + FROM busybox + RUN --mount=type=secret,target=/secret_1,required cat /secret_1 + RUN --mount=type=secret,target=/secret_2,required cat /secret_2 + """)) + with open(secret_path_1, 'w') as f: + f.write("secret 1\n") + with open(secret_path_2, 'w') as f: + f.write("secret 2\n") + + service = self.create_service('web', + build={ + 'context': base_dir, + 'secrets': [ + 'id=secret_1,src={}'.format(secret_path_1), + 'id=secret_2,src={}'.format(secret_path_2)] + }, + environment={ + 'DOCKER_BUILDKIT': '1', + }) + service.build(cli=True) + self.addCleanup(self.client.remove_image, service.image_name) + assert self.client.inspect_image('composetest_web') + def test_up_build_cli(self): base_dir = tempfile.mkdtemp() self.addCleanup(shutil.rmtree, base_dir) From 2a195fd7bfd11494bfe8833c8aace55af8008824 Mon Sep 17 00:00:00 2001 From: Ollie Edwards Date: Fri, 22 Nov 2019 13:20:38 +0000 Subject: [PATCH 3/3] Add abstraction for buildkit CLI args Rather than putting buildkit strings directly in compose file, use a mapping abstraction. The format is slightly odd looking string or dict as the only truly required mount arg is id but we may also want to provide a whole dict of options including source and any other flags that buildkit supports for a mount spec. Signed-off-by: Ollie Edwards --- compose/config/config_schema_v3.7.json | 20 +++++++++++++- compose/service.py | 27 ++++++++++++++----- tests/acceptance/cli_test.py | 1 + tests/fixtures/build-secrets/Dockerfile | 1 + .../fixtures/build-secrets/docker-compose.yml | 5 +++- tests/fixtures/build-secrets/secret_3 | 1 + tests/integration/service_test.py | 4 +-- 7 files changed, 49 insertions(+), 10 deletions(-) create mode 100644 tests/fixtures/build-secrets/secret_3 diff --git a/compose/config/config_schema_v3.7.json b/compose/config/config_schema_v3.7.json index dbdac57c3ed..ff4e130cf36 100644 --- a/compose/config/config_schema_v3.7.json +++ b/compose/config/config_schema_v3.7.json @@ -89,7 +89,7 @@ "network": {"type": "string"}, "target": {"type": "string"}, "shm_size": {"type": ["integer", "string"]}, - "secrets": {"$ref": "#/definitions/list_of_strings"} + "secrets": {"$ref": "#/definitions/build_secrets"} }, "additionalProperties": false } @@ -564,6 +564,24 @@ ] }, + "build_secrets": { + "type": "array", + "uniqueItems": true, + "items": { + "oneOf": [ + {"type": "string"}, + { + "type": "object", + "required": ["id"], + "properties": { + "id": {"type": "string"}, + "src": {"type": "string"} + } + } + ] + } + }, + "list_of_strings": { "type": "array", "items": {"type": "string"}, diff --git a/compose/service.py b/compose/service.py index 949da3ed6ef..c01571c15a2 100644 --- a/compose/service.py +++ b/compose/service.py @@ -1791,6 +1791,11 @@ def build(self, path, tag=None, quiet=False, fileobj=None, dockerfile = os.path.join(path, dockerfile) iidfile = tempfile.mktemp() + sub_env = merge_environment( + os.environ.copy(), + self._environment + ) + command_builder = _CommandBuilder() command_builder.add_params("--build-arg", buildargs) command_builder.add_list("--cache-from", cache_from) @@ -1803,16 +1808,16 @@ def build(self, path, tag=None, quiet=False, fileobj=None, command_builder.add_arg("--tag", tag) command_builder.add_arg("--target", target) command_builder.add_arg("--iidfile", iidfile) - if self._secrets is not None: - command_builder.add_list('--secret', self._secrets) + + if "DOCKER_BUILDKIT" in sub_env and self._secrets is not None: + buildkit_secrets = self.get_buildkit_mounts(mount_spec=self._secrets) + if buildkit_secrets is not None: + command_builder.add_list('--secret', buildkit_secrets) + args = command_builder.build([path]) magic_word = "Successfully built " appear = False - sub_env = merge_environment( - os.environ.copy(), - self._environment - ) with subprocess.Popen(args, stdout=subprocess.PIPE, universal_newlines=True, env=sub_env) as p: while True: line = p.stdout.readline() @@ -1837,6 +1842,16 @@ def build(self, path, tag=None, quiet=False, fileobj=None, if not appear: yield json.dumps({"stream": "{}{}\n".format(magic_word, image_id)}) + def get_buildkit_mounts(self, mount_spec=None): + if mount_spec is None: + return None + mounts = [] + for spec in mount_spec: + if isinstance(spec, six.string_types): + spec = {'id': spec} + mounts.append(','.join(["{}={}".format(k, v) for (k, v) in spec.items()])) + return mounts + class _CommandBuilder(object): def __init__(self): diff --git a/tests/acceptance/cli_test.py b/tests/acceptance/cli_test.py index 8d613725d09..c7f91c3dcd0 100644 --- a/tests/acceptance/cli_test.py +++ b/tests/acceptance/cli_test.py @@ -886,6 +886,7 @@ def test_build_with_secrets(self): assert 'Successfully built' in build_result.stdout run_result = self.dispatch(['run', 'foo']) assert 'secret 1' in run_result.stdout + assert 'secret 3' in run_result.stdout @mock.patch.dict(os.environ) def test_build_with_secrets_substitution(self): diff --git a/tests/fixtures/build-secrets/Dockerfile b/tests/fixtures/build-secrets/Dockerfile index 6998735ec78..db5ce8f04f0 100644 --- a/tests/fixtures/build-secrets/Dockerfile +++ b/tests/fixtures/build-secrets/Dockerfile @@ -5,4 +5,5 @@ FROM busybox ARG CACHEBUST RUN --mount=type=secret,target=/secret,required cp secret out +RUN --mount=type=secret,target=/secret_3,required cat secret_3 >> out CMD cat out diff --git a/tests/fixtures/build-secrets/docker-compose.yml b/tests/fixtures/build-secrets/docker-compose.yml index 29696a7b676..56e884c0629 100644 --- a/tests/fixtures/build-secrets/docker-compose.yml +++ b/tests/fixtures/build-secrets/docker-compose.yml @@ -4,4 +4,7 @@ services: build: context: . secrets: - - "id=secret,src=${FOO_SECRET:-secret_1}" + - + id: secret + src: "${FOO_SECRET:-secret_1}" + - secret_3 diff --git a/tests/fixtures/build-secrets/secret_3 b/tests/fixtures/build-secrets/secret_3 new file mode 100644 index 00000000000..6e9349d38e6 --- /dev/null +++ b/tests/fixtures/build-secrets/secret_3 @@ -0,0 +1 @@ +secret 3 diff --git a/tests/integration/service_test.py b/tests/integration/service_test.py index 039cb424c1a..289a591fa10 100644 --- a/tests/integration/service_test.py +++ b/tests/integration/service_test.py @@ -1013,8 +1013,8 @@ def test_build_cli_with_secrets(self): build={ 'context': base_dir, 'secrets': [ - 'id=secret_1,src={}'.format(secret_path_1), - 'id=secret_2,src={}'.format(secret_path_2)] + {'id': 'secret_1', 'src': secret_path_1}, + {'id': 'secret_2', 'src': secret_path_2}] }, environment={ 'DOCKER_BUILDKIT': '1',