search

openfaas / faas

OpenFaaS - Serverless Functions Made Simple
https://www.openfaas.com
MIT License
25.19k stars 1.94k forks source link

Move all Dockerfile samples to non-root user #1167

Closed alexellis closed 3 years ago

alexellis commented 5 years ago

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 commented 5 years ago

Example of what this would look like for a function:

https://github.com/openfaas/faas/commit/e903f0ef73db1533ccb2b489c450bbb2723d4842

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 commented 5 years ago

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

cpanato commented 5 years ago

@alexellis i can take care this one

alexellis commented 5 years ago

Thank you @cpanato :+1: 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 commented 5 years ago

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

kturcios commented 5 years ago

I'll take this one

cpanato commented 5 years ago

@alexellis sorry for the delay, I've been busy :/ I did a first PR to check if this is in the right direction: https://github.com/openfaas/faas/pull/1181

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

alexellis commented 5 years ago

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

paurosello commented 5 years ago

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

alexellis commented 5 years ago

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 commented 5 years ago

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

burtonr commented 5 years ago

@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 commented 5 years ago

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

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 commented 5 years ago

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

paurosello commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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?

alexellis commented 3 years ago

/lock: inactivity