Skip to content

nixos/nginx: fix config validation with hostnames in proxy_pass#207413

Closed
symphorien wants to merge 7 commits intoNixOS:masterfrom
symphorien:fix_nginx_dns
Closed

nixos/nginx: fix config validation with hostnames in proxy_pass#207413
symphorien wants to merge 7 commits intoNixOS:masterfrom
symphorien:fix_nginx_dns

Conversation

@symphorien
Copy link
Member

Description of changes

There are actually many directives which trigger domain name resolution, so using regexes to "fix" them all does not seem like a good approach. Instead I run a fake dns server that answers all requests with a dummy IP.
Unfortunately libredirect tricks to point nginx to a fake /etc/resolv.conf don't work, because nginx uses getaddrinfo, so it is glibc which reads /etc/resolv.conf, and one cannot fake glibc calls to glibc it seems.
So I use bubblewrap instead, but that requires user namespaces, which are not always available. If this is not the case, and validation fails on dns failure, then we ignore the failure. The goal is that if it builds somewhere, it builds everywhere (use case: heterogenous remote builders). I have not yet tested building without user namespaces.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

cc @delroth

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 23, 2022
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 23, 2022
@symphorien
Copy link
Member Author

now works with user namespaces disabled

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.

There is not an easier way to run config validation?
We are getting into territoriality where the overhead and workarounds are eventually outweighing the benefits.

Comment on lines 16 to 21
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
];
meta = {
];
pythonImportsCheck = [ "fakedns" ];
meta = {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pythonImportsCheck = [ "dns_messages" ];

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
doCheck = false;
# has no tests
doCheck = false;

Comment on lines 1 to 3
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{ buildPythonPackage, fetchPypi, lib }:
buildPythonPackage rec {
{ buildPythonPackage, fetchPypi, lib }:
buildPythonPackage rec {

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# has no tests
doCheck = false;
pythonImportsCheck = [ "cli_formatter" ];

Comment on lines 444 to 453
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
userns=yes;
$bwrun ${nginxWithResolver} > out 2>&1;
else
userns=no;
echo "user namespaces are not available, dns resolution will fail if required";
nginx -t -c $(readlink -f ./conf) > out 2>&1 || true;
userns=yes
$bwrun ${nginxWithResolver} > out 2>&1
else
userns=no
echo "user namespaces are not available, dns resolution will fail if required"
nginx -t -c $(readlink -f ./conf) > out 2>&1 || true

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
validatedConfigFile = pkgs.runCommand "validated-nginx.conf" { nativeBuildInputs = [ cfg.package pkgs.bubblewrap pkgs.fakedns pkgs.getent ]; } ''
validatedConfigFile = pkgs.runCommand "validated-nginx.conf" {
nativeBuildInputs = with pkgs; [ cfg.package bubblewrap fakedns getent ];
} ''

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for i in $(seq 1 1000); do
for i in $(seq 1 30); do

waiting 1000 seconds for that to come up is to long

Comment on lines 410 to 411
Copy link
Member

Choose a reason for hiding this comment

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

We should use private addresses or if there are some to be not resolveable them.

@symphorien
Copy link
Member Author

I pushed changes which

We are getting into territoriality where the overhead and workarounds are eventually outweighing the benefits.

Yes. I started #205561 because nixcloud-webservice has been doing such a validation since quite a long time, but it turns out it is pretty limited for more diverse configs. I can make it work with what is arguably an ugly pile of hacks. If you think this is too hacky, we can revert introducing the validation. I am also hesitating, other opinions welcome.

@SuperSandro2000
Copy link
Member

I can make it work with what is arguably an ugly pile of hacks. If you think this is too hacky, we can revert introducing the validation. I am also hesitating, other opinions welcome.

To be honest the cleanest solution would probably be to change the nginx code to better fit our usecase and try to upstream that.

@symphorien
Copy link
Member Author

TBH, that's not something I'm willing to attempt.

@symphorien
Copy link
Member Author

superseded by #209075

@symphorien symphorien closed this Feb 11, 2023
@symphorien symphorien deleted the fix_nginx_dns branch February 11, 2023 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python Python is a high-level, general-purpose programming language. 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants