Report FQDN #144

AndersonQ · 2023-01-04T10:10:09Z

This PR adds FQDN to types.HostInfo and expands the linux, darwin and windows providers to retrieve and populate the newly added fields.

The FQDN is obtained by:

Linux/Darwin:
- net.LookupCNAME for hostname
- reverse lookup by hostname:
  - net.LookupIP for hostname
  - net.LookupAddr for the obateined IP
Windows:
- get ComputerNamePhysicalDnsFullyQualified

How to test:

Integration tests:

There are integration tests for linux, which requires docker, to run them use the build tag integration. The TestHost_FQDN automatically starts docker containers and runs the tests.

GO_VERSION=1.19 go test -tags integration -v -count 1 -run TestHost_FQDN$ ./providers/linux/

Manually:

Run the test TestHost for the desired provider, linux, windows or darwin:

go test -v -run TestHost$ ./providers/PROVIDER/

On docker:

docker run --rm -v "$PWD":/usr/src/elastic/go-sysinfo -w /usr/src/elastic/go-sysinfo golang:1.19 go test -v -run TestHost$ ./providers/linux

The TestHost test only prints the information collected, you need to check it for correctness.

look for

          "name": "mokona-elastic",
          "fqdn": "mokona-elastic.lan",

full output

❯ go test -v -run TestHost$ ./providers/linux/   

=== RUN   TestHost
  host_linux_test.go:39: {
        "architecture": "x86_64",
        "boot_time": "2023-01-23T07:04:06+01:00",
        "containerized": false,
        "name": "mokona-elastic",
        "domain": "",
        "fqdn": "mokona-elastic.lan",
        "ip": [
          "127.0.0.1/8",
          "::1/128",
          "192.168.86.136/24",
          "fd66:61eb:174c:0:5961:a7be:b434:e131/64",
          "fd66:61eb:174c:0:95a:f7a1:cec2:6d8a/64",
          "fe80::780d:6555:d649:7934/64",
          "172.26.0.1/16",
          "172.17.0.1/16",
          "fe80::42:61ff:fec1:1585/64",
          "172.18.0.1/16",
          "fc00:f853:ccd:e793::1/64",
          "fe80::1/64",
          "172.25.0.1/16",
          "172.19.0.1/16",
          "172.21.0.1/16",
          "172.20.0.1/16"
        ],
        "kernel_version": "6.0.6-76060006-generic",
        "mac": [
          "00:e0:4c:ad:f7:bb",
          "7c:21:4a:a3:e2:dd",
          "02:42:c8:ee:00:03",
          "02:42:61:c1:15:85",
          "02:42:fd:6b:4f:5d",
          "02:42:e2:b4:8d:10",
          "02:42:61:c8:02:f2",
          "02:42:29:aa:6d:01",
          "02:42:77:1f:61:c9"
        ],
        "os": {
          "type": "linux",
          "family": "",
          "platform": "pop",
          "name": "Pop!_OS",
          "version": "22.04 LTS",
          "major": 22,
          "minor": 4,
          "patch": 0,
          "codename": "jammy"
        },
        "timezone": "CET",
        "timezone_offset_sec": 3600,
        "id": "e9ed195eddb96448172f94386284db62"
      }
--- PASS: TestHost (0.00s)
PASS
ok  	github.com/elastic/go-sysinfo/providers/linux	0.192s

Tested on:

elasticmachine · 2023-01-04T10:19:47Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-02-15T15:12:40.669+0000
Duration: 10 min 11 sec

Test stats 🧪

Test	Results
Failed	0
Passed	256
Skipped	4
Total	260

fearful-symmetry

Just leaving a few drive-by comments. Also, don't forget to update the main feature chart thing in the readme.md.

providers/linux/host_linux.go

providers/darwin/host_darwin.go

joshdover · 2023-01-16T09:04:47Z

@AndersonQ Just to make sure we're on the same page, this PR can still continue even before we make the decision on the default behavior in Beats/Agent.

leehinman · 2023-01-17T16:13:33Z

Results from when I ran on MacOS 12.6.2 arm64

Environment	CGO_ENABLED	name	fqdn
darwin	0	elastic2	elastic2
darwin	1	elastic2	elastic2.lan
darwin docker	0	dee3070d2dc3	dee3070d2dc3
darwin docker	1	a0914c8b7bf2	a0914c8b7bf2.lan

