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

nixos/nginx: make sslCertificate and sslCertificateKey nullable #40932

Closed
wants to merge 1 commit into from

Conversation

lukateras
Copy link
Member

This doesn't change any behavior, but extraordinarily simplifies virtualHost override via options.services.nginx.virtualHosts.apply.

@xeji
Copy link
Contributor

xeji commented May 22, 2018

One drawback is a confusing error message when the user enables SSL but doesn't set the certificate paths. Most users wouldn't understand what's wrong here:

error: cannot coerce null to a string, at /home/uli/nixpkgs/nixos/modules/services/web-servers/nginx/default.nix:217:35

Before this PR, the message was much clearer:

error: The option `services.nginx.virtualHosts.0.my.test.sslCertificate' is used but not defined.

@xeji
Copy link
Contributor

xeji commented May 22, 2018

Maybe the error message can be improved by adding an assertion.

@lukateras
Copy link
Member Author

lukateras commented May 22, 2018

That comes at a price of not being able to refer to these options in apply function. It would show the same error message, but even if I do builtins.tryEval (lib.hasAttr "sslCertificate" vhost) it shows the error anyway.

@lukateras
Copy link
Member Author

lukateras commented May 22, 2018

To be clear, here's what I'm doing:

{ config, lib, ... }:

with lib;

let
  inherit (config.networking) hostName;

  overrideVirtualHost = name: vhost: vhost // ({
    forceSSL = true;
  }) // (optionalAttrs (vhost.sslCertificate == null) {
    enableACME = true;
  }) // (optionalAttrs (vhost.serverName == null) {
    serverName = "${name}.${hostName}";
  });
in

{
  options.services.nginx.virtualHosts = mkOption {
    apply = vhosts: mapAttrs overrideVirtualHost vhosts;
  };
}

@dotlambda
Copy link
Member

Can't you use !(vhost ? sslCertificate) instead of vhost.sslCertificate == null?

@lukateras
Copy link
Member Author

@dotlambda I don't think that would work:

nix-repl> (a ? "hi")
error: undefined variable 'a' at (string):1:2

@dotlambda
Copy link
Member

Why not?

nix-repl> vhost = { }

nix-repl> !(vhost ? sslCertificate)
true

nix-repl> vhost = { sslCertificate = ""; }

nix-repl> !(vhost ? sslCertificate) 
false

@lukateras
Copy link
Member Author

Huh, that worked. Thank you a lot!

@lukateras lukateras closed this Aug 9, 2018
@lukateras
Copy link
Member Author

Hm, actually it did not:

error: The option `services.nginx.virtualHosts.aaaaaDefault.sslCertificate' is used but not defined.
(use '--show-trace' to show detailed location information)

I just changed (optionalAttrs (vhost.sslCertificate == null)) above to (optionalAttrs !(vhost ? sslCertificate)). And tryEval also doesn't work, this seems to evaluate weirdly.

@lukateras lukateras reopened this Aug 9, 2018
lukateras added a commit to serokell/nixpkgs that referenced this pull request Aug 9, 2018
lukateras added a commit to serokell/nixpkgs that referenced this pull request Aug 9, 2018
lukateras added a commit to serokell/nixpkgs that referenced this pull request Sep 11, 2018
yorickvP pushed a commit to serokell/nixpkgs that referenced this pull request Jan 16, 2019
@luzpaz
Copy link
Contributor

luzpaz commented Feb 5, 2019

Any progress on this ?

kirelagin pushed a commit to serokell/nixpkgs that referenced this pull request Apr 12, 2019
kirelagin pushed a commit to serokell/nixpkgs that referenced this pull request Apr 23, 2019
yorickvP pushed a commit to serokell/nixpkgs that referenced this pull request Apr 23, 2019
@mmahut
Copy link
Member

mmahut commented Aug 19, 2019

Are there any updates on this pull request, please?

kirelagin pushed a commit to kirelagin/nixpkgs that referenced this pull request Oct 26, 2019
kirelagin pushed a commit to serokell/nixpkgs that referenced this pull request Jan 23, 2020
yorickvP pushed a commit to serokell/nixpkgs that referenced this pull request Jan 23, 2020
@mmahut
Copy link
Member

mmahut commented Jan 24, 2020

Closing due to inactivity.

@mmahut mmahut closed this Jan 24, 2020
kirelagin pushed a commit to serokell/nixpkgs that referenced this pull request Jan 28, 2020
yorickvP pushed a commit to serokell/nixpkgs that referenced this pull request Feb 12, 2020
yorickvP pushed a commit to serokell/nixpkgs that referenced this pull request Feb 19, 2020
zhenyavinogradov pushed a commit to serokell/nixpkgs that referenced this pull request May 20, 2020
@lheckemann lheckemann deleted the yegortimoshenko-patch-3 branch July 13, 2020 12:19
zhenyavinogradov pushed a commit to serokell/nixpkgs that referenced this pull request Sep 2, 2020
zhenyavinogradov pushed a commit to serokell/nixpkgs that referenced this pull request Apr 8, 2021
zhenyavinogradov pushed a commit to serokell/nixpkgs that referenced this pull request Sep 7, 2021
cab404 pushed a commit to serokell/nixpkgs that referenced this pull request Mar 13, 2022
cab404 pushed a commit to serokell/nixpkgs that referenced this pull request Mar 13, 2022
cab404 pushed a commit to serokell/nixpkgs that referenced this pull request Jun 13, 2022
rvem pushed a commit to serokell/nixpkgs that referenced this pull request Oct 21, 2022
PhilTaken pushed a commit to serokell/nixpkgs that referenced this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants