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

Jump to bottom

nixos/haproxy: check conf on build #109250

Closed
wants to merge 1 commit into from
Closed

nixos/haproxy: check conf on build #109250

wants to merge 1 commit into from

Conversation

NeverBehave
Copy link
Member

@NeverBehave NeverBehave commented Jan 13, 2021 •
edited

Motivation for this change

Current services will continue deploying configuration even though config may contain syntax errors. This pull request check config syntax on build which makes it easier to notice errors.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

SuperSandro2000
SuperSandro2000 requested changes Jan 13, 2021
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please change the PR and commit message title to nixos/haproxy: check conf on build or similar.

@NeverBehave NeverBehave changed the title ref: check haproxy conf on build nixos/haproxy: check conf on build Jan 13, 2021
@aanderse
Copy link
Member

ping @symphorien @Ma27

@NeverBehave
Copy link
Member Author

@SuperSandro2000 Anything else needs to be revised? I am not sure about the editorConfig test tho: I intended to leave an extra newline when generating before appending user config.

@aanderse
Copy link
Member

@NeverBehave please review this PR: #62853

@symphorien
Copy link
Member

I'm not familiar with haproxy. Does it check that paths like certificates http://cbonte.github.io/haproxy-dconv/2.4/configuration.html#ca-file%20%28Bind%20options%29 exist during the configuration check ? If this is the case, the test will fail in the sandbox and there should be an option to disable checking (off by default).
Same concern for the chroot option, and probably may other that I don't know.
I had a quick look and it seems there is no "include" statement for the configuration file (which could point to outside of the store).

Disabling the check is also probably necessary if someone ever wants to cross compile a full nixos system with haproxy.

All those concerns are theoretical, they may or may not refer to realistic use cases.

hmenke
hmenke suggested changes Jan 16, 2021
View reviewed changes
Copy link
Member

@hmenke hmenke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the failing test there is trailing whitespace somewhere. Please remove it.

nixos/modules/services/networking/haproxy.nix
@@ -3,12 +3,15 @@
let
cfg = config.services.haproxy;

haproxyCfg = pkgs.writeText "haproxy.conf" ''
haproxyCfg = pkgs.runCommand "haproxy.conf" { CONFIG=cfg.config; } ''
cat > $out <<EOF
Copy link
Member

@hmenke hmenke Jan 16, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be quoted, otherwise parameter expansion and command substitution will be applied in the heredoc, see https://www.gnu.org/software/bash/manual/bash.html#Here-Documents

Suggested change
cat > $out <<EOF
cat > $out <<'EOF'

hmenke
hmenke reviewed Jan 16, 2021
View reviewed changes
nixos/modules/services/networking/haproxy.nix
Comment on lines -10 to +13

${cfg.config}

EOF
echo "$CONFIG" >> $out
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of this change actually?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanna separate the hard coded config with the user config, but yeah I think they could go together in the same doc part.

@NeverBehave
Copy link
Member Author

I'm not familiar with haproxy. Does it check that paths like certificates http://cbonte.github.io/haproxy-dconv/2.4/configuration.html#ca-file%20%28Bind%20options%29 exist during the configuration check ? If this is the case, the test will fail in the sandbox and there should be an option to disable checking (off by default).
Same concern for the chroot option, and probably may other that I don't know.
I had a quick look and it seems there is no "include" statement for the configuration file (which could point to outside of the store).

Disabling the check is also probably necessary if someone ever wants to cross compile a full nixos system with haproxy.

All those concerns are theoretical, they may or may not refer to realistic use cases.

From the manual it said “Only checks config file and exits with code 0 if no error was found, or exits with code 1 if a syntax error was found.” I believe only syntax error will fail the build, where the path won’t be touch during validation. I didn’t thought about that at first so I would give it a shot.

@NeverBehave
Copy link
Member Author 

[nix-shell:~]$ haproxy -c -f haproxy.conf 
[NOTICE] 016/052037 (9649) : haproxy version is 2.3.2-d522db7
[NOTICE] 016/052037 (9649) : path to executable is /nix/store/fw0yfwfpadhdsfsljrfmsckpafrmd8fg-haproxy-2.3.2/bin/haproxy
[ALERT] 016/052037 (9649) : parsing [haproxy.conf:3] : 'bind 10.0.0.3:443' : unable to stat SSL certificate from file '/etc/ssl/certs/mysite.pem' : No such file or directory.
[ALERT] 016/052037 (9649) : Error(s) found in configuration file : haproxy.conf
[ALERT] 016/052037 (9649) : Fatal errors found in configuration.

Well it looks like that the certificate file will be checked. Then I think it is better to keep it away from the sandbox for now. Thanks for pointing this out.

@NeverBehave NeverBehave deleted the patch-1 branch March 16, 2021 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hmenke hmenke hmenke requested changes

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@NeverBehave @aanderse @symphorien @hmenke @SuperSandro2000 @Janik-Haag