diff --git a/HACKING.md b/HACKING.md index bf56c270..ae987af6 100644 --- a/HACKING.md +++ b/HACKING.md @@ -1,8 +1,9 @@ # *AdGuardHome* Developer Guidelines -As of **2020-11-20**, this document is still a work-in-progress. Some of the -rules aren't enforced, and others might change. Still, this is a good place to -find out about how we **want** our code to look like. +As of **2020-11-27**, this document is a work-in-progress, but should still be +followed. Some of the rules aren't enforced as thoroughly or remain broken in +old code, but this is still the place to find out about what we **want** our +code to look like. The rules are mostly sorted in the alphabetical order. @@ -31,27 +32,17 @@ The rules are mostly sorted in the alphabetical order. ## *Go* - * . + ### Code And Naming - * . - - * - - * Add an empty line before `break`, `continue`, and `return`, unless it's the - only statement in that block. + * Avoid `goto`. * Avoid `init` and use explicit initialization functions instead. * Avoid `new`, especially with structs. - * Document everything, including unexported top-level identifiers, to build - a habit of writing documentation. - * Constructors should validate their arguments and return meaningful errors. As a corollary, avoid lazy initialization. - * Don't put variable names into any kind of quotes. - * Don't use naked `return`s. * Don't use underscores in file and package names, unless they're build tags @@ -76,25 +67,34 @@ The rules are mostly sorted in the alphabetical order. * Name the deferred errors (e.g. when closing something) `cerr`. - * No `goto`. - * No shadowing, since it can often lead to subtle bugs, especially with errors. * Prefer constants to variables where possible. Reduce global variables. Use [constant errors] instead of `errors.New`. - * Put comments above the documented entity, **not** to the side, to improve - readability. - - * Use `gofumpt --extra -s`. - - **TODO(a.garipov):** Add to the linters. - * Use linters. * Use named returns to improve readability of function signatures. + * Write logs and error messages in lowercase only to make it easier to `grep` + logs and error messages without using the `-i` flag. + +[constant errors]: https://dave.cheney.net/2016/04/07/constant-errors +[Linus said]: https://www.kernel.org/doc/html/v4.17/process/coding-style.html#indentation + + ### Commenting + + * See also the *Text, Including Comments* section below. + + * Document everything, including unexported top-level identifiers, to build + a habit of writing documentation. + + * Don't put identifiers into any kind of quotes. + + * Put comments above the documented entity, **not** to the side, to improve + readability. + * When a method implements an interface, start the doc comment with the standard template: @@ -105,8 +105,14 @@ The rules are mostly sorted in the alphabetical order. } ``` - * Write logs and error messages in lowercase only to make it easier to `grep` - logs and error messages without using the `-i` flag. + ### Formatting + + * Add an empty line before `break`, `continue`, `fallthrough`, and `return`, + unless it's the only statement in that block. + + * Use `gofumpt --extra -s`. + + **TODO(a.garipov):** Add to the linters. * Write slices of struct like this: @@ -123,8 +129,13 @@ The rules are mostly sorted in the alphabetical order. }} ``` -[constant errors]: https://dave.cheney.net/2016/04/07/constant-errors -[Linus said]: https://www.kernel.org/doc/html/v4.17/process/coding-style.html#indentation + ### Recommended Reading + + * . + + * . + + * ## *Markdown* diff --git a/internal/dhcpd/check_other_dhcp.go b/internal/dhcpd/check_other_dhcp.go index e77a7801..19512686 100644 --- a/internal/dhcpd/check_other_dhcp.go +++ b/internal/dhcpd/check_other_dhcp.go @@ -26,7 +26,7 @@ func CheckIfOtherDHCPServersPresentV4(ifaceName string) (bool, error) { return false, fmt.Errorf("couldn't find interface by name %s: %w", ifaceName, err) } - ifaceIPNet, err := ifaceIPv4Addrs(iface) + ifaceIPNet, err := ifaceIPAddrs(iface, ipVersion4) if err != nil { return false, fmt.Errorf("getting ipv4 addrs for iface %s: %w", ifaceName, err) } @@ -161,7 +161,7 @@ func CheckIfOtherDHCPServersPresentV6(ifaceName string) (bool, error) { return false, fmt.Errorf("dhcpv6: net.InterfaceByName: %s: %w", ifaceName, err) } - ifaceIPNet, err := ifaceIPv6Addrs(iface) + ifaceIPNet, err := ifaceIPAddrs(iface, ipVersion6) if err != nil { return false, fmt.Errorf("getting ipv6 addrs for iface %s: %w", ifaceName, err) } diff --git a/internal/dhcpd/v4.go b/internal/dhcpd/v4.go index d88272e4..686ee32f 100644 --- a/internal/dhcpd/v4.go +++ b/internal/dhcpd/v4.go @@ -16,7 +16,9 @@ import ( "github.com/insomniacslk/dhcp/dhcpv4/server4" ) -// v4Server - DHCPv4 server +// v4Server is a DHCPv4 server. +// +// TODO(a.garipov): Think about unifying this and v6Server. type v4Server struct { srv *server4.Server leasesLock sync.Mutex @@ -560,27 +562,6 @@ func (s *v4Server) packetHandler(conn net.PacketConn, peer net.Addr, req *dhcpv4 } } -// ifaceIPv4Addrs returns the interface's IPv4 addresses. -func ifaceIPv4Addrs(iface *net.Interface) (ips []net.IP, err error) { - addrs, err := iface.Addrs() - if err != nil { - return nil, err - } - - for _, a := range addrs { - ipnet, ok := a.(*net.IPNet) - if !ok { - continue - } - - if ip := ipnet.IP.To4(); ip != nil { - ips = append(ips, ip) - } - } - - return ips, nil -} - // Start starts the IPv4 DHCP server. func (s *v4Server) Start() error { if !s.conf.Enabled { @@ -595,26 +576,9 @@ func (s *v4Server) Start() error { log.Debug("dhcpv4: starting...") - dnsIPAddrs, err := ifaceIPv4Addrs(iface) + dnsIPAddrs, err := ifaceDNSIPAddrs(iface, ipVersion4, defaultMaxAttempts, defaultBackoff) if err != nil { - return fmt.Errorf("dhcpv4: getting ipv4 addrs for iface %s: %w", ifaceName, err) - } - - switch len(dnsIPAddrs) { - case 0: - log.Debug("dhcpv4: no ipv4 address for interface %s", iface.Name) - - return nil - case 1: - // Some Android devices use 8.8.8.8 if there is no secondary DNS - // server. Fix that by setting the secondary DNS address to our - // address as well. - // - // See https://github.com/AdguardTeam/AdGuardHome/issues/1708. - log.Debug("dhcpv4: setting secondary dns ip to iself for interface %s", iface.Name) - dnsIPAddrs = append(dnsIPAddrs, dnsIPAddrs[0]) - default: - // Go on. + return fmt.Errorf("dhcpv4: interface %s: %w", ifaceName, err) } s.conf.dnsIPAddrs = dnsIPAddrs diff --git a/internal/dhcpd/v46.go b/internal/dhcpd/v46.go new file mode 100644 index 00000000..c8301875 --- /dev/null +++ b/internal/dhcpd/v46.go @@ -0,0 +1,123 @@ +package dhcpd + +import ( + "fmt" + "net" + "time" + + "github.com/AdguardTeam/golibs/log" +) + +// ipVersion is a documentational alias for int. Use it when the integer means +// IP version. +type ipVersion = int + +// IP version constants. +const ( + ipVersion4 ipVersion = 4 + ipVersion6 ipVersion = 6 +) + +// netIface is the interface for network interface methods. +type netIface interface { + Addrs() ([]net.Addr, error) +} + +// ifaceIPAddrs returns the interface's IP addresses. +func ifaceIPAddrs(iface netIface, ipv ipVersion) (ips []net.IP, err error) { + addrs, err := iface.Addrs() + if err != nil { + return nil, err + } + + for _, a := range addrs { + var ip net.IP + switch a := a.(type) { + case *net.IPAddr: + ip = a.IP + case *net.IPNet: + ip = a.IP + default: + continue + } + + // Assume that net.(*Interface).Addrs can only return valid IPv4 + // and IPv6 addresses. Thus, if it isn't an IPv4 address, it + // must be an IPv6 one. + switch ipv { + case ipVersion4: + if ip4 := ip.To4(); ip4 != nil { + ips = append(ips, ip4) + } + case ipVersion6: + if ip6 := ip.To4(); ip6 == nil { + ips = append(ips, ip) + } + default: + return nil, fmt.Errorf("invalid ip version %d", ipv) + } + } + + return ips, nil +} + +// Currently used defaults for ifaceDNSAddrs. +const ( + defaultMaxAttempts int = 10 + + defaultBackoff time.Duration = 500 * time.Millisecond +) + +// ifaceDNSIPAddrs returns IP addresses of the interface suitable to send to +// clients as DNS addresses. If err is nil, addrs contains either no addresses +// or at least two. +// +// It makes up to maxAttempts attempts to get the addresses if there are none, +// each time using the provided backoff. Sometimes an interface needs a few +// seconds to really ititialize. +// +// See https://github.com/AdguardTeam/AdGuardHome/issues/2304. +func ifaceDNSIPAddrs( + iface netIface, + ipv ipVersion, + maxAttempts int, + backoff time.Duration, +) (addrs []net.IP, err error) { + var n int +waitForIP: + for n = 1; n <= maxAttempts; n++ { + addrs, err = ifaceIPAddrs(iface, ipv) + if err != nil { + return nil, fmt.Errorf("getting ip addrs: %w", err) + } + + switch len(addrs) { + case 0: + log.Debug("dhcpv%d: attempt %d: no ip addresses", ipv, n) + + time.Sleep(backoff) + case 1: + // Some Android devices use 8.8.8.8 if there is not + // a secondary DNS server. Fix that by setting the + // secondary DNS address to the same address. + // + // See https://github.com/AdguardTeam/AdGuardHome/issues/1708. + log.Debug("dhcpv%d: setting secondary dns ip to itself", ipv) + addrs = append(addrs, addrs[0]) + + fallthrough + default: + break waitForIP + } + } + + if len(addrs) == 0 { + // Don't return errors in case the users want to try and enable + // the DHCP server later. + log.Error("dhcpv%d: no ip address for interface after %d attempts and %s", ipv, n, time.Duration(n)*backoff) + } else { + log.Debug("dhcpv%d: got addresses %s after %d attempts", ipv, addrs, n) + } + + return addrs, nil +} diff --git a/internal/dhcpd/v46_test.go b/internal/dhcpd/v46_test.go new file mode 100644 index 00000000..6007205d --- /dev/null +++ b/internal/dhcpd/v46_test.go @@ -0,0 +1,189 @@ +package dhcpd + +import ( + "errors" + "net" + "testing" + + "github.com/AdguardTeam/AdGuardHome/internal/agherr" + "github.com/stretchr/testify/assert" +) + +type fakeIface struct { + addrs []net.Addr + err error +} + +// Addrs implements the netIface interface for *fakeIface. +func (iface *fakeIface) Addrs() (addrs []net.Addr, err error) { + if iface.err != nil { + return nil, iface.err + } + + return iface.addrs, nil +} + +func TestIfaceIPAddrs(t *testing.T) { + const errTest agherr.Error = "test error" + + ip4 := net.IP{1, 2, 3, 4} + addr4 := &net.IPNet{IP: ip4} + + ip6 := net.IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6} + addr6 := &net.IPNet{IP: ip6} + + testCases := []struct { + name string + iface netIface + ipv ipVersion + want []net.IP + wantErr error + }{{ + name: "ipv4_success", + iface: &fakeIface{addrs: []net.Addr{addr4}, err: nil}, + ipv: ipVersion4, + want: []net.IP{ip4}, + wantErr: nil, + }, { + name: "ipv4_success_with_ipv6", + iface: &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil}, + ipv: ipVersion4, + want: []net.IP{ip4}, + wantErr: nil, + }, { + name: "ipv4_error", + iface: &fakeIface{addrs: []net.Addr{addr4}, err: errTest}, + ipv: ipVersion4, + want: nil, + wantErr: errTest, + }, { + name: "ipv6_success", + iface: &fakeIface{addrs: []net.Addr{addr6}, err: nil}, + ipv: ipVersion6, + want: []net.IP{ip6}, + wantErr: nil, + }, { + name: "ipv6_success_with_ipv4", + iface: &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil}, + ipv: ipVersion6, + want: []net.IP{ip6}, + wantErr: nil, + }, { + name: "ipv6_error", + iface: &fakeIface{addrs: []net.Addr{addr6}, err: errTest}, + ipv: ipVersion6, + want: nil, + wantErr: errTest, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, gotErr := ifaceIPAddrs(tc.iface, tc.ipv) + assert.Equal(t, tc.want, got) + assert.True(t, errors.Is(gotErr, tc.wantErr)) + }) + } +} + +type waitingFakeIface struct { + addrs []net.Addr + err error + n int +} + +// Addrs implements the netIface interface for *waitingFakeIface. +func (iface *waitingFakeIface) Addrs() (addrs []net.Addr, err error) { + if iface.err != nil { + return nil, iface.err + } + + if iface.n == 0 { + return iface.addrs, nil + } + + iface.n-- + + return nil, nil +} + +func TestIfaceDNSIPAddrs(t *testing.T) { + const errTest agherr.Error = "test error" + + ip4 := net.IP{1, 2, 3, 4} + addr4 := &net.IPNet{IP: ip4} + + ip6 := net.IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6} + addr6 := &net.IPNet{IP: ip6} + + testCases := []struct { + name string + iface netIface + ipv ipVersion + want []net.IP + wantErr error + }{{ + name: "ipv4_success", + iface: &fakeIface{addrs: []net.Addr{addr4}, err: nil}, + ipv: ipVersion4, + want: []net.IP{ip4, ip4}, + wantErr: nil, + }, { + name: "ipv4_success_with_ipv6", + iface: &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil}, + ipv: ipVersion4, + want: []net.IP{ip4, ip4}, + wantErr: nil, + }, { + name: "ipv4_error", + iface: &fakeIface{addrs: []net.Addr{addr4}, err: errTest}, + ipv: ipVersion4, + want: nil, + wantErr: errTest, + }, { + name: "ipv4_wait", + iface: &waitingFakeIface{ + addrs: []net.Addr{addr4}, + err: nil, + n: 1, + }, + ipv: ipVersion4, + want: []net.IP{ip4, ip4}, + wantErr: nil, + }, { + name: "ipv6_success", + iface: &fakeIface{addrs: []net.Addr{addr6}, err: nil}, + ipv: ipVersion6, + want: []net.IP{ip6, ip6}, + wantErr: nil, + }, { + name: "ipv6_success_with_ipv4", + iface: &fakeIface{addrs: []net.Addr{addr6, addr4}, err: nil}, + ipv: ipVersion6, + want: []net.IP{ip6, ip6}, + wantErr: nil, + }, { + name: "ipv6_error", + iface: &fakeIface{addrs: []net.Addr{addr6}, err: errTest}, + ipv: ipVersion6, + want: nil, + wantErr: errTest, + }, { + name: "ipv6_wait", + iface: &waitingFakeIface{ + addrs: []net.Addr{addr6}, + err: nil, + n: 1, + }, + ipv: ipVersion6, + want: []net.IP{ip6, ip6}, + wantErr: nil, + }} + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + got, gotErr := ifaceDNSIPAddrs(tc.iface, tc.ipv, 2, 0) + assert.Equal(t, tc.want, got) + assert.True(t, errors.Is(gotErr, tc.wantErr)) + }) + } +} diff --git a/internal/dhcpd/v6.go b/internal/dhcpd/v6.go index f1ec57b3..300788e6 100644 --- a/internal/dhcpd/v6.go +++ b/internal/dhcpd/v6.go @@ -17,7 +17,9 @@ import ( const valueIAID = "ADGH" // value for IANA.ID -// v6Server - DHCPv6 server +// v6Server is a DHCPv6 server. +// +// TODO(a.garipov): Think about unifying this and v4Server. type v6Server struct { srv *server6.Server leasesLock sync.Mutex @@ -537,34 +539,6 @@ func (s *v6Server) packetHandler(conn net.PacketConn, peer net.Addr, req dhcpv6. } } -type netIface interface { - Addrs() ([]net.Addr, error) -} - -// ifaceIPv6Addrs returns the interface's IPv6 addresses. -func ifaceIPv6Addrs(iface netIface) (ips []net.IP, err error) { - addrs, err := iface.Addrs() - if err != nil { - return nil, err - } - - for _, a := range addrs { - ipnet, ok := a.(*net.IPNet) - if !ok { - continue - } - - if ip := ipnet.IP.To4(); ip == nil { - // Assume that net.(*Interface).Addrs can only return - // valid IPv4 and IPv6 addresses. Since this isn't an - // IPv4 address, it must be an IPv6 one. - ips = append(ips, ipnet.IP) - } - } - - return ips, nil -} - // initialize RA module func (s *v6Server) initRA(iface *net.Interface) error { // choose the source IP address - should be link-local-unicast @@ -598,24 +572,11 @@ func (s *v6Server) Start() error { return fmt.Errorf("dhcpv6: finding interface %s by name: %w", ifaceName, err) } - log.Debug("dhcpv4: starting...") + log.Debug("dhcpv6: starting...") - dnsIPAddrs, err := ifaceIPv6Addrs(iface) + dnsIPAddrs, err := ifaceDNSIPAddrs(iface, ipVersion6, defaultMaxAttempts, defaultBackoff) if err != nil { - return fmt.Errorf("dhcpv6: getting ipv6 addrs for iface %s: %w", ifaceName, err) - } - - switch len(dnsIPAddrs) { - case 0: - log.Debug("dhcpv6: no ipv6 address for interface %s", iface.Name) - - return nil - case 1: - // See the comment in (*v4Server).Start. - log.Debug("dhcpv6: setting secondary dns ip to iself for interface %s", iface.Name) - dnsIPAddrs = append(dnsIPAddrs, dnsIPAddrs[0]) - default: - // Go on. + return fmt.Errorf("dhcpv6: interface %s: %w", ifaceName, err) } s.conf.dnsIPAddrs = dnsIPAddrs @@ -631,7 +592,7 @@ func (s *v6Server) Start() error { return nil } - log.Debug("DHCPv6: starting...") + log.Debug("dhcpv6: listening...") if len(iface.HardwareAddr) != 6 { return fmt.Errorf("dhcpv6: invalid MAC %s", iface.HardwareAddr) diff --git a/internal/dhcpd/v6_test.go b/internal/dhcpd/v6_test.go index d51c695c..7d7dd678 100644 --- a/internal/dhcpd/v6_test.go +++ b/internal/dhcpd/v6_test.go @@ -6,7 +6,6 @@ import ( "net" "testing" - "github.com/AdguardTeam/AdGuardHome/internal/agherr" "github.com/insomniacslk/dhcp/dhcpv6" "github.com/insomniacslk/dhcp/iana" "github.com/stretchr/testify/assert" @@ -224,57 +223,3 @@ func TestV6GetDynamicLease(t *testing.T) { assert.True(t, ip6InRange(net.ParseIP("2001::2"), net.ParseIP("2001::2"))) assert.True(t, ip6InRange(net.ParseIP("2001::2"), net.ParseIP("2001::3"))) } - -type fakeIface struct { - addrs []net.Addr - err error -} - -// Addrs implements the netIface interface for *fakeIface. -func (iface *fakeIface) Addrs() (addrs []net.Addr, err error) { - if iface.err != nil { - return nil, iface.err - } - - return iface.addrs, nil -} - -func TestIfaceIPv6Addrs(t *testing.T) { - ip := net.IP{1, 2, 3, 4, 5, 6, 7, 8, 9, 0, 1, 2, 3, 4, 5, 6} - ip4 := net.IP{1, 2, 3, 4} - addr := &net.IPNet{IP: ip} - errTest := agherr.Error("test error") - - testCases := []struct { - name string - iface netIface - want []net.IP - wantErr error - }{{ - name: "success", - iface: &fakeIface{addrs: []net.Addr{addr}, err: nil}, - want: []net.IP{ip}, - wantErr: nil, - }, { - name: "success_with_ipv4", - iface: &fakeIface{ - addrs: []net.Addr{addr, &net.IPNet{IP: ip4}}, - err: nil, - }, - want: []net.IP{ip}, - wantErr: nil, - }, { - name: "error", - iface: &fakeIface{addrs: []net.Addr{addr}, err: errTest}, - want: nil, - wantErr: errTest, - }} - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - got, gotErr := ifaceIPv6Addrs(tc.iface) - assert.Equal(t, tc.want, got) - assert.Equal(t, tc.wantErr, gotErr) - }) - } -}