Skip to content

Ignore scoped link-local IPv6 DNS#1928

Open
melo-crypto wants to merge 1 commit into
firewalla:masterfrom
melo-crypto:my_dev
Open

Ignore scoped link-local IPv6 DNS#1928
melo-crypto wants to merge 1 commit into
firewalla:masterfrom
melo-crypto:my_dev

Conversation

@melo-crypto

Copy link
Copy Markdown
Contributor

No description provided.

@j-sallyjin

This comment has been minimized.

@j-sallyjin

Copy link
Copy Markdown

PR Review Summary

✅ What looks good

  • Correctly separates IPv4 and IPv6 DNS route handling in updateRouteForDNS(), avoiding the previous early return where missing IPv4 gateway could skip IPv6 DNS routes.
  • Adds getRoutableDns6Nameservers() instead of changing the general getDns6Nameservers() contract, preserving link-local DNS visibility for non-routing callers.
  • Updates routing paths to use the routable IPv6 DNS helper before programming DNS-specific IPv6 routes.
  • Added tests covering preservation of link-local IPv6 DNS in the general helper and route programming for IPv4 plus routable IPv6 DNS.
  • Issue requirement coverage: No linked issue found.

⚠️ Issues found

No blocking correctness, security, concurrency, or networking issues found in the provided diff.

💡 Suggestions

  • Add a direct unit test for getRoutableDns6Nameservers() with scoped link-local, unscoped link-local, global IPv6, and invalid input. The current route test stubs this helper, so it does not directly verify the new filtering behavior.
  • Consider adding focused coverage for the routing_plugin.js call sites, since those are the other production paths changed to depend on the new helper.

Verdict

APPROVE


Repo: firewalla/firerouter
PR: #1928
Head SHA: ccc2b3460f49b42865f666ce1adf2d77c685d671
Checked at: 2026-06-18 17:17:55 CST

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.

3 participants