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/sslh: update and refactor for RFC42 #245855

Merged
merged 4 commits into from
Oct 29, 2023
Merged

nixos/sslh: update and refactor for RFC42 #245855

merged 4 commits into from
Oct 29, 2023

Conversation

rnhmjoj
Copy link
Contributor

@rnhmjoj rnhmjoj commented Jul 28, 2023

Description of changes

Update the sslh package and rewrite the module for RFC42

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 via nixosTests.sslh
  • Tested compilation of all packages that depend on this change (sslh)
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 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
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: lib The Nixpkgs function library labels Jul 28, 2023
@rnhmjoj rnhmjoj requested a review from fpletz July 28, 2023 10:50
@rnhmjoj rnhmjoj mentioned this pull request Jul 28, 2023
Merged
7 tasks
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Aug 8, 2023

@symphorien This should superseed #225602 in fixing #225454.

@infinisil
Copy link
Member

There are already existing efforts to have a libconfig format generator: #246115

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Aug 8, 2023

Wow, that seems way overkill. In comparison, mine is a lot simpler purely Nix implementation of the conversion, it does not handle special ints and arrays, but they could be added in the same way, if needed.

@ckiee
Copy link
Member

ckiee commented Aug 14, 2023

There's a Nix-only implementation in #208747 that was iterated on a few times before this comment which I am taking out of a bit of context:

It's just that this adds a whole bunch of code that's slow to evaluate, hard to debug, test and maintain. And yes that's half of nixpkgs already but I don't think it's a good idea to make this worse. Others might review and merge this PR, and I won't interfere with that, but personally I'm gonna abstain from this and focus on cleaner code instead.

After a long period of silence @h7x4 came in, reviving that PR with #246115 which also has output validation and a test alongside being written in a strongly-typed language. It should be a lot more robust.

Sorry about the trouble though— I've also been rebasing my PRs until this thing wraps up.

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Aug 14, 2023

Well, I don't have any strong feelings about this as I'm just interested in fixing sslh.
I'd like to see one of these PRs merged, at least to save other people from reimplementing this over again.

@github-actions github-actions bot removed the 6.topic: lib The Nixpkgs function library label Sep 3, 2023
@rnhmjoj rnhmjoj mentioned this pull request Sep 3, 2023
Merged
12 tasks
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Oct 29, 2023
@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Oct 29, 2023

Rebased (since #246115 has been merged) and added a release note.
Unless there are objections, I'm going to merge this today to get it in 23.11.

h7x4
h7x4 reviewed Oct 29, 2023
View reviewed changes
Copy link
Member

@h7x4 h7x4 left a comment

Choose a reason for hiding this comment

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

A few small comments. Some of the comments regard stuff that has just been moved, so feel free to ignore if you don't think it's relevant for the refactor.

nixos/modules/services/networking/sslh.nix Outdated
};

options.verbose-connections = mkOption {
type = types.enum [ 0 1 2 3 4 ];
Copy link
Member

Choose a reason for hiding this comment

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

This is sort of a nit since both of these options work, but I feel like it's more semantically correct to look at this as a discrete range from least verbose to most verbose. Therefore, I think this would be more fitting

Suggested change
type = types.enum [ 0 1 2 3 4 ];
type = types.ints.between 0 4;

nixos/modules/services/networking/sslh.nix Outdated
freeformType = configFormat.type;

options.timeout = mkOption {
type = types.int;
Copy link
Member

Choose a reason for hiding this comment

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

Assuming timeout of 0 is possible as well.

Suggested change
type = types.int;
type = types.ints.unsigned;

nixos/modules/services/networking/sslh.nix Outdated
};

listenAddresses = mkOption {
type = types.coercedTo types.str singleton (types.listOf types.str);
Copy link
Member

Choose a reason for hiding this comment

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

nit, makes it a bit easier to read.

Suggested change
type = types.coercedTo types.str singleton (types.listOf types.str);
type = with types; coercedTo str singleton (listOf str);

@rnhmjoj
Copy link
Contributor Author

rnhmjoj commented Oct 29, 2023

@h7x4 thank you, I applied you suggestions.

@rnhmjoj rnhmjoj merged commit 800965c into NixOS:master Oct 29, 2023
8 of 10 checks passed
@rnhmjoj rnhmjoj deleted the pr-sslh branch November 29, 2024 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@h7x4 h7x4 h7x4 left review comments

@infinisil infinisil Awaiting requested review from infinisil

@edolstra edolstra Awaiting requested review from edolstra

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

@fpletz fpletz Awaiting requested review from fpletz

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rnhmjoj @infinisil @ckiee @h7x4