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/upnp tests: NAT-PMP support and improvements #80984

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

nixos/upnp tests: NAT-PMP support and improvements #80984

wants to merge 1 commit into from

Conversation

yesbox
Copy link
Contributor

@yesbox yesbox commented Feb 24, 2020 •
edited

Updates the nixos/upnp test:

  • Verifies the NAT-PMP feature.
  • Asserts that the server is not reachable before port mapping.
  • Tests closing ports via uPnP and NAT-PMP.
  • Cleaning up the test.
  • 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.

bobvanderlinden
bobvanderlinden approved these changes Feb 25, 2020
View reviewed changes
Copy link
Member

@bobvanderlinden bobvanderlinden left a comment

Choose a reason for hiding this comment

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

This certainly looks like an improvement 👍. Only a minor optional suggestion, I'd personally rename server to internalServer and client to externalClient, just to match their *Address variables.

@yesbox
Copy link
Contributor Author

yesbox commented Feb 25, 2020

Thanks for reviewing. Having tried the change I found it more busy and difficult for me to follow the testScript and the output of the test log. This is what it looks like:
https://github.com/yesbox/nixpkgs/blob/tests_upnp.nix2/nixos/tests/upnp.nix

I updated the current branch with more comments to make it more obvious what is internal/inside and external/outside, while keeping the short distinct names as an alternative.

Do you have a preference?

@bobvanderlinden
Copy link
Member

Ah, it already looks good and can follow along just fine with the comments 👍

@yesbox yesbox force-pushed the tests_upnp.nix branch 2 times, most recently from 33385f8 to 10eb8ef Compare March 21, 2020 10:30
@yesbox yesbox changed the title nixos/upnp: NAT-PMP support and more nixos/upnp tests: NAT-PMP support and improvements Sep 12, 2020
@SuperSandro2000
Copy link
Member

@yesbox please resolve the merge conflict.

@yesbox
Copy link
Contributor Author

yesbox commented Mar 10, 2021 •
edited

@SuperSandro2000, merge conflict resolved! 👍

@stale
Copy link

stale bot commented Sep 6, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 6, 2021
@yesbox yesbox requested a review from jgillich September 6, 2021 20:38
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 6, 2021
@yesbox yesbox requested a review from Ekleog September 6, 2021 20:39
@stale
Copy link

stale bot commented Apr 17, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 17, 2022
@yesbox
nixos/upnp tests: NAT-PMP support and improvements
f5053ad 
* Verifies the NAT-PMP feature.
* Asserts that the server is not reachable before port mapping.
* Tests closing ports via uPnP and NAT-PMP.
* Cleaning up the test.
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 19, 2022
@yesbox yesbox requested a review from misuzu July 19, 2022 16:07
@yesbox
Copy link
Contributor Author

yesbox commented Jul 19, 2022 •
edited

I find this test case (both master and this PR) fails since cf9ac2b, with the error message "chain MINIUPNPD not found". Flipping nftablesCompat in pkgs/os-specific/linux/iptables/default.nix back to false resolves the issue.

@misuzu
Copy link
Contributor

misuzu commented Jul 19, 2022

I find this test case (both master and this PR) fails since cf9ac2b544aca2b5af5cf1650aafefc88605c95f , with the error message "chain MINIUPNPD not found". Flipping nftablesCompat in pkgs/os-specific/linux/iptables/default.nix back to false resolves the issue.

Have you tried updating miniupnpd? It looks like miniupnpd has multiple backends in new versions:
https://packages.debian.org/source/buster/miniupnpd
https://packages.debian.org/source/bullseye/miniupnpd

@yesbox
Copy link
Contributor Author

yesbox commented Jul 19, 2022

Have you tried updating miniupnpd? It looks like miniupnpd has multiple backends in new versions: https://packages.debian.org/source/buster/miniupnpd https://packages.debian.org/source/bullseye/miniupnpd

Tried now, like so 35e81d9, but the test fails (and passes if flipping nftablesCompat) in the same manner.

@misuzu
Copy link
Contributor

misuzu commented Jul 19, 2022 •
edited

Have you tried updating miniupnpd? It looks like miniupnpd has multiple backends in new versions: https://packages.debian.org/source/buster/miniupnpd https://packages.debian.org/source/bullseye/miniupnpd

Tried now, like so 35e81d9, but the test fails (and passes if flipping nftablesCompat) in the same manner.

I doubt it is that simple. At least miniupnpd needs to be compiled with --firewall=nftables. miniupnpd module probably needs an update too.

@justinas justinas mentioned this pull request Mar 18, 2023
Merged
12 tasks
@justinas
Copy link
Contributor

FYI, I've opened a PR with an equivalent of the nftablesCompat hack. #221831

It is not ideal, but making iptables-nft <-> miniupnpd nftables backend compatible looks non-trivial as well.

This PR is a great improvement to the tests, so I hope this can be merged after my PR!

@wegank wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
@wegank wegank marked this pull request as draft March 25, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bobvanderlinden bobvanderlinden bobvanderlinden approved these changes

@aanderse aanderse Awaiting requested review from aanderse

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@jgillich jgillich Awaiting requested review from jgillich

@Ekleog Ekleog Awaiting requested review from Ekleog

@misuzu misuzu Awaiting requested review from misuzu

Assignees
No one assigned
Labels
2.status: merge conflict 6.topic: nixos 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
6 participants
@yesbox @bobvanderlinden @SuperSandro2000 @misuzu @justinas @wegank