plugin/forward: Fix incorrect failover counter reset (#8277)

* plugin/forward: Fix incorrect failover counter reset

This PR fixes the isseue where resetting the failover counter caused
retry the same upstreams until timeout instead of stopping after one pass.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

* golint fix

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

---------

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

plugin/forward/forward.go

@@ -10,6 +10,7 @@ import (
 	"errors"
 	"net"
 	"net/http"
+	"slices"
 	"sync/atomic"
 	"time"
 
@@ -126,6 +127,7 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 	}
 
 	fails := 0
+	failoverAttempts := 0
 	var span, child ot.Span
 	var upstreamErr error
 	span = ot.SpanFromContext(ctx)
@@ -236,16 +238,11 @@ func (f *Forward) ServeDNS(ctx context.Context, w dns.ResponseWriter, r *dns.Msg
 
 		// Check if we have a failover Rcode defined, check if we match on the code
 		tryNext := false
-		for _, failoverRcode := range f.failoverRcodes {
-			// if we match, we continue to the next upstream in the list
-			if failoverRcode == ret.Rcode {
-				if fails < len(f.proxies) {
-					tryNext = true
-				}
-			}
+		if slices.Contains(f.failoverRcodes, ret.Rcode) {
+			failoverAttempts++
+			tryNext = failoverAttempts < len(f.proxies)
 		}
 		if tryNext {
-			fails++
 			continue
 		}
 

plugin/forward/forward_test.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"net"
 	"strings"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -238,3 +239,68 @@ func TestForward_NextOnNodata(t *testing.T) {
 		})
 	}
 }
+
+func TestForwardFailoverStopsAfterAllUpstreams(t *testing.T) {
+	var first atomic.Int32
+	var second atomic.Int32
+
+	s1 := dnstest.NewMultipleServer(func(w dns.ResponseWriter, r *dns.Msg) {
+		first.Add(1)
+
+		m := new(dns.Msg)
+		m.SetRcode(r, dns.RcodeServerFailure)
+		w.WriteMsg(m)
+	})
+	defer s1.Close()
+
+	s2 := dnstest.NewMultipleServer(func(w dns.ResponseWriter, r *dns.Msg) {
+		second.Add(1)
+
+		m := new(dns.Msg)
+		m.SetRcode(r, dns.RcodeServerFailure)
+		w.WriteMsg(m)
+	})
+	defer s2.Close()
+
+	c := caddy.NewTestController("dns", fmt.Sprintf(`forward . %s %s {
+		policy sequential
+		failover SERVFAIL
+	}`, s1.Addr, s2.Addr))
+
+	fs, err := parseForward(c)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	f := fs[0]
+	if err := f.OnStartup(); err != nil {
+		t.Fatal(err)
+	}
+	defer f.OnShutdown()
+
+	req := new(dns.Msg)
+	req.SetQuestion("example.org.", dns.TypeA)
+
+	rec := dnstest.NewRecorder(&test.ResponseWriter{})
+
+	ctx, cancel := context.WithTimeout(
+		context.Background(),
+		200*time.Millisecond,
+	)
+	defer cancel()
+
+	if _, err := f.ServeDNS(ctx, rec, req); err != nil {
+		t.Fatalf("Expected the last SERVFAIL response, got error: %v", err)
+	}
+
+	if rec.Rcode != dns.RcodeServerFailure {
+		t.Fatalf("Expected SERVFAIL, got %d", rec.Rcode)
+	}
+
+	if got := first.Load(); got != 1 {
+		t.Errorf("Expected first upstream to be queried once, got %d", got)
+	}
+	if got := second.Load(); got != 1 {
+		t.Errorf("Expected second upstream to be queried once, got %d", got)
+	}
+}