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-mod-system: use DirectInterface for bound dropbear interface #7484

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

Conversation

dannil
Copy link
Contributor

@dannil dannil commented Dec 18, 2024

Since openwrt/openwrt@3f96246 the correct UCI option for specifying the bound interface is DirectInterface instead of Interface.

See https://forum.openwrt.org/t/openwrt-24-10-0-rc2-second-release-candidate/217483/154?u=dannil

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <my@email.address> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile N/A
  • Tested on: (x86/64, 24.10.0-rc2, Firefox) ✅
  • ( Preferred ) Mention: @ the original code author for feedback @jow-
  • ( Preferred ) Screenshot or mp4 of changes:
  • ( Optional ) Closes: e.g. openwrt/luci#issue-number
  • ( Optional ) Depends on: e.g. openwrt/packages#pr-number in sister repo
  • Description: (describe the changes proposed in this PR)

Since openwrt/openwrt@3f96246
the correct UCI option for specifying the bound interface is
DirectInterface instead of Interface.

Signed-off-by: Daniel Nilsson <dannil+github@protonmail.com>
@dannil
Copy link
Contributor Author

dannil commented Dec 18, 2024

As reported on the formu which I also noticed is that if you upgrade from 23.05.5 to 24.10.0-rc2 the UI will show the interface as "unspecified", as the option isn't specified. Do we have a way to handle that or do we even need to? It's purely a cosmetic issue as the init script still checks for Interface if specified, and if both DirectInterface and Interface is specified, DirectInterface takes precedence.

@systemcrash
Copy link
Contributor

It looks like we're forced to choose. I've used Interface for years and never had a single problem. I have several IPs on a single interface and each work fine.

@dannil
Copy link
Contributor Author

dannil commented Dec 18, 2024

It looks like we're forced to choose. I've used Interface for years and never had a single problem. I have several IPs on a single interface and each work fine.

As long as Interface is supported for specifying interfaces everything will continue to work as before, it's the wording of "taking precedence" that made me act as it sounds like DirectInterface will replace Interface at some point in the future for binding to interfaces, Interface is meant to bind to IP addresses and is the intended usage going forward, at least after the referenced commit.

@systemcrash
Copy link
Contributor

One thing I haven't yet understood - since the code comments in the init script are contradictory - what Direct is supposed to fix? I have only ever specified interfaces for Interface, and never had a problem. So the error message is misleading: Wed Dec 18 12:41:29 2024 daemon.info dropbear: Option 'Interface' binds to address(es) but not to interface

Based on the forum report - of course it will show unknown. Since the luci app is looking for 'Interface' which is not set, but only DirectInterface is set. Perhaps we should address this via .write and .load functions, which save to DirectInterface and/or Interface, and check and load from them in that order.

@systemcrash
Copy link
Contributor

Interface is meant to bind to IP addresses and is the intended usage going forward

Ah, so they changed the behaviour of the existing option?

@dannil
Copy link
Contributor Author

dannil commented Dec 18, 2024

Interface is meant to bind to IP addresses and is the intended usage going forward

Ah, so they changed the behaviour of the existing option?

That is at least my understanding, yes. You can see that they added a warning for specifying interfaces as an option for Interface at openwrt/openwrt@3f96246#diff-af469bfcaffdaa7e73a3e127c5a312143b09002c82d226f9fae9cc042227f189R210

@systemcrash
Copy link
Contributor

The code comments do not give me much confidence:

	# 'DirectInterface' should specify single interface
	# but end users may misinterpret this setting
	DirectInterface=$(normalize_list "${DirectInterface}")
	# 'Interface' should specify single interface
	# but end users are often misinterpret this setting
	Interface=$(normalize_list "${Interface}")

@systemcrash
Copy link
Contributor

systemcrash commented Dec 18, 2024

What we have today in luci works, so we can provide a parallel option. But only when it's clear what should do what. The original issue seems to discuss the problem when someone does the following, which is what the init script perhaps choked on

list Interface 'lan'
list Interface 'foo'
...

@systemcrash
Copy link
Contributor

Luci paradigm is currently correct. It allows one to create multiple instances, each instance with exactly one Interface.

See @stangri comment here openwrt/packages#25555

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