fix(nginx): link nginx.conf to dataDir (#173)

[szucsitg]

Fixes #173

Symlinks config file nginx's dataDir.

[shivaraj-bh]

Why not introduce configFilePath option and have it set to config.configFile by default? You can modify this path to any config present in your relative path

[szucsitg]

sure, if you'd like to have it that way, I'm happy to change. I'll force push this PR in a few minutes 👍

[shivaraj-bh]

It would perhaps be a feat instead of fix now, won’t it?

[shivaraj-bh]

Sorry for being picky about commits, we are using commits to auto-generate CHANGELOG for future releases, hence.

[szucsitg]

it's fine to be picky about commit messages, I am too 😄 I took your advice, and make it optional to symlink the config, let me know what you think about this approach

[shivaraj-bh]

Oh hold on, I totally missed the fact that we already expose an option configFile that is of type path. Wouldn’t this already work for you, without the changes in this PR:

{
  services.nginx.n1 = {
    enable = true;
    configFile = ./<path-to-my-config>/nginx.conf;
  };
}

[szucsitg]

What I did is I templated a Nix file (string ultimately) with some inputs, like static site builds as derivation and that is passed to httpConfig. But that config has a couple of imports. However I just realized that I should probably fix that anyway, because this will only run if the repo is checked out on the file system 🤔 so maybe this feature is rather a bad idea

[shivaraj-bh]

No, doesn’t look like it. @szucsitg I will be closing this for now, let me know if it has to be reopened. Happy to help!

[szucsitg]

No, it's fine. I haven't got time to test out a refactored approach for the config files I've got. But as I mentioned last week, this PR wouldn't be the solution for sure

[szucsitg]

I just tested. Actually my original PR would still make sense, because I generate a ssl cert with mkcert locally also put into the data folder, and that's definitely need a relative path to config, unless I put them into nix store, but that's not a solution that I'd like to have

[szucsitg]

please re-open the MR @shivaraj-bh , as this is a valid scenario for nginx config, and by symlinking the generated output, it wouldn't change any behaviour, but this would be possible. (assuming my cert is under ./data/mkcert

...
  server {
    ...
    ssl_certificate  ../mkcert/localhost.pem;
    ssl_certificate_key  ../mkcert/localhost-key.pem;
    ...
}
...

[srid]

@szucsitg How about if we decouple the cert dir using env variabe, like CERT_DIR (pointing to an absolute path), then you can reference in the nginx.conf during generation / startup.

We can also use envsubst: https://serverfault.com/questions/577370/how-can-i-use-environment-variables-in-nginx-conf

Let's explore the different solutions to a specific problem before picking the best one (for the general problems) to go with.

[szucsitg]

I cannot refer to full path in pure mode, unless I know what it'll be beforehand, so putting it into /nix/store. and private keys, even if they're self-signed, shouldn't be there. also the shell script you referred would modify the nginx.conf that's under nix store path.

[srid]

What if we always copied the config file from the nix store to the dataDir (along with doing envsubst as necessary) as part of the process startup script?

[srid]

I'm marking the PR as draft, until we come to a commonly agreed upon spec.

EDIT: Actually GitHub prevents me from doing that due to force-push

[szucsitg]

I can re-open the PR as a new one. Also I can change the ln to cp, but I didn't want to diverge from the generated config, nor manually modify it.

Additionally this the feature (relative paths) are already provided for the most of nginx (aka served files) by using by using -p flag, but it's not available for anything referred from the nginx config, because it's referred from nix store

[srid]

I have to admit I'm a bit confused here.

We already pass -p, set to $PWD:

https://github.com/juspay/services-flake/blob/8d64a960367e07f7d21fa890f77f56daabc5d8bd/nix/nginx.nix#L103

Does this not enable your usecase? Oh, I think I see. You have files relative to the dataDir. In that case, shouldn't this be -p ${config.dataDir} instead?

[srid]

Basically, it looks like we can just do -p ${config.dataDir} in addition to (always) symlinking (but not necessarily copying) the config file under dataDir.

[szucsitg]

I'll retest it, and create a small PoC as well. but when I was dealing with this, but I'm almost sure it didn't work setting relative to the prefix, as it was my expectation as well. However setting -p to dataDir would actually make it worse, as you cannot do path traversall anywhere outside of the prefix folder. I tried and didn't work.

[srid]

I'll retest it, and create a small PoC as well.

That'd be really helpful.

[szucsitg]

it's looking for the config at /nix/store/importedconfig.conf, so obviously it doesn't work. https://github.com/szucsitg/services-flake-nginx-error

[shivaraj-bh]

I see it now. Looks like we need to reopen the PR with your initial solution.

[shivaraj-bh]

It would be nice to also move the nginx.nix and nginx_test.nix to its own folder, like we have for mysql and include importedconfig.conf in it, which will be tested in nginx_test.nix.

[szucsitg]

I implemented all requested changes in the new PR