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.

Sign up for GitHub

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/nghttpx: add module for the nghttpx proxy server #31680

Merged
merged 2 commits into from Nov 16, 2017
Merged

nixos/nghttpx: add module for the nghttpx proxy server #31680

merged 2 commits into from Nov 16, 2017

Conversation

ixmatus
Copy link
Contributor

@ixmatus ixmatus commented Nov 15, 2017

Motivation for this change

The nghttpx proxy was recently included in nixpkgs with 825c08e in the bin output of the nghttp2 package. This pull request is a follow-on to that change providing a NixOS module for the nghttpx proxy. The nghttpx is useful because it handles reverse proxying both http/1.1 and http2 requests.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

This change also adds a global uid and gid for a nghttpx user and group as well as an integration test.

Mic92
Mic92 reviewed Nov 15, 2017
View reviewed changes
nixos/modules/misc/ids.nix Outdated
@@ -300,6 +300,7 @@
kanboard = 281;
pykms = 282;
kodi = 283;
nghttpx = 284;
Copy link
Member

Choose a reason for hiding this comment

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

It does not look like this service needs to access persistent files. If so please remove static uid/gid numbers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Okay, I didn't realize that was one of the purposes for our static uid and gid's. I'll fixup.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mic92 actually, two persistent files that it would access are the TLS certificate and key. So I think we might want to keep the static uid/gid numbers...

Copy link
Member

Choose a reason for hiding this comment

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

For a small number of files it is better to chown at startup. Later we might use DynamicUser for this service.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mic92 sorry, not sure I understand your feedback.

Copy link
Member

@Mic92 Mic92 Nov 15, 2017 •
edited

Choose a reason for hiding this comment

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

The ssl context is created before privileges are dropped. So uid/gid numbers are not relevant for loading certificates: https://github.com/nghttp2/nghttp2/blob/master/src/shrpx_worker_process.cc#L538

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I understand what you're saying now (also, TIL, thanks for pointing this out). I'll fix.

Mic92
Mic92 reviewed Nov 15, 2017
View reviewed changes
nixos/modules/services/networking/nghttpx/nghttpx-options.nix Outdated
'';
};

user = lib.mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Is there an important use case for making user/group configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was following an example, can't remember which, I think it was the nginx module. I personally don't think it's that important so if you're questioning its importance I'm happy to simplify and remove it.

@ixmatus ixmatus force-pushed the parnell/nghttpx-module branch 2 times, most recently from 53bd0ea to 9315c4f Compare November 15, 2017 16:21
@ixmatus
nghttpx: Add a new NixOS module for the nghttpx proxy server
adc2cd6 
This change also adds a global `uid` and `gid` for a `nghttpx` user
and group as well as an integration test.
@ixmatus
Copy link
Contributor Author

ixmatus commented Nov 15, 2017

Okay, @Mic92 I believe I've addressed your feedback. Thank you :)

@Mic92 Mic92 changed the title nghttpx: Add a new NixOS module for the nghttpx proxy server nixos/nghttpx: add module for the nghttpx proxy server Nov 16, 2017
@Mic92 Mic92 merged commit cb11bf7 into NixOS:master Nov 16, 2017
@Mic92
Copy link
Member

Mic92 commented Nov 16, 2017

Thanks!

ixmatus
ixmatus commented Nov 16, 2017
View reviewed changes
nixos/modules/services/networking/nghttpx/nghttpx-options.nix Outdated
@@ -132,7 +132,7 @@
type = lib.types.int;
default = 0;
description = ''
Set maximum number of open files (RLIMIT_NOFILE) to <N>. If 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mic92 how did you check this so I can know to fix this sort of thing in the future?

Copy link
Member

Choose a reason for hiding this comment

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

I noticed it, when adding the service to my configuration.nix because this triggers a manual rebuild.
I can be also build using: https://nixos.wiki/wiki/Contributing_to_Nix_documentation#How_to_build_the_NixOS_manual

@ixmatus ixmatus deleted the parnell/nghttpx-module branch November 16, 2017 18:23
@ixmatus
Copy link
Contributor Author

ixmatus commented Nov 16, 2017

Also, in the future, I'll follow how you wrote the PR title; I wasn't sure how to format it but what you've written makes a lot of sense.

orivej added a commit that referenced this pull request Nov 17, 2017
@orivej
Merge branch 'master' into staging
d99a2fc 
* master: (293 commits)
  go_1_9: skip flaky TestServerCancelsReadTimeoutWhenIdle
  qsyncthingtray: fix build
  qt56.qtwebengine: fix build
  stdman: d860212 -> 2017.04.02
  jackett: use mono50
  hg-git: disable with python3
  hg-git: 0.8.5 -> 0.8.10
  xfce4-settings: enable parallel building
  gcc-snapshot: mark as broken
  heaptrack: 2017-02-14 -> 2017-10-30
  nixos-container: Modify existing test to cover show-ip command
  nixos-container: Make show-ip work together with ipv4 + netmask
  linux-copperhead: 4.13.12.a -> 4.13.13.a
  matterbridge: 1.1.0 -> 1.4.1
  nixos/nghttpx: add module for the nghttpx proxy server (#31680)
  mattermost: 4.3.0 -> 4.4.0
  breakpad: delete
  simp_le: 0.2.0 -> 0.6.1
  certbot: 0.11.1 -> 0.19.0
  afl: 2.51b -> 2.52b
  ...
@Mic92
Copy link
Member

Mic92 commented Nov 17, 2017 •
edited

In general we follow the following format for commits: https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md#submitting-changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mic92 Mic92 Mic92 left review comments

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ixmatus @Mic92 @GrahamcOfBorg