From c90047c84105a7c345412f31c7a6409633dc88cb Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Thu, 15 Apr 2021 20:38:47 -0400 Subject: [PATCH 1/4] Add templating to network-interface option This PR also adds a fast-fail to in the case where an invalid interface is set or produced by the template --- command/agent/config.go | 49 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/command/agent/config.go b/command/agent/config.go index 2474bd8e5e0..0ad65d0bfcb 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -801,7 +801,7 @@ func (mode *devModeConfig) networkConfig() error { return fmt.Errorf(errMsg, err) } if len(ifAddrs) < 1 { - return fmt.Errorf(errMsg, "could not find public network inteface") + return fmt.Errorf(errMsg, "could not find public network interface") } iface := ifAddrs[0].Name mode.iface = iface @@ -1191,9 +1191,56 @@ func (c *Config) normalizeAddrs() error { c.AdvertiseAddrs.Serf = addr } + // Skip network_interface evaluation if not a client + if c.Client != nil && c.Client.Enabled && c.Client.NetworkInterface != "" { + parsed, err := parseSingleInterfaceTemplate(c.Client.NetworkInterface) + if err != nil { + return fmt.Errorf("Failed to parse network-interface: %v", err) + } + + c.Client.NetworkInterface = parsed + } + return nil } +// parseSingleInterfaceTemplate parses a go-sockaddr template and returns an +// error if it doesn't result in a single value. +func parseSingleInterfaceTemplate(tpl string) (string, error) { + out, err := template.Parse(tpl) + if err != nil { + // typically something like + // unable to parse template "{{printfl \"en50\"}}": template: sockaddr.Parse:1: function "printfl" not defined + return "", err + } + + ifaces := strings.Split(out, " ") + + if len(ifaces) > 1 { + return "", fmt.Errorf("multiple interfaces returned: %v", ifaces) + } + if len(ifaces) == 0 { + return "", fmt.Errorf("no interface returned") + } + + // This turns an invalid interface into a fast-fail + if !isValidInterface(ifaces[0]) { + return "", fmt.Errorf("invalid interface name: %v", ifaces[0]) + } + return ifaces[0], nil + +} +func isValidInterface(iface string) bool { + if iface == "" { + return true + } + parsed, err := template.Parse( + fmt.Sprintf("{{ GetAllInterfaces | include \"name\" \"%v\" | attr \"name\" }}", + iface)) + + return err == nil && parsed == iface +} + // parseSingleIPTemplate is used as a helper function to parse out a single IP // address from a config parameter. func parseSingleIPTemplate(ipTmpl string) (string, error) { From b2ff15d9a56e686c2e5a839b6081b42b6828d0cd Mon Sep 17 00:00:00 2001 From: Luiz Aoqui Date: Mon, 19 Apr 2021 17:49:54 -0400 Subject: [PATCH 2/4] add tests and check for valid interface --- command/agent/config.go | 39 ++++++++-------- command/agent/config_test.go | 90 ++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 18 deletions(-) diff --git a/command/agent/config.go b/command/agent/config.go index 0ad65d0bfcb..27d7e4734d9 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -1209,36 +1209,39 @@ func (c *Config) normalizeAddrs() error { func parseSingleInterfaceTemplate(tpl string) (string, error) { out, err := template.Parse(tpl) if err != nil { - // typically something like + // Typically something like: // unable to parse template "{{printfl \"en50\"}}": template: sockaddr.Parse:1: function "printfl" not defined return "", err } - ifaces := strings.Split(out, " ") - - if len(ifaces) > 1 { - return "", fmt.Errorf("multiple interfaces returned: %v", ifaces) - } - if len(ifaces) == 0 { - return "", fmt.Errorf("no interface returned") + // Remove any extra empty space around the rendered result and check if the + // result is also not empty if the user provided a template. + out = strings.TrimSpace(out) + if tpl != "" && out == "" { + return "", fmt.Errorf("template %q evaluated to empty result", tpl) } - // This turns an invalid interface into a fast-fail - if !isValidInterface(ifaces[0]) { - return "", fmt.Errorf("invalid interface name: %v", ifaces[0]) + // `template.Parse` returns a space-separated list of results, but on + // Windows network interfaces are allowed to have spaces, so there is no + // guaranteed separators that we can use to test if the template returned + // multiple interfaces. + // The test below checks if the template results to a single valid interface. + + if !isValidInterface(out) { + return "", fmt.Errorf("invalid interface name %q", out) } - return ifaces[0], nil + return out, nil } -func isValidInterface(iface string) bool { - if iface == "" { + +// isValidInterface returns true if the input interface name exists. +func isValidInterface(name string) bool { + if name == "" { return true } - parsed, err := template.Parse( - fmt.Sprintf("{{ GetAllInterfaces | include \"name\" \"%v\" | attr \"name\" }}", - iface)) - return err == nil && parsed == iface + _, err := net.InterfaceByName(name) + return err == nil } // parseSingleIPTemplate is used as a helper function to parse out a single IP diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 11681f002d3..5496af198fd 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -12,6 +12,7 @@ import ( "testing" "time" + sockaddr "github.com/hashicorp/go-sockaddr" "github.com/hashicorp/nomad/client/testutil" "github.com/hashicorp/nomad/helper" "github.com/hashicorp/nomad/helper/freeport" @@ -984,6 +985,95 @@ func TestConfig_normalizeAddrs(t *testing.T) { if c.AdvertiseAddrs.RPC != "127.0.0.1:4647" { t.Fatalf("expected RPC advertise address 127.0.0.1:4647, got %s", c.AdvertiseAddrs.RPC) } + + // find the first interface + ifaces, err := sockaddr.GetAllInterfaces() + if err != nil { + t.Fatalf("failed to get interfaces: %v", err) + } + iface := ifaces[0] + + // allow network_interface templates + c = &Config{ + BindAddr: "127.0.0.1", + Ports: &Ports{ + HTTP: 4646, + RPC: 4647, + Serf: 4648, + }, + Addresses: &Addresses{}, + AdvertiseAddrs: &AdvertiseAddrs{ + HTTP: "127.0.0.1:4646", + RPC: "127.0.0.1:4647", + Serf: "127.0.0.1:4648", + }, + DevMode: false, + Client: &ClientConfig{ + Enabled: true, + NetworkInterface: `{{ GetAllInterfaces | attr "name" }}`, + }, + } + + if err := c.normalizeAddrs(); err != nil { + t.Fatalf("unable to normalize addresses: %s", err) + } + + if c.Client.NetworkInterface != iface.Name { + t.Fatalf("expected client network_interface to be %q, got %q", iface.Name, c.Client.NetworkInterface) + } + + // allow raw network_interface + c = &Config{ + BindAddr: "127.0.0.1", + Ports: &Ports{ + HTTP: 4646, + RPC: 4647, + Serf: 4648, + }, + Addresses: &Addresses{}, + AdvertiseAddrs: &AdvertiseAddrs{ + HTTP: "127.0.0.1:4646", + RPC: "127.0.0.1:4647", + Serf: "127.0.0.1:4648", + }, + DevMode: false, + Client: &ClientConfig{ + Enabled: true, + NetworkInterface: iface.Name, + }, + } + + if err := c.normalizeAddrs(); err != nil { + t.Fatalf("unable to normalize addresses: %s", err) + } + + if c.Client.NetworkInterface != iface.Name { + t.Fatalf("expected client network_interface to be %q, got %q", iface.Name, c.Client.NetworkInterface) + } + + // invalid network_interface template + c = &Config{ + BindAddr: "127.0.0.1", + Ports: &Ports{ + HTTP: 4646, + RPC: 4647, + Serf: 4648, + }, + Addresses: &Addresses{}, + AdvertiseAddrs: &AdvertiseAddrs{ + HTTP: "127.0.0.1:4646", + RPC: "127.0.0.1:4647", + Serf: "127.0.0.1:4648", + }, + DevMode: false, + Client: &ClientConfig{ + Enabled: true, + NetworkInterface: "not an interface", + }, + } + if err := c.normalizeAddrs(); err == nil { + t.Fatal("expected normalizeAddrs to fail") + } } func TestIsMissingPort(t *testing.T) { From 68224bc4b60d4a97459a1725c7c5a7b1c1f1320f Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Mon, 19 Apr 2021 18:27:44 -0400 Subject: [PATCH 3/4] Add documentation --- website/content/docs/configuration/client.mdx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/website/content/docs/configuration/client.mdx b/website/content/docs/configuration/client.mdx index b3e2d5f6ad2..96f49a68365 100644 --- a/website/content/docs/configuration/client.mdx +++ b/website/content/docs/configuration/client.mdx @@ -48,7 +48,8 @@ client { to force network fingerprinting on. When run in dev mode, this defaults to the loopback interface. When not in dev mode, the interface attached to the default route is used. The scheduler chooses from these fingerprinted IP - addresses when allocating ports for tasks. + addresses when allocating ports for tasks. This value support [go-sockaddr/template + format][go-sockaddr/template]. If no non-local IP addresses are found, Nomad could fingerprint link-local IPv6 addresses depending on the client's From b72c31f8894a0446686e9781fc029d6a9005d7a7 Mon Sep 17 00:00:00 2001 From: Charlie Voiselle <464492+angrycub@users.noreply.github.com> Date: Tue, 20 Apr 2021 11:34:31 -0400 Subject: [PATCH 4/4] Incorporate suggestions from code review --- command/agent/config.go | 18 +- command/agent/config_test.go | 156 +++++++++--------- website/content/docs/configuration/client.mdx | 1 + 3 files changed, 87 insertions(+), 88 deletions(-) diff --git a/command/agent/config.go b/command/agent/config.go index 27d7e4734d9..c5681418713 100644 --- a/command/agent/config.go +++ b/command/agent/config.go @@ -1136,7 +1136,7 @@ func (c *Config) Merge(b *Config) *Config { } // normalizeAddrs normalizes Addresses and AdvertiseAddrs to always be -// initialized and have sane defaults. +// initialized and have reasonable defaults. func (c *Config) normalizeAddrs() error { if c.BindAddr != "" { ipStr, err := parseSingleIPTemplate(c.BindAddr) @@ -1226,22 +1226,12 @@ func parseSingleInterfaceTemplate(tpl string) (string, error) { // guaranteed separators that we can use to test if the template returned // multiple interfaces. // The test below checks if the template results to a single valid interface. - - if !isValidInterface(out) { + _, err = net.InterfaceByName(out) + if err != nil { return "", fmt.Errorf("invalid interface name %q", out) } - return out, nil - -} - -// isValidInterface returns true if the input interface name exists. -func isValidInterface(name string) bool { - if name == "" { - return true - } - _, err := net.InterfaceByName(name) - return err == nil + return out, nil } // parseSingleIPTemplate is used as a helper function to parse out a single IP diff --git a/command/agent/config_test.go b/command/agent/config_test.go index 5496af198fd..0233a510644 100644 --- a/command/agent/config_test.go +++ b/command/agent/config_test.go @@ -985,94 +985,102 @@ func TestConfig_normalizeAddrs(t *testing.T) { if c.AdvertiseAddrs.RPC != "127.0.0.1:4647" { t.Fatalf("expected RPC advertise address 127.0.0.1:4647, got %s", c.AdvertiseAddrs.RPC) } +} +func TestConfig_templateNetworkInterface(t *testing.T) { // find the first interface ifaces, err := sockaddr.GetAllInterfaces() if err != nil { t.Fatalf("failed to get interfaces: %v", err) } iface := ifaces[0] - - // allow network_interface templates - c = &Config{ - BindAddr: "127.0.0.1", - Ports: &Ports{ - HTTP: 4646, - RPC: 4647, - Serf: 4648, - }, - Addresses: &Addresses{}, - AdvertiseAddrs: &AdvertiseAddrs{ - HTTP: "127.0.0.1:4646", - RPC: "127.0.0.1:4647", - Serf: "127.0.0.1:4648", - }, - DevMode: false, - Client: &ClientConfig{ - Enabled: true, - NetworkInterface: `{{ GetAllInterfaces | attr "name" }}`, - }, - } - - if err := c.normalizeAddrs(); err != nil { - t.Fatalf("unable to normalize addresses: %s", err) - } - - if c.Client.NetworkInterface != iface.Name { - t.Fatalf("expected client network_interface to be %q, got %q", iface.Name, c.Client.NetworkInterface) - } - - // allow raw network_interface - c = &Config{ - BindAddr: "127.0.0.1", - Ports: &Ports{ - HTTP: 4646, - RPC: 4647, - Serf: 4648, + testCases := []struct { + name string + clientConfig *ClientConfig + expectedInterface string + expectErr bool + }{ + { + name: "empty string", + clientConfig: &ClientConfig{ + Enabled: true, + NetworkInterface: "", + }, + expectedInterface: "", + expectErr: false, }, - Addresses: &Addresses{}, - AdvertiseAddrs: &AdvertiseAddrs{ - HTTP: "127.0.0.1:4646", - RPC: "127.0.0.1:4647", - Serf: "127.0.0.1:4648", + { + name: "simple string", + clientConfig: &ClientConfig{ + Enabled: true, + NetworkInterface: iface.Name, + }, + expectedInterface: iface.Name, + expectErr: false, }, - DevMode: false, - Client: &ClientConfig{ - Enabled: true, - NetworkInterface: iface.Name, + { + name: "valid interface", + clientConfig: &ClientConfig{ + Enabled: true, + NetworkInterface: `{{ GetAllInterfaces | attr "name" }}`, + }, + expectedInterface: iface.Name, + expectErr: false, }, - } - - if err := c.normalizeAddrs(); err != nil { - t.Fatalf("unable to normalize addresses: %s", err) - } - - if c.Client.NetworkInterface != iface.Name { - t.Fatalf("expected client network_interface to be %q, got %q", iface.Name, c.Client.NetworkInterface) - } - - // invalid network_interface template - c = &Config{ - BindAddr: "127.0.0.1", - Ports: &Ports{ - HTTP: 4646, - RPC: 4647, - Serf: 4648, + { + name: "invalid interface", + clientConfig: &ClientConfig{ + Enabled: true, + NetworkInterface: `no such interface`, + }, + expectedInterface: iface.Name, + expectErr: true, }, - Addresses: &Addresses{}, - AdvertiseAddrs: &AdvertiseAddrs{ - HTTP: "127.0.0.1:4646", - RPC: "127.0.0.1:4647", - Serf: "127.0.0.1:4648", + { + name: "insignificant whitespace", + clientConfig: &ClientConfig{ + Enabled: true, + NetworkInterface: ` {{GetAllInterfaces | attr "name" }}`, + }, + expectedInterface: iface.Name, + expectErr: false, }, - DevMode: false, - Client: &ClientConfig{ - Enabled: true, - NetworkInterface: "not an interface", + { + name: "empty template return", + clientConfig: &ClientConfig{ + Enabled: true, + NetworkInterface: `{{ printf "" }}`, + }, + expectedInterface: iface.Name, + expectErr: true, }, } - if err := c.normalizeAddrs(); err == nil { - t.Fatal("expected normalizeAddrs to fail") + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + c := &Config{ + BindAddr: "127.0.0.1", + Ports: &Ports{ + HTTP: 4646, + RPC: 4647, + Serf: 4648, + }, + Addresses: &Addresses{}, + AdvertiseAddrs: &AdvertiseAddrs{ + HTTP: "127.0.0.1:4646", + RPC: "127.0.0.1:4647", + Serf: "127.0.0.1:4648", + }, + DevMode: false, + Client: tc.clientConfig, + } + err := c.normalizeAddrs() + if tc.expectErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, c.Client.NetworkInterface, tc.expectedInterface) + }) } } diff --git a/website/content/docs/configuration/client.mdx b/website/content/docs/configuration/client.mdx index 96f49a68365..cf562038791 100644 --- a/website/content/docs/configuration/client.mdx +++ b/website/content/docs/configuration/client.mdx @@ -456,3 +456,4 @@ client { [server-join]: /docs/configuration/server_join 'Server Join' [metadata_constraint]: /docs/job-specification/constraint#user-specified-metadata 'Nomad User-Specified Metadata Constraint Example' [task working directory]: /docs/runtime/environment#task-directories 'Task directories' +[go-sockaddr/template]: https://godoc.org/github.com/hashicorp/go-sockaddr/template