AppImage / AppImageKit

Package desktop applications as AppImages that run on common Linux-based operating systems, such as RHEL, CentOS, openSUSE, SLED, Ubuntu, Fedora, debian and derivatives. Join #AppImage on irc.libera.chat
http://appimage.org
Other
8.55k stars 545 forks source link

Automatically run --appimage-extract-and-run when in Docker #912

Open probonopd opened 5 years ago

probonopd commented 5 years ago

Consistent with our "it should just work" mantra, automatically run --appimage-extract-and-run when in a Docker container

@TheAssassin re-iterates that one should not enable FUSE for security reasons in Docker containers...

probonopd commented 5 years ago

Outside Docker:

me@host:~$ cat /proc/1/cgroup 
12:rdma:/
11:memory:/
10:perf_event:/
9:cpuset:/
8:freezer:/
7:pids:/
6:net_cls,net_prio:/
5:devices:/
4:hugetlb:/
3:cpu,cpuacct:/
2:blkio:/
1:name=systemd:/init.scope
0::/init.scope
me@host:~$ cat /proc/cmdline 
BOOT_IMAGE=(loop)/casper/vmlinuz file=/cdrom/preseed/xubuntu.seed boot=casper iso-scan/filename=/boot/iso/xubuntu-18.04-desktop-amd64.iso quiet splash --- for-casper --> iso-scan/filename=/boot/iso/xubuntu-18.04-desktop-amd64.iso console-setup/layoutcode=de locale=en_US timezone=Europe/Berlin username=me hostname=host noprompt init=/isodevice/boot/customize/init max_loop=256
TheAssassin commented 5 years ago

There are strong reasons not to do this:

probonopd commented 5 years ago

Inside Docker on GitLab CI:

cat /proc/1/cgroup
11:perf_event:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
10:hugetlb:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
9:net_cls,net_prio:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
8:pids:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
7:memory:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
6:blkio:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
5:devices:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
4:cpu,cpuacct:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
3:freezer:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
2:cpuset:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
1:name=systemd:/docker/a0182497e4c02394b5afca3fa0cfc1d7c500a2d27d55246a6b69cbd05d4981c1
TheAssassin commented 5 years ago

After the very intense discussion about this change (you know which I mean) in linuxdeployqt, I need to see some strong arguments why to make this the default after having had to make it a flag in the first place.

I'd be very open to promoting this flag in e.g., the FUSE error message, and the help texts. Also, I'd put it in docs.appimage.org.

probonopd commented 5 years ago

There is an optimization called $NO_CLEANUP.

It should just "do the right thing". Not even I knew (or want to know) about such details.

If running "properly" on Docker is not possible/recommended, then "just do the right thing".

I want my tools to have an "autopilot" for everything, that works for 99% of users.

probonopd commented 5 years ago

After the very intense discussion about this change (you know which I mean) in linuxdeployqt, I need to see some strong arguments why to make this the default after having had to make it a flag in the first place.

Indeed I was not happy about this, because I feared that everyone with a broken FUSE setup would not even notice the broken FUSE setup.

But if in Docker FUSE is not recommended, then why not "just do" what is optimal for FUSE, without bothering the Docker user?

TheAssassin commented 5 years ago

The flag works just fine for users, too (of course only once they know). That's not really convincing.

probonopd commented 5 years ago

That's the problem: Users will only learn about the flag once they already run into "problems". Hence it is perceived "problematic" whereas it should be perceived as "it just works"... https://www.youtube.com/watch?v=hdv1sPcADGE

TheAssassin commented 5 years ago

Error messages are never bad. But they become really helpful when adding information how to resolve/work around the issue.

probonopd commented 5 years ago

Error messages where unexpected errors are, yes. When you can already know that the "error" is not an error but expected (e.g., you say FUSE should not be used in Docker), then why run into an error in the first place - just "do the right thing"...

TheAssassin commented 5 years ago

How about showing a warning that we implicitly "add" --appimage-extract-and-run to the call?

probonopd commented 5 years ago

Something like

