Merge ~pappacena/launchpad-buildd:security-manifest into launchpad-buildd:master
- Git
- lp:~pappacena/launchpad-buildd
- security-manifest
- Merge into master
Proposed by
Thiago F. Pappacena
Status: | Merged |
---|---|
Approved by: | Thiago F. Pappacena |
Approved revision: | 8aefc4834005b63dbf9481978cd73e8121613877 |
Merged at revision: | be12be72f1425cf731437f8f7063676fb6fac427 |
Proposed branch: | ~pappacena/launchpad-buildd:security-manifest |
Merge into: | launchpad-buildd:master |
Diff against target: |
245 lines (+118/-42) 2 files modified
lpbuildd/target/build_oci.py (+65/-0) lpbuildd/target/tests/test_build_oci.py (+53/-42) |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Colin Watson (community) | Approve | ||
Tom Wardill (community) | Approve | ||
Review via email: mp+392400@code.launchpad.net |
Commit message
Including manifest file in resulting OCI image.
Description of the change
To post a comment you must log in.
Revision history for this message
Thiago F. Pappacena (pappacena) wrote : | # |
twom, I agree we should discuss this. The current approach (implemented in follow up MPs) is enclosing the manifest file content generation in try/except blocks, and always generating at least a minimum version of the file.
The steps to copy the file into the image are not protected, but there is a bit less uncertainty in those steps. For that reason, I thought it would be better to fail the build if something goes wrong in the file injection phase. But of course, if we agree I can add the guard there too.
Revision history for this message
Colin Watson (cjwatson) : | # |
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lpbuildd/target/build_oci.py b/lpbuildd/target/build_oci.py | |||
2 | index af56671..3e4f283 100644 | |||
3 | --- a/lpbuildd/target/build_oci.py | |||
4 | +++ b/lpbuildd/target/build_oci.py | |||
5 | @@ -6,6 +6,7 @@ from __future__ import print_function | |||
6 | 6 | __metaclass__ = type | 6 | __metaclass__ = type |
7 | 7 | 7 | ||
8 | 8 | from collections import OrderedDict | 8 | from collections import OrderedDict |
9 | 9 | import json | ||
10 | 9 | import logging | 10 | import logging |
11 | 10 | import os.path | 11 | import os.path |
12 | 11 | import sys | 12 | import sys |
13 | @@ -53,6 +54,10 @@ class BuildOCI(SnapBuildProxyOperationMixin, VCSOperationMixin, | |||
14 | 53 | super(BuildOCI, self).__init__(args, parser) | 54 | super(BuildOCI, self).__init__(args, parser) |
15 | 54 | self.bin = os.path.dirname(sys.argv[0]) | 55 | self.bin = os.path.dirname(sys.argv[0]) |
16 | 55 | self.buildd_path = os.path.join("/home/buildd", self.args.name) | 56 | self.buildd_path = os.path.join("/home/buildd", self.args.name) |
17 | 57 | # Temp directory where we store files that will be included in the | ||
18 | 58 | # final filesystem of the image. | ||
19 | 59 | self.backend_tmp_fs_dir = "/tmp/image-root-dir/" | ||
20 | 60 | self.security_manifest_target_path = "/.rocks/manifest.json" | ||
21 | 56 | 61 | ||
22 | 57 | def _add_docker_engine_proxy_settings(self): | 62 | def _add_docker_engine_proxy_settings(self): |
23 | 58 | """Add systemd file for docker proxy settings.""" | 63 | """Add systemd file for docker proxy settings.""" |
24 | @@ -119,6 +124,65 @@ class BuildOCI(SnapBuildProxyOperationMixin, VCSOperationMixin, | |||
25 | 119 | env = self.build_proxy_environment(proxy_url=self.args.proxy_url) | 124 | env = self.build_proxy_environment(proxy_url=self.args.proxy_url) |
26 | 120 | self.vcs_fetch(self.args.name, cwd="/home/buildd", env=env) | 125 | self.vcs_fetch(self.args.name, cwd="/home/buildd", env=env) |
27 | 121 | 126 | ||
28 | 127 | def createSecurityManifest(self): | ||
29 | 128 | """Generates the security manifest file, returning the tmp file name | ||
30 | 129 | where it is stored in the backend. | ||
31 | 130 | """ | ||
32 | 131 | content = { | ||
33 | 132 | "manifest-version": "0", | ||
34 | 133 | "name": self.args.name | ||
35 | 134 | } | ||
36 | 135 | local_filename = tempfile.mktemp() | ||
37 | 136 | destination_path = self.security_manifest_target_path.lstrip( | ||
38 | 137 | os.path.sep) | ||
39 | 138 | destination = os.path.join(self.backend_tmp_fs_dir, destination_path) | ||
40 | 139 | with open(local_filename, 'w') as fd: | ||
41 | 140 | json.dump(content, fd, indent=2) | ||
42 | 141 | self.backend.copy_in(local_filename, destination) | ||
43 | 142 | return destination | ||
44 | 143 | |||
45 | 144 | def initTempRootDir(self): | ||
46 | 145 | """Initialize in the backend the directories that will be included in | ||
47 | 146 | resulting image's filesystem. | ||
48 | 147 | """ | ||
49 | 148 | security_manifest_dir = os.path.dirname( | ||
50 | 149 | self.security_manifest_target_path) | ||
51 | 150 | dir = os.path.join( | ||
52 | 151 | self.backend_tmp_fs_dir, | ||
53 | 152 | security_manifest_dir.lstrip(os.path.sep)) | ||
54 | 153 | self.backend.run(["mkdir", "-p", dir]) | ||
55 | 154 | |||
56 | 155 | def createImageContainer(self): | ||
57 | 156 | """Creates a container from the built image, so we can play with | ||
58 | 157 | it's filesystem.""" | ||
59 | 158 | self.run_build_command([ | ||
60 | 159 | "docker", "create", "--name", self.args.name, self.args.name]) | ||
61 | 160 | |||
62 | 161 | def removeImageContainer(self): | ||
63 | 162 | self.run_build_command(["docker", "rm", self.args.name]) | ||
64 | 163 | |||
65 | 164 | def commitImage(self): | ||
66 | 165 | """Commits the tmp container, overriding the originally built image.""" | ||
67 | 166 | self.run_build_command([ | ||
68 | 167 | "docker", "commit", self.args.name, self.args.name]) | ||
69 | 168 | |||
70 | 169 | def addFilesToImageContainer(self): | ||
71 | 170 | """Flushes all files from temp root dir (in the backend) to the | ||
72 | 171 | resulting image container.""" | ||
73 | 172 | # The extra '.' in the end is important. It indicates to docker that | ||
74 | 173 | # the directory itself should be copied, instead of the list of | ||
75 | 174 | # files in the directory. It makes docker keep the paths. | ||
76 | 175 | src = os.path.join(self.backend_tmp_fs_dir, ".") | ||
77 | 176 | self.run_build_command(["docker", "cp", src, "%s:/" % self.args.name]) | ||
78 | 177 | |||
79 | 178 | def addSecurityManifest(self): | ||
80 | 179 | self.createImageContainer() | ||
81 | 180 | self.initTempRootDir() | ||
82 | 181 | self.createSecurityManifest() | ||
83 | 182 | self.addFilesToImageContainer() | ||
84 | 183 | self.commitImage() | ||
85 | 184 | self.removeImageContainer() | ||
86 | 185 | |||
87 | 122 | def build(self): | 186 | def build(self): |
88 | 123 | logger.info("Running build phase...") | 187 | logger.info("Running build phase...") |
89 | 124 | args = ["docker", "build", "--no-cache"] | 188 | args = ["docker", "build", "--no-cache"] |
90 | @@ -143,6 +207,7 @@ class BuildOCI(SnapBuildProxyOperationMixin, VCSOperationMixin, | |||
91 | 143 | self._check_path_escape(build_context_path) | 207 | self._check_path_escape(build_context_path) |
92 | 144 | args.append(build_context_path) | 208 | args.append(build_context_path) |
93 | 145 | self.run_build_command(args) | 209 | self.run_build_command(args) |
94 | 210 | self.addSecurityManifest() | ||
95 | 146 | 211 | ||
96 | 147 | def run(self): | 212 | def run(self): |
97 | 148 | try: | 213 | try: |
98 | diff --git a/lpbuildd/target/tests/test_build_oci.py b/lpbuildd/target/tests/test_build_oci.py | |||
99 | index e58344f..87b37b2 100644 | |||
100 | --- a/lpbuildd/target/tests/test_build_oci.py | |||
101 | +++ b/lpbuildd/target/tests/test_build_oci.py | |||
102 | @@ -290,6 +290,23 @@ class TestBuildOCI(TestCase): | |||
103 | 290 | cwd="/home/buildd/test-image", **env), | 290 | cwd="/home/buildd/test-image", **env), |
104 | 291 | ])) | 291 | ])) |
105 | 292 | 292 | ||
106 | 293 | def assertRanPostBuildCommands(self, build_oci): | ||
107 | 294 | self.assertThat(build_oci.backend.run.calls[1:], MatchesListwise([ | ||
108 | 295 | RanBuildCommand( | ||
109 | 296 | ['docker', 'create', '--name', 'test-image', 'test-image'], | ||
110 | 297 | cwd="/home/buildd/test-image"), | ||
111 | 298 | RanCommand(['mkdir', '-p', '/tmp/image-root-dir/.rocks']), | ||
112 | 299 | RanBuildCommand( | ||
113 | 300 | ['docker', 'cp', '/tmp/image-root-dir/.', 'test-image:/'], | ||
114 | 301 | cwd="/home/buildd/test-image"), | ||
115 | 302 | RanBuildCommand( | ||
116 | 303 | ['docker', 'commit', 'test-image', 'test-image'], | ||
117 | 304 | cwd="/home/buildd/test-image"), | ||
118 | 305 | RanBuildCommand( | ||
119 | 306 | ['docker', 'rm', 'test-image'], | ||
120 | 307 | cwd="/home/buildd/test-image"), | ||
121 | 308 | ])) | ||
122 | 309 | |||
123 | 293 | def test_build(self): | 310 | def test_build(self): |
124 | 294 | args = [ | 311 | args = [ |
125 | 295 | "build-oci", | 312 | "build-oci", |
126 | @@ -299,12 +316,11 @@ class TestBuildOCI(TestCase): | |||
127 | 299 | build_oci = parse_args(args=args).operation | 316 | build_oci = parse_args(args=args).operation |
128 | 300 | build_oci.backend.add_dir('/build/test-directory') | 317 | build_oci.backend.add_dir('/build/test-directory') |
129 | 301 | build_oci.build() | 318 | build_oci.build() |
136 | 302 | self.assertThat(build_oci.backend.run.calls, MatchesListwise([ | 319 | self.assertThat(build_oci.backend.run.calls[0], RanBuildCommand( |
137 | 303 | RanBuildCommand( | 320 | ["docker", "build", "--no-cache", "--tag", "test-image", |
138 | 304 | ["docker", "build", "--no-cache", "--tag", "test-image", | 321 | "/home/buildd/test-image/."], |
139 | 305 | "/home/buildd/test-image/."], | 322 | cwd="/home/buildd/test-image")) |
140 | 306 | cwd="/home/buildd/test-image"), | 323 | self.assertRanPostBuildCommands(build_oci) |
135 | 307 | ])) | ||
141 | 308 | 324 | ||
142 | 309 | def test_build_with_file(self): | 325 | def test_build_with_file(self): |
143 | 310 | args = [ | 326 | args = [ |
144 | @@ -316,13 +332,12 @@ class TestBuildOCI(TestCase): | |||
145 | 316 | build_oci = parse_args(args=args).operation | 332 | build_oci = parse_args(args=args).operation |
146 | 317 | build_oci.backend.add_dir('/build/test-directory') | 333 | build_oci.backend.add_dir('/build/test-directory') |
147 | 318 | build_oci.build() | 334 | build_oci.build() |
155 | 319 | self.assertThat(build_oci.backend.run.calls, MatchesListwise([ | 335 | self.assertThat(build_oci.backend.run.calls[0], RanBuildCommand( |
156 | 320 | RanBuildCommand( | 336 | ["docker", "build", "--no-cache", "--tag", "test-image", |
157 | 321 | ["docker", "build", "--no-cache", "--tag", "test-image", | 337 | "--file", "./build-aux/Dockerfile", |
158 | 322 | "--file", "./build-aux/Dockerfile", | 338 | "/home/buildd/test-image/."], |
159 | 323 | "/home/buildd/test-image/."], | 339 | cwd="/home/buildd/test-image")) |
160 | 324 | cwd="/home/buildd/test-image"), | 340 | self.assertRanPostBuildCommands(build_oci) |
154 | 325 | ])) | ||
161 | 326 | 341 | ||
162 | 327 | def test_build_with_path(self): | 342 | def test_build_with_path(self): |
163 | 328 | args = [ | 343 | args = [ |
164 | @@ -334,12 +349,11 @@ class TestBuildOCI(TestCase): | |||
165 | 334 | build_oci = parse_args(args=args).operation | 349 | build_oci = parse_args(args=args).operation |
166 | 335 | build_oci.backend.add_dir('/build/test-directory') | 350 | build_oci.backend.add_dir('/build/test-directory') |
167 | 336 | build_oci.build() | 351 | build_oci.build() |
174 | 337 | self.assertThat(build_oci.backend.run.calls, MatchesListwise([ | 352 | self.assertThat(build_oci.backend.run.calls[0], RanBuildCommand( |
175 | 338 | RanBuildCommand( | 353 | ["docker", "build", "--no-cache", "--tag", "test-image", |
176 | 339 | ["docker", "build", "--no-cache", "--tag", "test-image", | 354 | "/home/buildd/test-image/a-sub-directory/"], |
177 | 340 | "/home/buildd/test-image/a-sub-directory/"], | 355 | cwd="/home/buildd/test-image")) |
178 | 341 | cwd="/home/buildd/test-image"), | 356 | self.assertRanPostBuildCommands(build_oci) |
173 | 342 | ])) | ||
179 | 343 | 357 | ||
180 | 344 | def test_build_with_file_and_path(self): | 358 | def test_build_with_file_and_path(self): |
181 | 345 | args = [ | 359 | args = [ |
182 | @@ -352,13 +366,12 @@ class TestBuildOCI(TestCase): | |||
183 | 352 | build_oci = parse_args(args=args).operation | 366 | build_oci = parse_args(args=args).operation |
184 | 353 | build_oci.backend.add_dir('/build/test-directory') | 367 | build_oci.backend.add_dir('/build/test-directory') |
185 | 354 | build_oci.build() | 368 | build_oci.build() |
193 | 355 | self.assertThat(build_oci.backend.run.calls, MatchesListwise([ | 369 | self.assertThat(build_oci.backend.run.calls[0], RanBuildCommand( |
194 | 356 | RanBuildCommand( | 370 | ["docker", "build", "--no-cache", "--tag", "test-image", |
195 | 357 | ["docker", "build", "--no-cache", "--tag", "test-image", | 371 | "--file", "test-build-path/build-aux/Dockerfile", |
196 | 358 | "--file", "test-build-path/build-aux/Dockerfile", | 372 | "/home/buildd/test-image/test-build-path"], |
197 | 359 | "/home/buildd/test-image/test-build-path"], | 373 | cwd="/home/buildd/test-image")) |
198 | 360 | cwd="/home/buildd/test-image"), | 374 | self.assertRanPostBuildCommands(build_oci) |
192 | 361 | ])) | ||
199 | 362 | 375 | ||
200 | 363 | def test_build_with_args(self): | 376 | def test_build_with_args(self): |
201 | 364 | args = [ | 377 | args = [ |
202 | @@ -372,14 +385,13 @@ class TestBuildOCI(TestCase): | |||
203 | 372 | build_oci = parse_args(args=args).operation | 385 | build_oci = parse_args(args=args).operation |
204 | 373 | build_oci.backend.add_dir('/build/test-directory') | 386 | build_oci.backend.add_dir('/build/test-directory') |
205 | 374 | build_oci.build() | 387 | build_oci.build() |
214 | 375 | self.assertThat(build_oci.backend.run.calls, MatchesListwise([ | 388 | self.assertThat(build_oci.backend.run.calls[0], RanBuildCommand( |
215 | 376 | RanBuildCommand( | 389 | ["docker", "build", "--no-cache", "--tag", "test-image", |
216 | 377 | ["docker", "build", "--no-cache", "--tag", "test-image", | 390 | "--file", "test-build-path/build-aux/Dockerfile", |
217 | 378 | "--file", "test-build-path/build-aux/Dockerfile", | 391 | "--build-arg=VAR1=xxx", "--build-arg=VAR2=yyy", |
218 | 379 | "--build-arg=VAR1=xxx", "--build-arg=VAR2=yyy", | 392 | "/home/buildd/test-image/test-build-path"], |
219 | 380 | "/home/buildd/test-image/test-build-path"], | 393 | cwd="/home/buildd/test-image")) |
220 | 381 | cwd="/home/buildd/test-image"), | 394 | self.assertRanPostBuildCommands(build_oci) |
213 | 382 | ])) | ||
221 | 383 | 395 | ||
222 | 384 | def test_build_proxy(self): | 396 | def test_build_proxy(self): |
223 | 385 | args = [ | 397 | args = [ |
224 | @@ -391,14 +403,13 @@ class TestBuildOCI(TestCase): | |||
225 | 391 | build_oci = parse_args(args=args).operation | 403 | build_oci = parse_args(args=args).operation |
226 | 392 | build_oci.backend.add_dir('/build/test-directory') | 404 | build_oci.backend.add_dir('/build/test-directory') |
227 | 393 | build_oci.build() | 405 | build_oci.build() |
236 | 394 | self.assertThat(build_oci.backend.run.calls, MatchesListwise([ | 406 | self.assertThat(build_oci.backend.run.calls[0], RanBuildCommand( |
237 | 395 | RanBuildCommand( | 407 | ["docker", "build", "--no-cache", |
238 | 396 | ["docker", "build", "--no-cache", | 408 | "--build-arg", "http_proxy=http://proxy.example:3128/", |
239 | 397 | "--build-arg", "http_proxy=http://proxy.example:3128/", | 409 | "--build-arg", "https_proxy=http://proxy.example:3128/", |
240 | 398 | "--build-arg", "https_proxy=http://proxy.example:3128/", | 410 | "--tag", "test-image", "/home/buildd/test-image/."], |
241 | 399 | "--tag", "test-image", "/home/buildd/test-image/."], | 411 | cwd="/home/buildd/test-image")) |
242 | 400 | cwd="/home/buildd/test-image"), | 412 | self.assertRanPostBuildCommands(build_oci) |
235 | 401 | ])) | ||
243 | 402 | 413 | ||
244 | 403 | def test_run_succeeds(self): | 414 | def test_run_succeeds(self): |
245 | 404 | args = [ | 415 | args = [ |
This looks good to me as is, though I think we should try and work out what we do in the case of not being able to add/generate the manifest, and whether that's enough to fail a build or not.