luci-app-pbr-1.2.3: Two independent fixes to resolver_set and uplink_interface(6) handling - #34
Closed
egc112 wants to merge 191 commits into
Closed
luci-app-pbr-1.2.3: Two independent fixes to resolver_set and uplink_interface(6) handling#34egc112 wants to merge 191 commits into
resolver_set and uplink_interface(6) handling#34egc112 wants to merge 191 commits into
Conversation
This is the accompanying PR for: mossdef-org/pbr#29 This updates the help text, the read.me should probably also needs updating. As this PR changes previous behavior I am not sure if this is the optimal solution, so feel free to change/improve or discard this pull request. It only updates the template, the translations should also need updating not sure if that is done automatically? Signed-off-by: Erik Conijn <egc112@msn.com>
luci-app-pbr: Ignore WireGuard Server if on ignored list
Change `listenport` to `listen_port` Signed-off-by: Erik Conijn <egc112@msn.com>
luci-app-pbr: edit help text
Signed-off-by: Arthur K. <me@wzray.com>
Don't resolve names in `is_running_iptables` check
The SDK image tracked the rolling <ver>-SNAPSHOT branch, whose mirror tarballs rotate and occasionally arrive truncated -> sha256 mismatch and a red build. Map the release-line label (matrix.version) to a fixed stable point release for the SDK image only; artifact filenames and the sync_repo.sh label are unchanged.
Add a release-job step that deletes assets on the tag which the current build did not produce, so a dropped arch/release-line stops lingering on the release (and stops being re-published by sync_repo.sh, which pulls from releases). Gated on a fully green build and a non-empty keep set so a transient failure can't wipe good binaries.
The rename step used bare ${PKG_RELEASE}, so the release asset was
pkg-<ver>-5.apk while the SDK/local build produces pkg-<ver>-r5.apk. Compute a
per-ext release token (apk -> r${PKG_RELEASE}, opkg unchanged) so 25.12 apk
filenames keep the 'r', matching the local build and dev.mossdef.org.
Add a release-job step that fires repository_dispatch to repo.mossdef.org so a new release publishes immediately instead of waiting for the hourly poll. Non-fatal when SYNC_DISPATCH_TOKEN is absent.
Node 20 is deprecated on GitHub-hosted runners. Bump the affected actions to their current Node 24 majors (upload-artifact v7, download-artifact v8, setup-node v5). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Always render the IPv4 gateway (falling back to "0.0.0.0" when absent) and, when IPv6 is enabled, always render the IPv6 gateway segment (falling back to "::0"), instead of omitting the segment entirely when no gateway is present. Mirrors the backend's disp_gw_suffix() behavior in pbr.uc so the status page and the init-script/log output stay consistent. Signed-off-by: Erik Conijn <egc112@msn.com>
luci-app-pbr-1.2.3: status: always display gateway placeholders
…ossdef-org#28) Companion change to the pbr-side commit that downgrades unknown gateway detection from an error to a warning (warningInterfaceRoutingUnknownGateway4 / warningInterfaceRoutingUnknownGateway6, replacing errorInterfaceRoutingUnknownGateway). As with pbr, this same error was already turned into a warning once before in 1.2.2, so this change is largely reapplying/completing that on the LuCI side to match the current pbr package. - status.js: bump LuciCompat to 34; add warningInterfaceRoutingUnknownGateway4/6 message strings to warningTable; remove errorInterfaceRoutingUnknownGateway from errorTable. - luci.pbr: bump rpcdCompat to 34. Keeps LuciCompat, packageCompat, and rpcdCompat in sync at 34 so no spurious warningInternalVersionMismatch is shown once both PRs are installed together. Signed-off-by: Erik Conijn <egc112@msn.com>
…mossdef-org#29) Expose uplink_interface and uplink_interface6 as per-service config overrides on the Advanced tab, letting the user force the default IPv4/IPv6 uplink interface used by the service. These map directly to the UCI config_schema entries in config.uc; pbr.uc derives cfg.uplink_interface4 from cfg.uplink_interface internally at load time and already uses it throughout split-uplink table selection and status reporting. - form.Value combobox: suggests known interfaces from reply.interfaces (pbr's supported/detected list) but also accepts a custom interface name not currently reported by pbr - datatype "network" for validation - defaults: "wan" for uplink_interface, "wan6" for uplink_interface6 - uplink_interface6 only shown when ipv6_enabled is set Signed-off-by: Erik Conijn <egc112@msn.com>
version bump so that we can start testing recent changes Signed-off-by: Erik Conijn <egc112@msn.com>
Signed-off-by: Erik Conijn <egc112@msn.com>
Bump package release version from 77 to 79. Signed-off-by: Erik Conijn <egc112@msn.com>
…faults (mossdef-org#31) Add forcewrite to uplink_interface and uplink_interface6 so the default value ("wan"/"wan6") is always persisted to UCI on save, instead of being silently skipped because the form value already equals the default and LuCI treats that as "no change." rmempty stays enabled, so an explicitly cleared field still removes the option rather than writing an empty string. Signed-off-by: Erik Conijn <egc112@msn.com>
REQUIRES luci-base commit 974b5864e05ef30f38149389f15583c08bdd4eda
("luci-base: form: do not write values equal to the default").
That commit changed CBIAbstractValue.parse() in
modules/luci-base/htdocs/luci-static/resources/form.js to:
if (fval == null || fval == '' ||
(fval == this.default && (this.optional || this.rmempty))) {
... remove() ...
}
else if (this.forcewrite || !isEqual(cval, fval)) {
... write() ...
}
An option whose form value equals its declared o.default is now REMOVED
from UCI rather than written, whenever optional or rmempty is true. Both
rmempty (true) and the removal clause are on by default, so this silently
applies to nearly every option that does not explicitly opt out. It also
makes o.forcewrite = true dead code in precisely the case it is usually
added for: forcewrite lives in the else-if branch, which the new first
condition now short-circuits.
Changes here:
* resolver_set: rmempty = false. This is the only functional fix. pbr's
backend treats an absent resolver_set as disabled
(files/lib/pbr/platform.uc: resolver_set_supported is true when the
value is empty or 'none'), while LuCI declares a default of
'dnsmasq.nftset' on capable devices.
This is not merely a default failing to be written. pbr ships
resolver_set 'dnsmasq.nftset' in files/etc/config/pbr, and
files/etc/uci-defaults/90-pbr force-sets it on every install and upgrade
(nftset when dnsmasq supports it, 'none' otherwise), so the option is
present in practice on every device. After 974b5864 the first Save of
the overview page therefore DELETES that provisioned value, and because
90-pbr only re-runs on package upgrade, nft set handling stays off until
then: the UI keeps showing nft set support enabled while pbr runs with
it off and domain policies fall back to one-time resolveip lookups.
An explicit o.default = 'none' base is set as well: it is inert while
rmempty and optional are both false, but it documents the fallback on
devices without nft set support and keeps the option correct if rmempty
is ever restored.
* uplink_interface / uplink_interface6: rmempty = false, and the now-dead
forcewrite = true removed. pbr falls back to wan/wan6 when these are
absent, so this is visibility only -- the declared defaults are written
to /etc/config/pbr so users can see and edit them. Note that
uplink_interface6 is still removed when ipv6_enabled is false; that is
the inactive-depends() path (else if (!this.retain) remove()), which is
unconditional and unrelated to rmempty.
Safe on older branches that do not have 974b5864: the upstream change is a
single added clause, so on old form.js these options behave exactly as
before. resolver_set is a ListValue with no blank choice, so rmempty =
false is never reached; and the dropped forcewrite only ever forced a
rewrite of an already-identical value, since an absent option has cval ==
null and is written on the inequality check anyway. The one visible
difference on any branch is that the uplink_interface fields can no longer
be cleared to blank, which is intended. No config churn on up- or
downgrade.
Wider impact -- this luci-base change can break more than pbr:
* Any app that declares o.default and depends on that value actually
reaching /etc/config now gets nothing written. If the package's own
fallback differs from the LuCI default, the UI and the running service
silently disagree. That is exactly the resolver_set case above, and it
is invisible until something misbehaves at runtime.
* Any existing o.forcewrite = true added to force a default into the
config is now a no-op. The correct replacement is rmempty = false, since
cfgvalue() returns null for an absent option.
* rmempty = false additionally makes the field mandatory, so it is not a
drop-in fix for free-text form.Value fields where a blank entry used to
be legitimate. Fields with optional = true need optional cleared as
well, since optional arms the same removal clause independently.
Other pbr views, and other LuCI apps generally, likely need the same
audit.
Thanks to @pesa1234 for bringing this to our attention and for reviewing
the resolver_set analysis. The equivalent fix for 1.2.2 is openwrt/luci
PR #8927 ("luci-app-pbr: preserve resolver_set selection"); that version
carries three resolver branches instead of one, but the fix is the same:
openwrt/luci#8927 (comment)
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The resolver_set select only offers the dnsmasq.nftset choice when reply.platform.dnsmasq_nftset_support is true. That flag comes from files/lib/pbr/platform.uc, which leaves dnsmasq_nftset_supported at false unless dnsmasq is installed and its compile-time options can be probed, so detection fails transiently on a device where dnsmasq is missing, has just been removed, or is not yet running when the page loads. In that state the stored value is not among the choices. UISelect.render() marks an <option> selected only for values present in the choice list, so nothing is selected and the browser falls back to the first entry; UISelect.getValue() returns node.firstChild.value and therefore reports 'none'. Saving the page then writes resolver_set 'none' and silently disables nft set handling, with domain policies falling back to one-time resolveip lookups. The service keeps running, so nothing points at the overview page as the cause. Re-add dnsmasq.nftset as a selectable choice when it is the stored value, even if support is currently not detected. The value then round-trips untouched: cfgvalue equals formvalue, isEqual() matches and parse() writes nothing. o.default stays 'none', which is only consulted when the option is absent. The choice reuses the existing "Dnsmasq nft set" string, adding no new msgid. No annotation is needed on the entry itself because the field description is built immediately above and already reports "The dnsmasq.nftset is not supported on this system." (or "... support is unknown.") in exactly this branch. This bug predates 974b5864e05e and is independent of it, but it defeats the same protection: one is a value removed on save, the other a value downgraded on save. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent fixes to
resolver_setanduplink_interface(6)handling in the overview page. Each commit stands on its own; full reasoning is in the commit messages.1. Adapt to the luci-base default-vs-write change (
057d1f3)luci-base 974b5864e05e ("luci-base: fix uci write bug when input value equals default") changed
CBIAbstractValue.parse()so an option whose form value equals its declareddefaultis removed from UCI instead of written, wheneveroptionalorrmemptyis set. Sincermemptydefaults to true, this applies to nearly every option that does not explicitly opt out. It also makeso.forcewrite = truedead code in precisely the case it is usually added for:forcewritelives in theelse ifbranch that the new condition now short-circuits.resolver_setis the real bug, and it is worse than a default failing to be written. pbr shipsoption resolver_set 'dnsmasq.nftset'in/etc/config/pbranduci-defaults/90-pbrforce-sets it on every install and upgrade, so the option is present on essentially every device. After 974b5864 the first Save of the overview page deletes that provisioned value, and because 90-pbr only re-runs on package upgrade, nft set handling stays off until then — the UI keeps showing nft set support enabled while pbr runs with it off and domain policies fall back to one-time resolveip lookups. Fixed withrmempty = false, plus an explicito.default = "none"base that documents the fallback on devices without nft set support.uplink_interface/uplink_interface6replace the now-deadforcewritewithrmempty = false. pbr falls back towan/wan6when these are absent, so this is visibility only, but I want both always present in the config. This supersedes #31 — thoseforcewritelines never actually worked, having been added after 974b5864 had already landed.Safe on older branches without 974b5864: the upstream change is a single added clause, so on old
form.jsthese options behave exactly as before.resolver_setis aListValuewith no blank choice, sormempty = falseis never reached, and the droppedforcewriteonly ever forced a rewrite of an already-identical value. No config churn on up- or downgrade.The one visible behaviour change on any branch: the
uplink_interfacefields can no longer be cleared to blank, which is intended now that they always carry a value.2. Keep
resolver_setwhen nft set support is undetected (943647d)This bug predates 974b5864 and is independent of it, but it defeats the same protection — one is a value removed on save, the other a value downgraded on save.
The
dnsmasq.nftsetchoice is only offered whenplatform.dnsmasq_nftset_supportis true, andplatform.ucleaves that false unless dnsmasq is installed and its compile-time options can be probed. When detection fails transiently — dnsmasq missing, just removed, or not yet running as the page loads — the stored value is not among the choices.UISelect.render()marks an<option>selected only for values present in the choice list, so nothing is selected, the browser falls back to the first entry, andgetValue()reportsnone. Saving then writesresolver_set 'none'and silently disables nft set handling, with nothing pointing back at the overview page as the cause.Fixed by re-adding
dnsmasq.nftsetas a selectable choice when it is the stored value, even if support is not currently detected. The value then round-trips untouched. It reuses the existingDnsmasq nft setstring, so no new msgid, and needs no annotation because the field description already reports that support is missing in exactly that branch.Notes
No
PKG_RELEASEbump here — handled separately.Thanks to @pesa1234 for bringing the
resolver_setregression to our attention; openwrt/luci#8927 is the equivalent fix for 1.2.2, which carries three resolver branches instead of one.