Running in Docker, hence extracting the AppImage before running it. Set APPIMAGE_DO_NOT_EXTRACT_IN DOCKER=1 if this is not intended.

would probably be an excellent idea, yes.

TheAssassin commented 5 years ago

I could live with that. But I still need to see your references re. "recognizing Docker".

probonopd commented 5 years ago

Sorry, I had meant to paste them into the first post.

probonopd commented 4 years ago

Would probably help in cases like this.

TheAssassin commented 4 years ago

I guess people should just export APPIMAGE_EXTRACT_AND_RUN=1 in such containers. Recognizing Docker itself is anyway quite difficult, I'm not convinced the checks are reliable enough.

Just make that export part of your build "recipes", all our tools provide recent enough builds (at least for continuous tags) so their runtime supports this.

probonopd commented 4 years ago

My point is that one should not need to learn and think about such things, it should "just work". Most people want stuff to "just work" with no clue how things work. If you don't like this, you are in good company - Albert Einstein had complained about it as well:

https://www.einstein-website.de/z_biography/einstein1930.mp3

Sollen sich auch alle schämen, die gedankenlos sich der Wunder der Wissenschaft und Technik bedienen und nicht mehr davon geistig erfasst haben als die Kuh von der Botanik der Pflanzen, die sie mit Wohlbehagen frisst.

English translation:

Shame also on all those who thoughtlessly make use of the wonders of science and technology and have not grasped more of them intellectually than a cow has grasped of the botany of plants which she eats with delight.

(Albert Einstein)

TheAssassin commented 4 years ago

Developers aren't "most people". Developers want to know how things work. After all they are obliged to do so, they are responsible. Some developers might be too lazy for this. Then either you must force them to know (otherwise you'll get a huge workload, as they'll forward any issue to you...). Developers must be able to assess all aspects of their project. They need to be able to conduct technology assessments ("Technikfolgen-Abschätzungen").

Your way of thinking is too naive for this world. I also agree the Average Joe user doesn't need to be made aware of the problems. It'd be better, but that's not how the world works. But from developers, I want to expect a little bit of interest and effort.

probonopd commented 4 years ago

Then you probably also expect your bus driver to know how the engine works... ;) I'd say most application developers have no clue how e.g., a compiler works on its inside. (Especially true for Java/Android/JavaScript/... developers.)

probonopd commented 4 years ago

Another case where the author would have saved time and trouble if we silently ran --appimage-extract-and-run because we knew the user was running in Docker. I'd like to make this stuff to "just work" without users needing to "fiddle around".

https://github.com/cern-phone-apps/desktop-phone-app/issues/141#issuecomment-540940170

probonopd commented 4 years ago

Yet another case where someone did not know how to use --appimage-extract-and-run in Docker.

https://github.com/AppImage/AppImageKit/issues/997

probonopd commented 4 years ago

I'd welcome you to implement https://github.com/AppImage/AppImageKit/issues/912#issuecomment-456573013 @TheAssassin. It would make AppImages "just work" on Docker.

TheAssassin commented 4 years ago

You are free to send a PR. I don't see any real reason to change anything. As said, I could live with it, but I don't plan to implement it myself.

Even after you turned around 180° and started to accept and encourage the extraction, the error message still doesn't point to this parameter (or the respective environment variable). How about starting there? That change is way smaller, and a lot safer and pretty much as easy to use as anything else.

probonopd commented 4 years ago

I want an error message, discouraging from extracting the AppImage in any situation other than Docker, because in Docker this is the right thing to do.

Does this make sense?

probonopd commented 4 years ago

I don't see any real reason to change anything.

Currently it is manual work to find out why AppImages don't "just work" in Docker and what to do about it. And that is avoidable manual work.

TheAssassin commented 4 years ago

As said, I do not have the time to fix the runtime to implement your ideas. If you're reluctant to implementing this yourself, consider changing the error message. The only reason it's hard for users to figure out the solution is that the error message doesn't mention --appimage-extract etc., after all.

probonopd commented 4 years ago

Key is to have different behavior when we are inside Docker vs. when not.

TheAssassin commented 4 years ago

