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

miniupnpd: add force_forwarding option support #16011

Closed
wants to merge 1 commit into from

Conversation

ptpt52
Copy link
Contributor

@ptpt52 ptpt52 commented Jul 2, 2021

This option replace the 'ext_ip_reserved_ignore', and replace the
patch with new one

as addr_is_reserved() behavior should not be changed, so drop the
ext_ip_reserved_ignore

use a force_forwarding option to make the port forwarding force to
work when the router is behind NAT

Maintainer: me / @<github-user> (find it by checking history of the package Makefile)
Compile tested: (put here arch, model, OpenWrt version)
Run tested: (put here arch, model, OpenWrt version, tests done)

Description:

This option replace the 'ext_ip_reserved_ignore', and replace the
patch with new one

as addr_is_reserved() behavior should not be changed, so drop the
ext_ip_reserved_ignore

use a force_forwarding option to make the port forwarding force to
work when the router is behind NAT

Signed-off-by: Chen Minqiang <ptpt52@gmail.com>
@ptpt52
Copy link
Contributor Author

ptpt52 commented Jul 2, 2021

need some tested

@ptpt52
Copy link
Contributor Author

ptpt52 commented Jul 2, 2021

#15258
may related

Copy link
Member

@BKPepe BKPepe left a comment

Choose a reason for hiding this comment

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

Even though I appreciate your work, I am not in favor to merge this because what happened here: #15258

We can not carry patches, which are refused to be part of upstream and are causing several issues, and then they are reported upstream, which they need to solve and refuse it as they are right about it.

If you want to have this merged, send this upstream, cooperate with them to be sure that it is accepted, and once it is merged, it can be backported here or you can wait until it's part of a new release.

For now, I am closing this to prevent the situation, which happened in the past. You should really remove the patch which you created. Because this is not an acceptable state. Users are confused about what to do with the error message, which is cryptic and does not say many details what to do.

@BKPepe BKPepe closed this Jul 2, 2021
@Neustradamus
Copy link

@openips: What do you think about this PR from @ptpt52?
It replaces "301-ext_ip_reserved_ignore.patch"...

@openips
Copy link

openips commented Sep 15, 2021

@Neustradamus I test this pr in openwrt package and find not work, i can report bug here,thks a lot

@openips
Copy link

openips commented Sep 15, 2021

@ptpt52 大神 最新的补丁有问题 force_forwarding失效 具体描述请看 16480 有时间修复一下 需要提供测试请联系我 谢谢

@ptpt52
Copy link
Contributor Author

ptpt52 commented Sep 16, 2021

@openips try to enable stun

upnpd.config.use_stun='1'
upnpd.config.stun_host='iphone-stun.strato-iphone.de'
upnpd.config.stun_port='3478'

@ptpt52
Copy link
Contributor Author

ptpt52 commented Sep 16, 2021

specify the wan interface:

upnpd.config.external_iface='wan or what'

@openips
Copy link

openips commented Sep 16, 2021

@ptpt52 我还是习惯您之前的301补丁的工作方式 非常省心 能否修改一下

@ptpt52
Copy link
Contributor Author

ptpt52 commented Sep 16, 2021

try this one: x-wrt/packages@62b923a
@openips

@openips
Copy link

openips commented Sep 16, 2021

好的 我编译一下试试看 结果稍后反馈

@openips
Copy link

openips commented Sep 16, 2021

image
image
@ptpt52 @Neustradamus x-wrt/packages@62b923a worked very well ,thks a lot

@Neustradamus
Copy link

@openips: Nice!

Thanks @ptpt52 :)
Can you create a new PR?

@@ -2,6 +2,7 @@ config upnpd config
option enabled 0
option enable_natpmp 1
option enable_upnp 1
option force_forwarding 1
Copy link
Member

Choose a reason for hiding this comment

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

Please don't make this option default. Previously when you made ext_ip_reserved_ignore default, it resulted in cryptic syslog messages for miniupnpd users outside of China.

@champtar
Copy link
Member

Please do a PR for master only first, then we will backport once reviewed ;)

@Neustradamus
Copy link

Neustradamus commented Sep 16, 2021

@ptpt52: Important: Please do new PRs for 18.06.x/19.07.x/21.02.x/master branches.

Thanks a lot in advance.

PS: @neheb: Too late to have same miniupnp version in 18.06.x/19.07.x/21.07.x/master?

@skill7899
Copy link

it not work at 23.03 fw4,focre_forwarding or ext_ip_reserved_ignore Illegal parameter when i add in /var/etc/miniupnp.conf。
invalid option in file /var/etc/miniupnpd.conf line 11 : ext_ip_reserved_ignore=no
invalid option in file /var/etc/miniupnpd.conf line 11 : focre_forwarding=yes
@ptpt52

@ptpt52
Copy link
Contributor Author

ptpt52 commented Sep 18, 2022

it not work at 23.03 fw4,focre_forwarding or ext_ip_reserved_ignore Illegal parameter when i add in /var/etc/miniupnp.conf。 invalid option in file /var/etc/miniupnpd.conf line 11 : ext_ip_reserved_ignore=no invalid option in file /var/etc/miniupnpd.conf line 11 : focre_forwarding=yes @ptpt52

these two config options never supported by upstream miniupnpd

@skill7899
Copy link

it not work at 23.03 fw4,focre_forwarding or ext_ip_reserved_ignore Illegal parameter when i add in /var/etc/miniupnp.conf。 invalid option in file /var/etc/miniupnpd.conf line 11 : ext_ip_reserved_ignore=no invalid option in file /var/etc/miniupnpd.conf line 11 : focre_forwarding=yes @ptpt52

these two config options never supported by upstream miniupnpd

Does it mean that I need to merge patch and compile myself?

@skill7899
Copy link

@ptpt52
compile fail with ntfatbles and firewall4
Patch failed! Please fix ./patches/301-options-force_forwarding-support.patch!
make[3]: Leaving directory '/data/cpl/packages/net/miniupnpd'
time: package/feeds/packages/miniupnpd/nftables/compile#0.18#0.08#0.23
ERROR: package/feeds/packages/miniupnpd failed to build (build variant: nftables).

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.

7 participants