-
Notifications
You must be signed in to change notification settings - Fork 716
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
Distroless image for TF operator #1124
Distroless image for TF operator #1124
Conversation
TODOs:
|
Hey @krishnadurai, TravisBuddy Request Identifier: 98003e00-2dd8-11ea-a185-6fb6d15019ed |
RUN chmod a+x /opt/kubeflow/tf-operator.v1 | ||
|
||
ENTRYPOINT ["/opt/kubeflow/tf-operator.v1"] | ||
ENTRYPOINT ["/opt/tf-operator.v1"] |
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.
Probable permission issue.
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.
Good question. Here's the pattern for moving binaries to a distroless image given by the example:
The idea here is to keep the distroless image with just the essentials for golang and its binaries. That leaves this system with just the user who's running this binary. So I guess setting permissions isn't necessary as advocated by the example.
INFO:root:Message: failed to create containerd task: OCI runtime create failed: container_linux.go:346: starting container process caused "exec: "/opt/kubeflow/tf-operator.v1": stat /opt/kubeflow/tf-operator.v1: no such file or dir INFO:root:Command: command still seems to be using /opt/kubeflow/tf-operator.v1, but the change is to use /opt/. As per the manifest its still seems to use old command. https://github.com/kubeflow/manifests/blob/9c9acdb1c319e04cc2b9fbd31a59bfd7bc062fbd/tf-training/tf-job-operator/base/deployment.yaml#L15 |
@nrchakradhar thanks for an early review. I will look to fix these issues in some time. This is still a work in progress. |
/retest |
Hey @krishnadurai, TravisBuddy Request Identifier: dc7fea00-2e85-11ea-8eaa-0b08c4ee3d08 |
@krishnadurai Please do not consider this as a review. I was just trying to understand why the pre-submit failed and probably provide some help. |
@krishnadurai Could you please run both the images through a vuln scanner and detail how this change improves upon the existing image? |
@swiftdiaries I'm on it. |
Report with Anchore CLI DIstroless Image: CVE count 13 all negligible severity.
|
Report with Anchore CLI TF operator current image (gcr.io/kubeflow-images-public/tf_operator:v0.7.0): CVE count 38 - 34 Medium and 4 Low
|
/assign @swiftdiaries @jlewi @nrchakradhar This is ready for review. /hold cancel |
|
||
ADD . /go/src/github.com/kubeflow/tf-operator | ||
|
||
WORKDIR /go/src/github.com/kubeflow/tf-operator | ||
|
||
RUN go build -o tf-operator.v1 ./cmd/tf-operator.v1 | ||
|
||
FROM registry.access.redhat.com/ubi8/ubi:latest | ||
FROM gcr.io/distroless/base-debian10 |
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.
Why base-debian as opposed to base?
https://github.com/GoogleContainerTools/distroless/blob/5a460b2310a3e2e3c0590835614860747d6e8769/examples/go/Dockerfile#L10-L12
Which one is slimmer?
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.
@jlewi This is a good point.
The sizes are:
base
: 60301780
base-debian10
: 62581637
I observed that the CVE counts with base
image are a little higher: 16 Low priority
anchore-cli image vuln docker.io/krishnadurai/distless-tfop:base all
Vulnerability ID Package Severity Fix CVE Refs Vulnerability URL
CVE-2007-6755 libssl1.1-1.1.0l-1~deb9u1 Negligible None CVE-2007-6755 https://security-tracker.debian.org/tracker/CVE-2007-6755
CVE-2007-6755 openssl-1.1.0l-1~deb9u1 Negligible None CVE-2007-6755 https://security-tracker.debian.org/tracker/CVE-2007-6755
CVE-2010-0928 libssl1.1-1.1.0l-1~deb9u1 Negligible None CVE-2010-0928 https://security-tracker.debian.org/tracker/CVE-2010-0928
CVE-2010-0928 openssl-1.1.0l-1~deb9u1 Negligible None CVE-2010-0928 https://security-tracker.debian.org/tracker/CVE-2010-0928
CVE-2010-4051 libc6-2.24-11+deb9u4 Negligible None CVE-2010-4051 https://security-tracker.debian.org/tracker/CVE-2010-4051
CVE-2010-4052 libc6-2.24-11+deb9u4 Negligible None CVE-2010-4052 https://security-tracker.debian.org/tracker/CVE-2010-4052
CVE-2010-4756 libc6-2.24-11+deb9u4 Negligible None CVE-2010-4756 https://security-tracker.debian.org/tracker/CVE-2010-4756
CVE-2015-8985 libc6-2.24-11+deb9u4 Negligible None CVE-2015-8985 https://security-tracker.debian.org/tracker/CVE-2015-8985
CVE-2018-20796 libc6-2.24-11+deb9u4 Negligible None CVE-2018-20796 https://security-tracker.debian.org/tracker/CVE-2018-20796
CVE-2019-1010022 libc6-2.24-11+deb9u4 Negligible None CVE-2019-1010022 https://security-tracker.debian.org/tracker/CVE-2019-1010022
CVE-2019-1010023 libc6-2.24-11+deb9u4 Negligible None CVE-2019-1010023 https://security-tracker.debian.org/tracker/CVE-2019-1010023
CVE-2019-1010024 libc6-2.24-11+deb9u4 Negligible None CVE-2019-1010024 https://security-tracker.debian.org/tracker/CVE-2019-1010024
CVE-2019-1010025 libc6-2.24-11+deb9u4 Negligible None CVE-2019-1010025 https://security-tracker.debian.org/tracker/CVE-2019-1010025
CVE-2019-6488 libc6-2.24-11+deb9u4 Negligible None CVE-2019-6488 https://security-tracker.debian.org/tracker/CVE-2019-6488
CVE-2019-7309 libc6-2.24-11+deb9u4 Negligible None CVE-2019-7309 https://security-tracker.debian.org/tracker/CVE-2019-7309
CVE-2019-9192 libc6-2.24-11+deb9u4 Negligible None CVE-2019-9192 https://security-tracker.debian.org/tracker/CVE-2019-9192
Should we stick to a lower CVE count rather than opting for lesser size?
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.
@jlewi Do you have other questions about it?
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.
Looking at the guidance here:
https://github.com/GoogleContainerTools/distroless/tree/master/base
Unless we need glibc, libssl, or openssl we should be able to use the static image.
The "-debian" suffix is explained here.
https://github.com/GoogleContainerTools/distroless#base-operating-system
So IIUC it looks like base = "base-debian9" which likely has more CVE's then "debian10" probably because it is newer.
So I think we want to use "static-debian10" ?
I checked and it looks like the static images are an order of magnitude smaller than the non static images.
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.
@jlewi it seems the go binary which we are using requires C runtime for imports like 'net' packages.
If we build and run on 'static-debian10' we run into this problem:
standard_init_linux.go:211: exec user process caused "no such file or directory"
Since go starts looking for C runtime packages which we are building against.
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.
I think you need to build with CGO_ENABLED=0
.
Could you try this?
CGO_ENABLED=0 GOOS=linux go build -o tf-operator.v1 -ldflags "-w" -a ./cmd/tf-operator.v1
f5de369
to
60d4fed
Compare
/retest |
TF operator packed in distroless container tf_smoke.py removed
60d4fed
to
10392b6
Compare
Hey @krishnadurai, TravisBuddy Request Identifier: ab1dda70-331d-11ea-afd1-237c04ec133e |
/assign @richardsliu |
Hey @krishnadurai, TravisBuddy Request Identifier: 4cb22440-331e-11ea-afd1-237c04ec133e |
@jlewi @richardsliu could you PTAL? |
...-app/vendor/kubeflow/core@f7a68336ad7a65c2cbba8462e89d24a10626687e/tf-job-operator.libsonnet
Show resolved
Hide resolved
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.
/lgtm
/assign @gaocegege
/lgtm But I am not sure if @jlewi has questions. Thanks for your contribution! 🎉 👍 |
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.
@jlewi I've addressed your comments.
Thanks @gaocegege and @johnugeorge!
|
||
ADD . /go/src/github.com/kubeflow/tf-operator | ||
|
||
WORKDIR /go/src/github.com/kubeflow/tf-operator | ||
|
||
RUN go build -o tf-operator.v1 ./cmd/tf-operator.v1 | ||
|
||
FROM registry.access.redhat.com/ubi8/ubi:latest | ||
FROM gcr.io/distroless/base-debian10 |
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.
@jlewi it seems the go binary which we are using requires C runtime for imports like 'net' packages.
If we build and run on 'static-debian10' we run into this problem:
standard_init_linux.go:211: exec user process caused "no such file or directory"
Since go starts looking for C runtime packages which we are building against.
Thanks @krishnadurai /lgtm |
/hold cancel |
@gaocegege you may have to approve it? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jlewi, johnugeorge The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
2 similar comments
/retest |
/retest |
/test all |
/retest |
Thanks @jlewi and @johnugeorge! |
Relates to: kubeflow/kubeflow#4590
tf_smoke.py removed
Golang updated to 1.13.5
/hold
/cc @swiftdiaries @jlewi
This change is