Skip to content

Compare DHCP config based on its effect #6

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.

Sign up for GitHub

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

Open
milan-zededa wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Compare DHCP config based on its effect #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a0b1e52
Fix multiple statically configured DNS servers
milan-zededa Sep 9, 2022
4257781
Compare DHCP config based on its effect
milan-zededa Sep 5, 2022
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
3 changes: 0 additions & 3 deletions pkg/pillar/dpcmanager/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,9 +249,6 @@ func (m *DpcManager) getDHCPInfo(port *types.NetworkPortStatus) error {
}

func (m *DpcManager) getDNSInfo(port *types.NetworkPortStatus) error {
if port.Dhcp != types.DT_CLIENT {
return nil
}
ifIndex, exists, err := m.NetworkMonitor.GetInterfaceIndex(port.IfName)
if !exists {
return nil
Expand Down
126 changes: 81 additions & 45 deletions pkg/pillar/dpcreconciler/genericitems/dhcpcd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"net"
"os"
"os/exec"
"reflect"
"strconv"
"strings"
"syscall"
Expand All @@ -26,6 +25,8 @@ import (
const (
dhcpcdStartTimeout = 3 * time.Second
dhcpcdStopTimeout = 30 * time.Second

zeroIPv4Addr = "0.0.0.0"
)

// Dhcpcd : DHCP client (https://wiki.archlinux.org/title/dhcpcd).
Expand Down Expand Up @@ -54,7 +55,20 @@ func (c Dhcpcd) Type() string {
// Equal is a comparison method for two equally-named Dhcpcd instances.
func (c Dhcpcd) Equal(other depgraph.Item) bool {
c2 := other.(Dhcpcd)
return reflect.DeepEqual(c.DhcpConfig, c2.DhcpConfig)
// Consider two DHCP configs as equal if they result in the same set of arguments for dhcpcd.
// This avoids unnecessary restarts of dhcpcd (when e.g. going from override to zedagent DPC).
configurator := &DhcpcdConfigurator{}
op1, args1 := configurator.dhcpcdArgs(c.DhcpConfig)
op2, args2 := configurator.dhcpcdArgs(c2.DhcpConfig)
if op1 != op2 || len(args1) != len(args2) {
return false
}
for i := range args1 {
if args1[i] != args2[i] {
return false
}
}
return true
}

// External returns false.
Expand Down Expand Up @@ -94,31 +108,15 @@ func (c *DhcpcdConfigurator) Create(ctx context.Context, item depgraph.Item) err
ifName := client.AdapterIfName
config := client.DhcpConfig

// Prepare input arguments
var op string
var args []string
// Validate input arguments
switch config.Dhcp {
case types.DT_NONE:
// Nothing to do, return.
done(nil)
return

case types.DT_CLIENT:
op = "--request"
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "-b", "-t", "0"}
switch config.Type {
case types.NtIpv4Only:
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "--ipv4only", "-b", "-t", "0"}
case types.NtIpv6Only:
args = []string{"-f", "/dhcpcd.conf", "--ipv6only", "-b", "-t", "0"}
case types.NT_NOOP:
case types.NT_IPV4:
case types.NT_IPV6:
case types.NtDualStack:
default:
}
if config.Gateway != nil && config.Gateway.String() == "0.0.0.0" {
args = append(args, "--nogateway")
}
// Nothing to validate.

case types.DT_STATIC:
if config.AddrSubnet == "" {
Expand All @@ -138,30 +136,6 @@ func (c *DhcpcdConfigurator) Create(ctx context.Context, item depgraph.Item) err
done(err)
return
}
op = "--static"
args = []string{fmt.Sprintf("ip_address=%s", config.AddrSubnet)}
extras := []string{"-f", "/dhcpcd.conf", "-b", "-t", "0"}
if config.Gateway == nil || config.Gateway.String() == "0.0.0.0" {
extras = append(extras, "--nogateway")
} else if config.Gateway.String() != "" {
args = append(args, "--static",
fmt.Sprintf("routers=%s", config.Gateway.String()))
}
// XXX do we need to calculate a list for option?
for _, dns := range config.DnsServers {
args = append(args, "--static",
fmt.Sprintf("domain_name_servers=%s", dns.String()))
}
if config.DomainName != "" {
args = append(args, "--static",
fmt.Sprintf("domain_name=%s", config.DomainName))
}
if config.NtpServer != nil && !config.NtpServer.IsUnspecified() {
args = append(args, "--static",
fmt.Sprintf("ntp_servers=%s",
config.NtpServer.String()))
}
args = append(args, extras...)

default:
err := fmt.Errorf("unsupported DHCP type: %v", config.Dhcp)
Expand All @@ -170,6 +144,9 @@ func (c *DhcpcdConfigurator) Create(ctx context.Context, item depgraph.Item) err
return
}

// Prepare input arguments for dhcpcd.
op, args := c.dhcpcdArgs(config)

// Start DHCP client.
if c.dhcpcdExists(client.AdapterIfName) {
err := fmt.Errorf("dhcpcd for interface %s is already running", ifName)
Expand Down Expand Up @@ -287,6 +264,65 @@ func (c *DhcpcdConfigurator) NeedsRecreate(oldItem, newItem depgraph.Item) (recr
return true
}

func (c *DhcpcdConfigurator) dhcpcdArgs(config types.DhcpConfig) (op string, args []string) {
switch config.Dhcp {
case types.DT_CLIENT:
op = "--request"
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "-b", "-t", "0"}
switch config.Type {
case types.NtIpv4Only:
args = []string{"-f", "/dhcpcd.conf", "--noipv4ll", "--ipv4only", "-b", "-t", "0"}
case types.NtIpv6Only:
args = []string{"-f", "/dhcpcd.conf", "--ipv6only", "-b", "-t", "0"}
case types.NT_NOOP:
case types.NT_IPV4:
case types.NT_IPV6:
case types.NtDualStack:
default:
}
if config.Gateway != nil && config.Gateway.String() == zeroIPv4Addr {
args = append(args, "--nogateway")
}

case types.DT_STATIC:
op = "--static"
args = []string{fmt.Sprintf("ip_address=%s", config.AddrSubnet)}
extras := []string{"-f", "/dhcpcd.conf", "-b", "-t", "0"}
if config.Gateway == nil || config.Gateway.String() == zeroIPv4Addr {
extras = append(extras, "--nogateway")
} else if config.Gateway.String() != "" {
args = append(args, "--static",
fmt.Sprintf("routers=%s", config.Gateway.String()))
}
var dnsServers []string
for _, dns := range config.DnsServers {
dnsServers = append(dnsServers, dns.String())
}
if len(dnsServers) > 0 {
// dhcpcd uses a very odd space-separation for multiple DNS servers.
// For manual invocation one must be very careful to not forget
// to quote the argument so that the spaces don't make the shell
// break up the list into multiple args.
// Here we do not need quotes because we are passing the DNS server
// list as a single entry of the 'args' slice for exec.Command().
args = append(args, "--static",
fmt.Sprintf("domain_name_servers=%s",
strings.Join(dnsServers, " ")))
}
if config.DomainName != "" {
args = append(args, "--static",
fmt.Sprintf("domain_name=%s", config.DomainName))
}
if config.NtpServer != nil && !config.NtpServer.IsUnspecified() {
args = append(args, "--static",
fmt.Sprintf("ntp_servers=%s",
config.NtpServer.String()))
}
args = append(args, extras...)
}
return
}

func (c *DhcpcdConfigurator) dhcpcdCmd(op string, extras []string,
ifName string, background bool) error {
name := "/sbin/dhcpcd"
Expand Down
141 changes: 141 additions & 0 deletions pkg/pillar/dpcreconciler/genericitems/dhcpcd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
package genericitems_test

import (
"net"
"testing"

configitems "github.com/lf-edge/eve/pkg/pillar/dpcreconciler/genericitems"
"github.com/lf-edge/eve/pkg/pillar/types"
)

func TestDhcpcdEqual(t *testing.T) {
type test struct {
name string
item1 configitems.Dhcpcd
item2 configitems.Dhcpcd
expEqual bool
}
var tests = []test{
{
name: "DHCP client for IPv4 only",
item1: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
AddrSubnet: "192.168.1.44/24", // irrelevant
Gateway: net.ParseIP("192.168.1.1"), // irrelevant
DomainName: "mydomain", // irrelevant
NtpServer: net.ParseIP("192.168.1.1"), // irrelevant
Type: types.NtIpv4Only, // must match
},
},
item2: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
Type: types.NtIpv4Only, // must match
},
},
expEqual: true,
},
{
name: "DHCP client with effectively equivalent IP types",
item1: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
Type: types.NT_NOOP, // sometimes we get this in override.json
},
},
item2: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
Type: types.NT_IPV4, // effectively equivalent to NT_NOOP
},
},
expEqual: true,
},
{
name: "DHCP client with effectively different IP types",
item1: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
Type: types.NT_IPV4,
},
},
item2: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
Type: types.NtIpv4Only, // differs in --ipv4only arg
},
},
expEqual: false,
},
{
name: "DHCP client with disabled default route",
item1: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
Gateway: net.ParseIP("0.0.0.0"), // default route is disabled

},
},
item2: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_CLIENT,
// default route is enabled
},
},
expEqual: false,
},
{
name: "equivalent static IP config",
item1: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_STATIC,
AddrSubnet: "192.168.1.44/24",
DomainName: "mydomain",
NtpServer: net.ParseIP("192.168.1.1"),
DnsServers: []net.IP{net.ParseIP("8.8.8.8")},
Type: types.NtIpv4Only, // irrelevant
},
},
item2: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_STATIC,
AddrSubnet: "192.168.1.44/24",
DomainName: "mydomain",
NtpServer: net.ParseIP("192.168.1.1"),
DnsServers: []net.IP{net.ParseIP("8.8.8.8")},
Type: types.NT_IPV4, // irrelevant
},
},
expEqual: true,
},
{
name: "different statically configured DNS servers",
item1: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_STATIC,
AddrSubnet: "192.168.1.44/24",
DomainName: "mydomain",
NtpServer: net.ParseIP("192.168.1.1"),
DnsServers: []net.IP{net.ParseIP("8.8.8.8")}, // does not match
},
},
item2: configitems.Dhcpcd{
DhcpConfig: types.DhcpConfig{
Dhcp: types.DT_STATIC,
AddrSubnet: "192.168.1.44/24",
DomainName: "mydomain",
NtpServer: net.ParseIP("192.168.1.1"),
DnsServers: []net.IP{net.ParseIP("1.1.1.1")}, // does not match
},
},
expEqual: false,
},
}
for _, test := range tests {
if test.item1.Equal(test.item2) != test.expEqual {
t.Errorf("TEST CASE \"%s\" FAILED - Equal() returned: %t, expected: %t",
test.name, test.item1.Equal(test.item2), test.expEqual)
}
}
}