From 1251e78275d4894a3c85678e94847cbc118932dd Mon Sep 17 00:00:00 2001 From: Zhao Congqi Date: Sat, 12 Oct 2024 14:02:04 +0800 Subject: [PATCH] fix: dns overwrites dhcp when enabling dhcp and setting dhcp options at the same time (#4597) * fix: dns overwrites dhcp when enabling dhcp and setting dhcp options at the same time Signed-off-by: zcq98 * add unit test for UpdateDHCPv4Options and UpdateDHCPv6Options Signed-off-by: zcq98 --------- Signed-off-by: zcq98 --- pkg/ovs/ovn-nb-dhcp_options.go | 46 ++++--------------- pkg/ovs/ovn-nb-dhcp_options_test.go | 17 +++---- pkg/ovs/util.go | 70 +++++++++++++++++++++++++++++ pkg/ovs/util_test.go | 27 +++++++++++ 4 files changed, 114 insertions(+), 46 deletions(-) diff --git a/pkg/ovs/ovn-nb-dhcp_options.go b/pkg/ovs/ovn-nb-dhcp_options.go index 8543f18bfa3..a76b8373d6d 100644 --- a/pkg/ovs/ovn-nb-dhcp_options.go +++ b/pkg/ovs/ovn-nb-dhcp_options.go @@ -110,32 +110,17 @@ func (c *OVNNbClient) updateDHCPv4Options(lsName, cidr, gateway, options string, return } - if len(options) == 0 { - mac := util.GenerateMac() - if dhcpOpt != nil && len(dhcpOpt.Options) != 0 { - mac = dhcpOpt.Options["server_mac"] - } - - options = fmt.Sprintf("lease_time=%d,router=%s,server_id=%s,server_mac=%s,mtu=%d", 3600, gateway, "169.254.0.254", mac, mtu) - } - /* update */ if dhcpOpt != nil { + mac := dhcpOpt.Options["server_mac"] dhcpOpt.Cidr = cidr - newOptions := parseDHCPOptions(options) - // append necessary options to new options - if dhcpOpt.Options != nil { - for _, option := range necessaryV4DHCPOptions { - if _, ok := newOptions[option]; !ok { - newOptions[option] = dhcpOpt.Options[option] - } - } - } - dhcpOpt.Options = newOptions + dhcpOpt.Options = buildDHCPv4Options(options, gateway, mac, mtu, necessaryV4DHCPOptions) return dhcpOpt.UUID, c.updateDHCPOptions(dhcpOpt, &dhcpOpt.Cidr, &dhcpOpt.Options) } /* create */ + mac := util.GenerateMac() + options = formatDHCPOptions(buildDHCPv4Options(options, gateway, mac, mtu, necessaryV4DHCPOptions)) if err := c.CreateDHCPOptions(lsName, cidr, options); err != nil { klog.Error(err) return "", fmt.Errorf("create dhcp options: %w", err) @@ -164,32 +149,17 @@ func (c *OVNNbClient) updateDHCPv6Options(lsName, cidr, options string) (uuid st return } - if len(options) == 0 { - mac := util.GenerateMac() - if dhcpOpt != nil && len(dhcpOpt.Options) != 0 { - mac = dhcpOpt.Options["server_id"] - } - - options = fmt.Sprintf("server_id=%s", mac) - } - /* update */ if dhcpOpt != nil { + mac := dhcpOpt.Options["server_id"] dhcpOpt.Cidr = cidr - newOptions := parseDHCPOptions(options) - // append necessary options to new options - if dhcpOpt.Options != nil { - for _, option := range necessaryV6DHCPOptions { - if _, ok := newOptions[option]; !ok { - newOptions[option] = dhcpOpt.Options[option] - } - } - } - dhcpOpt.Options = newOptions + dhcpOpt.Options = buildDHCPv6Options(options, mac, necessaryV6DHCPOptions) return dhcpOpt.UUID, c.updateDHCPOptions(dhcpOpt, &dhcpOpt.Cidr, &dhcpOpt.Options) } /* create */ + mac := util.GenerateMac() + options = formatDHCPOptions(buildDHCPv6Options(options, mac, necessaryV6DHCPOptions)) if err := c.CreateDHCPOptions(lsName, cidr, options); err != nil { klog.Error(err) return "", fmt.Errorf("create dhcp options: %w", err) diff --git a/pkg/ovs/ovn-nb-dhcp_options_test.go b/pkg/ovs/ovn-nb-dhcp_options_test.go index aa6c6f54b15..c87dad068f4 100644 --- a/pkg/ovs/ovn-nb-dhcp_options_test.go +++ b/pkg/ovs/ovn-nb-dhcp_options_test.go @@ -158,6 +158,7 @@ func (suite *OvnClientTestSuite) testUpdateDHCPv4Options() { require.Equal(t, cidr, dhcpOpt.Cidr) require.Equal(t, map[string]string{ "lease_time": "7200", + "mtu": "1500", "router": "192.168.30.1", "server_id": "169.254.0.1", "server_mac": "00:00:00:11:22:33", @@ -198,7 +199,7 @@ func (suite *OvnClientTestSuite) testUpdateDHCPv4Options() { err := nbClient.CreateDHCPOptions(lsName+"-1", cidr, options) require.NoError(t, err) - uuid, err := nbClient.updateDHCPv4Options(lsName+"-1", cidr, gateway, "dns_server=8.8.8.8", 1500) + uuid, err := nbClient.updateDHCPv4Options(lsName+"-1", cidr, gateway, "dns_server={8.8.8.8;8.8.4.4}", 1500) require.NoError(t, err) dhcpOpt, err := nbClient.GetDHCPOptions(lsName+"-1", "IPv4", false) @@ -207,12 +208,12 @@ func (suite *OvnClientTestSuite) testUpdateDHCPv4Options() { require.Equal(t, uuid, dhcpOpt.UUID) require.Equal(t, cidr, dhcpOpt.Cidr) require.Equal(t, map[string]string{ - "dns_server": "8.8.8.8", - "lease_time": "", + "dns_server": "{8.8.8.8,8.8.4.4}", + "lease_time": "3600", + "mtu": "1500", "router": "192.168.30.1", - "server_id": "", + "server_id": "169.254.0.254", "server_mac": "", - "mtu": "", }, dhcpOpt.Options) }) } @@ -289,7 +290,7 @@ func (suite *OvnClientTestSuite) testUpdateDHCPv6Options() { err := nbClient.CreateDHCPOptions(lsName+"-1", cidr, options) require.NoError(t, err) - uuid, err := nbClient.updateDHCPv6Options(lsName+"-1", cidr, "dns_server=fc00::0af4:01") + uuid, err := nbClient.updateDHCPv6Options(lsName+"-1", cidr, "dns_server={fc00::0af4:01}") require.NoError(t, err) dhcpOpt, err := nbClient.GetDHCPOptions(lsName+"-1", "IPv6", false) @@ -298,7 +299,7 @@ func (suite *OvnClientTestSuite) testUpdateDHCPv6Options() { require.Equal(t, uuid, dhcpOpt.UUID) require.Equal(t, cidr, dhcpOpt.Cidr) require.Equal(t, map[string]string{ - "dns_server": "fc00::0af4:01", + "dns_server": "{fc00::0af4:01}", "server_id": "00:00:00:55:22:33", }, dhcpOpt.Options) }) @@ -604,7 +605,7 @@ func (suite *OvnClientTestSuite) testCreateDHCPOptions() { t.Run("create valid IPv4 DHCP options", func(t *testing.T) { cidr := "192.168.60.0/24" - options := "router=192.168.60.1,dns_server=8.8.8.8" + options := "router=192.168.60.1,dns_server={8.8.8.8}" err := nbClient.CreateDHCPOptions(lsName, cidr, options) require.NoError(t, err) diff --git a/pkg/ovs/util.go b/pkg/ovs/util.go index b99586e21e8..a00410d792d 100644 --- a/pkg/ovs/util.go +++ b/pkg/ovs/util.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "regexp" + "strconv" "strings" "sync/atomic" "time" @@ -96,6 +97,75 @@ func getIpv6Prefix(networks []string) []string { return ipv6Prefix } +// buildDHCPv4Options constructs the DHCP options string for ipv4 +func buildDHCPv4Options(options, gateway, mac string, mtu int, necessaryOptions []string) map[string]string { + if len(options) == 0 { + return map[string]string{ + "lease_time": "3600", + "router": gateway, + "server_id": "169.254.0.254", + "server_mac": mac, + "mtu": strconv.Itoa(mtu), + } + } + + parsedOptions := parseDHCPOptions(options) + for _, opt := range necessaryOptions { + if _, ok := parsedOptions[opt]; !ok { + switch opt { + case "lease_time": + parsedOptions[opt] = "3600" + case "router": + parsedOptions[opt] = gateway + case "server_id": + parsedOptions[opt] = "169.254.0.254" + case "server_mac": + parsedOptions[opt] = mac + case "mtu": + parsedOptions[opt] = strconv.Itoa(mtu) + } + } + } + + return parsedOptions +} + +// buildDHCPv6Options constructs the DHCP options string for ipv6 +func buildDHCPv6Options(options, mac string, necessaryOptions []string) map[string]string { + if len(options) == 0 { + return map[string]string{ + "server_id": mac, + } + } + + parsedOptions := parseDHCPOptions(options) + for _, opt := range necessaryOptions { + if _, ok := parsedOptions[opt]; !ok { + if opt == "server_id" { + parsedOptions[opt] = mac + } + } + } + + return parsedOptions +} + +// formatDHCPOptions converts the parsed options map into a string format +// e.g. dns_server="{8.8.8.8,8.8.4.4}", lease_time="3600", mtu="1500", router="192.168.80.1", server_id="169.254.0.254", server_mac="5e:4e:e7:48:3d:7d" +func formatDHCPOptions(options map[string]string) string { + var sb strings.Builder + for k, v := range options { + if sb.Len() > 0 { + sb.WriteString(",") + } + if k == "dns_server" { + v = strings.ReplaceAll(v, ",", ";") + } + sb.WriteString(fmt.Sprintf("%s=%s", k, v)) + } + return sb.String() +} + // parseDHCPOptions parses dhcp options, // the raw option's format is: server_id=192.168.123.50,server_mac=00:00:00:08:0a:11 func parseDHCPOptions(raw string) map[string]string { diff --git a/pkg/ovs/util_test.go b/pkg/ovs/util_test.go index ab721aac428..3aae0367b6f 100644 --- a/pkg/ovs/util_test.go +++ b/pkg/ovs/util_test.go @@ -508,3 +508,30 @@ func TestLogicalSwitchPortName(t *testing.T) { }) } } + +func TestFormatDHCPOptions(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + options map[string]string + expected string + }{ + { + name: "DNS server with commas", + options: map[string]string{ + "dns_server": "{8.8.8.8,1.1.1.1}", + }, + expected: "dns_server={8.8.8.8;1.1.1.1}", + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + result := formatDHCPOptions(tc.options) + require.Equal(t, tc.expected, result) + }) + } +}