diff --git a/CHANGELOG.md b/CHANGELOG.md index 393a5e59..b7e8f4d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,4 +20,6 @@ and this project adheres to ### Fixed -- Infinite loop in `/dhcp/find_active_dhcp` (#2301). \ No newline at end of file +- `404 Not Found` errors on the DHCP settings page on *Windows*. The page now + correctly shows that DHCP is not currently available on that OS (#2295). +- Infinite loop in `/dhcp/find_active_dhcp` (#2301). diff --git a/client/src/actions/index.js b/client/src/actions/index.js index efd432fd..04054091 100644 --- a/client/src/actions/index.js +++ b/client/src/actions/index.js @@ -373,10 +373,14 @@ export const getDhcpStatusFailure = createAction('GET_DHCP_STATUS_FAILURE'); export const getDhcpStatus = () => async (dispatch) => { dispatch(getDhcpStatusRequest()); try { - const status = await apiClient.getDhcpStatus(); const globalStatus = await apiClient.getGlobalStatus(); - status.dhcp_available = globalStatus.dhcp_available; - dispatch(getDhcpStatusSuccess(status)); + if (globalStatus.dhcp_available) { + const status = await apiClient.getDhcpStatus(); + status.dhcp_available = globalStatus.dhcp_available; + dispatch(getDhcpStatusSuccess(status)); + } else { + dispatch(getDhcpStatusFailure()); + } } catch (error) { dispatch(addErrorToast({ error })); dispatch(getDhcpStatusFailure()); diff --git a/client/src/components/Settings/Dhcp/Interfaces.js b/client/src/components/Settings/Dhcp/Interfaces.js index dcf5e3fa..987a84a9 100644 --- a/client/src/components/Settings/Dhcp/Interfaces.js +++ b/client/src/components/Settings/Dhcp/Interfaces.js @@ -74,28 +74,28 @@ const Interfaces = () => { const interfaceValue = interface_name && interfaces[interface_name]; - return !processingInterfaces - && interfaces - && <> -
-
- - - {renderInterfaces(interfaces)} - -
- {interfaceValue - && renderInterfaceValues(interfaceValue)} -
- ; + if (processingInterfaces || !interfaces) { + return null; + } + + return
+
+ + + {renderInterfaces(interfaces)} + +
+ {interfaceValue + && renderInterfaceValues(interfaceValue)} +
; }; renderInterfaceValues.propTypes = { diff --git a/client/src/components/Settings/Dhcp/index.js b/client/src/components/Settings/Dhcp/index.js index 05732c19..3be4efdb 100644 --- a/client/src/components/Settings/Dhcp/index.js +++ b/client/src/components/Settings/Dhcp/index.js @@ -65,9 +65,14 @@ const Dhcp = () => { useEffect(() => { dispatch(getDhcpStatus()); - dispatch(getDhcpInterfaces()); }, []); + useEffect(() => { + if (dhcp_available) { + dispatch(getDhcpInterfaces()); + } + }, [dhcp_available]); + useEffect(() => { const [ipv4] = interfaces?.[interface_name]?.ipv4_addresses ?? []; const [ipv6] = interfaces?.[interface_name]?.ipv6_addresses ?? []; diff --git a/internal/dhcpd/check_other_dhcp.go b/internal/dhcpd/check_other_dhcp.go index 674949aa..aba9e446 100644 --- a/internal/dhcpd/check_other_dhcp.go +++ b/internal/dhcpd/check_other_dhcp.go @@ -86,7 +86,7 @@ func CheckIfOtherDHCPServersPresentV4(ifaceName string) (bool, error) { } for { - ok, next, err := tryConn(req, c, iface) + ok, next, err := tryConn4(req, c, iface) if next { if err != nil { log.Debug("dhcpv4: trying a connection: %s", err) @@ -103,12 +103,12 @@ func CheckIfOtherDHCPServersPresentV4(ifaceName string) (bool, error) { } } -// TODO(a.garipov): Refactor further. Inspect error handling, remove the next -// parameter, address the TODO, etc. -func tryConn(req *dhcpv4.DHCPv4, c net.PacketConn, iface *net.Interface) (ok, next bool, err error) { +// TODO(a.garipov): Refactor further. Inspect error handling, remove parameter +// next, address the TODO, merge with tryConn6, etc. +func tryConn4(req *dhcpv4.DHCPv4, c net.PacketConn, iface *net.Interface) (ok, next bool, err error) { // TODO: replicate dhclient's behavior of retrying several times with // progressively longer timeouts. - log.Tracef("waiting %v for an answer", defaultDiscoverTime) + log.Tracef("dhcpv4: waiting %v for an answer", defaultDiscoverTime) b := make([]byte, 1500) err = c.SetDeadline(time.Now().Add(defaultDiscoverTime)) @@ -127,7 +127,7 @@ func tryConn(req *dhcpv4.DHCPv4, c net.PacketConn, iface *net.Interface) (ok, ne return false, false, fmt.Errorf("receiving packet: %w", err) } - log.Tracef("received packet, %d bytes", n) + log.Tracef("dhcpv4: received packet, %d bytes", n) response, err := dhcpv4.FromBytes(b[:n]) if err != nil { @@ -149,7 +149,7 @@ func tryConn(req *dhcpv4.DHCPv4, c net.PacketConn, iface *net.Interface) (ok, ne return false, true, nil } - log.Tracef("the packet is from an active dhcp server") + log.Tracef("dhcpv4: the packet is from an active dhcp server") return true, false, nil } @@ -208,43 +208,77 @@ func CheckIfOtherDHCPServersPresentV6(ifaceName string) (bool, error) { } for { - log.Debug("DHCPv6: Waiting %v for an answer", defaultDiscoverTime) - b := make([]byte, 4096) - _ = c.SetReadDeadline(time.Now().Add(defaultDiscoverTime)) - n, _, err := c.ReadFrom(b) - if isTimeout(err) { - log.Debug("DHCPv6: didn't receive DHCP response") - return false, nil - } - if err != nil { - return false, fmt.Errorf("couldn't receive packet: %w", err) - } + ok, next, err := tryConn6(req, c) + if next { + if err != nil { + log.Debug("dhcpv6: trying a connection: %s", err) + } - log.Debug("DHCPv6: Received packet (%v bytes)", n) - - resp, err := dhcpv6.FromBytes(b[:n]) - if err != nil { - log.Debug("DHCPv6: dhcpv6.FromBytes: %s", err) continue } - - log.Debug("DHCPv6: received message from server: %s", resp.Summary()) - - cid := req.Options.ClientID() - msg, err := resp.GetInnerMessage() - if err != nil { - log.Debug("DHCPv6: resp.GetInnerMessage: %s", err) - continue - } - rcid := msg.Options.ClientID() - if resp.Type() == dhcpv6.MessageTypeAdvertise && - msg.TransactionID == req.TransactionID && - rcid != nil && - cid.Equal(*rcid) { - log.Debug("DHCPv6: The packet is from an active DHCP server") + if ok { return true, nil } - - log.Debug("DHCPv6: received message from server doesn't match our request") + if err != nil { + return false, err + } } } + +// TODO(a.garipov): See the comment on tryConn4. Sigh… +func tryConn6(req *dhcpv6.Message, c net.PacketConn) (ok, next bool, err error) { + // TODO: replicate dhclient's behavior of retrying several times with + // progressively longer timeouts. + log.Tracef("dhcpv6: waiting %v for an answer", defaultDiscoverTime) + + b := make([]byte, 4096) + err = c.SetDeadline(time.Now().Add(defaultDiscoverTime)) + if err != nil { + return false, false, fmt.Errorf("setting deadline: %w", err) + } + + n, _, err := c.ReadFrom(b) + if err != nil { + if isTimeout(err) { + log.Debug("dhcpv6: didn't receive dhcp response") + + return false, false, nil + } + + return false, false, fmt.Errorf("receiving packet: %w", err) + } + + log.Tracef("dhcpv6: received packet, %d bytes", n) + + response, err := dhcpv6.FromBytes(b[:n]) + if err != nil { + log.Debug("dhcpv6: encoding: %s", err) + + return false, true, err + } + + log.Debug("dhcpv6: received message from server: %s", response.Summary()) + + cid := req.Options.ClientID() + msg, err := response.GetInnerMessage() + if err != nil { + log.Debug("dhcpv6: resp.GetInnerMessage(): %s", err) + + return false, true, err + } + + rcid := msg.Options.ClientID() + if !(response.Type() == dhcpv6.MessageTypeAdvertise && + msg.TransactionID == req.TransactionID && + rcid != nil && + cid.Equal(*rcid)) { + + log.Debug("dhcpv6: received message from server doesn't match our request") + + return false, true, nil + } + + log.Tracef("dhcpv6: the packet is from an active dhcp server") + + return true, false, nil +} diff --git a/internal/dhcpd/dhcpd.go b/internal/dhcpd/dhcpd.go index f678388e..999e9c7a 100644 --- a/internal/dhcpd/dhcpd.go +++ b/internal/dhcpd/dhcpd.go @@ -1,3 +1,4 @@ +// Package dhcpd provides a DHCP server. package dhcpd import ( @@ -5,6 +6,7 @@ import ( "net" "net/http" "path/filepath" + "runtime" "strconv" "strings" "time" @@ -13,8 +15,10 @@ import ( "github.com/AdguardTeam/golibs/log" ) -const defaultDiscoverTime = time.Second * 3 -const leaseExpireStatic = 1 +const ( + defaultDiscoverTime = time.Second * 3 + leaseExpireStatic = 1 +) var webHandlersRegistered = false @@ -82,7 +86,8 @@ func (s *Server) CheckConfig(config ServerConfig) error { // Create - create object func Create(config ServerConfig) *Server { - s := Server{} + s := &Server{} + s.conf.Enabled = config.Enabled s.conf.InterfaceName = config.InterfaceName s.conf.HTTPRegister = config.HTTPRegister @@ -90,8 +95,21 @@ func Create(config ServerConfig) *Server { s.conf.DBFilePath = filepath.Join(config.WorkDir, dbFilename) if !webHandlersRegistered && s.conf.HTTPRegister != nil { + if runtime.GOOS == "windows" { + // Our DHCP server doesn't work on Windows yet, so + // signal that to the front with an HTTP 501. + // + // TODO(a.garipov): This needs refactoring. We + // shouldn't even try and initialize a DHCP server on + // Windows, but there are currently too many + // interconnected parts--such as HTTP handlers and + // frontend--to make that work properly. + s.registerNotImplementedHandlers() + } else { + s.registerHandlers() + } + webHandlersRegistered = true - s.registerHandlers() } var err4, err6 error @@ -130,7 +148,7 @@ func Create(config ServerConfig) *Server { // we can't delay database loading until DHCP server is started, // because we need static leases functionality available beforehand s.dbLoad() - return &s + return s } // server calls this function after DB is updated diff --git a/internal/dhcpd/dhcp_http.go b/internal/dhcpd/dhcphttp.go similarity index 85% rename from internal/dhcpd/dhcp_http.go rename to internal/dhcpd/dhcphttp.go index c91d5d64..f4ce801b 100644 --- a/internal/dhcpd/dhcp_http.go +++ b/internal/dhcpd/dhcphttp.go @@ -11,7 +11,6 @@ import ( "time" "github.com/AdguardTeam/AdGuardHome/internal/util" - "github.com/AdguardTeam/golibs/jsonutil" "github.com/AdguardTeam/golibs/log" ) @@ -499,11 +498,48 @@ func (s *Server) handleReset(w http.ResponseWriter, r *http.Request) { } func (s *Server) registerHandlers() { - s.conf.HTTPRegister("GET", "/control/dhcp/status", s.handleDHCPStatus) - s.conf.HTTPRegister("GET", "/control/dhcp/interfaces", s.handleDHCPInterfaces) - s.conf.HTTPRegister("POST", "/control/dhcp/set_config", s.handleDHCPSetConfig) - s.conf.HTTPRegister("POST", "/control/dhcp/find_active_dhcp", s.handleDHCPFindActiveServer) - s.conf.HTTPRegister("POST", "/control/dhcp/add_static_lease", s.handleDHCPAddStaticLease) - s.conf.HTTPRegister("POST", "/control/dhcp/remove_static_lease", s.handleDHCPRemoveStaticLease) - s.conf.HTTPRegister("POST", "/control/dhcp/reset", s.handleReset) + s.conf.HTTPRegister(http.MethodGet, "/control/dhcp/status", s.handleDHCPStatus) + s.conf.HTTPRegister(http.MethodGet, "/control/dhcp/interfaces", s.handleDHCPInterfaces) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/set_config", s.handleDHCPSetConfig) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/find_active_dhcp", s.handleDHCPFindActiveServer) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/add_static_lease", s.handleDHCPAddStaticLease) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/remove_static_lease", s.handleDHCPRemoveStaticLease) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/reset", s.handleReset) +} + +// jsonError is a generic JSON error response. +type jsonError struct { + // Message is the error message, an opaque string. + Message string `json:"message"` +} + +// notImplemented returns a handler that replies to any request with an HTTP 501 +// Not Implemented status and a JSON error with the provided message msg. +// +// TODO(a.garipov): Either take the logger from the server after we've +// refactored logging or make this not a method of *Server. +func (s *Server) notImplemented(msg string) (f func(http.ResponseWriter, *http.Request)) { + return func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotImplemented) + + err := json.NewEncoder(w).Encode(&jsonError{ + Message: msg, + }) + if err != nil { + log.Debug("writing 501 json response: %s", err) + } + } +} + +func (s *Server) registerNotImplementedHandlers() { + h := s.notImplemented("dhcp is not supported on windows") + + s.conf.HTTPRegister(http.MethodGet, "/control/dhcp/status", h) + s.conf.HTTPRegister(http.MethodGet, "/control/dhcp/interfaces", h) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/set_config", h) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/find_active_dhcp", h) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/add_static_lease", h) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/remove_static_lease", h) + s.conf.HTTPRegister(http.MethodPost, "/control/dhcp/reset", h) } diff --git a/internal/dhcpd/dhcphttp_test.go b/internal/dhcpd/dhcphttp_test.go new file mode 100644 index 00000000..47b926dc --- /dev/null +++ b/internal/dhcpd/dhcphttp_test.go @@ -0,0 +1,22 @@ +package dhcpd + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestServer_notImplemented(t *testing.T) { + s := &Server{} + h := s.notImplemented("never!") + + w := httptest.NewRecorder() + r, err := http.NewRequest(http.MethodGet, "/unsupported", nil) + assert.Nil(t, err) + + h(w, r) + assert.Equal(t, http.StatusNotImplemented, w.Code) + assert.Equal(t, `{"message":"never!"}`+"\n", w.Body.String()) +} diff --git a/internal/dhcpd/network_utils.go b/internal/dhcpd/network_utils.go index 487196cc..41a1d7ec 100644 --- a/internal/dhcpd/network_utils.go +++ b/internal/dhcpd/network_utils.go @@ -17,7 +17,8 @@ import ( "github.com/AdguardTeam/golibs/log" ) -// Check if network interface has a static IP configured +// HasStaticIP check if the network interface has a static IP configured +// // Supports: Raspbian. func HasStaticIP(ifaceName string) (bool, error) { if runtime.GOOS == "linux" { @@ -36,7 +37,7 @@ func HasStaticIP(ifaceName string) (bool, error) { return false, fmt.Errorf("cannot check if IP is static: not supported on %s", runtime.GOOS) } -// Set a static IP for the specified network interface +// SetStaticIP sets a static IP for the network interface. func SetStaticIP(ifaceName string) error { if runtime.GOOS == "linux" { return setStaticIPDhcpdConf(ifaceName) diff --git a/internal/home/control.go b/internal/home/control.go index bb52a479..00334b62 100644 --- a/internal/home/control.go +++ b/internal/home/control.go @@ -6,6 +6,7 @@ import ( "net" "net/http" "net/url" + "runtime" "strconv" "strings" @@ -46,6 +47,7 @@ func handleStatus(w http.ResponseWriter, r *http.Request) { if Context.dnsServer != nil { Context.dnsServer.WriteDiskConfig(&c) } + data := map[string]interface{}{ "dns_addresses": getDNSAddresses(), "http_port": config.BindPort, @@ -56,7 +58,17 @@ func handleStatus(w http.ResponseWriter, r *http.Request) { "protection_enabled": c.ProtectionEnabled, } - data["dhcp_available"] = (Context.dhcpServer != nil) + + if runtime.GOOS == "windows" { + // Set the DHCP to false explicitly, because Context.dhcpServer + // is probably not nil, despite the fact that there is no + // support for DHCP on Windows in AdGuardHome. + // + // See also the TODO in dhcpd.Create. + data["dhcp_available"] = false + } else { + data["dhcp_available"] = (Context.dhcpServer != nil) + } jsonVal, err := json.Marshal(data) if err != nil { diff --git a/internal/home/home.go b/internal/home/home.go index 178de4a7..9b8e6692 100644 --- a/internal/home/home.go +++ b/internal/home/home.go @@ -20,18 +20,16 @@ import ( "syscall" "time" - "gopkg.in/natefinch/lumberjack.v2" - "github.com/AdguardTeam/AdGuardHome/internal/agherr" - "github.com/AdguardTeam/AdGuardHome/internal/update" - "github.com/AdguardTeam/AdGuardHome/internal/util" - "github.com/AdguardTeam/AdGuardHome/internal/dhcpd" "github.com/AdguardTeam/AdGuardHome/internal/dnsfilter" "github.com/AdguardTeam/AdGuardHome/internal/dnsforward" "github.com/AdguardTeam/AdGuardHome/internal/querylog" "github.com/AdguardTeam/AdGuardHome/internal/stats" + "github.com/AdguardTeam/AdGuardHome/internal/update" + "github.com/AdguardTeam/AdGuardHome/internal/util" "github.com/AdguardTeam/golibs/log" + "gopkg.in/natefinch/lumberjack.v2" ) const ( @@ -216,12 +214,12 @@ func run(args options) { config.DHCP.WorkDir = Context.workDir config.DHCP.HTTPRegister = httpRegister config.DHCP.ConfigModified = onConfigModified - if runtime.GOOS != "windows" { - Context.dhcpServer = dhcpd.Create(config.DHCP) - if Context.dhcpServer == nil { - log.Fatalf("Can't initialize DHCP module") - } + + Context.dhcpServer = dhcpd.Create(config.DHCP) + if Context.dhcpServer == nil { + log.Fatalf("can't initialize dhcp module") } + Context.autoHosts.Init("") Context.updater = update.NewUpdater(update.Config{ diff --git a/internal/util/helpers.go b/internal/util/helpers.go index 4b38746e..e023da08 100644 --- a/internal/util/helpers.go +++ b/internal/util/helpers.go @@ -1,3 +1,7 @@ +// Package util contains various utilities. +// +// TODO(a.garipov): Such packages are widely considered an antipattern. Remove +// this when we refactor our project structure. package util import ( diff --git a/openapi/openapi.yaml b/openapi/openapi.yaml index 29e3904c..d42ee24c 100644 --- a/openapi/openapi.yaml +++ b/openapi/openapi.yaml @@ -339,6 +339,12 @@ paths: application/json: schema: $ref: "#/components/schemas/DhcpStatus" + "501": + content: + application/json: + schema: + $ref: "#/components/schemas/Error" + description: Not implemented (for example, on Windows). /dhcp/set_config: post: tags: @@ -353,6 +359,12 @@ paths: responses: "200": description: OK + "501": + content: + application/json: + schema: + $ref: "#/components/schemas/Error" + description: Not implemented (for example, on Windows). /dhcp/find_active_dhcp: post: tags: @@ -366,6 +378,12 @@ paths: application/json: schema: $ref: "#/components/schemas/DhcpSearchResult" + "501": + content: + application/json: + schema: + $ref: "#/components/schemas/Error" + description: Not implemented (for example, on Windows). /dhcp/add_static_lease: post: tags: @@ -377,6 +395,12 @@ paths: responses: "200": description: OK + "501": + content: + application/json: + schema: + $ref: "#/components/schemas/Error" + description: Not implemented (for example, on Windows). /dhcp/remove_static_lease: post: tags: @@ -388,6 +412,12 @@ paths: responses: "200": description: OK + "501": + content: + application/json: + schema: + $ref: "#/components/schemas/Error" + description: Not implemented (for example, on Windows). /dhcp/reset: post: tags: @@ -397,6 +427,12 @@ paths: responses: "200": description: OK + "501": + content: + application/json: + schema: + $ref: "#/components/schemas/Error" + description: Not implemented (for example, on Windows). /filtering/status: get: tags: @@ -1976,3 +2012,10 @@ components: password: type: string description: Password + Error: + description: A generic JSON error response. + properties: + message: + type: string + description: The error message, an opaque string. + type: object