Skip to content

Commit

Permalink
fix: dns overwrites dhcp when enabling dhcp and setting dhcp options …
Browse files Browse the repository at this point in the history 
…at the same time (kubeovn#4597)

* fix: dns overwrites dhcp when enabling dhcp and setting dhcp options at the same time

Signed-off-by: zcq98 <[email protected]>

* add unit test for UpdateDHCPv4Options and UpdateDHCPv6Options

Signed-off-by: zcq98 <[email protected]>

---------

Signed-off-by: zcq98 <[email protected]>
  • Loading branch information
@zhaocongqi
zhaocongqi authored Oct 12, 2024
1 parent 42ebd7c commit 1251e78
Show file tree
Hide file tree
Showing 4 changed files with 114 additions and 46 deletions.
46 changes: 8 additions & 38 deletions pkg/ovs/ovn-nb-dhcp_options.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
17 changes: 9 additions & 8 deletions pkg/ovs/ovn-nb-dhcp_options_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand All @@ -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)
})
}
Expand Down Expand Up @@ -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)
Expand All @@ -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)
})
Expand Down Expand Up @@ -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)

Expand Down
70 changes: 70 additions & 0 deletions pkg/ovs/util.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"regexp"
"strconv"
"strings"
"sync/atomic"
"time"
Expand Down Expand Up @@ -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 {
Expand Down
27 changes: 27 additions & 0 deletions pkg/ovs/util_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}

0 comments on commit 1251e78

Please sign in to comment.