I will write this one last time: I don't mind having auto extraction, but I don't have time to implement it, so you either have to wait or you do it yourself. I am not a time wizard! Please stop begging.

probonopd commented 4 years ago

Yes, I get it ;-)

TheAssassin commented 4 years ago

Tempted to say "at last"...

probonopd commented 4 years ago

I am implementing Docker detection based on the above theory of operation in my experimental Go code for now and if it turns out to work properly there, then we can port it over to C for runtime.c.

Edit: Looks like it is working:

https://travis-ci.org/probonopd/appstream/jobs/619067924#L4750

Here are the 5 lines of code this took me in Go:

https://github.com/probonopd/go-appimage/blob/da4c75e32db388385d935d6c95f78fc48383fa4e/src/appimagetool/appimagetool.go#L41-L46

TheAssassin commented 4 years ago

Please feel free to send a PR to the runtime.

probonopd commented 4 years ago

Does this look about right @TheAssassin?

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/param.h>
#include <unistd.h>

int main()
{
    // Detect whether we are running inside a Docker/LXC container and if we are,
    // set APPIMAGE_EXTRACT_AND_RUN because the AppImage might fail otherwise
    FILE *f;
    char line[MAXPATHLEN];
    f = fopen("/proc/1/cgroup", "r");
    if (f) {
        fgets(line, MAXPATHLEN, f);
        if ((strstr(line, "/docker") != NULL)  || (strstr(line, "/lxc") != NULL) || (access("/.dockerenv", F_OK ) != -1)) {
            fprintf(stderr, "%s", "Running inside Docker/LXC, hence using APPIMAGE_EXTRACT_AND_RUN\n");
            setenv("APPIMAGE_EXTRACT_AND_RUN", "1", 1);
        }
    }
}
TheAssassin commented 4 years ago

You do not evaluate the return values of fgets at all. strstr calls are a bit unsafe. Also I'm not entirely sure whether the calls to strstr are completely safe. I'd have to check the man page.

MAXPATHLEN is just a random constant, it doesn't mean allocating a buffer with its size will be large enough. You should always use strn* methods which also check for boundaries. You can pass MAXPATHLEN as buffer size to those methods.

You should not misuse environment variables as global variables IMO.

probonopd commented 4 years ago

Thanks for your review. Especially in the runtime, I am always trying to get away with the fewest lines of code possible.

You do not evaluate the return values of fgets at all.

My line of thought was that the worst thing that can happen is that in case of an error, we won't set APPIMAGE_EXTRACT_AND_RUN. Which is no worse than without having this check at all. Am I overlooking some error case that needs to be handled specially?

MAXPATHLEN is just a random constant, it doesn't mean allocating a buffer with its size will be large enough.

I never know how large a string can get. What value would you put there?

You should always use strn* methods which also check for boundaries. You can pass MAXPATHLEN as buffer size to those methods.

Can you give an example?

You should not misuse environment variables as global variables IMO.

I tried to do it in a "minimally invasive" way, with the least potential of introducing additional lines of code, additional variables, and additional complexity. Do you just consider the "misuse" of environment variables bad style or do you see real downsides here?

Always happy to learn.

TheAssassin commented 4 years ago

Especially in the runtime, I am always trying to get away with the fewest lines of code possible.

The focus should IMO be on secure code, not on "short" code.

I never know how large a string can get. What value would you put there?

It's pretty unlikely MAXPATHLEN will be needed anyway. You could reallocate memory and enlarge your buffers if needed, but hacking this into the single huge function is too much. Just use strn* to avoid overflows and you're good to go.

Can you give an example?

There's plenty out on the Internet. They work like their non-n counterparts, except for taking a buffer length. E.g., strncmp will not only look for a null byte for string termination but won't go any further than the buffer length. If you have a string which is not null-terminated, strncmp will not read any more bytes than the provided length.

I tried to do it in a "minimally invasive" way, with the least potential of introducing additional lines of code, additional variables, and additional complexity. Do you just consider the "misuse" of environment variables bad style or do you see real downsides here?

