Skip to content
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

Enhancing dockerfiles (multi-stage) of kyverno components and adding non-root user to the docker images #1495

Merged
merged 7 commits into from Jan 29, 2021

Conversation

imrajdas
Copy link
Contributor

Changes

  • [Multi-stage] Adding multi-stage docker build of the following docker images

    • kyverno/kyverno
    • kyverno/cmd/cli/kubectl-kyverno
    • kyverno/kyvernopre
  • [Non-root] Base image changed from scratch to alpine (Because non-root user can't be added in scratch image)
    Following is the screenshot of the user inside the kyverno pod (which is non-root user)
    image

  • [Size] Image sizes

    • kyverno/kyverno - 47.3MB
    • kyverno/cmd/cli/kubectl-kyverno - 43.9MB
    • kyverno/kyvernopre - 33.5MB
  • [Makefile] Refactoring docker build step in Makefile to accommodate the new docker file changes

Related issue

What type of PR is this?

Proposed changes

Checklist

Further comments

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
@imrajdas
Copy link
Contributor Author

@JimBugwadia / @chipzoller / @realshuting Please take a look.

@imrajdas
Copy link
Contributor Author

/kind feature

@chipzoller
Copy link
Member

You don't need to use alpine to get the non-root results. In the build stage, put the useradd command and in the copy stage copy over /etc/passwd so it is then present in the scratch image. /etc/passwd will then have the user that was created earlier allowing the container to run as that user (still need the USER directive).

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
@imrajdas
Copy link
Contributor Author

imrajdas commented Jan 24, 2021

You don't need to use alpine to get the non-root results. In the build stage, put the useradd command and in the copy stage copy over /etc/passwd so it is then present in the scratch image. /etc/passwd will then have the user that was created earlier allowing the container to run as that user (still need the USER directive).

Thanks, @chipzoller for reviewing this PR, I reverted to scratch image and adding the UID and GID as args to the Dockerfile which will be used by both stages. Let me know if it requires more change

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
Copy link
Member

@realshuting realshuting left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rajdas98 thanks for the contribution! It looks good to me.

@chipzoller
Copy link
Member

@rajdas98 Have you tested this build process after these commits to ensure the container runs as this user/UID?

@imrajdas
Copy link
Contributor Author

imrajdas commented Jan 25, 2021

@rajdas98 Have you tested this build process after these commits to ensure the container runs as this user/UID?

Yes @chipzoller

  1. I replaced both images in deployment with my custom image, it was working. Here are the logs-
    kyverno.log ----- (With scratch imge)

  2. we can't exec inside the scratch container, so I change the base image to alpine and then run the ps aux command inside that pod --- (With Alpine image)
    Selection_131

@realshuting
Copy link
Member

@chipzoller Any other comments? Shall I merge this?

@chipzoller
Copy link
Member

I have not yet tested this, however I am a bit unsure of the process that doesn't involve copying /etc/passwd inside the scratch image and instead just setting those env vars. The kyverno log file will not be helpful in this instance.

@realshuting
Copy link
Member

I've built a test image locally for the kyverno container, it seems to work as expected. And I did not see any issue in the log.

@chipzoller
Copy link
Member

The question isn't so much "will Kyverno run" but is it running as non-root? We don't build test cases for this, and switching the base temporarily to alpine to check isn't sufficient. It will need to be checked from the host side as well.

@realshuting
Copy link
Member

We have securityContext set to make sure Kyverno is not running as root:

securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- all
privileged: false
readOnlyRootFilesystem: true
runAsNonRoot: true
runAsUser: 1000

Is there anything else we need to check?

@chipzoller
Copy link
Member

Need to check running the image without those restrictions imposed to see if the image itself runs as non-root.

@imrajdas
Copy link
Contributor Author

imrajdas commented Jan 26, 2021

Hi @chipzoller, I have created the non-root docker file by copying /etc/passwd to the scratch image. Let me know if you want to change to this docker file.

# Multi-stage docker build
# Build stage
FROM golang:1.14 AS builder

LABEL maintainer="Kyverno"

# LD_FLAGS is passed as argument from Makefile. It will be empty, if no argument passed
ARG LD_FLAGS

ADD . /kyverno
WORKDIR /kyverno

RUN CGO_ENABLED=0 GOOS=linux go build -o /output/kyverno -ldflags="${LD_FLAGS}" -v ./cmd/kyverno/

RUN useradd -u 10001 kyverno

# Packaging stage
FROM scratch

LABEL maintainer="Kyverno"

COPY --from=builder /output/kyverno /
COPY --from=builder /etc/passwd /etc/passwd

USER kyverno

ENTRYPOINT ["./kyverno"]

@chipzoller
Copy link
Member

Thanks, looks good to me.

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
@imrajdas
Copy link
Contributor Author

Hi @realshuting, I have made the change according to @chipzoller. Please take a look. cc: @chipzoller

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
@realshuting
Copy link
Member

realshuting commented Jan 28, 2021

@rajdas98 Looks like one of the tests stuck in pending state. Do you have a test image so that I can verify?

AME                       READY   STATUS                  RESTARTS   AGE
kyverno-76c49d5f55-kw4l2   0/1     Init:CrashLoopBackOff   3          111s
ghcr.io/kyverno/cmd/cli/kubectl-kyverno              v1.3.2-rc1-13-gf52c0cab   768fc01ac6e5        3 minutes ago       38.3MB
ghcr.io/kyverno/kyverno                              v1.3.2-rc1-13-gf52c0cab   08a7005854d7        4 minutes ago       41.7MB
ghcr.io/kyverno/kyvernopre                           v1.3.2-rc1-13-gf52c0cab   6c5455ed8867        5 minutes ago       27.9MB
Error: unknown shorthand flag: 'o' in -o

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
@imrajdas
Copy link
Contributor Author

imrajdas commented Jan 29, 2021

Yes, @realshuting, there was a minor typo in the dockerfile of init container, but I fixed it in my last commit.
I have tested in my cluster, the log looks good to me.

log.txt

@imrajdas
Copy link
Contributor Author

images:

  1. imrajdas/kyvernopre:latest
  2. imrajdas/kyverno:latest

@realshuting realshuting merged commit 9da94d5 into kyverno:main Jan 29, 2021
@realshuting
Copy link
Member

realshuting commented Jan 29, 2021

@rajdas98 Seems like this PR builds CLI image with the wrong registry name -
9da94d5#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52L101

The image should use $(REPO)/$(KYVERNO_CLI_IMAGE):$(IMAGE_TAG), not $(REPO)/$(CLI_PATH):$(IMAGE_TAG).

Can you send a fix for this?

@imrajdas imrajdas deleted the dockerfile-refactor branch January 29, 2021 21:52
JimBugwadia pushed a commit that referenced this pull request Feb 1, 2021
…non-root user to the docker images (#1495)

* Dockerfile refactored

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* Adding non-root commands to docker images and enhanced the dockerfiles

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* changing base image to scratch

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* Minor typo fix

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* changing dockerfiles to use /etc/passwd to use non-root user'

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* minor typo

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* minor typo

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>
realshuting added a commit that referenced this pull request Feb 1, 2021
* initial commit for api server lookups

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* initial commit for API server lookups

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* Enhancing dockerfiles (multi-stage) of kyverno components and adding non-root user to the docker images (#1495)

* Dockerfile refactored

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* Adding non-root commands to docker images and enhanced the dockerfiles

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* changing base image to scratch

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* Minor typo fix

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* changing dockerfiles to use /etc/passwd to use non-root user'

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* minor typo

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>

* minor typo

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* revert cli image name (#1507)

Signed-off-by: Raj Babu Das <mail.rajdas@gmail.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* Refactor resourceCache; Reduce throttling requests (background controller) (#1500)

* skip sending API request for filtered resource

* fix PR comment

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fixes #1490

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fix bug - namespace is not returned properly

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* reduce throttling - list resource using lister

* refactor resource cache

* fix test

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fix label selector

Signed-off-by: Shuting Zhao <shutting06@gmail.com>

* fix build failure

Signed-off-by: Shuting Zhao <shutting06@gmail.com>
Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* fix merge issues

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* fix unit test

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

* add nil check for API client

Signed-off-by: Jim Bugwadia <jim@nirmata.com>

Co-authored-by: Raj Babu Das <mail.rajdas@gmail.com>
Co-authored-by: shuting <shutting06@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants