mathieuprog / tz

Time zone support for Elixir
Apache License 2.0
146 stars 11 forks source link

Release crashes when configuring data_dir at runtime #21

Closed nshafer closed 3 weeks ago

nshafer commented 1 year ago

I just migrated a project from tzdata to your tz module, but I'm running into an issue when trying to run a release that sets the :data_dir config variable at runtime, in the config/runtime.exs file.

ERROR! the application :tz has a different value set for key :data_dir during runtime compared to compile time. Since this application environment entry was marked as compile time, this difference can lead to different behaviour than expected:

  * Compile time value was not set
  * Runtime value was set to: "local/tz/"

To fix this error, you might:

  * Make the runtime value match the compile time one

  * Recompile your project. If the misconfigured application is a dependency, you may need to run "mix deps.compile tz --force"

  * Alternatively, you can disable this check. If you are using releases, you can set :validate_compile_env to false in your release configuration. If you are using Mix to start your system, you can pass the --no-validate-compile-env flag

{"init terminating in do_boot",{<<"aborting boot">>,[{'Elixir.Config.Provider',boot,2,[]}]}}
init terminating in do_boot ({,[{Elixir.Config.Provider,boot,2,[]}]})

Crash dump is being written to: erl_crash.dump...done

First, I found Issue #11 where you added runtime fetching of the :data_dir config, so thank you for that. And technically, this works. However, due to some checking that's done by releases when they start up, a release that uses this module will crash if it also sets the :data_dir config env during runtime.

I duplicated this issue with a bare-bones elixir project at https://github.com/nshafer/tzrel. I wrote up the description of the problem and how to duplicate it in the README. So it should be trivial for you to clone that repo, create a release, and duplicate the problem.

But in short, while the code is actually running just fine because Tz.IanaDataDir.dir/0 is doing a Application.get_env(:tz, :data_dir) at runtime, the problem instead is being caused in Tz.CompileRunner. In there, in the module level code, a call to Application.compile_env(:tz, :data_dir) is being made. So the elixir release is spotting this and throwing the error because the value of :data_dir that Tz.CompileRunner received during the compile is different than what it's going to get during runtime. I confirmed this is the problem by commenting out lines 29-39 in that file, and the release ran fine. I think it's a basic sanity check that the release code is doing, and it can be disabled, but that seems a bit like a sledgehammer approach, and I would prefer to keep this sanity check enabled for my releases.

Looking at the code in Tz.CompileRunner, it looks like it's intended to throw a compile-time error if the user of the module doesn't configure :iana_version and :data_dir correctly. I don't know the exact intent or impact of changing this code, or the story behind it, so I didn't feel comfortable submitting a PR that just removed it. At least not without discussing it first. And I'm not sure there's any other way to do this compile-time check without calling Application.compile_env(). But, since the data_dir can be changed at runtime, this code won't catch runtime misconfigurations anyways. So maybe it needs to be moved into runtime code somewhere. But I'm not sure where.

Anyways, thanks for your time and the module.

mathieuprog commented 1 year ago

As I mentioned in #11 I am not sure why releases require data_dir to change at runtime. Really basic question because I have no experience with releases yet, but could you explain simply?

As you know time zone periods are computed at compilation time. So we read the data_dir at compilation time and we also need the data_dir to be configurable at runtime for releases. And if we use get_env instead of compile_env that triggers a warning? I feel like there must be something basic that we are missing or don't understand.

nshafer commented 1 year ago

It's not necessarily that release require being able to set the data_dir at runtime, rather the environment that releases are run in. In my case, I compile my software then package it in a .deb archive for installation. This means that when it's installed, all of the files in the release are owned by root and are not user-writable, which includes the priv/ directory of each module included. This software is then run by an unprivileged non-root user for security reasons, which can run the release, but can't write anywhere in it. So for many things I allow the user to configure a data directory, such as $XDG_DATA_HOME which defaults to ~/.local/share. Or the admin may install it as a system service, and want to configure the writable data directory as /usr/share/<appname>/ which only allows the unprivileged user to write to it. So this directory is not known at compile time, nor when the release is created as part of the compile. It's only known when runtime.exs is ran as part of the release startup, and the actual directory to write to is configured by a Config provider, or simply an OS environment variable, or whatever mechanism is desired. Without being able to set the tz modules :data_dir at runtime, automatic updates are not possible because the default :data_dir is not writable. I could possibly work around this for some cases, but that might also introduce serious security issues.

I understand that tz needs to parse the included tzdataXXXXX dir during compilation. And this is fine, for both when the tz module is compiled, and the project using it as a dependency. But for the auto-update, a writable directory needs to be provided. Perhaps they need to be two separate directory variables. One for compile time, which would be the tz/priv/ directory included in the module, and another for auto updates. Perhaps the Tz.UpdatePeriodically genserver should take an optional data dir to write to when it starts up, instead of using the app env. Or I think more simply, the sanity-checking code in Tz.CompileRunner should be moved to runtime. Perhaps as a one-shot genserver that starts as part of the tz's application definition, that simply checks all the variables at runtime and issues a warning on the console.

The person that submitted #11 mentioned they were having issues with this when dockerizing their container. I'm not sure why that is, since you can easily make the release as part of the docker image creation, but they obviously needed the flexibility there, and went back to tzdata as a result. I could also see this being a problem in the embedded world in Nerves projects, as embedded devices will often use read-only filesystems for security and a/b updates. So auto-updates wouldn't work there, and a runtime configurable data dir that can be written to would need to be specifiable.

In short, I think your module has tremendous potential to become a core module used by a lot of projects, but it needs to be flexible at runtime. There's a very good reason why the runtime.exs system was added to releases in 1.10 (I think). Dealing with similar issues when releases were added in 1.9 was a serious headache.

Anyways, hope that helps.

mathieuprog commented 9 months ago

Can you check what happens if we just remove the checks using compile_env? Would the runtime data dir just replace the compile time data dir seamlessly? If so, I'm just open to move these checks elsewhere using get_env.

nshafer commented 9 months ago

I started to take a look into this again the other day. I've been doing some metaprogramming for another project, so a lot more of this makes sense to me now. When I get some time I'll play around with some options now that I know you're ok with removing the checks at compile time.

mathieuprog commented 2 months ago

Any update on this?

nshafer commented 2 months ago

Sorry, no. I haven't forgotten about it, just been slammed with work for months. Hopefully I will get some time to poke at it again in the next couple months.

mathieuprog commented 3 weeks ago

I am closing this but feel free to reopen to have another look at it