Here it's fine, I guess. We intend to write a new runtime anyway, there we can implement a proper state management.

shoogle commented 4 years ago

In my opinion auto extract should happen everywhere or nowhere. There shouldn't be any special casing of certain environments. What happens when users say "well it worked in Docker... why not here?". You'll just end up with the same problem you have now, but even harder to diagnose!

Why make Docker special? Is there any disadvantage to extracting everywhere? The user must already trust the AppImage if they are attempting to run it, and an untrustworthy AppImage could always replace your runtime with a custom one anyway.

TheAssassin commented 4 years ago

@shoogle that's what I think, too. I wouldn't bind it to Docker. But I've been outvoted it seems.

The problem with Docker, as explained, is that there's no FUSE usually for security reasons.

probonopd commented 4 years ago

Why make Docker special? Is there any disadvantage to extracting everywhere?

Yes. Sloppy users might think "oh well, AppImages need to be extracted". Except for situations like Docker we want to force users to fix their FUSE setup rather than accept a half-broken system.

What happens when users say "well it worked in Docker... why not here?".

We could print a very clear message to stderr, like

++++++++++++++++++++++++++++++++++++++++++++++++
Running from Docker
Since this AppImage is running within Docker, it is extracted
rather than mounted.
++++++++++++++++++++++++++++++++++++++++++++++++
shoogle commented 4 years ago

we want to force users to fix their FUSE setup rather than accept a half-broken system

What are the advantages of FUSE compared to extracting? (Genuinely curious)

probonopd commented 4 years ago

Think "10 GB game AppImage". Thinking in such extremes make the advantage of mounting abundantly clear.

shoogle commented 4 years ago

Indeed! How about auto extracting if the AppImage is smaller than a certain size (e.g. 20 MB) and not otherwise? That would cover developer apps in Docker and small consumer applications, while excluding bigger apps and games. Naturally, there would be environment variables available for users that always (or never) want to auto extract. This avoid special-casing build environments and works everywhere, not just Docker.

Edit: There could even be an envronment variable to set the size limit for auto-extraction.

TheAssassin commented 4 years ago

@shoogle I think it might be easier to just "mark" AppImages which may be auto-extracted if FUSE is not available with a semi-hidden flag. Our dev tools can be marked that way. That way, we don't bind ourselves to Docker.

@probonopd just for clarification (I don't want to support your case), but extracting should be done only when in Docker and when FUSE is not available. Both can be checked.

In any case, we can fix this in the short term, as in the long term we'll have a type 3, hopefully.

shoogle commented 4 years ago

What is type 3? Is this your codeword for AppImages that are fully supported by the base system (like .app on macOS?) or do you have a specific technology in mind?

TheAssassin commented 4 years ago

@shoogle please see https://github.com/TheAssassin/type3-runtime.

serapath commented 3 years ago

Hey, just stumbled upon this issue. Is this now a thing?

I'm planning to use AppImage but sometimes I need to run it in Docker and I guess extracting the AppImage according to this issue is what is necessary, but it's not the right thing to do in other environments.

Is there an example or some more information so I could learn more about that? :-)

TheAssassin commented 3 years ago

Nothing has been implemented so far. The easiest fix is to follow https://github.com/AppImage/AppImageKit/issues/912#issuecomment-528669441.

stefdoerr commented 1 year ago

Just to chime in as well, I spent some few hours trying to get fuse to work since the instructions on the error said to install it, before finding out it's not doable or recommended inside containers. I was about to give up on AppImage until I randomly stumbled upon this thread. Also I'm running inside singularity/apptainer and not docker so Docker autodetect is also in my opinion not the best solution as there are other containers.

Maybe switching to extract-and-run when fuse fails would be a good option if it goes along with a warning that you should set the env var if you are inside a container.

In any case for me either is a no-go because it's a 2GB AppImage which needs to run every few seconds and extracting seems to take a good while. So I will just move to some other solution.

soredake commented 1 year ago

It will be nice if --appimage-extract-and-run can be applied when running in flatpak too, preferably with TMPDIR set to /dev/shm as /tmp can be too small in flatpak.