andrewkroh

Given that FQDNs are case-insensitive I think it would be helpful for this library to normalize the values to lowercase despite what the OS gives.

types/host.go

AndersonQ · 2023-01-23T14:37:57Z

/test

leehinman · 2023-01-23T16:06:13Z

Re-ran tests on MacOS 12.6.2 arm64

Environment	CGO_ENABLED	name	domain	fqdn
darwin	0	"elastic2"	""	""
darwin	1	"elastic2"	""	"elastic2.lan"
darwin docker	0	"5bad5ca5beca"	""	""
darwin docker	1	"96b268cfda81"	""	"96b268cfda81.lan"

AndersonQ · 2023-01-24T16:33:39Z

Given that FQDNs are case-insensitive I think it would be helpful for this library to normalize the values to lowercase despite what the OS gives.

it does make sense. but then it'd also be true for the hostname and domain, right? For consistency, I think they should be aligned. However it'd be a breaking change for hostname, what I'd say is less than ideal...

In that sense, also considering it's just a lib, I'd let this decision for whoever is using it. We returns as we get, the user transforms as they need.

What do you think @andrewkroh?

fearful-symmetry · 2023-01-24T20:43:09Z

Alright, current round of CI errors seems unrelated?

AndersonQ · 2023-01-25T13:20:57Z

Alright, current round of CI errors seems unrelated?

@fearful-symmetry, @andrewkroh do any of you are familiar with these tests "failing"?

The culprit is Archive JUnit-formatted test results, but its output is the same as for others that are green

[2023-01-24T16:45:09.128Z] Recording test results

[2023-01-24T16:45:11.474Z] [Checks API] No suitable checks publisher found.

andrewkroh · 2023-01-25T20:50:13Z

In that sense, also considering it's just a lib, I'd let this decision for whoever is using it. We returns as we get, the user transforms as they need.

That's fine. Either when we use this in Beats / Agent or when we create the mappings for this field we should ensure that the value is normalized.

andrewkroh · 2023-01-25T20:56:16Z

do any of you are familiar with these tests "failing"?

Some of the FQDN tests are failing on ubuntu under both CGO_ENABLED=0 and CGO_ENABLED=1. See the "Tests" tab:

https://beats-ci.elastic.co/blue/organizations/jenkins/Library%2Fgo-sysinfo-mbp/detail/PR-144/37/tests

andrewkroh · 2023-01-26T16:44:51Z

providers/darwin/host_fqdn_nocgo_darwin.go

