Skip to content

51sdwan: add OpenWrt edge agent - #30176

Closed
21hkcloud wants to merge 1 commit into
openwrt:masterfrom
21hkcloud:add-51sdwan
Closed

51sdwan: add OpenWrt edge agent#30176
21hkcloud wants to merge 1 commit into
openwrt:masterfrom
21hkcloud:add-51sdwan

Conversation

@21hkcloud

Copy link
Copy Markdown

This adds the architecture-independent 51SDWAN OpenWrt edge agent.

The agent enrolls a router with an existing 51SDWAN member account,
creates and manages a WireGuard interface, applies smart or global
routing policy, sends heartbeats and reconnects after WAN recovery.
The router private key remains local and the member password is used
only for the HTTPS login request.

The package is licensed under GPL-2.0-or-later and has no dependencies
outside OpenWrt core and the packages feed.

Runtime-tested on OpenWrt 25.12.5 x86_64:

  • package installation and service startup
  • account enrollment and WireGuard connection
  • smart and global routing modes
  • WAN loss and automatic recovery
  • LAN access and original WAN fallback after disconnect

@21hkcloud
21hkcloud force-pushed the add-51sdwan branch 3 times, most recently from 8dac3d1 to 01bdce4 Compare August 6, 2026 16:55
@BKPepe BKPepe added AI It looks like this is issue or pull request was created by AI or it includes AI comments NAK pull request with at least one NAK from maintainers labels Aug 6, 2026

@BKPepe BKPepe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed at 01bdce4.

Before the code details: this is the same pattern as #30158 — a client for a
commercial, vendor-hosted service whose console and site are Chinese-only,
from the same author. That PR is currently NAK'd on exactly that ground, and
this one goes further: there is no upstream repository at all, so merging it
would make the feed the upstream for ~1000 lines of shell. My question
there applies here too, and I'd resolve it before spending time on the diff.

If it does get considered on technical merit, these look blocking to me:

  • DEPENDS is missing +jshn (scripts source jshn.sh) and +rpcd.
  • sw_heartbeat() commits UCI unconditionally every 15 s, so
    /etc/config/51sdwan is rewritten to the overlay ~5760x/day. Combined with
    procd_add_reload_trigger 51sdwan and a reload_service() that does
    stop; start (which disconnects), this should also flap the tunnel
    continuously — please verify on a live device.
  • The package writes permanent sections into /etc/config/network and
    /etc/config/firewall and never removes them; there is no prerm/postrm.
    After removal a stale interface, firewall zone and the WireGuard private key
    in /etc/51sdwan/ are left behind.
  • postinst does kill -TERM $(pidof netifd), which drops networking on a
    running router.
  • Everything is ip -4 and allowed_ips is only 0.0.0.0/0, so IPv6 bypasses
    the tunnel entirely in global mode.
  • Smart mode installs its ip rule at priority 42751, i.e. after main (32766),
    so once the wg default route is in main the rule can never match.

User-facing strings, including ubus error output, are in Chinese; those belong
in English here, with localisation in the LuCI app.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 commit at 01bdce4.

The commit message matches the diff, so nothing to flag there. The inline
comments below are limited to points not already raised in the existing review —
the missing DEPENDS, the 15 s UCI commit churn on /etc/config/51sdwan, the
absent prerm/postrm, the kill -TERM $(pidof netifd) in postinst, the
IPv4-only tunnel and the ip-rule priority are not repeated here.

Of the new findings, the fixed /tmp/51sdwan-curl.err path is the one I'd treat
as blocking on its own merits; the two flash-write loops are close behind, since
they run unattended on a device whose overlay is the only writable storage.


Generated by Claude Code

Comment on lines +60 to +62
http_code="$(curl -sS --proto '=https' --tlsv1.2 --connect-timeout 12 --max-time 30 \
-o "$tmp" -w '%{http_code}' -X GET -H 'Accept: application/json' \
-H "Authorization: Bearer $token" "$base$path" 2>/tmp/51sdwan-curl.err)"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

2>/tmp/51sdwan-curl.err is a fixed, predictable path in a world-writable directory, written by a process running as root. Any local unprivileged user can pre-create /tmp/51sdwan-curl.err as a symlink to an arbitrary file, and this redirect will then truncate and overwrite the target with root privileges. The response body one line above already goes through mktemp (line 57) — the stderr capture should too.

