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

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

Open
probonopd opened this issue Jan 19, 2019 · 49 comments
Open

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

probonopd opened this issue Jan 19, 2019 · 49 comments

Comments

@probonopd
Copy link
Member

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

  • If the file /.dockerenv exists, and/or
  • If /proc/1/cgroup begins with /lxc/ or /docker/

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

@probonopd
Copy link
Member Author

probonopd commented Jan 19, 2019

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
Copy link
Member

There are strong reasons not to do this:

  • AppImages != self-extracting archives. We should never ever promote that, hence the "hidden" flag. You were the one who suggested to do it as a flag, and also not to include this flag anywhere (e.g., in help texts).
  • There is an optimization called $NO_CLEANUP. If set, the runtime will leave the files extracted, and won't have to re-extract everything on every invocation. This wouldn't be used at all any more, since noone could predict when it would help.
  • The .dockerenv stuff just doesn't exist in a majority of containers. You can try yourself.
  • I don't think the cgroup "method" you suggested is working on every Docker host. Please provide some references.

@probonopd
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

probonopd commented Jan 19, 2019

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
Copy link
Member

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

@probonopd
Copy link
Member Author

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
Copy link
Member

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

@probonopd
Copy link
Member Author

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
Copy link
Member

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

@probonopd
Copy link
Member Author

probonopd commented Jan 22, 2019

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
Copy link
Member

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

@probonopd
Copy link
Member Author

probonopd commented Jan 22, 2019

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

@probonopd
Copy link
Member Author

Would probably help in cases like this.

@TheAssassin
Copy link
Member

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
Copy link
Member Author

probonopd commented Sep 6, 2019

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
Copy link
Member

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
Copy link
Member Author

probonopd commented Sep 6, 2019

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
Copy link
Member Author

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".

cern-phone-apps/desktop-phone-app#141 (comment)

@probonopd
Copy link
Member Author

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

#997

@probonopd
Copy link
Member Author

probonopd commented Nov 30, 2019

I'd welcome you to implement #912 (comment) @TheAssassin. It would make AppImages "just work" on Docker.

@TheAssassin
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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

@TheAssassin
Copy link
Member

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
Copy link
Member Author

Yes, I get it ;-)

@TheAssassin
Copy link
Member

Tempted to say "at last"...

@probonopd
Copy link
Member Author

probonopd commented Nov 30, 2019

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
Copy link
Member

Please feel free to send a PR to the runtime.

@probonopd
Copy link
Member Author

probonopd commented Mar 14, 2020

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member

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
Copy link

shoogle commented Mar 22, 2020

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
Copy link
Member

@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
Copy link
Member Author

probonopd commented Mar 22, 2020

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
Copy link

shoogle commented Mar 22, 2020

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
Copy link
Member Author

probonopd commented Mar 22, 2020

  • Not needing temp space
  • Not needing the time to extract everything before being able to run
  • Not needing to clean up temp space

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

@shoogle
Copy link

shoogle commented Mar 22, 2020

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
Copy link
Member

@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
Copy link

shoogle commented Mar 23, 2020

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
Copy link
Member

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

@serapath
Copy link

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
Copy link
Member

Nothing has been implemented so far. The easiest fix is to follow #912 (comment).

jellespijker pushed a commit to Ultimaker/cura-build-environment that referenced this issue Apr 8, 2022
Setting the APPIMAGE_EXTRA_AND_RUN=1 as ENV
allows reusing the target packaging on a non docker environment

AppImage/AppImageKit#912 (comment)

Contributes to CURA-8640
@stefdoerr
Copy link

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
Copy link

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.

YuviGold added a commit to YuviGold/mercado that referenced this issue Oct 31, 2022
At first tried installing libfuse as the AppImage error message presents
s3fs-fuse/s3fs-fuse#647 (comment)

But eventually found out about
AppImage/AppImageKit#912 (comment)
which is not an optimized solution, but it is preffered instead of
providing sys admin privileges to a docker container.
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

No branches or pull requests

6 participants