+}
+
+func domainname() (string, error) {
+	return "", nil


Could we use sysctl to fetch kern.nisdomainname here? That looks like it returns the same thing in my testing. Then you would not need a CGO and non-CGO implementation.

this is already the no Cgo implementation. I'm already using Cgo for the getdomainname syscall.

sysctl is an external application, right? I'm avoiding invoking any external application.

Sorry, I wasn't clear. We can invoke the sysctl syscall without needing CGO. Then we can have a single implementation that works for all darwin builds. There should be some examples already in the codebase (if not see the os.Hostname() implementation in the stdlib for darwin).

andrewkroh · 2023-01-26T16:50:30Z

providers/darwin/host_fqdn_cgo_darwin.go

+		return "", err
+	}
+
+	if domain == "" || domain == "(none)" { // mimicking 'hostname -f' behaviour


When I run hostname -f on my machine it doesn't add .local?

% hostname mac16-m1 % hostname -f mac16-m1 % hostname -d mac16-m1

are you sure you haven't set your domain name to "" somehow?
I'm assuming it's your Elastic laptop, thus with Elastic Shield installed. Am I correct?

Have you used the tool in Elastic shield to rename the laptop? That would explain this behaviour

andrewkroh · 2023-01-26T16:56:05Z

providers/darwin/host_fqdn_cgo_darwin.go

+)
+
+func fqdn() (string, error) {
+	hostname, err := os.Hostname()


If I use sudo scutil --set HostName foo.engineering.example.com. Then hostname returns:

% sudo scutil --set HostName foo.engineering.example.com % hostname -f foo.engineering.example.com % hostname -s foo % hostname -d engineering.example.com

But go-sysinfo returns:

"name": "foo.engineering.example.com", "domain": "", "fqdn": "foo.engineering.example.com.local",

It gets even more confusing if the machine has a NIS domain set AND the HostName is fully-qualified. I think we need to be clearify what we are reporting. Do we want to report the NIS domain or do we want to report domain suffix that is part of the machine's fully-qualified DNS name (man hostname has some details).

sudo sysctl kern.nisdomainname=nis.domain

"name": "foo.engineering.example.com", "domain": "nis.domain", "fqdn": "foo.engineering.example.com.nis.domain",

providers/darwin/process_darwin_test.go

andrewkroh · 2023-01-26T17:31:18Z

providers/linux/host_fqdn_nocgo_linux.go

+	return "", nil
+}
+
+func domainname() (string, error) {


I think the NIS domain can be read from /proc/sys/kernel/domainname. Then you wouldn't need separate CGO and non-CGO implementations. And probably the docker based integration testing could be removed since you could simulate various hostname and domainname via the filesystem (probably like newLinuxSystem("testdata/my_test_proc")).

https://www.kernel.org/doc/html/latest/admin-guide/sysctl/kernel.html#domainname-hostname

Do we really want to bring the NIS domain into FQDN? As I understand it, FQDN is part of DNS not NIS. Gnu libc and RFC1594 seem to agree.

I have the same question. I was alluding to this in my other comment at #144 (comment). With this comment I was mainly focused on the code as it was without debating DNS vs NIS. I haven't looked at the latest changes to see how this has evolved, but I still think we need to clear on what we are reporting.

leehinman · 2023-02-06T16:24:21Z

providers/shared/fqdn.go

+	fqdn, err := fqdnFromEtcHosts(hostname)
+	if err != nil {
+		errs = fmt.Errorf("could not get FQDN, all methods failed: %w", err)
+	}
+	if fqdn != "" {
+		return fqdn, nil
+	}


I think I'm missing something. Why parse /etc/hosts? The net.LookupCNAME, net.LookuIP and net.LookupAddr will use /etc/hosts if the resolver on the system uses that file for name resolution. Is there a specific use case this is addressing? If so I really think we need it described in the docstring.

That's the recommendation I found on man hostname:

The recommended method of setting the FQDN is to make the hostname be an alias for the fully qualified name using /etc/hosts, DNS, or NIS.

And I didn't find on net, net.LookupCNAME/IP/Addr docs saying explicitly they use etc/hosts.

On Unix systems, the resolver has two options for resolving names. It can use a pure Go resolver that sends DNS requests directly to the servers listed in /etc/resolv.conf, or it can use a cgo-based resolver that calls C library routines such as getaddrinfo and getnameinfo.

By default the pure Go resolver is used [...]

But if you're sure it's redundant I can remove it.

yep they go through '/etc/hosts' the pure go one use readHost where the testHookHostsPath is /etc/hosts and the cgo implementations all use the libc resolver which will use /etc/hosts if your system is setup to do that, and if it isn't setup that way you don't want to parse /etc/hosts anyway.

providers/linux/host_fqdn_integration_docker_linux_test.go

providers/shared/fqdn.go

Co-authored-by: Lee E Hinman <[email protected]>

michalpristas · 2023-02-07T13:10:40Z

go.mod

 	howett.net/plist v0.0.0-20181124034731-591f970eefbb
 )

 require (
-	github.com/davecgh/go-spew v1.1.0 // indirect
-	github.com/pkg/errors v0.8.1 // indirect
+	github.com/Microsoft/go-winio v0.6.0 // indirect


this seems off, so many new imports?

They are indirect, I believe most come from test dependencies, most likely github.com/docker/docker. I'm using it to be able to do a integration test for the FQDN.

Here is why go-winio is needed

# github.com/Microsoft/go-winio github.com/elastic/go-sysinfo/providers/linux github.com/elastic/go-sysinfo/providers/linux.test github.com/docker/docker/client github.com/docker/go-connections/sockets github.com/Microsoft/go-winio # v0.6.0 (main module does not need module v0.6.0)

besides the only direct dependency added was github.com/docker/docker

providers/shared/fqdn.go

michalpristas · 2023-02-07T13:19:58Z

providers/shared/fqdn.go

+		return ""
+	}
+
+	fileds := strings.FieldsFunc(line, func(r rune) bool {


s/fileds/fields

michalpristas · 2023-02-07T13:20:45Z

providers/shared/fqdn.go

+	// fields[0] is the ip address
+	cannonical, aliases := fileds[1], fileds[1:]
+
+	// TODO: confirm: a name should not repeat on different addresses.


adressed or issue please

michalpristas · 2023-02-07T13:23:14Z

providers/windows/host_windows.go

+						"but data size should fit into buffer")
+			} else {
+				// Grow the buffer and try again.
+				// Should we limit its growth?


but then what would be the limit?
size is the needed size for a buffer to fit the FQDN. The unix implementation does not limit the size, so I'd say it should be fine. it hardly wouldn't fit into memory, if it don't, I'd say windows is on a lot more trouble than us.

jlind23 · 2023-02-09T13:39:27Z

@AndersonQ would it also be possible to update the description with the algorithm you used when default values were used, etc..

AndersonQ · 2023-02-10T10:17:28Z

@AndersonQ would it also be possible to update the description with the algorithm you used when default values were used, etc..

@jlind23 done

AndersonQ · 2023-02-14T19:37:11Z

@michalpristas, @andrewkroh, @leehinman, @fearful-symmetry I updated the PR, it's ready for final review. I still need to add the changes to the changelog/readme though

fearful-symmetry · 2023-02-14T20:37:20Z

providers/darwin/host_darwin_test.go

+
+	info := host.Info()
+	data, _ := json.MarshalIndent(info, "", "  ")
+	t.Logf(string(data))


Do we need this test? Doesn't seem to be doing much beyond logging the result.

No it does not. I just followed the pattern that was already in place for linux.

fearful-symmetry · 2023-02-14T21:00:39Z

providers/linux/host_fqdn_integration_linux_test.go

+		t.Fatalf("environment variable %s not set, please set a Go version",
+			envKey)
+	}
+	image := "golang:" + goversion