The same file is read back at line 82 into message, which sw_set_error stores in 51sdwan.main.last_error and sw_status exports over ubus, so planted content is also echoed back into the UI.

Secondary: the fixed name collides between concurrent sw_api calls — the agent loop and an rpcd connect/login can both be inside sw_api at once, and each rm -f /tmp/51sdwan-curl.err (lines 84 and 89) deletes the other's file.

Same applies to the two other 2>/tmp/51sdwan-curl.err redirects, at lines 67 and 73.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed in the public upstream source pinned by this PR. Each API call now creates private response and stderr files with mktemp inside a mode-0700 runtime directory, so concurrent calls do not share predictable paths.

SW_STATE_DIR="/etc/51sdwan"
SW_PRIVATE_KEY="$SW_STATE_DIR/private.key"
SW_PUBLIC_KEY="$SW_STATE_DIR/public.key"
SW_ROUTE_FILE="$SW_STATE_DIR/china_ipv4.txt"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The China IPv4 list is a downloadable cache, but it lives under /etc/51sdwan/, i.e. on the overlay/flash. sw_download_routes rewrites it unconditionally (line 317) on every sw_connect and every refresh-routes, and sw_connect is retried from the agent loop every 15 s while the handshake is failing. That list is on the order of 8k CIDRs / ~100 KB, so a router that cannot reach its gateway will rewrite ~100 KB of flash every 15 s indefinitely.

Nothing here needs to survive a reboot: sw_connect re-downloads before use anyway, and the [ -s "$SW_ROUTE_FILE" ] && return 0 stale-cache fallback at line 307 only has to hold within one boot.

Suggested change
SW_ROUTE_FILE="$SW_STATE_DIR/china_ipv4.txt"
SW_ROUTE_FILE="/tmp/51sdwan-china_ipv4.txt"

(chmod 600 at line 319 still applies; mkdir -p "$SW_STATE_DIR" at line 302 then becomes unnecessary for this path.)


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. The downloaded route list is now kept under /tmp/51sdwan and is never written to the persistent overlay.

Comment on lines +419 to +420
uci set network.sdwan51_peer.route_allowed_ips=1
uci commit network

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

route_allowed_ips is flipped to 1 and committed here on every connect, and back to 0 and committed again in sw_disconnect (lines 447-448). /etc/config/network is therefore rewritten twice per connect attempt — and since the agent loop calls sw_connect every 15 s while the tunnel is down, and every failure path in this function ends in sw_disconnect, that is a sustained write loop against the router's whole network config, not just against 51sdwan.

sw_apply_network already writes this key once at line 239, and the actual default route is otherwise managed imperatively in this file (sw_remove_interface_default, sw_restore_wan_default). Driving this one toggle through a UCI commit plus ifdown/ifup is what forces the rewrite; please either set it once in sw_apply_network and manage the default route at runtime, or at minimum only commit when the stored value actually differs.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. route_allowed_ips remains 0 in UCI. The default route and smart-routing table are managed only with runtime ip route/ip rule operations, so connect/disconnect no longer rewrites /etc/config/network.

Comment on lines +197 to +209
sw_backup_configs() {
local backup_dir="$SW_STATE_DIR/backup"
mkdir -p "$backup_dir" || return 1
chmod 700 "$backup_dir" || return 1
if [ ! -e "$backup_dir/network.before-51sdwan" ]; then
cp -p /etc/config/network "$backup_dir/network.before-51sdwan" || return 1
fi
if [ ! -e "$backup_dir/firewall.before-51sdwan" ]; then
cp -p /etc/config/firewall "$backup_dir/firewall.before-51sdwan" || return 1
fi
chmod 600 "$backup_dir"/* 2>/dev/null || true
return 0
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These backups are written but never read. Nothing in the package restores network.before-51sdwan or firewall.before-51sdwansw_disconnect, sw_logout and sw_reset_identity all leave the injected network.51sdwan / network.sdwan51_peer / firewall.sdwan51_* sections in place, and there is no prerm/postrm. The only lasting effect is that a full copy of /etc/config/network — which on most routers carries the PPPoE credentials and any existing tunnel keys — is parked in the overlay permanently.

The [ ! -e ... ] guard also means the copy is taken exactly once, so after the first connect it is a stale snapshot that would be wrong to restore even if something did use it.

Either wire up a restore path (and remove the backup directory on uninstall), or drop sw_backup_configs together with the sw_backup_configs || ... call at line 222 — the error string there ("backup failed, no changes made") currently promises a rollback capability that does not exist.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. The unused persistent configuration backups were removed. The package now has prerm/postrm cleanup hooks that remove only its owned network/firewall sections and private state on uninstall, while preserving them on upgrade.

Comment on lines +8 to +10
while [ "$(sw_cfg enabled 0)" = "1" ]; do
if sw_heartbeat; then
if [ "$(sw_cfg auto_connect 1)" = "1" ] && ! sw_is_connected; then

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

sw_cfg returns the raw UCI string, so these compare against the literal "1". UCI booleans are also legally on, true, yes and enabled — which is what a LuCI checkbox or a hand-edited config produces — and any of those makes this loop exit on the first iteration, so the daemon starts and immediately dies into procd's respawn cycle.

The init script gets this right (config_get_bool enabled main enabled 0), so the two disagree about whether the service should run. The same raw comparison is in 95-51sdwan:6, and sw_status at common.sh:542-543 passes the raw value straight into json_add_boolean, which emits garbage for anything that is not 0/1.

Please normalise in one place — either add a sw_cfg_bool helper built on config_get_bool, or make every reader and writer agree that these are strictly 0/1.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. Boolean values are normalized through a shared sw_cfg_bool helper and all agent, hotplug, and status readers now agree on the accepted UCI boolean forms.

[ "$INTERFACE" = "wan" ] || [ "$INTERFACE" = "wan6" ] || exit 0
[ "$(uci -q get 51sdwan.main.enabled)" = "1" ] || exit 0

( sleep 5; /usr/bin/51sdwanctl refresh-routes >/dev/null 2>&1 ) &

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: this spawns a detached refresh on every wan/wan6 ifup with no serialisation, so a flapping WAN (or a dual-stack link, where wan and wan6 both fire) queues several overlapping refresh-routes runs. Each one calls sw_apply_smart_routes, which first flushes table 42751 and deletes the ip rule and then batch-installs thousands of routes — two concurrent runs will have one flushing the table the other just populated, and ip rule add does not deduplicate, so the priority-42751 rule accumulates one copy per run.

A flock/mkdir guard around the 51sdwanctl refresh-routes call would make this safe. Also worth noting that the wan6 trigger currently does nothing useful, since sw_refresh_routes and the whole smart-route path are IPv4-only.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. The hotplug handler is limited to wan, uses an atomic mkdir lock, and the routing teardown removes all owned priorities before reapplying the table, preventing overlapping refreshes and duplicate rules.

Comment thread net/51sdwan/files/etc/config/51sdwan Outdated
option mode 'smart'
option api_base 'https://www.51sdwan.com'
option route_list_url 'https://www.51sdwan.com/downloads/china_ipv4_merged.txt'
option interface '51sdwan'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: this option is never read. common.sh hardcodes SW_INTERFACE="51sdwan", and the UCI section/zone names in sw_apply_network (network.51sdwan, network.sdwan51_peer, firewall.sdwan51_zone) are literals too. Changing this option would silently do nothing, or half-work. Either honour it in SW_INTERFACE or drop it from the shipped config.


Generated by Claude Code

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Fixed. The unused interface option was removed from the shipped configuration; the owned interface name remains intentionally fixed.

@21hkcloud

Copy link
Copy Markdown
Author

Thank you for the detailed review. The package has been reworked around a public upstream and the blocking issues have been addressed.

Current package commit: 5de98a3b6
Public upstream: https://github.com/21hkcloud/51sdwan-openwrt
Pinned upstream commit: 19425b10ac5449eb3510a8e367a73c1548a3797e
English product/integration page: https://www.51sdwan.com/en/openwrt

Highlights:

  • added the missing jshn and rpcd dependencies;
  • removed periodic UCI writes and persistent route-list writes;
  • moved API temporary files to private mktemp paths and serialized hotplug refreshes;
  • removed the unused configuration backups and added upgrade-safe postinst/prerm/postrm lifecycle handling;
  • removed the netifd kill and preserved the existing WAN route on every failure path;
  • kept route_allowed_ips=0, moved the policy rule before main, and added explicit main-table bypass rules;
  • blocked IPv6 leaks only while global acceleration is connected, restoring IPv6 forwarding on disconnect;
  • converted all core/user-facing messages to English and normalized UCI booleans.

Validation performed:

  • ShellCheck (busybox, excluding sourced-file warning) passes for the upstream shell sources;
  • OpenWrt 24.10 SDK builds 51sdwan_0.2.0-r1_all.ipk from the pinned source and mirror hash;
  • fresh install, real version upgrade, and clean removal were tested on 24.10;
  • upgrade from 0.1.2 to 0.2.0 was tested on a live OpenWrt 25.12.5 mt7621 router with account/tunnel configuration preserved;
  • the live router completed a WireGuard handshake, used policy priority 10000 before main, resolved DNS and reached HTTPS through the tunnel;
  • repeated heartbeats left the UCI configuration hash unchanged;
  • disconnect removed the policy table/rule, disabled the IPv6 leak rule, and restored WAN DNS/HTTPS.

@BKPepe, when convenient, could you please re-review the updated implementation?

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed the one new commit (5de98a3b) on top of my previous review at 01bdce4.

This commit removes every shipped script from the feed and builds the package from 21hkcloud/51sdwan-openwrt at a pinned commit, so none of the shell-code findings from the earlier reviews can be re-verified here — those files no longer exist in this repository, and the replies marking them fixed point at a tree this PR does not contain. I did verify the pin itself: the commit 19425b10 exists on that repository's main, and every path the install recipe references (core/LICENSE, core/files/...) is present in it.

Commit checks

  • 5de98a3b "51sdwan: address package review feedback" — only the first line of the body ("Move runtime sources to a public upstream repository and pin the release commit") describes this diff. The remaining paragraphs — avoiding periodic UCI writes, correcting policy routing, preventing global-mode IPv6 leaks, English core messages — describe changes that happen entirely in the external repository; none of them are in this commit, which is a 939-line deletion plus a Makefile rewrite. Please describe what the commit does to this feed. Since the PR as a whole adds a new package, the two commits would normally be squashed into a single 51sdwan: add OpenWrt edge agent.

Of the inline comments, the removed test-version.sh is the one I would expect to break the package test outright; the rest are packaging correctness, not blockers on their own.


Generated by Claude Code

Comment thread net/51sdwan/Makefile
$(INSTALL_DIR) $(1)/etc/uci-defaults
$(INSTALL_BIN) $(PKG_BUILD_DIR)/core/files/etc/uci-defaults/90-51sdwan $(1)/etc/uci-defaults/90-51sdwan
$(INSTALL_DIR) $(1)/usr/bin
$(INSTALL_BIN) $(PKG_BUILD_DIR)/core/files/usr/bin/51sdwanctl $(1)/usr/bin/51sdwanctl

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

net/51sdwan/test-version.sh is deleted by this commit, but none of the installed executables report a version: 51sdwanctl accepts only status|connect|disconnect|heartbeat|refresh-routes|logout and exits 2 on anything else — --version and --help included — and 51sdwan-agent parses no arguments at all. The feed's generic runtime test runs every packaged executable with version/help flags and expects PKG_VERSION in the output, so it fails for this package unless a test-version.sh override exists.

The core/test-version.sh that upstream ships does not help: it lives inside the fetched source tree, while the harness looks for the override in the package directory in this repository (as in libs/lmdb/test-version.sh). Please restore net/51sdwan/test-version.sh.


Generated by Claude Code

Comment thread net/51sdwan/Makefile Outdated
Comment on lines +10 to +14
PKG_SOURCE_PROTO:=git
PKG_SOURCE_URL:=https://github.com/21hkcloud/51sdwan-openwrt.git
PKG_SOURCE_DATE:=2026-08-07
PKG_SOURCE_VERSION:=19425b10ac5449eb3510a8e367a73c1548a3797e
PKG_MIRROR_HASH:=ab51ac293b2dea0747ff78086a0489485053819c7b6ca569d31811c4a5be72c2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

PKG_SOURCE_PROTO:=git is meant as a last resort, for upstreams that publish no release archive. I checked this one: git ls-remote https://github.com/21hkcloud/51sdwan-openwrt.git returns only HEAD and refs/heads/main — there are no tags, so PKG_SOURCE_VERSION here pins whatever main happened to be on 2026-08-07 rather than a release, and PKG_VERSION:=0.2.0 has nothing upstream to correspond to.

Since you own the upstream repository, please tag the release and fetch the tarball instead, e.g.

PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz
PKG_SOURCE_URL:=https://codeload.github.com/21hkcloud/51sdwan-openwrt/tar.gz/v$(PKG_VERSION)?
PKG_HASH:=<sha256 of the tarball>

That also makes later version bumps in this feed a two-line change instead of a commit-SHA-plus-mirror-hash update.


Generated by Claude Code

Comment thread net/51sdwan/Makefile Outdated
Comment on lines +68 to +73
chmod 600 /etc/config/51sdwan 2>/dev/null || true
/etc/init.d/rpcd restart 2>/dev/null || true
enabled="$$(uci -q get 51sdwan.main.enabled)"
case "$${enabled}" in 1|on|true|yes|enabled)
/etc/init.d/51sdwan enable >/dev/null 2>&1 || true
/etc/init.d/51sdwan start >/dev/null 2>&1 || true

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The shipped core/files/etc/uci-defaults/90-51sdwan — installed two hunks above at line 52 — already does chmod 600 /etc/config/51sdwan, /etc/init.d/51sdwan enable and /etc/init.d/rpcd restart, and it runs before this scriptlet body: default_postinst executes and then deletes /etc/uci-defaults/*, and the generated post-install script emits default_postinst first, then the package's own postinst. So every install and every upgrade restarts rpcd twice and chmods the config twice.

The two also disagree about policy: the uci-defaults script enables the service unconditionally, while this block enables it only when 51sdwan.main.enabled is set — which means the enabled=0 default never actually prevents the init script from being enabled. Please drop the duplicated lines here and keep the enable decision in one place:

Suggested change
chmod 600 /etc/config/51sdwan 2>/dev/null || true
/etc/init.d/rpcd restart 2>/dev/null || true
enabled="$$(uci -q get 51sdwan.main.enabled)"
case "$${enabled}" in 1|on|true|yes|enabled)
/etc/init.d/51sdwan enable >/dev/null 2>&1 || true
/etc/init.d/51sdwan start >/dev/null 2>&1 || true
enabled="$$(uci -q get 51sdwan.main.enabled)"
case "$${enabled}" in 1|on|true|yes|enabled)
/etc/init.d/51sdwan enable >/dev/null 2>&1 || true
/etc/init.d/51sdwan start >/dev/null 2>&1 || true

Generated by Claude Code

Comment thread net/51sdwan/Makefile Outdated
Comment on lines +86 to +93
changed=0
for section in 51sdwan sdwan51_peer; do
if uci -q get "network.$${section}" >/dev/null; then
uci -q delete "network.$${section}"
changed=1
fi
done
[ "$${changed}" = 0 ] || uci -q commit network

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Line 84 deliberately skips disable when $${action} is upgrade, and postrm keeps /etc/51sdwan on upgrade, but this teardown is unguarded: opkg invokes the outgoing package's prerm with upgrade, so network.51sdwan and network.sdwan51_peer — and the three firewall.sdwan51_* sections in the second loop at line 95 — are deleted and committed on every version bump, taking the WireGuard interface and its firewall zone with them until the agent reconnects and recreates them.

Is that intentional? If these sections are meant to survive an upgrade like the private key does, both loops need the same guard as line 84. Note that the 25.12 upgrade test you describe would not surface this: apk uses separate pre-upgrade/post-upgrade scripts, so it is the opkg path (24.10 and older) that runs this code with upgrade.


Generated by Claude Code

@21hkcloud

Copy link
Copy Markdown
Author

Addressed the latest package review in 111f5b59cc and squashed the PR to a single add-package commit:

  • published the public upstream v0.2.0 tag at 539f28edcf633eed662e4bb5c962d8dbffee748b;
  • switched the feed package from a git snapshot to the tagged codeload archive with verified SHA-256 7ea02e94f369aa906eec6b2a89e8e0f406ea1e9ae80864bee4bc87e6ea5db675;
  • restored net/51sdwan/test-version.sh as an executable feed test override;
  • removed duplicate chmod/rpcd work from postinst, while upstream uci-defaults no longer enables the service unconditionally;
  • preserved the network and firewall UCI sections during opkg upgrades, while retaining full cleanup on removal;
  • rewrote the single commit message to describe only the package-feed changes.

Local git diff --check, source tag target, archive layout and archive hash validations pass.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at 111f5b59. The branch was rebased into a single commit since my last review at 5de98a3b, so I re-read the whole diff rather than just the delta; the substantive change is PKG_SOURCE_PROTO:=git → a pinned v0.2.0 tarball plus a test-version.sh and a reworked prerm. Switching to a release tarball and guarding the UCI cleanup with != upgrade both address earlier feedback, and the single squashed commit message matches what the diff does, so nothing to flag there.

Two findings inline. The PKG_BUILD_DIR mismatch is the blocking one — as written the package cannot build at all, since the codeload archive unpacks to 51sdwan-openwrt-0.2.0 while every install line reads from $(PKG_BUILD_DIR) = 51sdwan-0.2.0. Nothing in CI catches this here: only the three FormalityCheck jobs ran on this head and all passed; no build job has run.

Two things I could not check from this repository, for the maintainer's awareness rather than as new asks:

  • The runtime shell still lives entirely in 21hkcloud/51sdwan-openwrt, so the findings from the earlier reviews — the 15 s UCI commit churn, the ip-rule priority, IPv4-only allowed_ips, the fixed /tmp/51sdwan-curl.err path, Chinese user-facing strings — are not verifiable in this diff. I did confirm the pin resolves: tag v0.2.0 exists on that repository and every path the install recipe references is present in it.
  • I was unable to fetch the tarball itself, so PKG_HASH is unverified here.

The NAK label and the upstream-ownership question raised in the first review are unchanged by this revision and are the maintainers' call, not mine.


Generated by Claude Code

Comment thread net/51sdwan/Makefile

PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.gz
PKG_SOURCE_URL:=https://codeload.github.com/21hkcloud/51sdwan-openwrt/tar.gz/v$(PKG_VERSION)?
PKG_HASH:=7ea02e94f369aa906eec6b2a89e8e0f406ea1e9ae80864bee4bc87e6ea5db675

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The switch from PKG_SOURCE_PROTO:=git to a codeload tarball leaves PKG_BUILD_DIR pointing at a directory the tarball never creates, so the build cannot succeed.

PKG_BUILD_DIR defaults to $(BUILD_DIR)/$(PKG_NAME)-$(PKG_VERSION) = .../51sdwan-0.2.0, and the unpack step extracts into $(PKG_BUILD_DIR)/.. rather than into PKG_BUILD_DIR itself. The GitHub archive for tag v0.2.0 of 21hkcloud/51sdwan-openwrt has the top-level prefix 51sdwan-openwrt-0.2.0/, so the sources land in .../51sdwan-openwrt-0.2.0 while .../51sdwan-0.2.0 is created empty. Every $(INSTALL_*) $(PKG_BUILD_DIR)/core/files/... line below then fails with "No such file or directory".

Same situation as net/mrmctl/Makefile:17, which pulls ewsi/macremapper under PKG_NAME:=mrmctl and overrides the build dir accordingly:

Suggested change
PKG_HASH:=7ea02e94f369aa906eec6b2a89e8e0f406ea1e9ae80864bee4bc87e6ea5db675
PKG_HASH:=7ea02e94f369aa906eec6b2a89e8e0f406ea1e9ae80864bee4bc87e6ea5db675
PKG_BUILD_DIR:=$(BUILD_DIR)/51sdwan-openwrt-$(PKG_VERSION)

Generated by Claude Code

Comment thread net/51sdwan/Makefile
define Package/51sdwan/postinst
#!/bin/sh
[ -n "$${IPKG_INSTROOT}" ] || {
enabled="$$(uci -q get 51sdwan.main.enabled)"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

/etc/init.d/rpcd restart was dropped from postinst in this revision, but the install recipe still ships an rpcd plugin (line 53, $(1)/usr/libexec/rpcd/51sdwan). rpcd enumerates /usr/libexec/rpcd only at start, so after a fresh install the 51sdwan ubus object does not exist until the router is rebooted or rpcd is restarted by hand — and 51sdwanctl/the LuCI app talk to it through ubus.

The asymmetry looks accidental rather than intended: postrm below still restarts rpcd, and the upstream core/Makefile at tag v0.2.0 still has the call in its own postinst. Same pattern as net/tor-hs/Makefile:45.

(The chmod 600 /etc/config/51sdwan that went with it is genuinely redundant — INSTALL_CONF is install -m0600 — so only the rpcd restart needs to come back.)

Suggested change
enabled="$$(uci -q get 51sdwan.main.enabled)"
/etc/init.d/rpcd restart >/dev/null 2>&1 || true
enabled="$$(uci -q get 51sdwan.main.enabled)"

Generated by Claude Code

Add an all-architecture package for the 51SDWAN OpenWrt edge agent.
Fetch the tagged v0.2.0 source archive from the public upstream with a
verified SHA-256 hash, install the runtime files, and declare the required
dependencies and persistent configuration.

Provide lifecycle hooks that preserve configuration across upgrades and
clean package-owned network and firewall sections on removal. Include a
runtime-test override for executables that do not expose version flags.

Signed-off-by: Jia Liu <support@51sdwan.com>
@21hkcloud

21hkcloud commented Aug 9, 2026

Copy link
Copy Markdown
Author

Addressed the two latest review findings in ed521c346:

  • set PKG_BUILD_DIR to the actual codeload archive root, 51sdwan-openwrt-$(PKG_VERSION);
  • restart rpcd in postinst so the newly installed ubus object is available immediately.

I also re-downloaded the v0.2.0 archive and verified both its top-level directory and the existing SHA-256 hash. All three FormalityCheck jobs are green on the amended head.

@openwrt-ai openwrt-ai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed 1 new commit; no new issues found.

The branch was amended again since my last review at 111f5b59, so I diffed 111f5b59..ed521c34. The delta is two lines, both addressing the findings from that review, and both look right to me:

  • PKG_BUILD_DIR:=$(BUILD_DIR)/51sdwan-openwrt-$(PKG_VERSION) is set before include $(INCLUDE_DIR)/package.mk, so it wins over the ?= default at package.mk:11, and it now matches the top-level prefix the codeload archive for v0.2.0 unpacks into. The $(INSTALL_*) $(PKG_BUILD_DIR)/core/files/... lines resolve as a result.
  • /etc/init.d/rpcd restart is back in postinst, so the 51sdwan ubus object exists after a fresh install without a reboot, and postinst/postrm are symmetric again.

The commit message still matches what the diff does, so nothing to flag there.

Unchanged from my last review, for the maintainer's awareness rather than as new asks: the runtime shell lives entirely in 21hkcloud/51sdwan-openwrt, so the earlier shell findings (UCI commit churn, ip-rule priority, IPv4-only allowed_ips, the fixed /tmp path, Chinese user-facing strings) remain unverifiable from this diff, and PKG_HASH is unverified here as I could not fetch the tarball. Only the three FormalityCheck jobs ran on this head and all passed; no build job has run, so the PKG_BUILD_DIR fix has not actually been exercised by CI.

The NAK label and the upstream-ownership question from the first review are unaffected by this revision and remain the maintainers' call.


Generated by Claude Code

@21hkcloud

Copy link
Copy Markdown
Author

Thank you for the feedback. To avoid submitting two related vendor-hosted networking clients to the feed at the same time, I am withdrawing this PR and will focus on resolving the documentation, LuCI separation, and usability concerns raised on #30158. The two services have different networking models, but keeping both submissions open before the first package has established an acceptable integration and documentation standard creates unnecessary review burden.

We may revisit 51SDWAN separately in the future only after those concerns are fully addressed.

@21hkcloud 21hkcloud closed this Aug 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Add package AI It looks like this is issue or pull request was created by AI or it includes AI comments NAK pull request with at least one NAK from maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants