Skip to content

Fix externalhosts to advertise both IPv4 and IPv6 addresses #10145

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix externalhosts to advertise both IPv4 and IPv6 addresses #10145

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1913,11 +1913,34 @@ func newServer(ctx context.Context, cfg *Config, listenAddrs []net.Addr,
s.hostAnn = netann.NewHostAnnouncer(netann.HostAnnouncerConfig{
Hosts: cfg.ExternalHosts,
RefreshTicker: ticker.New(defaultHostSampleInterval),
LookupHost: func(host string) (net.Addr, error) {
return lncfg.ParseAddressString(
host, strconv.Itoa(defaultPeerPort),
cfg.net.ResolveTCPAddr,
)
LookuLookupHost: func(host string) ([]net.Addr, error) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

There's a typo in the field name. It should be LookupHost instead of LookuLookupHost for the code to compile correctly.

            LookupHost: func(host string) ([]net.Addr, error) {

// Use net.LookupHost to get all IP addresses for the host
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This comment, and the ones on lines 1923 and 1937, explain what the code does rather than why. According to the style guide, comments should explain the 'why' if the code isn't self-explanatory. In this case, the code is clear, so these comments can be removed.1

Style Guide References

Footnotes

  1. Comments should explain the 'why' behind the code, not what the code does, especially if the code is self-documenting. (link) ↩

ips, err := net.LookupHost(host)
if err != nil {
return nil, err
}

// Create addresses for each IP (both IPv4 and IPv6)
var addrs []net.Addr
for _, ip := range ips {
addr, err := lncfg.ParseAddressString(
ip, strconv.Itoa(defaultPeerPort),
cfg.net.ResolveTCPAddr,
)
if err != nil {
// Log the error but continue with other IPs
continue
Comment on lines +1931 to +1932
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The comment here is inaccurate as no logging is performed. It's good practice to log such errors for debugging purposes. Also, per the style guide, comments should explain the 'why', not the 'what'.1

                            // Log the error and continue with other IPs.
                            srvrLog.Warnf("unable to parse address %s for host %s: %v", ip, host, err)
                            continue

Style Guide References

Footnotes

  1. Comments should explain the 'why' behind the code, not what the code does, especially if the code is self-documenting. (link) ↩

}
addrs = append(addrs, addr)
}

// Return error if no valid addresses were found
if len(addrs) == 0 {
return nil, fmt.Errorf("no valid addresses found for host %s", host)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This line exceeds the 80-character limit specified in the style guide. To adhere to the style guide, you can introduce a variable for the error message string.1

                        msg := "no valid addresses found for host %s"
                        return nil, fmt.Errorf(msg, host)

Style Guide References

Footnotes

  1. The line length must not exceed 80 characters. Tabs are counted as 8 spaces. (link) ↩

}

return addrs, nil
})
},
AdvertisedIPs: advertisedIPs,
AnnounceNewIPs: netann.IPAnnouncer(
Expand Down