Extremely pedantic question: What, exactly, are we trying to test with the containers? Just how this will behave in a containerized environment in general? If we wanted to test this under different OS-specific conditions, we might want to target containers built around centos, Feodra, ubuntu, etc.

yes that is correct. I haven't found any testing checking hostname info. That was the easiest way I found to set up an environment I can control hostname and domain to test. It isn't comprehensive, but it's more that there was before.

leehinman · 2023-02-14T22:15:12Z

providers/shared/fqdn.go

+	fqdn, err := fqdnFromEtcHosts(hostname)
+	if err != nil {
+		errs = fmt.Errorf("could not get FQDN, all methods failed: %w", err)
+	}
+	if fqdn != "" {
+		return fqdn, nil
+	}


yep they go through '/etc/hosts' the pure go one use readHost where the testHookHostsPath is /etc/hosts and the cgo implementations all use the libc resolver which will use /etc/hosts if your system is setup to do that, and if it isn't setup that way you don't want to parse /etc/hosts anyway.

leehinman

LGTM

AndersonQ force-pushed the report-fqdn branch from e423d6c to b929ead Compare January 4, 2023 15:41

fearful-symmetry reviewed Jan 4, 2023

View reviewed changes

providers/linux/host_linux.go Show resolved Hide resolved

providers/linux/host_linux.go Outdated Show resolved Hide resolved

andrewkroh reviewed Jan 5, 2023

View reviewed changes

providers/darwin/host_darwin.go Outdated Show resolved Hide resolved

AndersonQ force-pushed the report-fqdn branch 2 times, most recently from 295121c to dc6f651 Compare January 5, 2023 16:31

AndersonQ marked this pull request as ready for review January 17, 2023 10:32

AndersonQ changed the title ~~WIP - report FQDN~~ Report FQDN Jan 17, 2023

AndersonQ mentioned this pull request Jan 17, 2023

Report host name as FQDN on all platforms elastic/beats#1070

Closed

AndersonQ requested review from andrewkroh and fearful-symmetry and removed request for andrewkroh and fearful-symmetry January 17, 2023 16:21

andrewkroh reviewed Jan 18, 2023

View reviewed changes

types/host.go Outdated Show resolved Hide resolved

AndersonQ force-pushed the report-fqdn branch from 2e442ef to bb1df24 Compare January 23, 2023 13:44

AndersonQ mentioned this pull request Jan 24, 2023

