Fix MSS clamping for site-to-site networking #453
Conversation
WalkthroughThe pull request modifies two scripts related to Maximum Segment Size (MSS) clamping for the Tailscale network interface. The changes introduce a new constant Changes
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🔇 Additional comments (7)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/mss-clamping/finish (1)
24-31: Consider reducing duplication between IPv4 and IPv6 blocks.The IPv4 and IPv6 blocks are nearly identical except for the command used (iptables vs ip6tables). Consider extracting this logic into a function to reduce duplication.
Example refactor:
+ remove_clamping() { + local cmd="$1" + local ip_version="$2" + for interface in $( \ + ${cmd} -t mangle -S FORWARD \ + | { grep -E "^-A FORWARD -o tailscale\d ${CLAMPING_IPTABLES_OPTIONS}$" || true ;} \ + | sed -nr 's/^.*?-o\s(\S+)\s.*$/\1/p') + do + bashio::log.info "Removing the MSS clamping for interface ${interface} (${ip_version})" + if ! ${cmd} -t mangle -D FORWARD -o ${interface} ${CLAMPING_IPTABLES_OPTIONS}; then + bashio::log.warning "Removing clamping is unsuccessful" + fi + done + } + + remove_clamping "iptables" "IPv4" + remove_clamping "ip6tables" "IPv6"tailscale/rootfs/etc/s6-overlay/s6-rc.d/mss-clamping/run (1)
14-24: Consider reducing duplication between IPv4 and IPv6 blocks.The implementation is correct but contains duplicated logic with the IPv6 block. Consider extracting this into a function.
Example refactor:
+ setup_clamping() { + local cmd="$1" + local ip_version="$2" + bashio::log.info "Clamping the MSS to the MTU for interface ${TAILSCALE_INTERFACE}," \ + "to support site-to-site networking better (${ip_version})" + if ${cmd} -t mangle -S FORWARD \ + | grep -Eq "^-A FORWARD -o tailscale\d ${CLAMPING_IPTABLES_OPTIONS}$" + then + bashio::log.notice "Clamping is already set" + else + if ! ${cmd} -t mangle -A FORWARD -o ${TAILSCALE_INTERFACE} ${CLAMPING_IPTABLES_OPTIONS}; then + bashio::log.warning "Setting up clamping is unsuccessful" + fi + fi + } + + setup_clamping "iptables" "IPv4" + setup_clamping "ip6tables" "IPv6"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/mss-clamping/finish(1 hunks)tailscale/rootfs/etc/s6-overlay/s6-rc.d/mss-clamping/run(1 hunks)
🔇 Additional comments (5)
tailscale/rootfs/etc/s6-overlay/s6-rc.d/mss-clamping/finish (2)
8-8: Well-structured constant definition!Good practice to extract the iptables options into a constant, improving maintainability and reducing duplication. The options correctly match Tailscale's documentation for MSS clamping.
14-21: Improved error handling for IPv4 clamping removal.Good improvements:
- Using
|| truewith grep ensures script resilience- Added error handling for the iptables command with appropriate warning message
- Consistent use of the new constant in the grep pattern
tailscale/rootfs/etc/s6-overlay/s6-rc.d/mss-clamping/run (3)
8-9: Well-structured constant definitions!Good practices:
- Interface name defined as a constant for better maintainability
- Consistent use of CLAMPING_IPTABLES_OPTIONS across both scripts
11-13: Clear and well-documented implementation!Excellent documentation:
- Clear explanation of the purpose
- Includes reference to Tailscale documentation
- Explains the proactive configuration approach
25-34: Consistent implementation with IPv4 block.The IPv6 implementation correctly mirrors the IPv4 implementation, ensuring consistent behavior across both IP versions.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Proposed Changes
TS docs got updated, doc was wrong, showed the exact opposite where should this be applied (incoming or outgoing traffic on tailscale0 interface).
Currently the docs are equal with the real code in TS-s docker image (here).
This PR follows the new TS doc.
Related Issues
Summary by CodeRabbit
tailscale0interface.