diff --git a/go-controller/pkg/util/dns.go b/go-controller/pkg/util/dns.go index 9466ad16f5..86d8a9e054 100644 --- a/go-controller/pkg/util/dns.go +++ b/go-controller/pkg/util/dns.go @@ -16,8 +16,12 @@ import ( ) const ( - // defaultTTL is used if an invalid or zero TTL is provided. - defaultTTL = 30 * time.Minute + // defaultMinTTL is the minimum TTL value that will be used for a domain name if an invalid or zero TTL is found + defaultMinTTL = 5 * time.Second + // defaultMaxTTL is the maximum TTL value that will be used for a domain name if an invalid or zero TTL is found + defaultMaxTTL = 2 * time.Minute + // maxRetryBeforeBackoff is the maximum number of times to retry a DNS lookup before exponential backoff starts + maxRetryBeforeBackoff = 10 ) type dnsValue struct { @@ -27,6 +31,8 @@ type dnsValue struct { ttl time.Duration // Holds (last dns lookup time + ttl), tells when to refresh IPs next time nextQueryTime time.Time + // Number of times the DNS lookup has been retried before backoff starts + retryCount int } type DNS struct { @@ -105,11 +111,22 @@ func (d *DNS) updateOne(dns string) (bool, error) { return false, fmt.Errorf("DNS value not found in dnsMap for domain: %q", dns) } - ips, ttl, err := d.getIPsAndMinTTL(dns) - if err != nil { - res.nextQueryTime = time.Now().Add(defaultTTL) - d.dnsMap[dns] = res - return false, err + ips, ttl, retry, err := d.getIPsAndMinTTL(dns) + if retry { + // If the DNS lookup has been retried maxRetryCount times, use exponential backoff + // by doubling the previous TTL. The TTL is capped at defaultMaxTTL. + if res.retryCount >= maxRetryBeforeBackoff { + ttl = min(res.ttl*2, defaultMaxTTL) + } else { + // Increment the retry count + res.retryCount++ + } + // If no valid IPs were found, use the previous IPs as fallback. + if len(ips) == 0 { + ips = res.ips + } + } else { + res.retryCount = 0 } changed := false @@ -120,10 +137,10 @@ func (d *DNS) updateOne(dns string) (bool, error) { res.ttl = ttl res.nextQueryTime = time.Now().Add(res.ttl) d.dnsMap[dns] = res - return changed, nil + return changed, err } -func (d *DNS) getIPsAndMinTTL(domain string) ([]net.IP, time.Duration, error) { +func (d *DNS) getIPsAndMinTTL(domain string) ([]net.IP, time.Duration, bool, error) { ips := []net.IP{} ttlSet := false var ttlSeconds uint32 @@ -197,19 +214,27 @@ func (d *DNS) getIPsAndMinTTL(domain string) ([]net.IP, time.Duration, error) { } if !ttlSet || (len(ips) == 0) { - return nil, defaultTTL, fmt.Errorf("IPv4 or IPv6 addr not found for domain: %q, nameservers: %v", domain, d.nameservers) + return nil, defaultMinTTL, true, fmt.Errorf("IPv4 or IPv6 addr not found for domain: %q, nameservers: %v", domain, d.nameservers) } + ips = removeDuplicateIPs(ips) + ttl, err := time.ParseDuration(fmt.Sprintf("%ds", minTTL)) if err != nil { - utilruntime.HandleError(fmt.Errorf("invalid TTL value for domain: %q, err: %v, defaulting ttl=%s", domain, err, defaultTTL.String())) - ttl = defaultTTL + utilruntime.HandleError(fmt.Errorf("invalid TTL value for domain: %q, err: %v", domain, err)) + return ips, defaultMinTTL, true, nil } if ttl == 0 { - ttl = defaultTTL + // If the TTL is 0, return the default minimum TTL. The retry is set to false as this + // is not an error scenario. TTL being 0 is a valid scenario for some DNS servers + // and it means that the IP addresses should be refreshed everytime whenever the DNS + // name is being used. From the point of view of OVN-Kubernetes, the IP addresses are + // refreshed every defaultMinTTL. + klog.V(5).Infof("TTL value is 0 for domain: %q, defaulting ttl=%s", domain, defaultMinTTL.String()) + return ips, defaultMinTTL, false, nil } - return removeDuplicateIPs(ips), ttl, nil + return ips, ttl, false, nil } func (d *DNS) GetNextQueryTime() (time.Time, string, bool) { diff --git a/go-controller/pkg/util/dns_test.go b/go-controller/pkg/util/dns_test.go index a9d248042b..9f40c176ba 100644 --- a/go-controller/pkg/util/dns_test.go +++ b/go-controller/pkg/util/dns_test.go @@ -70,13 +70,16 @@ func TestGetIPsAndMinTTL(t *testing.T) { tests := []struct { desc string errExp bool + retry bool ipv4Mode bool ipv6Mode bool dnsOpsMockHelper []ovntest.TestifyMockHelper + expectedTTL time.Duration }{ { desc: "call to Exchange fails IPv4 only", errExp: true, + retry: true, ipv4Mode: true, ipv6Mode: false, dnsOpsMockHelper: []ovntest.TestifyMockHelper{ @@ -89,10 +92,12 @@ func TestGetIPsAndMinTTL(t *testing.T) { CallTimes: 1, }, }, + expectedTTL: defaultMinTTL, }, { desc: "Exchange returns correctly but Rcode != RcodeSuccess IPv4 only", errExp: true, + retry: true, ipv4Mode: true, ipv6Mode: false, dnsOpsMockHelper: []ovntest.TestifyMockHelper{ @@ -105,6 +110,46 @@ func TestGetIPsAndMinTTL(t *testing.T) { CallTimes: 1, }, }, + expectedTTL: defaultMinTTL, + }, + { + desc: "Exchange returns correctly but with TTL 0 IPv4 only", + errExp: false, + retry: false, + ipv4Mode: true, + ipv6Mode: false, + dnsOpsMockHelper: []ovntest.TestifyMockHelper{ + {OnCallMethodName: "SetQuestion", OnCallMethodArgType: []string{"*dns.Msg", "string", "uint16"}, RetArgList: []interface{}{&dns.Msg{}}, CallTimes: 1}, + {OnCallMethodName: "Fqdn", OnCallMethodArgType: []string{"string"}, RetArgList: []interface{}{"www.test.com"}, CallTimes: 1}, + {OnCallMethodName: "Exchange", OnCallMethodArgType: []string{"*dns.Client", "*dns.Msg", "string"}, RetArgList: []interface{}{&dns.Msg{MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, Answer: []dns.RR{&dns.A{A: net.ParseIP("1.2.3.4")}}}, 0 * time.Second, nil}, CallTimes: 1}, + }, + expectedTTL: defaultMinTTL, + }, + { + desc: "Exchange returns correctly but no Answer IPv4 only", + errExp: true, + retry: true, + ipv4Mode: true, + ipv6Mode: false, + dnsOpsMockHelper: []ovntest.TestifyMockHelper{ + {OnCallMethodName: "SetQuestion", OnCallMethodArgType: []string{"*dns.Msg", "string", "uint16"}, RetArgList: []interface{}{&dns.Msg{}}, CallTimes: 1}, + {OnCallMethodName: "Fqdn", OnCallMethodArgType: []string{"string"}, RetArgList: []interface{}{"www.test.com"}, CallTimes: 1}, + {OnCallMethodName: "Exchange", OnCallMethodArgType: []string{"*dns.Client", "*dns.Msg", "string"}, RetArgList: []interface{}{&dns.Msg{MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, Answer: []dns.RR{}}, 0 * time.Second, nil}, CallTimes: 1}, + }, + expectedTTL: defaultMinTTL, + }, + { + desc: "Exchange returns correctly but with non-zero TTL IPv4 only", + errExp: false, + retry: false, + ipv4Mode: true, + ipv6Mode: false, + dnsOpsMockHelper: []ovntest.TestifyMockHelper{ + {OnCallMethodName: "SetQuestion", OnCallMethodArgType: []string{"*dns.Msg", "string", "uint16"}, RetArgList: []interface{}{&dns.Msg{}}, CallTimes: 1}, + {OnCallMethodName: "Fqdn", OnCallMethodArgType: []string{"string"}, RetArgList: []interface{}{"www.test.com"}, CallTimes: 1}, + {OnCallMethodName: "Exchange", OnCallMethodArgType: []string{"*dns.Client", "*dns.Msg", "string"}, RetArgList: []interface{}{&dns.Msg{MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, Answer: []dns.RR{&dns.A{Hdr: dns.RR_Header{Ttl: 100}, A: net.ParseIP("1.2.3.4")}}}, 0 * time.Second, nil}, CallTimes: 1}, + }, + expectedTTL: 100 * time.Second, }, } @@ -128,19 +173,22 @@ func TestGetIPsAndMinTTL(t *testing.T) { } config.IPv4Mode = tc.ipv4Mode config.IPv6Mode = tc.ipv6Mode - res, _, err := testDNS.getIPsAndMinTTL("www.test.com") - t.Log(res, err) + res, ttl, retry, err := testDNS.getIPsAndMinTTL("www.test.com") + t.Log(res, ttl, retry, err) if tc.errExp { require.Error(t, err) } else { require.NoError(t, err) } + assert.Equal(t, tc.retry, retry, "the exponentialBackoff variable should match the return from dns.getIPsAndMinTTL()") + assert.Equal(t, tc.expectedTTL, ttl, "the ttl variable should match the return from dns.getIPsAndMinTTL()") mockDNSOps.AssertExpectations(t) }) } } func TestUpdate(t *testing.T) { + config.IPv4Mode = true mockDNSOps := new(util_mocks.DNSOps) SetDNSLibOpsMockInst(mockDNSOps) @@ -252,6 +300,7 @@ func TestUpdate(t *testing.T) { } func TestAdd(t *testing.T) { + config.IPv4Mode = true dnsName := "www.testing.com" mockDNSOps := new(util_mocks.DNSOps) SetDNSLibOpsMockInst(mockDNSOps) @@ -319,3 +368,211 @@ func TestAdd(t *testing.T) { } } + +func TestIPsEqual(t *testing.T) { + tests := []struct { + desc string + oldips []net.IP + newips []net.IP + expEqual bool + }{ + { + desc: "oldips and newips are the same", + oldips: []net.IP{net.ParseIP("1.2.3.4")}, + newips: []net.IP{net.ParseIP("1.2.3.4")}, + expEqual: true, + }, + { + desc: "oldips and newips are different", + oldips: []net.IP{net.ParseIP("1.2.3.4")}, + newips: []net.IP{net.ParseIP("1.2.3.5")}, + expEqual: false, + }, + { + desc: "oldips and newips are different length", + oldips: []net.IP{net.ParseIP("1.2.3.4")}, + newips: []net.IP{net.ParseIP("1.2.3.4"), net.ParseIP("1.2.3.5")}, + expEqual: false, + }, + { + desc: "oldips is nil and newips is not nil", + oldips: nil, + newips: []net.IP{net.ParseIP("1.2.3.4"), net.ParseIP("1.2.3.5")}, + expEqual: false, + }, + { + desc: "oldips is empty and newips is not empty", + oldips: []net.IP{}, + newips: []net.IP{net.ParseIP("1.2.3.4"), net.ParseIP("1.2.3.5")}, + expEqual: false, + }, + { + desc: "oldips is not nil and newips is nil", + oldips: []net.IP{net.ParseIP("1.2.3.4"), net.ParseIP("1.2.3.5")}, + newips: nil, + expEqual: false, + }, + { + desc: "oldips is not empty and newips is empty", + oldips: []net.IP{net.ParseIP("1.2.3.4"), net.ParseIP("1.2.3.5")}, + newips: []net.IP{}, + expEqual: false, + }, + { + desc: "oldips and newips are both nil", + oldips: nil, + newips: nil, + expEqual: true, + }, + { + desc: "oldips and newips are both empty", + oldips: []net.IP{}, + newips: []net.IP{}, + expEqual: true, + }, + { + desc: "oldips is nil and newips is empty", + oldips: nil, + newips: []net.IP{}, + expEqual: true, + }, + { + desc: "oldips is empty and newips is nil", + oldips: []net.IP{}, + newips: nil, + expEqual: true, + }, + } + for i, tc := range tests { + t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) { + res := ipsEqual(tc.oldips, tc.newips) + assert.Equal(t, tc.expEqual, res) + }) + } +} + +func TestUpdateOne(t *testing.T) { + config.IPv4Mode = true + dnsName := "www.testing.com" + newIP := net.ParseIP("1.2.3.4") + fqdnOpsMockHelper := ovntest.TestifyMockHelper{ + OnCallMethodName: "Fqdn", OnCallMethodArgType: []string{"string"}, RetArgList: []interface{}{dnsName}, CallTimes: 1, + } + setQuestionOpsMockHelper := ovntest.TestifyMockHelper{ + OnCallMethodName: "SetQuestion", OnCallMethodArgType: []string{"*dns.Msg", "string", "uint16"}, RetArgList: []interface{}{&dns.Msg{}}, CallTimes: 1, + } + exchangeSuccessNoAnswerOpsMockHelper := ovntest.TestifyMockHelper{ + OnCallMethodName: "Exchange", OnCallMethodArgType: []string{"*dns.Client", "*dns.Msg", "string"}, RetArgList: []interface{}{&dns.Msg{MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, Answer: []dns.RR{}}, 0 * time.Second, nil}, CallTimes: 1, + } + exchangeSuccessZeroTTLOpsMockHelper := ovntest.TestifyMockHelper{ + OnCallMethodName: "Exchange", OnCallMethodArgType: []string{"*dns.Client", "*dns.Msg", "string"}, RetArgList: []interface{}{&dns.Msg{MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, Answer: []dns.RR{&dns.A{A: newIP}}}, 0 * time.Second, nil}, CallTimes: 1, + } + exchangeSuccessNonZeroTTLOpsMockHelper := ovntest.TestifyMockHelper{ + OnCallMethodName: "Exchange", OnCallMethodArgType: []string{"*dns.Client", "*dns.Msg", "string"}, RetArgList: []interface{}{&dns.Msg{MsgHdr: dns.MsgHdr{Rcode: dns.RcodeSuccess}, Answer: []dns.RR{&dns.A{Hdr: dns.RR_Header{Ttl: 100}, A: newIP}}}, 0 * time.Second, nil}, CallTimes: 1, + } + exchangeFailureOpsMockHelper := ovntest.TestifyMockHelper{ + OnCallMethodName: "Exchange", OnCallMethodArgType: []string{"*dns.Client", "*dns.Msg", "string"}, RetArgList: []interface{}{&dns.Msg{MsgHdr: dns.MsgHdr{Rcode: dns.RcodeServerFailure}}, 0 * time.Second, nil}, CallTimes: 1, + } + tests := []struct { + desc string + numCalls int + exchangeOpsMockHelper ovntest.TestifyMockHelper + expTTL time.Duration + }{ + { + desc: "when Exchange function returns with Rcode != RcodeSuccess, defaultMinTTL is used", + numCalls: 1, + exchangeOpsMockHelper: exchangeFailureOpsMockHelper, + expTTL: defaultMinTTL, + }, + { + desc: "when Exchange function returns successfully but without Answer, defaultMinTTL is used", + numCalls: 1, + exchangeOpsMockHelper: exchangeSuccessNoAnswerOpsMockHelper, + expTTL: defaultMinTTL, + }, + { + desc: "when TTL returned is 0 by Exchange function, defaultMinTTL is used", + numCalls: 1, + exchangeOpsMockHelper: exchangeSuccessZeroTTLOpsMockHelper, + expTTL: defaultMinTTL, + }, + { + desc: "when TTL returned is 0 by Exchange function 2 times, defaultMinTTL is used", + numCalls: 2, + exchangeOpsMockHelper: exchangeSuccessZeroTTLOpsMockHelper, + expTTL: defaultMinTTL, + }, + { + desc: "when TTL returned is 0 by Exchange function 11 times, defaultMinTTL is used", + numCalls: 11, + exchangeOpsMockHelper: exchangeSuccessZeroTTLOpsMockHelper, + expTTL: defaultMinTTL, + }, + { + desc: "when Exchange function returns with Rcode != RcodeSuccess twice, defaultMinTTL is used", + numCalls: 2, + exchangeOpsMockHelper: exchangeFailureOpsMockHelper, + expTTL: defaultMinTTL, + }, + { + desc: "when Exchange function returns with Rcode != RcodeSuccess 10 times, defaultMinTTL is used", + numCalls: 10, + exchangeOpsMockHelper: exchangeFailureOpsMockHelper, + expTTL: defaultMinTTL, + }, + { + desc: "when Exchange function returns with Rcode != RcodeSuccess 11 times, defaultMinTTL is doubled", + numCalls: 11, + exchangeOpsMockHelper: exchangeFailureOpsMockHelper, + expTTL: 2 * defaultMinTTL, + }, + { + desc: "when Exchange function returns with Rcode != RcodeSuccess 14 times, 16 (2^4) times defaultMinTTL is used", + numCalls: 14, + exchangeOpsMockHelper: exchangeFailureOpsMockHelper, + expTTL: 16 * defaultMinTTL, + }, + { + desc: "when Exchange function returns with Rcode != RcodeSuccess 15 times, defaultMaxTTL is used", + numCalls: 15, + exchangeOpsMockHelper: exchangeFailureOpsMockHelper, + expTTL: defaultMaxTTL, + }, + { + desc: "when TTL returned is non-zero by Exchange function, it is used", + numCalls: 1, + exchangeOpsMockHelper: exchangeSuccessNonZeroTTLOpsMockHelper, + expTTL: 100 * time.Second, + }, + } + for i, tc := range tests { + t.Run(fmt.Sprintf("%d:%s", i, tc.desc), func(t *testing.T) { + mockDNSOps := new(util_mocks.DNSOps) + SetDNSLibOpsMockInst(mockDNSOps) + dnsOpsMockHelper := []ovntest.TestifyMockHelper{fqdnOpsMockHelper, setQuestionOpsMockHelper, tc.exchangeOpsMockHelper} + for index := 0; index < tc.numCalls; index++ { + for _, item := range dnsOpsMockHelper { + call := mockDNSOps.On(item.OnCallMethodName) + for _, arg := range item.OnCallMethodArgType { + call.Arguments = append(call.Arguments, mock.AnythingOfType(arg)) + } + for _, ret := range item.RetArgList { + call.ReturnArguments = append(call.ReturnArguments, ret) + } + call.Once() + } + } + dns := DNS{ + dnsMap: make(map[string]dnsValue), + nameservers: []string{"1.1.1.1"}, + } + dns.dnsMap[dnsName] = dnsValue{} + for i := 0; i < tc.numCalls; i++ { + _, _ = dns.updateOne(dnsName) + } + assert.Equal(t, tc.expTTL, dns.dnsMap[dnsName].ttl) + mockDNSOps.AssertExpectations(t) + }) + } +}