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

Iodine: ipv6 support, updates, hardening, nixos test.... #79120

Merged
merged 9 commits into from Mar 16, 2020

Conversation

@symphorien
Copy link
Contributor

@symphorien symphorien commented Feb 2, 2020

Motivation for this change
  • update iodine for ipv6 support
  • update nm-iodine because it is buggy (100% cpu usage after the vpn disconnects)
  • add some hardening flags to the nixos service
  • add a nixos test to check that the hardening didn't break anything

I ran nixpkgs-fmt on the nixos module, so best reviewed commit by commit :)

Things done

Tested on a real setup.

  • 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.
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Feb 3, 2020

@symphorien The commits for 73b8fdb and b2b6645 should be like

$package: 1.2.0 -> YYYY-MM-DD

for the unstable version.

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Feb 3, 2020

@GrahamcOfBorg test iodine

symphorien added 5 commits Jan 28, 2020
Adds ipv6 support
the released version is old and cannot reconnect after the first
connection is closed. This is fixed in master.
@symphorien symphorien force-pushed the symphorien:iodine branch from 0fe1e3c to b7f27cb Feb 4, 2020
@symphorien
Copy link
Contributor Author

@symphorien symphorien commented Feb 4, 2020

Reworded the commits as required.
... And I forgot to have a look at ofborg's output before so we lost the results. Sorry.

@flokli flokli requested a review from mweinelt Feb 4, 2020
example = "8.8.8.8";
};

extraConfig = mkOption {

This comment has been minimized.

@mweinelt

mweinelt Feb 5, 2020
Member

Sounds more like extraArgs to me.

This comment has been minimized.

@symphorien

symphorien Feb 5, 2020
Author Contributor

I'd like not to do this renaming:

  1. it's a breaking change for an arguably low improvement
  2. iodine only means of configuration are arguments file so it's not ambiguous
  3. This appears in the diff because I reformatted the file with nixpkgs-fmt, but this is outside the scope of this PR. I welcome "en passant" improvements, but let's restrain ourselves to trivial changes.
};

passwordFile = mkOption {
type = types.str;

This comment has been minimized.

@mweinelt

mweinelt Feb 5, 2020
Member

Can probably be a path instead of a str, no?

This comment has been minimized.

@worldofpeace

worldofpeace Feb 5, 2020
Member

see details at #78640

This comment has been minimized.

@symphorien

symphorien Feb 5, 2020
Author Contributor

As far as I understand, it should be a non-path string, to the contrary. Please correct me if I'm wrong.

This comment has been minimized.

@flokli

flokli Feb 10, 2020
Contributor

See how it's done here

This comment has been minimized.

@symphorien

symphorien Feb 13, 2020
Author Contributor

I put uses of these options under toString. Was that what you meant? (your link is 404)

symphorien and others added 4 commits Feb 5, 2020
Co-Authored-By: Martin Weinelt <mweinelt@users.noreply.github.com>
Co-Authored-By: Martin Weinelt <mweinelt@users.noreply.github.com>
Co-Authored-By: Martin Weinelt <mweinelt@users.noreply.github.com>
It should prevent copying the files to a store path
@symphorien
Copy link
Contributor Author

@symphorien symphorien commented Feb 19, 2020

Can I request a new round of review now that most comments have been taken into account I think?

@flokli flokli requested a review from mweinelt Mar 2, 2020
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Mar 15, 2020

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

https://discourse.nixos.org/t/prs-ready-for-review-may-2019/3032/127

@Ekleog
Copy link
Member

@Ekleog Ekleog commented Mar 16, 2020

@ofborg test iodine

@Ekleog Ekleog merged commit a0307ba into NixOS:master Mar 16, 2020
17 checks passed
17 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
iodine on aarch64-linux Success
Details
iodine on x86_64-linux Success
Details
tests.iodine on aarch64-linux Success
Details
tests.iodine on x86_64-linux Success
Details
@symphorien symphorien deleted the symphorien:iodine branch Mar 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.