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.

Sign up for GitHub

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

Jump to bottom

use distroless base image #55

Merged
merged 1 commit into from
Aug 15, 2023
Merged

use distroless base image #55

merged 1 commit into from
Aug 15, 2023

Conversation

matthyx
Copy link
Contributor

@matthyx matthyx commented Aug 8, 2023

Overview

@codiumai-pr-agent
Copy link

PR Analysis

  • 🎯 Main theme: Change the base image of the Dockerfile to distroless
  • 📌 Type of PR: Enhancement
  • 🧪 Relevant tests added: No
  • ✨ Focused PR: Yes, the PR is focused as it only changes the Dockerfile to use a distroless base image
  • 🔒 Security concerns: No, the PR does not introduce any obvious security issues. However, running the application as root in the Docker container could potentially pose a security risk if the container is compromised.

PR Feedback

  • General suggestions: The PR is well-structured and the changes are clear. However, it would be beneficial to include a brief explanation in the PR description about why the base image is being changed to distroless. This would provide context to reviewers and future developers. Additionally, it would be helpful to include tests that verify the application still functions as expected with the new base image.

  • 🤖 Code feedback:

    • relevant file: build/Dockerfile
      suggestion: Consider using a specific version of the distroless image instead of 'latest'. Using 'latest' can lead to non-deterministic builds, as the 'latest' tag can point to different images over time. It's a best practice to use a specific version to ensure the build is repeatable. [important]
      relevant line: FROM gcr.io/distroless/static-debian11:latest

    • relevant file: build/Dockerfile
      suggestion: It's a good practice to run the application as a non-root user for security reasons. Consider creating a non-root user in the Dockerfile and use the USER directive to switch to that user. [important]
      relevant line: WORKDIR /root

    • relevant file: build/Dockerfile
      suggestion: The ARG 'image_version' is used but not defined in the Dockerfile. It's a good practice to provide a default value for ARGs in case they're not provided at build time. [medium]
      relevant line: ARG image_version

How to use

To invoke the PR-Agent, add a comment using one of the following commands:
/review [-i]: Request a review of your Pull Request. For an incremental review, which only considers changes since the last review, include the '-i' option.
/describe: Modify the PR title and description based on the contents of the PR.
/improve: Suggest improvements to the code in the PR.
/ask <QUESTION>: Pose a question about the PR.

To edit any configuration parameter from 'configuration.toml', add --config_path=new_value
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, use the /config command.

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Summary:

  • License scan: success
  • Credentials scan: success
  • Vulnerabilities scan: success
  • Unit test: success
  • Go linting: success

@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Summary:

  • License scan: success
  • Credentials scan: success
  • Vulnerabilities scan: success
  • Unit test: success
  • Go linting: success

@matthyx
use distroless base image
11adc1f 
Signed-off-by: Matthias Bertschy <matthias.bertschy@gmail.com>
@github-actions
Copy link

github-actions bot commented Aug 9, 2023

Summary:

  • License scan: success
  • Credentials scan: success
  • Vulnerabilities scan: success
  • Unit test: success
  • Go linting: success

@matthyx matthyx marked this pull request as ready for review August 9, 2023 06:26
@matthyx matthyx requested a review from dwertent August 9, 2023 06:26
@matthyx
Copy link
Contributor Author

matthyx commented Aug 9, 2023

we don't fail more tests than before... so I guess it's fine

@Bezbran
Copy link
Collaborator

Bezbran commented Aug 9, 2023

Why not from scratch?
We can always copy busybox into the running container for debugging.
And we can create yet another image for each build with busybox inside.

@matthyx
Copy link
Contributor Author

matthyx commented Aug 9, 2023

Why not from scratch? We can always copy busybox into the running container for debugging. And we can create yet another image for each build with busybox inside.

distroless is scratch + passwd + ssl certificates

you cannot copy things inside a container with kubectl cp without a shell and tar

@matthyx matthyx requested review from alegrey91 and removed request for dwertent August 10, 2023 12:33
alegrey91
alegrey91 reviewed Aug 14, 2023
View reviewed changes
Copy link
Collaborator

@alegrey91 alegrey91 left a comment

Choose a reason for hiding this comment

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

Thanks @matthyx for this great improvement!
Please, correct me if I'm wrong, but we should adjust the pipeline according to the new ARG items provided (TARGETOS, TARGETARCH, image_version), right?

build/Dockerfile
ENTRYPOINT [ "./kube-host-sensor" ]
ENV GO111MODULE=on CGO_ENABLED=0
WORKDIR /work
ARG TARGETOS TARGETARCH
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't we provide TARGETOS and TARGETARCH in the pipeline?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, this is already provided by builds during the Docker build

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, great, what about the image_version instead?

build/Dockerfile

COPY --from=builder /out/kube-host-sensor /usr/bin/kube-host-sensor

ARG image_version
Copy link
Collaborator

Choose a reason for hiding this comment

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

same as above

alegrey91
alegrey91 approved these changes Aug 15, 2023
View reviewed changes
Copy link
Collaborator

@alegrey91 alegrey91 left a comment

Choose a reason for hiding this comment

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

LGTM

@alegrey91 alegrey91 merged commit 6e97c07 into master Aug 15, 2023
20 of 26 checks passed
@dwertent
Copy link
Contributor

TODO: Open a PR in the helm-chart

@matthyx matthyx deleted the distroless branch August 16, 2023 08:45
@matthyx
Copy link
Contributor Author

matthyx commented Aug 16, 2023

TODO: Open a PR in the helm-chart

kubescape/helm-charts#259

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alegrey91 alegrey91 alegrey91 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@matthyx @Bezbran @dwertent @alegrey91