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

CBG-1424 - Show network interface stats when configured API listen adresses use hostnames instead of IPs #5001

Merged
merged 2 commits into from
May 19, 2021

Conversation

bbrks
Copy link
Member

@bbrks bbrks commented May 17, 2021

  • Lookup non-IP host to resolve addresses in discoverInterfaceName, prior to searching for a matching network interface.
    • Note: This resolves hostnames at each stat collection - which can be advantageous for dynamic addressing, and is not expected to be detremental given DNS caching on both the local resolver, and upstream DNS servers.
  • Simplify networkInterfaceStatsForHostnamePort:
    • Remove intermediate allInterfaces bool in favour of single perNic
    • Remove special case for localhost->127.0.0.1 (let the resolver deal with that, along with others like localhost4, localhost6, and proper hostnames like admin.sgw.example.com)

Manual testing:

  1. Add localhost alias to /etc/hosts for resolver to see.
$ cat /etc/hosts | grep localhost
# localhost is used to configure the loopback interface
127.0.0.1	localhost
::1             localhost
127.0.0.1 speciallocalhost
  1. Bind admin interface to that alias
$ cat config.json
{
  "adminInterface": "speciallocalhost:4985",
  "unsupported": {
    "stats_log_freq_secs": 10
  },
...
  1. Run SG and wait for first stat collection.
  2. See expvar for working (>0) admin_net_bytes_recv stat and no warning in logs.
  3. Repeat from step 2 with IP address instead of hostname for regression.

Before:

Screenshot 2021-05-17 at 21 51 47

2021-05-17T21:50:58.825+01:00 [WRN] Error getting admin network interface resource stats: Unable to find interface for host: speciallocalhost -- rest.(*ServerContext).logNetworkInterfaceStats() at server_context.go:1122

After:

Screenshot 2021-05-17 at 21 52 53

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 64.15% when pulling 5ba69c9 on CBG-1424 into feaed5d on master.

Copy link
Collaborator

@adamcfraser adamcfraser left a comment

Choose a reason for hiding this comment

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

Approach looks fine - question about a multiple address/multiple interface scenario.

for _, ifaceCIDRAddr := range ifaceAddresses {

ipAddr, _, err := net.ParseCIDR(ifaceCIDRAddr.String())
if err != nil {
return "", err
}

if ipAddr.String() == host {
if _, ok := hosts[ipAddr.String()]; ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the scenario where a host has multiple addresses, is it possible for those addresses to assigned to different interfaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it's possible, but the interface actually being used for the REST API depends what was first in the resolved list of addresses at the time of net.Listen() which does not support multiple addresses.

I think that means that as long as LookupHost resolves in the same order as net.Listen, you'll get correct results - but if that does not happen, you'll see stats for another interface. I don't know how likely or unlikely this is.

There's a "correct" implementation that resolves an address before starting up the HTTP servers, so we know exactly what to look for in this stat code, but I'm not sure it's worth that to cover this specific case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense - thanks.

@adamcfraser adamcfraser merged commit 58dd304 into master May 19, 2021
@adamcfraser adamcfraser deleted the CBG-1424 branch May 19, 2021 16:21
IsaacLambat pushed a commit that referenced this pull request Oct 1, 2021
…ured API listen adresses use hostnames instead of IPs (#5001)

* Resolve hostnames in discoverInterfaceName when searching for matching network interface addresses

* Simplify networkInterfaceStatsForHostnamePort
bbrks added a commit that referenced this pull request Oct 4, 2021
…n configured API listen adresses use hostnames instead of IPs (#5001) (#5268)

Co-authored-by: Ben Brooks <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants