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

Improve docker images #3737

Closed

Conversation

reneleonhardt
Copy link
Contributor

@reneleonhardt reneleonhardt commented Oct 31, 2023

Summary

🤖 Generated by Copilot at 24e5b67

This pull request refactors and improves the docker-related files and workflows of the project. It adds a Dockerfile that builds a single image for both frpc and frps, using build arguments and multi-stage build. It also adds a .dockerignore file and a .github/dependabot.yml file, and simplifies the .github/workflows/build-and-push-image.yml file. It deletes the dockerfiles/Dockerfile-for-frpc and dockerfiles/Dockerfile-for-frps files, as they are no longer needed.

WHY

Docker improvements:

  • Improve security by running rootless
  • Add standard labels
  • Unify docker build into single file

@fatedier
Copy link
Owner

I am not inclined to use dependabot as the risks outweigh the benefits.

I apologize, but I do not have the energy to review so many changes that are unrelated to the code. It might be more appropriate if you could first describe your expected changes and reasons in an issue, and then break them down into smaller, individual changes.

@reneleonhardt
Copy link
Contributor Author

Didn't want to cause stress, I will break them into 2 PRs without dependabot (which doesn't automerge, only suggests PRs automatically).

@reneleonhardt reneleonhardt force-pushed the update-and-improve-docker branch from 24e5b67 to 54218d0 Compare October 31, 2023 09:47
@reneleonhardt reneleonhardt changed the title Improve docker images and use dependabot Improve docker images Oct 31, 2023
Dockerfile Show resolved Hide resolved

ARG APP
ARG TITLE
LABEL org.opencontainers.image.authors="fatedier <[email protected]>"
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need these labels? What benefits will it bring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Best practice, a docker image tag is a also only a "label", those labels make it easier for your users what's inside this image.
The specific app version would be good too, but a bit harder to integrate in this case.
https://snyk.io/blog/how-and-when-to-use-docker-labels-oci-container-annotations/

Copy link
Owner

Choose a reason for hiding this comment

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

I do not prefer to add these redundant information.

Dockerfile Outdated

ARG APP
RUN addgroup -g 1000 -S ${APP} && adduser -u 1000 -S ${APP} -G ${APP} --home /app \
&& echo -e "#!/bin/sh\nexec /usr/local/bin/${APP} \$@" > /app/entrypoint.sh \
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose of this script? What are the differences compared to the previous execution method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for introducing this script was the unification of the 2 Dockerfiles into 1.
It's not allowed to use ARG/ENV variables inside ENTRYPOINT, your binaries have different names, it's best practice to create a minimal exec script in this case.
I will improve it a bit to make hadolint happy 😄

FROM alpine:3.18 AS runtime

ARG APP
RUN addgroup -g 1000 -S ${APP} && adduser -u 1000 -S ${APP} -G ${APP} --home /app \
Copy link
Owner

Choose a reason for hiding this comment

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

Given that frp is often used to listen on port 80, will not using the root user have any impact? On the other hand, what problems may arise from using the root user?

Copy link
Contributor Author

@reneleonhardt reneleonhardt Oct 31, 2023

Choose a reason for hiding this comment

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

Using root opens the attack surface to a whole lot more security bug possibilities.
The privileged port problem was solved 3 years ago:
https://stackoverflow.com/questions/66431299/i-can-bind-to-port-80-as-a-non-root-user-in-a-docker-container-why-whats-goin
Can you try both new images to verify the expected functionality?

It would be optimal if the Docker build would automatically verify the expected functionality... 😉

@fatedier
Copy link
Owner

I am not very professional in many aspects.

From my perspective, I am currently replacing some very simple and easy-to-understand Dockfile with a more complex version. I feel that there needs to be sufficient reasons to explain why we must do this.

I don't want to frequently modify these configurations/scripts that are unrelated to the core code unless there is official documentation stating that certain methods are explicitly prohibited or not recommended. This is similar to submitting a PR to modify code style, which may sometimes be acceptable but for maintainers, the benefits are minimal and it only increases maintenance costs.

@reneleonhardt
Copy link
Contributor Author

reneleonhardt commented Oct 31, 2023

I wasn't accusing you of anything, I am a professional 😄

You don't have to accept it if you don't want it.
I just improved all aspects including the content of the actual Dockerfiles.

For me it's 2 times easier to maintain 1 Dockerfile instead of 2 Dockerfiles doing "mostly" the same 😉

Maybe you didn't notice, but I was able to actually remove quite a lot of code lines in the process...

@BustedSec
Copy link

I think the addition of not running the code as root is a very good improvement. I think this is a good request and the guy did most of the heavy lifting already. Not seeing any issues with his changes. Would recommend pushing change.

@fatedier
Copy link
Owner

fatedier commented Nov 1, 2023

The build time of the new Dockerfile increased from 43s to 188s on my mac. This is likely due to the influence of Docker cache.

Perhaps some changes are indeed necessary, but I find it difficult to accept the complexity introduced into originally simple rules. I don't see where the core value and benefits lie.

Maybe we can just make the necessary changes.

@wuai1024
Copy link

wuai1024 commented Nov 7, 2023

我试过,如果不以root权限启动,就不能添加1024以下的端口了。

Copy link

github-actions bot commented Dec 8, 2023

PRs go stale after 30d of inactivity. Stale PRs rot after an additional 7d of inactivity and eventually close.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants