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

luci-app-upnp: Finalise changes for OpenWrt 24.10 release #7407

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Self-Hosting-Group
Copy link

@Self-Hosting-Group Self-Hosting-Group commented Nov 18, 2024

  • Complete plugin wording/UI revision
    Add missing protocol term in status/overview UI and shorten menu option to UPnP IGD & PCP, otherwise two lines, rearrange table columns, improve wording and add help texts and note upstream change miniupnp/miniupnp@9339f0e
  • Simplify UI by moving rarely used options to advanced settings, rest to service setup and rearrangement
  • Hide dependent UI fields if UPnP IGD protocol is disabled
  • Remove clean_ruleset_interval/threshold UI options as not working
  • Require client address and improve defaults in ACL entry UI
  • Manual adaption of changed non-translatable protocol acronyms in translations, see Integrity of translation source texts and changes. #7175

This PR completes committed changes #6863 (merged in #6975) and continues #7217 for the UPnP IGD & PCP/NAT-PMP autonomous port mapping service.

Service setup UI:

openwrt-settings
Screenshot 23.05 release

Advanced settings UI:

openwrt-advanced-settings
Screenshot 23.05 release

ACL entry UI:

openwrt-acl-entry

Status/overview UI:

openwrt-status-overview

The Port Control Protocol (PCP) is the successor to NAT-PMP, has similar protocol concepts and packet formats, but adds support for IPv6 port maps and options/extensions. For more information, see:
Port Mapping Protocols Overview and Comparison 2024 - About UPnP IGD & PCP/NAT-PMP
https://github.com/Self-Hosting-Group/wiki/wiki/Port-Mapping-Protocols-Overview

Maintainer: @jow-

@systemcrash
Copy link
Contributor

rebase, squash please

@Self-Hosting-Group
Copy link
Author

Simplify the UI by removing the unimportant option Report system instead on daemon uptime?

@systemcrash
Copy link
Contributor

keep it around

Self-Hosting-Group pushed a commit to Self-Hosting-Group/luci that referenced this pull request Nov 22, 2024
@Self-Hosting-Group
Copy link
Author

Last but not least.

  1. I had renamed general settings to general setup, but now to service setup, with the corresponding advanced settings. Good choice?
  2. The UPnP IGD & PCP/NAT-PMP protocols allow clients on the local network to configure port maps/forwards on the router autonomously. Or: The UPnP IGD & PCP/NAT-PMP protocols allow clients on the local network to configure autonomously port maps/forwards on the router. Autonomous is the best word, better than e.g. independent?
  3. Would a help text in the UI with e.g. two example STUN servers be useful or in the wiki?
  4. There is currently a bug in the 24.10 package that does not correctly remove the nftables rules for the port maps. I think this should be fixed in the release. I could possibly include this miniupnp/miniupnp@d07b0a1 in the package update. What do you think? Can the daemon be built directly from the latest upstream commit or does the fix need to be backported as there is unfortunately no release yet that fixes the bug. This affects the daemon starting with 2.3.6, see miniupnpd: Port map listing broken since version 2.3.6 miniupnp/miniupnp#768
  5. In another project (maybe later in LuCI) I would like to propose the UI option Allow third-party (port?) mapping or Allow third-party maps, whichever is better, with the help text Allow (adding?) port maps for non-requesting IP addresses. This will replace the current secure mode there. Is this good wording, and perhaps as a variable allow_third_party_mapping?

Any comments on this would be very welcome.

Self-Hosting-Group pushed a commit to Self-Hosting-Group/luci that referenced this pull request Nov 22, 2024
Self-Hosting-Group pushed a commit to Self-Hosting-Group/luci that referenced this pull request Nov 22, 2024
Self-Hosting-Group pushed a commit to Self-Hosting-Group/luci that referenced this pull request Nov 22, 2024
@Self-Hosting-Group Self-Hosting-Group marked this pull request as ready for review November 22, 2024 16:32
@systemcrash
Copy link
Contributor

Last but not least.

1. I had renamed general settings to general setup, but now to service setup, with the corresponding advanced settings. Good choice?

In so doing we orphan a bunch of already good i18n, with no obvious improvement. Just seems synonymous.

2. The UPnP IGD & PCP/NAT-PMP protocols allow clients on the local network to configure port maps/forwards on the router _autonomously_. Or: The UPnP IGD & PCP/NAT-PMP protocols allow clients on the local network to configure _autonomously_ port maps/forwards on the router. Autonomous is the best word, better than e.g. independent?

They're more or less synonyms. Either.

3. Would a help text in the UI with e.g. two example STUN servers be useful or in the wiki?

??

Self-Hosting-Group pushed a commit to Self-Hosting-Group/luci that referenced this pull request Nov 22, 2024
Self-Hosting-Group pushed a commit to Self-Hosting-Group/luci that referenced this pull request Nov 23, 2024
Self-Hosting-Group pushed a commit to Self-Hosting-Group/luci that referenced this pull request Nov 23, 2024
@systemcrash
Copy link
Contributor

So, you've tested these changes in situ, correct? What happens when you edit an ACL, delete one of the port fields, and press save?

Comment on lines 124 to 125
s.taboption('setup', form.Flag, 'enable_upnp', _('Enable UPnP IGD protocol')).default = '1'
s.taboption('setup', form.Flag, 'enable_natpmp', _('Enable PCP/NAT-PMP protocols')).default = '1'
Copy link
Contributor

Choose a reason for hiding this comment

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

terminate every expression with ; (same for all remaining lines in this file)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants