T6455: build zerotier-one with extosdep support#1195
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
📝 WalkthroughWalkthroughAdds an ExtOsdep config-update command and packed message, implements ExtOsdep::configUpdate and a tap iterator erase fix, calls the new API from OneService's managed-network apply paths under ZT_EXTOSDEP, and applies the patch during the zerotier-one package build. ChangesExtOsdep config update messaging
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
❌ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/package-build/zerotier-one/package.toml`:
- Line 6: Update the pre_build_hook so patch application is idempotent: before
running git apply on
../patches/zerotier-one/0001-extosdep-config-update-nudge.patch, run a check
(e.g. git apply --reverse --check <patch> to detect already-applied patches, or
git apply --check <patch> to detect if it can be applied) and only run git apply
<patch> when the check indicates the patch is not yet applied; modify the
pre_build_hook entry that currently runs git apply to perform this conditional
check-and-apply sequence so rebuilds don’t fail.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: fbfabc42-5414-40fc-b55e-ff58f5fefd5a
📒 Files selected for processing (2)
scripts/package-build/zerotier-one/package.tomlscripts/package-build/zerotier-one/patches/zerotier-one/0001-extosdep-config-update-nudge.patch
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: assign-author / assign-author
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
🔍 Remote MCP Context7
Based on my research, I now have sufficient context to provide a comprehensive summary. Let me compile the relevant findings.
Summary of Additional Context for PR Review
ZeroTier's ExtOsDep System
The extosdep feature provides third-party device vendors the ability to manage ZeroTier without exposing the typical HTTPS control plane on localhost. It can be enabled at compile-time via ZT_EXTOSDEP=1. This is the core functionality the PR enables for VyOS integration.
VyOS T6455 Context
The task is for adding ZeroTier support to VyOS to provide bolt-on SD-WAN support. Each configured ZeroTier interface will be run in a new ZeroTier process as a container. Running in a container allows for easy updating of the software without needing to fully update VyOS, which is important if an upstream fix for a vulnerability is patched.
Build Architecture Context
The PR pins ZeroTier to version 1.16.0, applying a temporary local patch for the CONFIGUPDATE extosdep message before building. ZeroTier's osdep directory contains code to support and integrate with operating systems, including platform-specific stuff only built for certain targets. The service wraps the ZeroTier core and provides VPN-like connectivity to virtual networks.
Review-Relevant Points
-
Temporary Patch Strategy: The PR acknowledges this is a temporary solution until the upstream change (zerotier/ZeroTierOne#2587) is included in a packaged ZeroTier release.
-
Build Validation: The PR describes comprehensive validation including package compilation, confirmation of compiler flags, installation testing, and verification that config update messages reach the wrapper process.
-
Version Management: Package iteration
vyos1distinguishes this patched build from upstream, important for tracking local modifications. -
Existing Integration Context: Previous VyOS requests for ZeroTier integration were blocked due to licensing concerns with ZeroTier's BSL license. The new approach solves this by running ZeroTier as a container, pushing the requirement to acquire the software and adhere to all licensing on the user/operator rather than the distro/maintainers.,, [::web_search::]
🔇 Additional comments (6)
scripts/package-build/zerotier-one/patches/zerotier-one/0001-extosdep-config-update-nudge.patch (5)
32-32: LGTM!Also applies to: 40-44, 53-53
71-78: Verify the intent ofconfigUpdateduring network teardown.The second call site (lines 3626-3628) is in a code path that immediately calls
n.tap().reset()at line 3630, suggesting network shutdown. The first call site (lines 3607-3609) is clearly in a configuration update scenario aftersetMtu(). Sending a config update notification during teardown may be intentional (to notify the wrapper of final state) or unintended (incorrect call site). Clarify the design intent.
75-76: Structure is correct and intentional.The Windows-specific code at lines 75-76 properly closes with
#endif, followed by a separate#ifdef ZT_EXTOSDEPblock for the newconfigUpdate()call. This layered conditional design isolates Windows-specific functionality (instanceId extraction) from the new EXTOSDEP code path, with common cleanup executing after both blocks. No scope violation occurs.
66-66: Type cast touint64_tis safe. ThenetconfRevisionfield in ZeroTier'sNetworkConfigstructure isunsigned longor alreadyuint64_t. Both call sites perform safe widening or no-op casts with no truncation or sign-extension risk. The second call site at line 3625 is also appropriate—it notifies the external OS dependency of the final configuration state before network teardown.
9-19: No changes required; implementation follows the established ExtOsdep pattern.The
configUpdate()function correctly mirrors other ExtOsdep message handlers likezt_eod_msg_addtap, which also assignuint64_tfields (e.g.,nwid) without byte order conversion. ExtOsdep is a local IPC mechanism, not a network protocol, so byte order conversion is unnecessary. The struct layout with__attribute__((packed))is consistent with all other message types in the protocol.> Likely an incorrect or invalid review comment.scripts/package-build/zerotier-one/package.toml (1)
9-9: LGTM!Also applies to: 16-16
| commit_id = "1.16.0" | ||
| scm_url = "https://github.com/zerotier/ZeroTierOne.git" | ||
|
|
||
| pre_build_hook = "git apply ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch" |
There was a problem hiding this comment.
Patch application is not idempotent and will fail on rebuild.
git apply will fail if the patch is already applied, breaking incremental build scenarios. Consider using git apply --reverse --check to test if the patch is already applied before attempting to apply it, or use git apply --check with conditional logic.
🔧 Suggested fix for idempotent patch application
-pre_build_hook = "git apply ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch"
+pre_build_hook = """
+if ! git apply --reverse --check ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch 2>/dev/null; then
+ git apply ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch
+fi
+"""📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pre_build_hook = "git apply ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch" | |
| pre_build_hook = """ | |
| if ! git apply --reverse --check ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch 2>/dev/null; then | |
| git apply ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch | |
| fi | |
| """ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/package-build/zerotier-one/package.toml` at line 6, Update the
pre_build_hook so patch application is idempotent: before running git apply on
../patches/zerotier-one/0001-extosdep-config-update-nudge.patch, run a check
(e.g. git apply --reverse --check <patch> to detect already-applied patches, or
git apply --check <patch> to detect if it can be applied) and only run git apply
<patch> when the check indicates the patch is not yet applied; modify the
pre_build_hook entry that currently runs git apply to perform this conditional
check-and-apply sequence so rebuilds don’t fail.
|
I have read the CLA Document and I hereby sign the CLA |
aadnehovda
changed the title
| commit_id = "1.16.0" | ||
| scm_url = "https://github.com/zerotier/ZeroTierOne.git" | ||
|
|
||
| pre_build_hook = "git apply ../patches/zerotier-one/0001-extosdep-config-update-nudge.patch" |
There was a problem hiding this comment.
The build.py script will always check the patches/<pkg-name> directory if it is exist it will patch the package during build process
So as I see we do not need additional pre_build_hook here
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 invalid-title label must be absent to mergeWaiting for
This rule is failing.Block merge while the invalid-title label is present. The label is auto-toggled by the
|
2 participants
Summary
Build the ZeroTier agent package with extosdep (external interface mgmt) support for the alternative VyOS ZeroTier integration in vyos/vyos-1x#5211. Please consider that before merging this.
This also adds a small optional patch for the
CONFIGUPDATEnotification proposed upstream in zerotier/ZeroTierOne#2587.Changes
zerotier-onewithZT_EXTOSDEP=1andZT_SSO_SUPPORTED=0.make/fpmbuild.vyos1so the patched build is distinguishable.