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

autodetect interfaces on private networks #126

Merged
merged 5 commits into from
Feb 10, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
48 changes: 48 additions & 0 deletions netutil/netutil.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package netutil

import (
"net"

"github.com/go-kit/log"
"github.com/go-kit/log/level"
)

var (
getInterfaceAddrs = (*net.Interface).Addrs
)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is very interesting, I think it's not very common in Go to create a variable which refers to a method of a type that takes a pointer receiver and then call it standalone while "manually" passing a pointer to an object of the type into it.

As far as I understand you did that so you can mock the interface, right? I think the more common solution for that in Go would to just create an interface with the Addrs() method and then make privateNetworkInterfaces use the interface type. Unfortunately this might require breaking the content of the for loop in privateNetworkInterfaces out into a separate function.

However, I think this pattern works too, I was just surprised to see it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wanted to suggest the same but we also need to read the Name field from net.Interface which we can't put into an interface. We would need to defined our our interface with both Name() and Addrs() and then wrap net.Interface to our own interface.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ic, I have missed the access to the .Name property.
(one of my long-time pet-peeves with Go is that you can't define a property as part of an interface)

In that case I agree that this solution is easier, because otherwise we'd have to wrap net.Interface in something which allows us to return the name via a function call, just to make the mocking work. that gets unnecessarily complicated...


// PrivateNetworkInterfaces lists network interfaces and returns those having an address conformant to RFC1918
func PrivateNetworkInterfaces(logger log.Logger) []string {
aldernero marked this conversation as resolved.
Show resolved Hide resolved
ints, err := net.Interfaces()
if err != nil {
level.Warn(logger).Log("msg", "error getting network interfaces", "err", err)
}
return privateNetworkInterfaces(ints, logger)
}

// private testable function that checks each given interface
func privateNetworkInterfaces(all []net.Interface, logger log.Logger) []string {
var privInts []string
for _, i := range all {
addrs, err := getInterfaceAddrs(&i)
if err != nil {
level.Warn(logger).Log("msg", "error getting addresses from network interface", "interface", i.Name, "err", err)
}
for _, a := range addrs {
s := a.String()
ip, _, err := net.ParseCIDR(s)
if err != nil {
level.Warn(logger).Log("msg", "error parsing network interface IP address", "inf", i.Name, "addr", s, "err", err)
aldernero marked this conversation as resolved.
Show resolved Hide resolved
aldernero marked this conversation as resolved.
Show resolved Hide resolved
}
if ip.IsPrivate() {
privInts = append(privInts, i.Name)
break
}
}
}
if len(privInts) == 0 {
return []string{"eth0", "en0"}
}
aldernero marked this conversation as resolved.
Show resolved Hide resolved
level.Debug(logger).Log("msg", "found network interfaces with private IP addresses assigned", "interfaces", privInts)
return privInts
}
126 changes: 126 additions & 0 deletions netutil/netutil_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
package netutil

import (
"net"
"os"
"testing"

"github.com/go-kit/log"
"github.com/stretchr/testify/assert"
)

// A type that implements the net.Addr interface
// Only String() is called by netutil logic
type mockAddr struct {
netAddr string
}

func (ma mockAddr) Network() string {
return "tcp"
}

func (ma mockAddr) String() string {
return ma.netAddr
}

// Helper function to test a list of interfaces
func generateTestInterfaces(names []string) []net.Interface {
testInts := []net.Interface{}
for i, j := range names {
k := net.Interface{
Index: i + 1,
MTU: 1500,
Name: j,
HardwareAddr: []byte{},
Flags: 0,
}
testInts = append(testInts, k)
}
return testInts
}

func TestPrivateInterface(t *testing.T) {
testIntsAddrs := map[string][]string{
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test on a invalid address that fails to be parsed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is another test below that one, TestPrivateInterfaceError that tests an invalid address.

Copy link
Contributor

@pracucci pracucci Feb 10, 2022

Choose a reason for hiding this comment

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

Doh! My bad, sorry.

"privNetA": {"10.6.19.34/8"},
"privNetB": {"172.16.0.7/12"},
"privNetC": {"192.168.3.29/24"},
"pubNet": {"34.120.177.193/24"},
"multiPriv": {"10.6.19.34/8", "172.16.0.7/12"},
"multiMix": {"1.1.1.1/24", "192.168.0.42/24"},
"multiPub": {"1.1.1.1/24", "34.120.177.193/24"},
}
defaultOutput := []string{"eth0", "en0"}
type testCases struct {
description string
interfaces []string
expectedOutput []string
}
for _, scenario := range []testCases{
{
description: "empty interface list",
interfaces: []string{},
expectedOutput: defaultOutput,
},
{
description: "single private interface",
interfaces: []string{"privNetA"},
expectedOutput: []string{"privNetA"},
},
{
description: "single public interface",
interfaces: []string{"pubNet"},
expectedOutput: defaultOutput,
},
{
description: "single interface multi address private",
interfaces: []string{"multiPriv"},
expectedOutput: []string{"multiPriv"},
},
{
description: "single interface multi address mix",
interfaces: []string{"multiMix"},
expectedOutput: []string{"multiMix"},
},
{
description: "single interface multi address public",
interfaces: []string{"multiPub"},
expectedOutput: defaultOutput,
},
{
description: "all private interfaces",
interfaces: []string{"privNetA", "privNetB", "privNetC"},
expectedOutput: []string{"privNetA", "privNetB", "privNetC"},
},
{
description: "mix of public and private interfaces",
interfaces: []string{"pubNet", "privNetA", "privNetB", "privNetC", "multiPriv", "multiMix", "multiPub"},
expectedOutput: []string{"privNetA", "privNetB", "privNetC", "multiPriv", "multiMix"},
},
} {
getInterfaceAddrs = func(i *net.Interface) ([]net.Addr, error) {
addrs := []net.Addr{}
for _, ip := range testIntsAddrs[i.Name] {
addrs = append(addrs, mockAddr{netAddr: ip})
}
return addrs, nil
}
t.Run(scenario.description, func(t *testing.T) {
privInts := privateNetworkInterfaces(
generateTestInterfaces(scenario.interfaces),
log.NewNopLogger(),
)
assert.Equal(t, privInts, scenario.expectedOutput)
})
}
}

func TestPrivateInterfaceError(t *testing.T) {
interfaces := generateTestInterfaces([]string{"eth9"})
ipaddr := "not_a_parseable_ip_string"
getInterfaceAddrs = func(i *net.Interface) ([]net.Addr, error) {
return []net.Addr{mockAddr{netAddr: ipaddr}}, nil
}
logger := log.NewLogfmtLogger(os.Stdout)
privInts := privateNetworkInterfaces(interfaces, logger)
assert.Equal(t, privInts, []string{"eth0", "en0"})
}
3 changes: 2 additions & 1 deletion ring/lifecycler.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (

"github.com/grafana/dskit/flagext"
"github.com/grafana/dskit/kv"
"github.com/grafana/dskit/netutil"
"github.com/grafana/dskit/services"
)

Expand Down Expand Up @@ -81,7 +82,7 @@ func (cfg *LifecyclerConfig) RegisterFlagsWithPrefix(prefix string, f *flag.Flag
panic(fmt.Errorf("failed to get hostname %s", err))
}

cfg.InfNames = []string{"eth0", "en0"}
Copy link
Contributor

Choose a reason for hiding this comment

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

We also want a fallback mechanism. If unable to detect the interfaces, we should fallback to []string{"eth0", "en0"}. Since this is a logic we'll need in downstream projects too, you could define a function netutil.PrivateNetworkInterfacesWithFallback()

cfg.InfNames = netutil.PrivateNetworkInterfaces(log.NewLogfmtLogger(os.Stdout))
Copy link
Contributor

Choose a reason for hiding this comment

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

Take the logger in input to RegisterFlagsWithPrefix() (as 3rd parameter) and then in RegisterFlags() as 2nd parameter. It's a pattern we already use elsewhere and there's an interface defined for that (see RegistererWithLogger interface in flagext/register.go).

f.Var((*flagext.StringSlice)(&cfg.InfNames), prefix+"lifecycler.interface", "Name of network interface to read address from.")
f.StringVar(&cfg.Addr, prefix+"lifecycler.addr", "", "IP address to advertise in the ring.")
f.IntVar(&cfg.Port, prefix+"lifecycler.port", 0, "port to advertise in consul (defaults to server.grpc-listen-port).")
Expand Down