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

Add exec permission to controller in docker image #644

Closed
wants to merge 1 commit into from

Conversation

liuyuan10
Copy link

metallb.yaml runs controller as non-root user but in docker image,
controller binary is owned by root. Because docker ADD keeps file
permission, this change makes sure that /controller can be exeucted by
non-root user in the docker image.

fixes #643

Thanks for sending a pull request! A few things before we get started:

  1. If this is your first time, please read the contributing guide
  2. For non-trivial pull requests, please file an
    issue
    first, and get
    agreement that the change is a good idea, and a general guideline
    for how it should be implemented, before sending code. Large PRs
    that weren't first discussed and agreed upon in an issue won't be
    accepted.
  3. If the PR fixes a particular bug, please include the words "Fixed
    #" in the PR text, so that the bug auto-closes when
    the PR is merged.

metallb.yaml runs controller as non-root user but in docker image,
controller binary is owned by root. Because docker ADD keeps file
permission, this change makes sure that /controller can be exeucted by
non-root user in the docker image.

fixes metallb#643
@rata
Copy link
Contributor

rata commented Jun 27, 2020

I'd like to understand under which setups this is needed, to see if other things might need fixing too, but seems fine to add this line if it solves a problem for someone :-)

@liuyuan10 heh, sorry, just realize I asked the same in the issue 😂. Please share, but only in one place is fine :)

cc @johananl @russellb @daxmc99 WDYT? :)

Copy link
Contributor

@daxmc99 daxmc99 left a comment

Choose a reason for hiding this comment

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

I agree this could be fixed by user permissions but anything that slows down contributions from users should be fixed.

LGTM

@rata
Copy link
Contributor

rata commented Jun 29, 2020

LGTM. I'd add a comment mentioning umask, though, so we don't forget why we changed it in the first place :)

Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

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

Added a suggestion for the comment, please squash it. Otherwise, LGTM :)

controller/Dockerfile Show resolved Hide resolved
@rata
Copy link
Contributor

rata commented Jun 30, 2020

If you don't mind, I'll amend that simple comment myself and merge the ammended version :)

Copy link
Contributor

@rata rata left a comment

Choose a reason for hiding this comment

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

LGTM. Closing the PR as I merged this locally and pushed.

As I mentioned, I amended the commit. For transparency, modified the commit message with what I changed.

You can find the commit here. Basically did the same for the speaker Dockerfile and changed the commit to reflect that (mentioned only the controller) and the comment I suggested changing here.

Thanks again for the PR!

@rata rata closed this Jun 30, 2020
@liuyuan10 liuyuan10 deleted the build branch July 2, 2020 00:51
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.

standard_init_linux.go:211: exec user process caused "permission denied"
3 participants