Skip to content
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

Jump to bottom

Allow FQDN lookup functions to take context #199

Merged
merged 8 commits into from
Feb 1, 2024
Merged

Allow FQDN lookup functions to take context #199

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fd1ad25
Pass context to FQDN lookup functions
ycombinator Jan 25, 2024
01b75c6
Adding CHANGELOG entry
ycombinator Jan 25, 2024
6eea02f
Update test
ycombinator Jan 25, 2024
2b70234
Closer replacement
ycombinator Jan 25, 2024
b05739a
Include timeout test case
ycombinator Jan 25, 2024
f64eb1b
Tighten regex
ycombinator Jan 25, 2024
8ef547d
Add FQDNWithContext functions to prevent breaking change
ycombinator Jan 25, 2024
ee500ea
Fix call
ycombinator Jan 31, 2024
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
3 changes: 3 additions & 0 deletions .changelog/199.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:enhancement
Allow FQDN lookup functions to take a context.
```
5 changes: 3 additions & 2 deletions providers/aix/host_aix_ppc64.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ package aix
import "C"

import (
"context"
"errors"
"fmt"
"os"
Expand Down Expand Up @@ -128,8 +129,8 @@ func (*host) Memory() (*types.HostMemoryInfo, error) {
return &mem, nil
}

func (h *host) FQDN() (string, error) {
return shared.FQDN()
func (h *host) FQDN(ctx context.Context) (string, error) {
return shared.FQDN(ctx)
}

func newHost() (*host, error) {
Expand Down
5 changes: 3 additions & 2 deletions providers/darwin/host_darwin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package darwin

import (
"context"
"errors"
"fmt"
"os"
Expand Down Expand Up @@ -139,8 +140,8 @@ func (h *host) Memory() (*types.HostMemoryInfo, error) {
return &mem, nil
}

func (h *host) FQDN() (string, error) {
return shared.FQDN()
func (h *host) FQDN(ctx context.Context) (string, error) {
return shared.FQDN(ctx)
}

func (h *host) LoadAverage() (*types.LoadAverageInfo, error) {
Expand Down
12 changes: 10 additions & 2 deletions providers/linux/host_fqdn_integration_docker_linux_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
package linux

import (
"context"
"fmt"
"testing"
"time"

"github.com/stretchr/testify/require"
)
Expand All @@ -32,7 +34,10 @@ func TestHost_FQDN_set(t *testing.T) {
t.Fatal(fmt.Errorf("could not get host information: %w", err))
}

gotFQDN, err := host.FQDN()
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

gotFQDN, err := host.FQDN(ctx)
require.NoError(t, err)
if gotFQDN != wantFQDN {
t.Errorf("got FQDN %q, want: %q", gotFQDN, wantFQDN)
Expand All @@ -45,7 +50,10 @@ func TestHost_FQDN_not_set(t *testing.T) {
t.Fatal(fmt.Errorf("could not get host information: %w", err))
}

gotFQDN, err := host.FQDN()
ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

gotFQDN, err := host.FQDN(ctx)
require.NoError(t, err)
hostname := host.Info().Hostname
if gotFQDN != hostname {
Expand Down
5 changes: 3 additions & 2 deletions providers/linux/host_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package linux

import (
"context"
"errors"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -73,8 +74,8 @@ func (h *host) Memory() (*types.HostMemoryInfo, error) {
return parseMemInfo(content)
}

func (h *host) FQDN() (string, error) {
return shared.FQDN()
func (h *host) FQDN(ctx context.Context) (string, error) {
return shared.FQDN(ctx)
}

// VMStat reports data from /proc/vmstat on linux.
Expand Down
13 changes: 7 additions & 6 deletions providers/shared/fqdn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package shared

import (
"context"
"fmt"
"net"
"os"
Expand All @@ -40,18 +41,18 @@ import (
//
// 4. If steps 2 and 3 both fail, an empty string is returned as the FQDN along with
// errors from those steps.
func FQDN() (string, error) {
func FQDN(ctx context.Context) (string, error) {
hostname, err := os.Hostname()
if err != nil {
return "", fmt.Errorf("could not get hostname to look for FQDN: %w", err)
}

return fqdn(hostname)
return fqdn(ctx, hostname)
}

func fqdn(hostname string) (string, error) {
func fqdn(ctx context.Context, hostname string) (string, error) {
var errs error
cname, err := net.LookupCNAME(hostname)
cname, err := net.DefaultResolver.LookupCNAME(ctx, hostname)
if err != nil {
errs = fmt.Errorf("could not get FQDN, all methods failed: failed looking up CNAME: %w",
err)
Expand All @@ -60,13 +61,13 @@ func fqdn(hostname string) (string, error) {
return strings.ToLower(strings.TrimSuffix(cname, ".")), nil
}

ips, err := net.LookupIP(hostname)
ips, err := net.DefaultResolver.LookupIP(ctx, "ip", hostname)
if err != nil {
errs = fmt.Errorf("%s: failed looking up IP: %w", errs, err)
}

for _, ip := range ips {
names, err := net.LookupAddr(ip.String())
names, err := net.DefaultResolver.LookupAddr(ctx, ip.String())
Copy link
Member

Choose a reason for hiding this comment

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

The type of ips here changed from []net.IP to []net.IPAddr which doesn't have the same implementation of String(), since net.IPAddr embeds IP and extends it with a zone.

If you want the exact same conversion as before you want to call ip.IP.String() I think.

func (a *IPAddr) String() string {
	if a == nil {
		return "<nil>"
	}
	ip := ipEmptyString(a.IP)
	if a.Zone != "" {
		return ip + "%" + a.Zone
	}
	return ip
}

The original call to net.LookupIP stripped off the IPv6 zone: https://cs.opensource.google/go/go/+/refs/tags/go1.21.6:src/net/lookup.go;l=195

// LookupIP looks up host using the local resolver.
// It returns a slice of that host's IPv4 and IPv6 addresses.
func LookupIP(host string) ([]IP, error) {
	addrs, err := DefaultResolver.LookupIPAddr(context.Background(), host)
	if err != nil {
		return nil, err
	}
	ips := make([]IP, len(addrs))
	for i, ia := range addrs {
		ips[i] = ia.IP
	}
	return ips, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type of ips here changed from []net.IP to []net.IPAddr...

Are you sure? Before, ips was initialized as:

ips, err := net.LookupIP(hostname)

Now, it's initialized as:

ips, err := net.DefaultResolver.LookupIP(ctx, "ip", hostname)

In both cases, the type of ips is the same: []IP.

Copy link
Member

Choose a reason for hiding this comment

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

Ah you are right for some reason I ended up focusing on LookupIPAddr as if it were called early, I think this was because net.LookupIP calls LookupIPAddr but func (r *Resolver) LookupIP does something else.

I don't have enough brainpower left for the day or network knowledge to know if that is significant, I was trying to quickly check if this worked the same as it did before.

if err != nil || len(names) == 0 {
continue
}
Expand Down
32 changes: 27 additions & 5 deletions providers/shared/fqdn_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
package shared

import (
"context"
"fmt"
"testing"
"time"

"github.com/stretchr/testify/require"
)
Expand All @@ -31,6 +33,7 @@ func TestFQDN(t *testing.T) {
osHostname string
expectedFQDN string
expectedErrRegex string
timeout time.Duration
}{
// This test case depends on network, particularly DNS,
// being available. If it starts to fail often enough
Expand All @@ -44,23 +47,37 @@ func TestFQDN(t *testing.T) {
"long_nonexistent_hostname": {
osHostname: "foo.bar.elastic.co",
expectedFQDN: "",
expectedErrRegex: makeErrorRegex("foo.bar.elastic.co"),
expectedErrRegex: makeErrorRegex("foo.bar.elastic.co", false),
},
"short_nonexistent_hostname": {
osHostname: "foobarbaz",
expectedFQDN: "",
expectedErrRegex: makeErrorRegex("foobarbaz"),
expectedErrRegex: makeErrorRegex("foobarbaz", false),
},
"long_mixed_case_hostname": {
osHostname: "eLaSTic.co",
expectedFQDN: "elastic.co",
expectedErrRegex: "",
},
"nonexistent_timeout": {
osHostname: "foobarbaz",
expectedFQDN: "",
expectedErrRegex: makeErrorRegex("foobarbaz", true),
timeout: 1 * time.Millisecond,
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
actualFQDN, err := fqdn(test.osHostname)
timeout := test.timeout
if timeout == 0 {
timeout = 10 * time.Second
}

ctx, cancel := context.WithTimeout(context.Background(), timeout)
defer cancel()

actualFQDN, err := fqdn(ctx, test.osHostname)
require.Equal(t, test.expectedFQDN, actualFQDN)

if test.expectedErrRegex == "" {
Expand All @@ -72,11 +89,16 @@ func TestFQDN(t *testing.T) {
}
}

func makeErrorRegex(osHostname string) string {
func makeErrorRegex(osHostname string, withTimeout bool) string {
timeoutStr := ""
if withTimeout {
timeoutStr = ": i/o timeout"
}

return fmt.Sprintf(
"could not get FQDN, all methods failed: "+
"failed looking up CNAME: lookup %s.*: "+
"failed looking up IP: lookup %s.*",
"failed looking up IP: lookup %s"+timeoutStr,
osHostname,
osHostname,
)
Expand Down
3 changes: 2 additions & 1 deletion providers/windows/host_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package windows

import (
"context"
"errors"
"fmt"
"os"
Expand Down Expand Up @@ -84,7 +85,7 @@ func (h *host) Memory() (*types.HostMemoryInfo, error) {
}, nil
}

func (h *host) FQDN() (string, error) {
func (h *host) FQDN(_ context.Context) (string, error) {
fqdn, err := getComputerNameEx(stdwindows.ComputerNamePhysicalDnsFullyQualified)
if err != nil {
return "", fmt.Errorf("could not get windows FQDN: %s", err)
Expand Down
7 changes: 5 additions & 2 deletions types/host.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@

package types

import "time"
import (
"context"
"time"
)

// Host is the interface that wraps methods for returning Host stats
// It may return partial information if the provider
Expand All @@ -28,7 +31,7 @@ type Host interface {
Memory() (*HostMemoryInfo, error)

// FQDN returns the fully-qualified domain name of the host, lowercased.
FQDN() (string, error)
Copy link
Member

Choose a reason for hiding this comment

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

This is a breaking change to the API. I suggest adding an FQDNWithContext(context.Context) unless we are prepared to rollout a github.com/elastic/go-sysinfo/v2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 8ef547d.

FQDN(ctx context.Context) (string, error)
}

// NetworkCounters represents network stats from /proc/net
Expand Down
Loading