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.

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

[processor/resourcedetection]: add "cname" and "lookup" hostname sources #10015

Merged
merged 4 commits into from
May 19, 2022
Merged

Conversation

pmcollins
Copy link
Member

@pmcollins pmcollins commented May 13, 2022

[processor/resourcedetection]: add "cname" and "lookup" hostname sources

The existing "dns" hostname source uses the Showmax/go-fqdn library, whose FqdnHostname()
function tries to get the fully qualified domain name of the current host by

  1. Reading the hosts file and looking for a match of the os.Hostname and a corresponding fqdn
    on that line. If that fails, then it falls back to:
  2. Getting the canonical name via net.LookupCNAME. If that fails, then it falls back to:
  3. Getting the current IP address, and doing a reverse DNS lookup of that address

A problem that least one Collector user encountered while running on Windows in Azure,
is that the call to net.LookupCNAME just returns the simple hostname, which
then gets returned by FqdnHostname(), even though on a Linux host, deployed in the same
environment, net.LookupCNAME returns a fully qualified domain name.

Instead of trying to work around what might be an issue with net.LookupCNAME by modifying
the Showmax/go-fqdn code, which has hard-coded fallback logic, and changing behavior for
existing users, this change proposes creating two new hostname sources, "cname", and "lookup".
The "cname" source corresponds to a call to net.LookupCNAME, and the "lookup" source corresponds
to a call to net.LookupHost and then a call to net.LookupAddr for each ip address it returns.
The "lookup" source produces the same result on Windows as net.LookupCNAME does on Linux and would
solve the problem of the Azure user in question. In addition, users can then build the fallback logic that they prefer.
Eventually, we could also add a "hostfile" hostname source that uses the same logic as the
corresponding part of the Showmax/"dns" implementation, so that users could replace the "dns"
hostname source with the equivalent three sources, explicitly indicating the fallback logic rather
than using Showmax's hard-coded logic:

processors:
  resourcedetection:
    detectors: ["system"]
    system:
      hostname_sources: ["hostsfile", "cname", "lookup"]

At that point, we could deprecate the "dns" source or make it effectively an alias for ["hostsfile",
"cname", "lookup"] and remove the dependency.

For now, the customer that would benefit from this change, however, will just use the "lookup" source on
their Windows hosts, as "cname" on Windows doesn't produce a useful result.

The "dns" hostname source uses the Showmax/go-fqdn library, whose FqdnHostname()
function tries to get the fully qualified domain name of the current host by
1) Reading the hosts file and looking for a match of the os.Hostname and a corresponding fqdn
on that line. If that fails, then it falls back to:
2) Getting the canonical name via net.LookupCNAME. If that fails, then it falls back to:
3) Getting the current IP address, and doing a reverse DNS lookup of that address

A problem that least one Collector user encountered running on Windows in Azure, is that the call
to net.LookupCNAME just returns the simple hostname, which then gets returned by FqdnHostname(),
even though on a Linux host net.LookupCNAME returns a fully qualified domain name.

Instead of trying to work around what might be an issue with net.LookupCNAME by modifying
the Showmax/go-fqdn code, which has hard-coded fallback logic, and changing behavior for
existing users, this change proposes creating two new hostname sources, "cname", and "lookup".
The "cname" source corresponds to a call to net.LookupCNAME, and the "lookup" source corresponds
to a call to net.LookupHost and then a call to net.LookupAddr for each ip address it returns.
The "lookup" source produces the same result on Windows as net.LookupCNAME does on Linux and would
solve the problem of the Azure user in question. In addition, users can then build the fallback logic
that they prefer. Eventually, we could also add a "hostfile" hostname source that uses the same logic
as the corresponding part of the Showmax/"dns" implementation, so that users could replace the "dns"
hostname source with the equivalent three sources, explicitly indicating the fallback logic rather
than using Showmax's hard-coded logic:

processors:
  resourcedetection:
    detectors: ["system"]
    system:
      hostname_sources: ["hostsfile", "cname", "lookup"]

At that point, we could deprecate the "dns" source or make it effectively an alias for ["hostsfile",
"cname", "lookup"] and remove the dependency.

For now, the user that would benefit from this change, however, will just use the "lookup" source on
their Windows hosts, as "cname" on Windows doesn't produce a useful result.
@pmcollins pmcollins marked this pull request as ready for review May 17, 2022 14:44
@pmcollins pmcollins requested a review from a team May 17, 2022 14:44
@dmitryax
Copy link
Member

At that point, we could deprecate the "dns" source or make it effectively an alias for ["hostsfile",
"cname", "lookup"] and remove the dependency.

I like this idea. We should do that and eventually remove Showmax/go-fqdn dependency. Could you please create a follow up issue?

Copy link
Member

@dmitryax dmitryax left a comment

