The Wayback Machine - https://web.archive.org/web/20200919071011/https://github.com/openfaas/faas/issues/1167
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

Move all Dockerfile samples to non-root user #1167

Open
alexellis opened this issue Apr 23, 2019 · 19 comments
Open

Move all Dockerfile samples to non-root user #1167

alexellis opened this issue Apr 23, 2019 · 19 comments

Comments

@alexellis
Copy link
Member

@alexellis alexellis commented Apr 23, 2019

Move all Dockerfile samples to non-root user

Expected Behaviour

As an OpenFaaS end-user, I want to run all samples on my OpenFaaS cluster, even with the new non-root feature enabled.

Current Behaviour

Some functions are/were blocked which are in the samples. An example is the SentimentAnalysis function found by @LucasRoesler

Possible Solution

I've specifically fixed SentimentAnalysis, but there are others which now need work in the sample-functions/ folder.

Steps to Reproduce (for bugs)

  1. Each function must be tested as a non-root user
@alexellis
Copy link
Member Author

@alexellis alexellis commented Apr 23, 2019

Example of what this would look like for a function:

e903f0e

Other functions which need edits/checking: https://github.com/openfaas/faas/tree/master/sample-functions

Where possible move samples to use the OpenFaaS templates which already use non-root.

@alexellis
Copy link
Member Author

@alexellis alexellis commented Apr 23, 2019

For the Webhooks stash function, make sure it only writes to /tmp/.

@cpanato
Copy link
Contributor

@cpanato cpanato commented Apr 24, 2019

@alexellis i can take care this one

@alexellis
Copy link
Member Author

@alexellis alexellis commented Apr 24, 2019

Thank you @cpanato 👍 do you have a rough idea on when you can get it done by?

This change will also help with OpenShift users (something I didn't think of at the time)

@alexellis
Copy link
Member Author

@alexellis alexellis commented Apr 24, 2019

Join Slack to connect with the community
https://docs.openfaas.com

@kturcios
Copy link
Contributor

@kturcios kturcios commented May 6, 2019

I'll take this one

@cpanato
Copy link
Contributor

@cpanato cpanato commented May 7, 2019

@alexellis sorry for the delay, I've been busy :/
I did a first PR to check if this is in the right direction: #1181

@kturcios feel free to open PR for others sample functions as well, just let syncronize

@alexellis
Copy link
Member Author

@alexellis alexellis commented Jun 26, 2019

@ivanayov @kturcios any interest from either of you in continuing this work?

@paurosello
Copy link
Contributor

@paurosello paurosello commented Jun 26, 2019

I can help on this, will open PR as soon as posible

@alexellis
Copy link
Member Author

@alexellis alexellis commented Jun 26, 2019

Thank you @paurosello that is great to hear.

Whilst inside the Dockerfiles it would be ideal if you can also change from using curl to get the watchdog, to using the new approach such as: https://github.com/openfaas/faas/blob/master/sample-functions/AlpineFunction/Dockerfile#L1

@mikechernev
Copy link

@mikechernev mikechernev commented Oct 3, 2019

Does this still need attention?
If so, I can look into it.

@burtonr
Copy link
Contributor

@burtonr burtonr commented Oct 3, 2019

@mikechernev Looking through the sample functions It seems most have been updated, however there look to be a handful remaining. In my quick look-through, I see MarkdownRender, Nmap, and WebhookStash. There may be a few others as well.

@audioboxer217
Copy link
Contributor

@audioboxer217 audioboxer217 commented Oct 8, 2019

I'm out of time for today, but I did find a few more that I believe need to be done:

  • BaseFunctions/cobol
  • echo
  • figlet
  • gif-maker
  • HostnameIntent
  • NodeHelloEnv
  • NodeInfo
  • pwgen
  • WordCountFunction

Also, I don't see any Dockerfile at all for:
business-strategy-generator
haveibeenpwned

Note: I used checkboxes so people can mark when they're working on these in an attempt to prevent duplicate work.

edit: removed checkboxes on the "missing Dockerfiles" note since Alex pointed out that this is expected.

@alexellis
Copy link
Member Author

@alexellis alexellis commented Oct 8, 2019

Where a template is used, no separate dockerfile is needed since it gets imported by the template.

@paurosello
Copy link
Contributor

@paurosello paurosello commented Oct 8, 2019

Hello @audioboxer217 the image functions/alpine:latest is defined in AlpineFunction which already contains the modification to make it non root and use the watchdog so it might be better to extend all functions from that one and improve the base.

What do you think?

@audioboxer217
Copy link
Contributor

@audioboxer217 audioboxer217 commented Oct 8, 2019

That's a good idea. I didn't even realize that that's what that was. I was just viewing it as a sort of template to follow based on the way it was laid out with the comment.

I might play around with that idea tomorrow. If it does work out then several of the Dockerfiles that are already "done" might be able to be updated this way as well.

The main thing is, what are these samples intended to show and what's the best way to go about that? For that part, I think some of the maintainers, like @alexellis, might want to weigh in.

@audioboxer217
Copy link
Contributor

@audioboxer217 audioboxer217 commented Oct 9, 2019

I did some preliminary testing with this. It seems to cause some permissions issues for some of these if functions/alpine is used at the beginning. For instance, the nmap sample has RUN apk add --no-cache nmap which fails with:

ERROR: Unable to lock database: Permission denied
ERROR: Failed to open apk database: Permission denied

I believe this is because it's attempting to use the 1000 user. Of course, there are ways to get around this with Multi-Stage Builds if we'd still like to do that. Although this particular example (an app installed via an APK repo) can be a bit difficult to work with since there are a lot of items that get put in place as part of the 'install'. It would work well for others though like MarkdownRender.

@paurosello
Copy link
Contributor

@paurosello paurosello commented Oct 21, 2019

Hmm you are right... it doesn't feel right to make this change in all Dockerfiles, but installing packages is something that needs to be enabled.

We can either remove that from the top Dockerfile and modify all children Dockerfile or document how to install packages in child images:

USER 0
RUN apt, apk ...
USER 1000

What do you think?

@audioboxer217
Copy link
Contributor

@audioboxer217 audioboxer217 commented Oct 25, 2019

yeah, I think that makes sense. Especially since these are intended to be "samples" which I usually take to mean, "an example of how to do x".

The question is, should these updates be made based on this same Issue or should a new one be opened for that work?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.