-
Notifications
You must be signed in to change notification settings - Fork 480
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[build] Add safety report to docker image #1186
Changes from 22 commits
ab8487a
d385f59
e38004f
e286853
f3df287
8400170
821a4e2
f610d45
af82f00
ea87368
e20b379
67b969a
6dd087c
7143bd8
2f23d6d
f1f36c6
3ca89e5
7603d30
d887fb1
8d6982e
cb6c347
36fb18f
61641ec
c5a20a2
a5d4fb7
b091ea0
9674ebc
2c43fb1
a83067d
bbf0060
9c168d5
7eeac6f
e4ddf98
096ff19
c629e9b
549d1da
4313a84
372bfae
cc79be7
36d822e
4c6d379
89666a6
800a5b8
a8cea00
db5f9a4
50adb38
ef62fa9
41edfbd
8d2c4e3
94c0733
dd5c6bd
5dce6c7
ed24724
8181a38
71dce0e
a1fb7e4
73bd4a6
1f48f08
49a6e2a
6286f8e
9cdfaaa
d576ae9
3923a68
63d31d1
301c663
5ec5776
ddd25aa
d9db790
9a4af96
a5ca249
a15e23c
c71628c
8146137
f093cde
2bb790a
3eb309f
e3fccd3
f62553e
699caca
3d0cde0
9b6ea43
664e946
0a0aeeb
d9d09b8
4bf75d9
742d1c9
2b3d735
dd1c08c
cae71c0
76335c1
0f7416c
feed6bd
9c192c9
bde69df
0996ac0
e90439e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
# Use the Deep Learning Container as a base Image | ||
ARG FIRST_STAGE_IMAGE="" | ||
|
||
FROM $FIRST_STAGE_IMAGE | ||
|
||
# Add any script or repo as required | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Comment: Copy safety report generated from |
||
COPY safety_report.json /var/safety_report.json | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the location for the file should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
|
@@ -26,7 +26,7 @@ class DockerImage: | |||||||||
""" | ||||||||||
|
||||||||||
def __init__( | ||||||||||
self, info, dockerfile, repository, tag, to_build, context=None, | ||||||||||
self, info, dockerfile, repository, tag, to_build, stage, context=None, | ||||||||||
): | ||||||||||
|
||||||||||
# Meta-data about the image should go to info. | ||||||||||
|
@@ -36,6 +36,7 @@ def __init__( | |||||||||
self.summary = {} | ||||||||||
self.build_args = {} | ||||||||||
self.labels = {} | ||||||||||
self.stage = stage | ||||||||||
|
||||||||||
self.dockerfile = dockerfile | ||||||||||
self.context = context | ||||||||||
|
@@ -69,11 +70,7 @@ def collect_installed_packages_information(self): | |||||||||
docker_client.containers.prune() | ||||||||||
return command_responses | ||||||||||
|
||||||||||
def build(self): | ||||||||||
""" | ||||||||||
The build function builds the specified docker image | ||||||||||
""" | ||||||||||
self.summary["start_time"] = datetime.now() | ||||||||||
def pre_build_configuration(self): | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please change this function name to a verb, like "update_pre_build_configuration", because it does not have a return value, and it would be unclear exactly why this function is invoked in the code where it is used. Also, please add a docstring to describe why this function exists, and what it does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||||||
|
||||||||||
if not self.to_build: | ||||||||||
self.log = ["Not built"] | ||||||||||
|
@@ -84,19 +81,42 @@ def build(self): | |||||||||
if self.info.get("base_image_uri"): | ||||||||||
self.build_args["BASE_IMAGE"] = self.info["base_image_uri"] | ||||||||||
|
||||||||||
if self.ecr_url: | ||||||||||
self.build_args["FIRST_STAGE_IMAGE"] = self.ecr_url | ||||||||||
|
||||||||||
if self.info.get("extra_build_args"): | ||||||||||
self.build_args.update(self.info.get("extra_build_args")) | ||||||||||
|
||||||||||
if self.info.get("labels"): | ||||||||||
self.labels.update(self.info.get("labels")) | ||||||||||
|
||||||||||
print(f"self.build_args {self.build_args}") | ||||||||||
print(f"self.labels {self.labels}") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are these print statements permanent additions? If so, could you change this to be printed with better readability?
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will remove this. |
||||||||||
|
||||||||||
with open(self.context.context_path, "rb") as context_file: | ||||||||||
response = [] | ||||||||||
|
||||||||||
for line in self.client.build( | ||||||||||
fileobj=context_file, | ||||||||||
def build(self): | ||||||||||
""" | ||||||||||
The build function builds the specified docker image | ||||||||||
""" | ||||||||||
self.summary["start_time"] = datetime.now() | ||||||||||
self.pre_build_configuration() | ||||||||||
print(f"self.context {self.context}") | ||||||||||
if self.context: | ||||||||||
with open(self.context.context_path, "rb") as context_file: | ||||||||||
print("within context") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||||||
self.docker_build(fileobj=context_file, custom_context=True) | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the If Please check the return value of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good point There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, will do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||||||
self.context.remove() | ||||||||||
else: | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a case where the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't seen any yet. It was already existing in the code, so I thought of not playing with it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Existing code as in on the PR branch or the master, if PR branch then lets just remove it, probably it might have been added for experimental purposes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I will remove it. |
||||||||||
print("out of context") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||||||
self.docker_build() | ||||||||||
#check the size after image is built. | ||||||||||
self.image_size_check() | ||||||||||
shantanutrip marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||
|
||||||||||
def docker_build(self, fileobj=None, custom_context=False): | ||||||||||
response = [] | ||||||||||
for line in self.client.build( | ||||||||||
fileobj=fileobj, | ||||||||||
path=self.dockerfile, | ||||||||||
custom_context=True, | ||||||||||
custom_context=custom_context, | ||||||||||
rm=True, | ||||||||||
decode=True, | ||||||||||
tag=self.ecr_url, | ||||||||||
|
@@ -121,39 +141,51 @@ def build(self): | |||||||||
else: | ||||||||||
response.append(str(line)) | ||||||||||
|
||||||||||
self.context.remove() | ||||||||||
self.log = response | ||||||||||
print(f"self.log {self.log}") | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this, or insert improvements upon the way that it is printed. The logs printed in the build job take up too much space on the logs, and affect readability on the job. An example of the logs is:
for thousands of lines. If we really want to see the build logs after each build (do we need this for every build, or for only those builds that fail?), perhaps they could be uploaded somewhere with better formatting. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have removed this log display line. Also, I have changed the way in which logs are now displayed and made it such that it doesn't take up a lot of space. |
||||||||||
self.build_status = constants.SUCCESS | ||||||||||
#TODO: return required? | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: comment can be deleted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||||||
return self.build_status | ||||||||||
|
||||||||||
self.summary["image_size"] = int( | ||||||||||
|
||||||||||
def image_size_check(self): | ||||||||||
response = [] | ||||||||||
self.summary["image_size"] = int( | ||||||||||
self.client.inspect_image(self.ecr_url)["Size"] | ||||||||||
) / (1024 * 1024) | ||||||||||
if self.summary["image_size"] > self.info["image_size_baseline"] * 1.20: | ||||||||||
response.append("Image size baseline exceeded") | ||||||||||
response.append(f"{self.summary['image_size']} > 1.2 * {self.info['image_size_baseline']}") | ||||||||||
response += self.collect_installed_packages_information() | ||||||||||
self.build_status = constants.FAIL_IMAGE_SIZE_LIMIT | ||||||||||
else: | ||||||||||
self.build_status = constants.SUCCESS | ||||||||||
|
||||||||||
for line in self.client.push( | ||||||||||
self.repository, self.tag, stream=True, decode=True | ||||||||||
): | ||||||||||
if line.get("error") is not None: | ||||||||||
response.append(line["error"]) | ||||||||||
|
||||||||||
self.log = response | ||||||||||
self.build_status = constants.FAIL | ||||||||||
self.summary["status"] = constants.STATUS_MESSAGE[self.build_status] | ||||||||||
self.summary["end_time"] = datetime.now() | ||||||||||
|
||||||||||
return self.build_status | ||||||||||
if line.get("stream") is not None: | ||||||||||
response.append(line["stream"]) | ||||||||||
else: | ||||||||||
response.append(str(line)) | ||||||||||
if self.summary["image_size"] > self.info["image_size_baseline"] * 1.20: | ||||||||||
response.append("Image size baseline exceeded") | ||||||||||
response.append(f"{self.summary['image_size']} > 1.2 * {self.info['image_size_baseline']}") | ||||||||||
response += self.collect_installed_packages_information() | ||||||||||
self.build_status = constants.FAIL_IMAGE_SIZE_LIMIT | ||||||||||
else: | ||||||||||
self.build_status = constants.SUCCESS | ||||||||||
self.log = response | ||||||||||
print(f"self.log {self.log}") | ||||||||||
#TODO: return required? | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: comment can be deleted |
||||||||||
return self.build_status | ||||||||||
|
||||||||||
def push_image(self): | ||||||||||
|
||||||||||
for line in self.client.push(self.repository, self.tag, stream=True, decode=True): | ||||||||||
response = [] | ||||||||||
if line.get("error") is not None: | ||||||||||
response.append(line["error"]) | ||||||||||
|
||||||||||
self.summary["status"] = constants.STATUS_MESSAGE[self.build_status] | ||||||||||
self.summary["end_time"] = datetime.now() | ||||||||||
self.summary["ecr_url"] = self.ecr_url | ||||||||||
self.log = response | ||||||||||
self.log = response | ||||||||||
self.build_status = constants.FAIL | ||||||||||
self.summary["status"] = constants.STATUS_MESSAGE[self.build_status] | ||||||||||
self.summary["end_time"] = datetime.now() | ||||||||||
|
||||||||||
return self.build_status | ||||||||||
return self.build_status | ||||||||||
if line.get("stream") is not None: | ||||||||||
response.append(line["stream"]) | ||||||||||
else: | ||||||||||
response.append(str(line)) | ||||||||||
|
||||||||||
self.summary["status"] = constants.STATUS_MESSAGE[self.build_status] | ||||||||||
self.summary["end_time"] = datetime.now() | ||||||||||
self.summary["ecr_url"] = self.ecr_url | ||||||||||
self.log = response | ||||||||||
#TODO: return required? | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: comment can be deleted There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||||||||||
return self.build_status |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename this file as
Dockerfile.safetyreport
or something, becausemultipart
is much too generic, and could confuse readers about its purpose, especially with other PRs such as #1278.Also, consider moving this dockerfile into another folder (that is not
src/
) which could contain many different kinds of multipart dockerfiles meant to add specific layers to images on which they are used. For example, we might want to include the dependency_check report, or the ECR Scan report, or pull the OSSCompliance layer out from all images into a dockerfile similar to this.It doesn't make sense to have this in
src/
becausesrc/
contains "source code" used to build images, and adding more of these multipart dockerfiles could bloat thesrc/
folder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current design decision is to use 1 single docker file to host stuff that are applicable to all containers. So something like
Dockerfile.common
will work.Having multi-staged capability will be definitely better so we can have
Dockerfile.oss_compliance
,Dockerfile.dependency_check
etc. And a DLC build process can pick and choose which ones to run. But we are not there yet.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dockerfile is meant to be built as a
final
stage in the build process. Any functionality including ECR sans, OSS compliance script which is common to all the DLC should be placed in this final stage Dockerfile.I agree that the file name can be renamed to
Dockerfile.common
orDockerfile.finalstage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to
Docker.common
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I will change the name to
Dockerfile.common
and make a different folder calledmiscellaneous_dockerfiles
to consist of all dockerfiles that will be used for embellishing base images.