TestProcesses failing on CI for darwin #148

Closed

AndersonQ mentioned this pull request Jan 25, 2023

Report FQDN on host.name enabled by a feature flag elastic/elastic-agent#2185

Closed

AndersonQ force-pushed the report-fqdn branch 2 times, most recently from f970dae to 15c572b Compare January 26, 2023 16:35

andrewkroh reviewed Jan 26, 2023

View reviewed changes

AndersonQ requested review from andrewkroh and fearful-symmetry February 2, 2023 18:11

leehinman requested changes Feb 6, 2023

View reviewed changes

jlind23 and others added 3 commits February 7, 2023 14:02

Update providers/shared/fqdn.go

c31ec2d

Co-authored-by: Lee E Hinman <[email protected]>

Update providers/linux/host_fqdn_integration_docker_linux_test.go

8b7b674

Co-authored-by: Lee E Hinman <[email protected]>

Update providers/linux/host_fqdn_integration_docker_linux_test.go

687a468

Co-authored-by: Lee E Hinman <[email protected]>

michalpristas reviewed Feb 7, 2023

View reviewed changes

providers/shared/fqdn.go Outdated Show resolved Hide resolved

michalpristas reviewed Feb 7, 2023

View reviewed changes

providers/shared/fqdn.go Outdated

return ""

}

fileds := strings.FieldsFunc(line, func(r rune) bool {

Copy link

michalpristas Feb 7, 2023

Choose a reason for hiding this comment

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

s/fileds/fields

michalpristas reviewed Feb 7, 2023

View reviewed changes

AndersonQ added 2 commits February 14, 2023 12:30

review changes

01676da

Merge remote-tracking branch 'origin/report-fqdn' into report-fqdn

a34bbd3

AndersonQ requested review from leehinman and michalpristas February 14, 2023 11:32

add missing import

88dc9f8

fearful-symmetry reviewed Feb 14, 2023

View reviewed changes

leehinman requested changes Feb 14, 2023

View reviewed changes

AndersonQ added 2 commits February 15, 2023 10:47

don't parse /etc/hosts to get FQDN on linux and darwin

f8d318e

remove tests for removed things

9c01d40

AndersonQ requested a review from leehinman February 15, 2023 14:40

leehinman approved these changes Feb 15, 2023

View reviewed changes

add changelog

d88406a

AndersonQ merged commit f544eca into elastic:main Feb 15, 2023

AndersonQ deleted the report-fqdn branch February 15, 2023 15:25

ycombinator mentioned this pull request Mar 24, 2023

Cannot start Beats - fails with error: could not get FQDN elastic/beats#34910

Closed

andrewkroh mentioned this pull request May 31, 2023

Return FQDN in hostname #38

Closed

Report FQDN #144

Report FQDN #144

Conversation

AndersonQ commented Jan 4, 2023 • edited Loading

How to test:

Integration tests:

Manually:

elasticmachine commented Jan 4, 2023 • edited Loading

💚 Build Succeeded

Build stats

Test stats 🧪

fearful-symmetry left a comment

Choose a reason for hiding this comment

joshdover commented Jan 16, 2023

leehinman commented Jan 17, 2023

andrewkroh left a comment • edited Loading

Choose a reason for hiding this comment

AndersonQ commented Jan 23, 2023

leehinman commented Jan 23, 2023

AndersonQ commented Jan 24, 2023 • edited Loading

fearful-symmetry commented Jan 24, 2023

AndersonQ commented Jan 25, 2023

andrewkroh commented Jan 25, 2023

andrewkroh commented Jan 25, 2023

andrewkroh Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewkroh Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalpristas Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlind23 commented Feb 9, 2023

AndersonQ commented Feb 10, 2023

AndersonQ commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leehinman left a comment

Choose a reason for hiding this comment

AndersonQ commented Jan 4, 2023 •

edited

Loading

elasticmachine commented Jan 4, 2023 •

edited

Loading

andrewkroh left a comment •

edited

Loading

AndersonQ commented Jan 24, 2023 •

edited

Loading

andrewkroh Jan 26, 2023 •

edited

Loading

andrewkroh Jan 26, 2023 •

edited

Loading

andrewkroh Feb 6, 2023 •

edited

Loading

michalpristas Feb 7, 2023 •

edited

Loading