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/stunnel: Make free-form #152065

Merged
merged 3 commits into from Jul 26, 2022
Merged

nixos/stunnel: Make free-form #152065

merged 3 commits into from Jul 26, 2022

Conversation

chkno
Copy link
Member

@chkno chkno commented Dec 25, 2021

Motivation for this change

Allow the use of stunnel functionality beyond the very small (5%) subset explicitly supported by the NixOS configuration schema.
Also, add some tests of core stunnel functionality.

This supersedes PR #96401, which proposed just adding a few more config directives rather than a general escape hatch.

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:
    • NixOS test(s) (look inside nixos/tests)
    • and/or package tests
    • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
    • made sure NixOS tests are linked to the relevant packages
      • I did this, but I noticed that nix-build . -A stunnel.tests doesn't run the tests as the linked documentation suggests it should. nix-build . -A stunnel.tests.stunnel does run the tests.
  • 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/)
  • 22.05 Release Notes (or backporting 21.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.

FYI: stunnel maintainer @thoughtpolice, stunnel NixOS module author @lschuermann.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/733

@chkno chkno requested review from siraben, lschuermann, dasJ, thoughtpolice, roberth, astro and martinetd and removed request for siraben February 19, 2022 05:34
Copy link
Member

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

extraConfig never hurts... And adding tests is great, thanks!

nixos/tests/stunnel.nix Outdated Show resolved Hide resolved
Copy link
Member

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

cheers!
I ran the new tests and confirmed it all works.

Copy link
Member

@lschuermann lschuermann left a comment

Choose a reason for hiding this comment

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

LGTM.

@mohe2015
Copy link
Contributor

It's not possible to do this via a freeform module? Because I think that's preferred

nixos/tests/stunnel.nix Outdated Show resolved Hide resolved
nixos/tests/stunnel.nix Outdated Show resolved Hide resolved
@chkno
Copy link
Member Author

chkno commented Feb 22, 2022

@mohe2015 - freeform module - Ah! Yes! That is definitely worth trying. Thanks! I'm flipping this PR to draft now & will be right back with that.

@chkno chkno marked this pull request as draft February 22, 2022 19:23
@chkno chkno force-pushed the stunnel-extraConfig branch 2 times, most recently from 28f8ee6 to 633416c Compare February 23, 2022 01:07
@chkno chkno changed the title nixos/stunnel: extraConfig escape hatches nixos/stunnel: Make free-form Feb 23, 2022
@chkno chkno marked this pull request as ready for review February 23, 2022 01:36
@chkno
Copy link
Member Author

chkno commented Feb 23, 2022

Per @mohe2015 's suggestion, this PR now converts stunnel into a free-form module, rather than merely adding text-based extraConfig escape hatches.

This is now a more invasive change. I believe I've preserved backward compatibility: I added tests for the existing functionality in a separate commit before changing anything.

Please have another look. Thanks!

@roberth roberth removed their request for review February 23, 2022 15:29
Copy link
Member

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

hi! I've just been looking at converting something else to freeform and was pointed out at this: NixOS/rfcs#42

long story short, it looks like there's a generic way of creating freeform modules now instead of your generateConfig -- I'm not sure if that's important though as long as it works? clueless

that aside it looks good at first glance, I'd need to actually try a few old configs to convince myself it's mostly compatible but need to find some more time to do that..

EDIT: in particular for freeform things you could probably use pkgs.formats.ini or toml as is? it looks like a good match

@chkno
Copy link
Member Author

chkno commented Mar 11, 2022

@martinetd - Unfortunately, the stunnel configuration format is kind of a mess. It is much less sophisticated than INI or TOML. For example, it doesn't have any notion of quoting and escaping. Given a section name with a ] in it, the INI generator will escape it: [The \] Section]. Stunnel doesn't parse this way, and generating configuration as if it would be parsed this way would be confusing to users. The INI generator can be configured not to do this, and configured to use "yes" and "no" instead of "true" and "false", and we could remove keys with null values before handing off to the generator, and so on, but by that point it's just as much configuration-code to interface with the generator library as to just generate it directly, and requires future maintainers to read much more configuration-code to understand what's going on.

I added a brief note to the configuration generator with this rationale.

This unlocks stunnel's other ~100 configuration directives, allowing
full stunnel use in NixOS.
@chkno
Copy link
Member Author

chkno commented Mar 11, 2022

@martinetd, @chkno -- Eh, actually it's not so bad. Now updated to use generators.toINI. Thanks!

Copy link
Member

@martinetd martinetd left a comment

Choose a reason for hiding this comment

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

Sorry, didn't mean to make you do so much work - I was happy by a config is messed up the generator code wasn't that bad... But well ok thanks ;)

I've read through it (looks fine) and verified it doesn't break my simple setup. Thanks!

@chkno
Copy link
Member Author

chkno commented Mar 30, 2022

Thanks, martinetd!

Any other reviewers have any feedback?

@chkno
Copy link
Member Author

chkno commented May 20, 2022

I'd like to progress this toward merge. The PRs already reviewed thread says that it is for reviewers to submit things there, not PR authors, so can reviewers either link this PR there or give feedback here about what is needed before this PR can be merged? Thank you!

@Mindavi Mindavi merged commit 9e9f6fc into NixOS:master Jul 26, 2022
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

8 participants