Choose a reason for hiding this comment

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

Can you add some test coverage?

@pmcollins
Copy link
Member Author

Can you add some test coverage?

Sure -- I've added some.

@dmitryax dmitryax added the ready to merge Code review completed; ready to merge by maintainers label May 18, 2022
@pmcollins
Copy link
Member Author

Created follow-up issue #10138

@codeboten codeboten merged commit 3aa6851 into open-telemetry:main May 19, 2022
jamesmoessis pushed a commit to jamesmoessis/opentelemetry-collector-contrib that referenced this pull request May 20, 2022
…ces (open-telemetry#10015)

* [processor/resourcedetection]: add "cname" and "lookup" hostname sources

The "dns" hostname source uses the Showmax/go-fqdn library, whose FqdnHostname()
function tries to get the fully qualified domain name of the current host by
1) Reading the hosts file and looking for a match of the os.Hostname and a corresponding fqdn
on that line. If that fails, then it falls back to:
2) Getting the canonical name via net.LookupCNAME. If that fails, then it falls back to:
3) Getting the current IP address, and doing a reverse DNS lookup of that address

A problem that least one Collector user encountered running on Windows in Azure, is that the call
to net.LookupCNAME just returns the simple hostname, which then gets returned by FqdnHostname(),
even though on a Linux host net.LookupCNAME returns a fully qualified domain name.

Instead of trying to work around what might be an issue with net.LookupCNAME by modifying
the Showmax/go-fqdn code, which has hard-coded fallback logic, and changing behavior for
existing users, this change proposes creating two new hostname sources, "cname", and "lookup".
The "cname" source corresponds to a call to net.LookupCNAME, and the "lookup" source corresponds
to a call to net.LookupHost and then a call to net.LookupAddr for each ip address it returns.
The "lookup" source produces the same result on Windows as net.LookupCNAME does on Linux and would
solve the problem of the Azure user in question. In addition, users can then build the fallback logic
that they prefer. Eventually, we could also add a "hostfile" hostname source that uses the same logic
as the corresponding part of the Showmax/"dns" implementation, so that users could replace the "dns"
hostname source with the equivalent three sources, explicitly indicating the fallback logic rather
than using Showmax's hard-coded logic:

processors:
  resourcedetection:
    detectors: ["system"]
    system:
      hostname_sources: ["hostsfile", "cname", "lookup"]

At that point, we could deprecate the "dns" source or make it effectively an alias for ["hostsfile",
"cname", "lookup"] and remove the dependency.

For now, the user that would benefit from this change, however, will just use the "lookup" source on
their Windows hosts, as "cname" on Windows doesn't produce a useful result.
kentquirk pushed a commit to McSick/opentelemetry-collector-contrib that referenced this pull request Jun 14, 2022
…ces (open-telemetry#10015)

* [processor/resourcedetection]: add "cname" and "lookup" hostname sources

The "dns" hostname source uses the Showmax/go-fqdn library, whose FqdnHostname()
function tries to get the fully qualified domain name of the current host by
1) Reading the hosts file and looking for a match of the os.Hostname and a corresponding fqdn
on that line. If that fails, then it falls back to:
2) Getting the canonical name via net.LookupCNAME. If that fails, then it falls back to:
3) Getting the current IP address, and doing a reverse DNS lookup of that address

A problem that least one Collector user encountered running on Windows in Azure, is that the call
to net.LookupCNAME just returns the simple hostname, which then gets returned by FqdnHostname(),
even though on a Linux host net.LookupCNAME returns a fully qualified domain name.

Instead of trying to work around what might be an issue with net.LookupCNAME by modifying
the Showmax/go-fqdn code, which has hard-coded fallback logic, and changing behavior for
existing users, this change proposes creating two new hostname sources, "cname", and "lookup".
The "cname" source corresponds to a call to net.LookupCNAME, and the "lookup" source corresponds
to a call to net.LookupHost and then a call to net.LookupAddr for each ip address it returns.
The "lookup" source produces the same result on Windows as net.LookupCNAME does on Linux and would
solve the problem of the Azure user in question. In addition, users can then build the fallback logic
that they prefer. Eventually, we could also add a "hostfile" hostname source that uses the same logic
as the corresponding part of the Showmax/"dns" implementation, so that users could replace the "dns"
hostname source with the equivalent three sources, explicitly indicating the fallback logic rather
than using Showmax's hard-coded logic:

processors:
  resourcedetection:
    detectors: ["system"]
    system:
      hostname_sources: ["hostsfile", "cname", "lookup"]

At that point, we could deprecate the "dns" source or make it effectively an alias for ["hostsfile",
"cname", "lookup"] and remove the dependency.

For now, the user that would benefit from this change, however, will just use the "lookup" source on
their Windows hosts, as "cname" on Windows doesn't produce a useful result.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants