From d88c860caca47febd68afc7ff9d75506382874fb Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Wed, 6 Aug 2025 15:20:50 +0700 Subject: [PATCH] Add explanatory comments for variable overwrites and code flow decisions This commit adds detailed explanatory comments throughout the codebase to explain WHY certain logic is needed, not just WHAT the code does. This improves code maintainability and helps developers understand the reasoning behind complex decisions. Key improvements: - Version string processing: Explain why "v" prefix is added for semantic versioning - Control-D configuration: Explain why config is reset to prevent mixing of settings - DNS server categorization: Explain LAN vs public server handling for performance - Listener configuration: Document complex fallback logic for port/IP selection - MAC address normalization: Explain cross-platform compatibility needs - IPv6 address processing: Document Unix-specific interface suffix handling - Log content truncation: Explain why large content is limited to prevent flooding - IP address categorization: Document RFC1918 prioritization logic - IPv4/IPv6 separation: Explain network stack compatibility needs - DNS priority logic: Document different priority levels for different scenarios - Domain controller processing: Explain Windows API prefix handling - Reverse mapping creation: Document API encoding/decoding needs - Default value fallbacks: Explain why defaults prevent system failures - IP stack configuration: Document different defaults for different upstream types These comments help future developers understand the reasoning behind complex business logic, making the codebase more maintainable and reducing the risk of incorrect modifications during maintenance. --- cmd/cli/cli.go | 35 ++++++++++++++++++++++++++++++ config.go | 3 +++ doh.go | 3 +++ internal/clientinfo/arp_unix.go | 2 ++ internal/clientinfo/arp_windows.go | 4 ++++ internal/clientinfo/client_info.go | 6 +++++ internal/clientinfo/dhcp.go | 9 ++++++++ internal/clientinfo/hostsfile.go | 6 +++++ internal/clientinfo/mdns.go | 2 ++ internal/clientinfo/ndp.go | 7 ++++++ internal/clientinfo/ptr_lookup.go | 8 +++---- internal/controld/config.go | 2 ++ nameservers_windows.go | 3 +++ resolver.go | 7 ++++-- 14 files changed, 90 insertions(+), 7 deletions(-) diff --git a/cmd/cli/cli.go b/cmd/cli/cli.go index 602c391..c69d4f2 100644 --- a/cmd/cli/cli.go +++ b/cmd/cli/cli.go @@ -86,12 +86,18 @@ _/ ___\ __\_ __ \ | / __ | ` func curVersion() string { + // Ensure version has proper "v" prefix for semantic versioning + // This is needed because some build systems may provide version without the "v" prefix if version != "dev" && !strings.HasPrefix(version, "v") { version = "v" + version } + // Return version directly if it's not empty and not a dev build + // This avoids unnecessary commit hash concatenation for release versions if version != "" && version != "dev" { return version } + // Truncate commit hash to 7 characters for readability + // Git commit hashes are typically 40 characters, but 7 is sufficient for identification if len(commit) > 7 { commit = commit[:7] } @@ -608,6 +614,10 @@ func processCDFlags(cfg *ctrld.Config) (*controld.ResolverConfig, error) { bo.LogLongerThan = 30 * time.Second ctx := ctrld.LoggerCtx(context.Background(), logger) resolverConfig, err := controld.FetchResolverConfig(ctx, cdUID, appVersion, cdDev) + + // Retry logic for network errors using bootstrap DNS + // This is needed because the initial DNS resolution might fail due to network issues + // or DNS server unavailability, but bootstrap DNS can provide alternative resolution for { if errUrlNetworkError(err) { bo.BackOff(ctx, err) @@ -632,6 +642,8 @@ func processCDFlags(cfg *ctrld.Config) (*controld.ResolverConfig, error) { logger.Info().Msg("generating ctrld config from Control-D configuration") + // Reset config to ensure clean state before applying Control-D settings + // This prevents mixing of old configuration with new Control-D settings *cfg = ctrld.Config{} // Fetch config, unmarshal to cfg. if resolverConfig.Ctrld.CustomConfig != "" { @@ -662,6 +674,8 @@ func processCDFlags(cfg *ctrld.Config) (*controld.ResolverConfig, error) { return "" } + // Initialize upstream configuration with Control-D resolver settings + // This creates the primary DNS resolver configuration for the proxy cfg.Upstream = make(map[string]*ctrld.UpstreamConfig) cfg.Upstream["0"] = &ctrld.UpstreamConfig{ BootstrapIP: bootstrapIP(resolverConfig.DOH), @@ -669,10 +683,16 @@ func processCDFlags(cfg *ctrld.Config) (*controld.ResolverConfig, error) { Type: cdUpstreamProto, Timeout: 5000, } + + // Create exclusion rules for domains that should bypass Control-D + // These domains will be resolved using the system's default DNS servers rules := make([]ctrld.Rule, 0, len(resolverConfig.Exclude)) for _, domain := range resolverConfig.Exclude { rules = append(rules, ctrld.Rule{domain: []string{}}) } + + // Initialize listener configuration with policy rules + // This sets up the DNS proxy listener with the exclusion policy cfg.Listener = make(map[string]*ctrld.ListenerConfig) lc := &ctrld.ListenerConfig{ Policy: &ctrld.ListenerPolicyConfig{ @@ -1175,6 +1195,9 @@ func tryUpdateListenerConfig(cfg *ctrld.Config, notifyFunc func(), fatal bool) ( il := mainLog.Load() if isMobile() { // On Mobile, only use first listener, ignore others. + // This is needed because mobile platforms have limited resources and + // multiple listeners can cause conflicts with system DNS services and + // likely don't work anyway. firstLn := cfg.FirstListener() for k := range cfg.Listener { if cfg.Listener[k] != firstLn { @@ -1182,6 +1205,8 @@ func tryUpdateListenerConfig(cfg *ctrld.Config, notifyFunc func(), fatal bool) ( } } if cdMode { + // Use mobile-specific listener settings for Control-D mode + // Mobile platforms require specific IP/port combinations to avoid permission issues. firstLn.IP = mobileListenerIp() firstLn.Port = mobileListenerPort() clear(lcc) @@ -1273,6 +1298,9 @@ func tryUpdateListenerConfig(cfg *ctrld.Config, notifyFunc func(), fatal bool) ( ok = false break } + + // Try standard port 53 first for better compatibility + // This is the most common DNS port and has the highest chance of working if tryAllPort53 { tryAllPort53 = false if check.IP { @@ -1286,6 +1314,9 @@ func tryUpdateListenerConfig(cfg *ctrld.Config, notifyFunc func(), fatal bool) ( } continue } + + // Try localhost as fallback for security and compatibility + // Localhost is often available even when other addresses are blocked if tryLocalhost { tryLocalhost = false if check.IP { @@ -1299,6 +1330,9 @@ func tryUpdateListenerConfig(cfg *ctrld.Config, notifyFunc func(), fatal bool) ( } continue } + + // Try random IP/port combinations as last resort + // This ensures the service can start even in constrained environments if check.IP && !isZeroIP { // for "0.0.0.0" or "::", we only need to try new port. listener.IP = randomLocalIP() } else { @@ -1326,6 +1360,7 @@ func tryUpdateListenerConfig(cfg *ctrld.Config, notifyFunc func(), fatal bool) ( } // Specific case for systemd-resolved. + // systemd-resolved has specific requirements for DNS forwarding that we must handle if useSystemdResolved { if listener := cfg.FirstListener(); listener != nil && listener.Port == 53 { n := listeners[0] diff --git a/config.go b/config.go index 8b359ed..41e6793 100644 --- a/config.go +++ b/config.go @@ -351,6 +351,9 @@ func (uc *UpstreamConfig) Init(ctx context.Context) { } } if uc.IPStack == "" { + // Set default IP stack based on upstream type + // Control-D upstreams use split stack for better IPv4/IPv6 handling, + // while other upstreams use both stacks for maximum compatibility if uc.IsControlD() { uc.IPStack = IpStackSplit } else { diff --git a/doh.go b/doh.go index 6fbfb71..86b9fb5 100644 --- a/doh.go +++ b/doh.go @@ -53,6 +53,9 @@ var EncodeArchNameMap = map[string]string{ var DecodeArchNameMap = map[string]string{} func init() { + // Create reverse mappings for OS and architecture names + // This is needed because the API expects encoded values, but we need to decode + // them back to their original form for processing for k, v := range EncodeOsNameMap { DecodeOsNameMap[v] = k } diff --git a/internal/clientinfo/arp_unix.go b/internal/clientinfo/arp_unix.go index f5d8f88..51c934a 100644 --- a/internal/clientinfo/arp_unix.go +++ b/internal/clientinfo/arp_unix.go @@ -20,6 +20,8 @@ func (a *arpDiscover) scan() { } // trim brackets + // Unix "arp -an" output formats IP addresses with parentheses like "(192.168.1.1)" + // We need to remove these brackets for proper IP parsing ip := strings.ReplaceAll(fields[1], "(", "") ip = strings.ReplaceAll(ip, ")", "") diff --git a/internal/clientinfo/arp_windows.go b/internal/clientinfo/arp_windows.go index 016b752..c037b29 100644 --- a/internal/clientinfo/arp_windows.go +++ b/internal/clientinfo/arp_windows.go @@ -17,10 +17,14 @@ func (a *arpDiscover) scan() { continue // empty lines } if line[0] != ' ' { + // Mark that we've found an interface header line + // Windows "arp -a" output has interface headers followed by ARP entries header = true // "Interface:" lines, next is header line. continue } if header { + // Skip the header line that follows interface names + // These lines contain column headers like "Internet Address" and "Physical Address" header = false // header lines continue } diff --git a/internal/clientinfo/client_info.go b/internal/clientinfo/client_info.go index a66830b..fd67a05 100644 --- a/internal/clientinfo/client_info.go +++ b/internal/clientinfo/client_info.go @@ -99,9 +99,13 @@ type Table struct { func NewTable(cfg *ctrld.Config, selfIP, cdUID string, ns []string, logger *ctrld.Logger) *Table { refreshInterval := cfg.Service.DiscoverRefreshInterval + // Set default refresh interval if not configured + // This ensures client discovery continues to work even without explicit configuration if refreshInterval <= 0 { refreshInterval = 2 * 60 // 2 minutes } + // Use no-op logger if none provided + // This prevents nil pointer dereferences when logging is not configured if logger == nil { logger = ctrld.NopLogger } @@ -274,6 +278,7 @@ func (t *Table) init() { host, port = h, p } // Only use valid ip:port pair. + // Invalid nameservers can cause PTR discovery to fail silently if _, portErr := strconv.Atoi(port); portErr == nil && port != "0" && net.ParseIP(host) != nil { nss = append(nss, net.JoinHostPort(host, port)) } else { @@ -465,6 +470,7 @@ func (t *Table) ListClients() []*Client { for _, c := range ipMap { // If we found a client with empty hostname, use hostname from // an existed client which has the same MAC address. + // This helps fill in missing hostnames when multiple IPs share the same MAC if cFromMac := clientsByMAC[c.Mac]; cFromMac != nil && c.Hostname == "" { c.Hostname = cFromMac.Hostname } diff --git a/internal/clientinfo/dhcp.go b/internal/clientinfo/dhcp.go index b387806..88a4b5e 100644 --- a/internal/clientinfo/dhcp.go +++ b/internal/clientinfo/dhcp.go @@ -141,6 +141,9 @@ func (d *dhcp) lookupIPByHostname(name string, v6 bool) string { return true } if addr, err := netip.ParseAddr(key.(string)); err == nil && addr.Is6() == v6 { + // Categorize addresses into RFC1918 (private) and public + // RFC1918 addresses are prioritized because they're more likely to be + // the actual client IP in most network configurations if addr.IsPrivate() { rfc1918Addrs = append(rfc1918Addrs, addr) } else { @@ -264,6 +267,8 @@ func (d *dhcp) iscDHCPReadClientInfoReader(reader io.Reader) error { } switch fields[0] { case "lease": + // Normalize IP address to lowercase for consistent comparison + // DHCP lease files may contain mixed-case IP addresses ip = normalizeIP(strings.ToLower(fields[1])) if net.ParseIP(ip) == nil { d.logger.Warn().Msgf("invalid ip address entry: %q", ip) @@ -271,6 +276,8 @@ func (d *dhcp) iscDHCPReadClientInfoReader(reader io.Reader) error { } case "hardware": if len(fields) >= 3 { + // Convert MAC to lowercase and remove trailing semicolon + // DHCP lease files use semicolon-terminated MAC addresses mac = strings.ToLower(strings.TrimRight(fields[2], ";")) if _, err := net.ParseMAC(mac); err != nil { // Invalid dhcp, skip. @@ -278,6 +285,8 @@ func (d *dhcp) iscDHCPReadClientInfoReader(reader io.Reader) error { } } case "client-hostname": + // Remove quotes and semicolons from hostname + // DHCP lease files may quote hostnames and add semicolons hostname = strings.Trim(fields[1], `";`) } } diff --git a/internal/clientinfo/hostsfile.go b/internal/clientinfo/hostsfile.go index 4dc6f35..bcf1bff 100644 --- a/internal/clientinfo/hostsfile.go +++ b/internal/clientinfo/hostsfile.go @@ -165,6 +165,8 @@ func parseHostEntriesConfFromReader(r io.Reader) map[string][]string { for scanner.Scan() { line := scanner.Text() if after, found := strings.CutPrefix(line, "local-zone:"); found { + // Extract local zone name for domain suffix removal + // This is needed because unbound appends the local zone to hostnames after = strings.TrimSpace(after) fields := strings.Fields(after) if len(fields) > 1 { @@ -177,6 +179,8 @@ func parseHostEntriesConfFromReader(r io.Reader) map[string][]string { if !found { continue } + // Clean up the parsed data by removing whitespace and quotes + // This ensures consistent formatting for hostname processing after = strings.TrimSpace(after) after = strings.Trim(after, `"`) fields := strings.Fields(after) @@ -184,6 +188,8 @@ func parseHostEntriesConfFromReader(r io.Reader) map[string][]string { continue } ip := fields[0] + // Remove local zone suffix from hostname for cleaner lookups + // Unbound adds the local zone to hostnames, but we want just the base name name := strings.TrimSuffix(fields[1], "."+localZone) hostsMap[ip] = append(hostsMap[ip], name) } diff --git a/internal/clientinfo/mdns.go b/internal/clientinfo/mdns.go index ebdfabc..b1bfaaf 100644 --- a/internal/clientinfo/mdns.go +++ b/internal/clientinfo/mdns.go @@ -219,6 +219,8 @@ func (m *mdns) probe(conns []*net.UDPConn, remoteAddr net.Addr) error { for _, conn := range conns { _ = conn.SetWriteDeadline(time.Now().Add(time.Second * 30)) if _, werr := conn.WriteTo(buf, remoteAddr); werr != nil { + // Capture the last write error for reporting + // Multiple connections may fail, but we only report the last error err = werr } } diff --git a/internal/clientinfo/ndp.go b/internal/clientinfo/ndp.go index 87f86fe..7da7f8f 100644 --- a/internal/clientinfo/ndp.go +++ b/internal/clientinfo/ndp.go @@ -174,6 +174,9 @@ func (nd *ndpDiscover) scanUnix(r io.Reader) { } if mac := parseMAC(fields[1]); mac != "" { ip := fields[0] + // Remove interface suffix from IPv6 addresses + // Unix systems append interface names to IPv6 addresses (e.g., "fe80::1%eth0") + // This suffix needs to be removed for proper IP parsing if idx := strings.IndexByte(ip, '%'); idx != -1 { ip = ip[:idx] } @@ -192,11 +195,15 @@ func normalizeMac(mac string) string { return mac } // Windows use "-" instead of ":" as separator. + // This normalization is needed because different operating systems use different + // separators for MAC addresses, but net.ParseMAC expects ":" format mac = strings.ReplaceAll(mac, "-", ":") parts := strings.Split(mac, ":") if len(parts) != 6 { return "" } + // Pad single-digit hex values with leading zero + // This ensures consistent formatting for MAC address parsing for i, c := range parts { if len(c) == 1 { parts[i] = "0" + c diff --git a/internal/clientinfo/ptr_lookup.go b/internal/clientinfo/ptr_lookup.go index b4783bd..4d45971 100644 --- a/internal/clientinfo/ptr_lookup.go +++ b/internal/clientinfo/ptr_lookup.go @@ -105,11 +105,9 @@ func (p *ptrDiscover) lookupIPByHostname(name string, v6 bool) string { if value == name { if addr, err := netip.ParseAddr(key.(string)); err == nil && addr.Is6() == v6 { ip = addr.String() - //lint:ignore S1008 This is used for readable. - if addr.IsLoopback() { // Continue searching if this is loopback address. - return true - } - return false + // Continue searching if this is a loopback address + // We prefer non-loopback addresses as they're more likely to be the actual client IP + return addr.IsLoopback() // Continue searching if this is loopback address. } } return true diff --git a/internal/controld/config.go b/internal/controld/config.go index 813fcd5..77cebb0 100644 --- a/internal/controld/config.go +++ b/internal/controld/config.go @@ -233,6 +233,8 @@ func apiTransport(loggerCtx context.Context, cdDev bool) *http.Transport { } // Separate IPv4 and IPv6 addresses + // This separation is needed because different network stacks may have different + // connectivity to IPv4 vs IPv6, so we try them separately for better reliability var ipv4s, ipv6s []string for _, ip := range ips { if strings.Contains(ip, ":") { diff --git a/nameservers_windows.go b/nameservers_windows.go index b02be53..b19c5ad 100644 --- a/nameservers_windows.go +++ b/nameservers_windows.go @@ -165,6 +165,9 @@ func getDNSServers(ctx context.Context) ([]string, error) { if info.DomainControllerAddress != nil { dcAddr := windows.UTF16PtrToString(info.DomainControllerAddress) + // Remove "\\" prefix from domain controller address + // Windows domain controller addresses are returned with "\\" prefix, + // but we need just the IP address for DNS resolution dcAddr = strings.TrimPrefix(dcAddr, "\\\\") logger.Debug().Msgf("Found domain controller address: %s", dcAddr) if ip := net.ParseIP(dcAddr); ip != nil { diff --git a/resolver.go b/resolver.go index 1c4bf28..0565c2b 100644 --- a/resolver.go +++ b/resolver.go @@ -126,10 +126,11 @@ func InitializeOsResolver(ctx context.Context, guardAgainstNoNameservers bool) [ // - First available LAN servers are saved and store. // - Later calls, if no LAN servers available, the saved servers above will be used. func initializeOsResolver(servers []string) []string { - var lanNss, publicNss []string - // First categorize servers + // Categorize DNS servers into LAN and public servers + // This is needed because LAN servers should be tried first for better performance, + // while public servers serve as fallback for external queries for _, ns := range servers { addr, err := netip.ParseAddr(ns) if err != nil { @@ -143,6 +144,8 @@ func initializeOsResolver(servers []string) []string { } } + // Ensure we have at least one public DNS server as fallback + // This prevents DNS resolution failures when no public servers are configured if len(publicNss) == 0 { publicNss = []string{controldPublicDnsWithPort} }