Add a Retrying modifier for spoof.ResponseChecker

test/conformance/blue_green_test.go

@@ -60,8 +60,7 @@ func probeDomain(logger *zap.SugaredLogger, clients *test.Clients, domain string
 		return err
 	}
 	// TODO(tcnghia): Replace this probing with Status check when we have them.
-	client.RetryCodes = []int{http.StatusNotFound, http.StatusServiceUnavailable}
-	_, err = client.Poll(req, test.MatchesAny)
+	_, err = client.Poll(req, test.Retrying(test.MatchesAny, http.StatusNotFound, http.StatusServiceUnavailable))
 	return err
 }
 

test/conformance/route_test.go

@@ -19,6 +19,7 @@ limitations under the License.
 package conformance
 
 import (
+	"net/http"
 	"strings"
 	"testing"
 
@@ -80,7 +81,12 @@ func assertResourcesUpdatedWhenRevisionIsReady(t *testing.T, logger *zap.Sugared
 		t.Fatalf("Error fetching Route %s: %v", names.Route, err)
 	}
 
-	err = test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, updatedRoute.Status.Domain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText")
+	err = test.WaitForEndpointState(
+		clients.Kube,
+		logger,
+		updatedRoute.Status.Domain,
+		test.Retrying(test.EventuallyMatchesBody(expectedText), http.StatusServiceUnavailable, http.StatusNotFound),
+		"WaitForEndpointToServeText")
 	if err != nil {
 		t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, updatedRoute.Status.Domain, expectedText, err)
 	}

test/conformance/service_test.go

@@ -20,6 +20,7 @@ package conformance
 
 import (
 	"encoding/json"
+	"net/http"
 	"strings"
 	"testing"
 
@@ -61,7 +62,12 @@ func updateServiceWithImage(clients *test.Clients, names test.ResourceNames, ima
 // Shamelessly cribbed from route_test. We expect the Route and Configuration to be ready if the Service is ready.
 func assertServiceResourcesUpdated(t *testing.T, logger *zap.SugaredLogger, clients *test.Clients, names test.ResourceNames, routeDomain, expectedGeneration, expectedText string) {
 	// TODO(#1178): Remove "Wait" from all checks below this point.
-	err := test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, routeDomain, test.EventuallyMatchesBody(expectedText), "WaitForEndpointToServeText")
+	err := test.WaitForEndpointState(
+		clients.Kube,
+		logger,
+		routeDomain,
+		test.Retrying(test.EventuallyMatchesBody(expectedText), http.StatusNotFound),
+		"WaitForEndpointToServeText")
 	if err != nil {
 		t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, routeDomain, expectedText, err)
 	}

test/e2e/autoscale_test.go

@@ -19,6 +19,7 @@ limitations under the License.
 package e2e
 
 import (
+	"net/http"
 	"strings"
 	"testing"
 
@@ -57,9 +58,8 @@ func generateTrafficBurst(clients *test.Clients, logger *zap.SugaredLogger, num
 		go func() {
 			test.WaitForEndpointState(clients.Kube,
 				logger,
-				test.Flags.ResolvableDomain,
 				domain,
-				test.EventuallyMatchesBody(autoscaleExpectedOutput),
+				test.Retrying(test.EventuallyMatchesBody(autoscaleExpectedOutput), http.StatusNotFound),
 				"MakingConcurrentRequests")
 			concurrentRequests <- true
 		}()
@@ -158,9 +158,10 @@ func TestAutoscaleUpDownUp(t *testing.T) {
 	err = test.WaitForEndpointState(
 		clients.Kube,
 		logger,
-		test.Flags.ResolvableDomain,
 		domain,
-		test.EventuallyMatchesBody(autoscaleExpectedOutput),
+		// Istio doesn't expose a status for us here: https://github.com/istio/istio/issues/6082
+		// TODO(tcnghia): Remove this when https://github.com/istio/istio/issues/882 is fixed.
+		test.Retrying(test.EventuallyMatchesBody(autoscaleExpectedOutput), http.StatusNotFound, http.StatusServiceUnavailable),
 		"CheckingEndpointAfterUpdating")
 	if err != nil {
 		t.Fatalf(`The endpoint for Route %s at domain %s didn't serve

test/e2e/build_test.go

@@ -18,6 +18,7 @@ limitations under the License.
 package e2e
 
 import (
+	"net/http"
 	"strings"
 	"testing"
 
@@ -70,7 +71,8 @@ func TestBuildAndServe(t *testing.T) {
 	}
 	domain := route.Status.Domain
 
-	if err := test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, domain, test.MatchesBody(helloWorldExpectedOutput), "HelloWorldServesText"); err != nil {
+	endState := test.Retrying(test.MatchesBody(helloWorldExpectedOutput), http.StatusNotFound)
+	if err := test.WaitForEndpointState(clients.Kube, logger, domain, endState, "HelloWorldServesText"); err != nil {
 		t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, domain, helloWorldExpectedOutput, err)
 	}
 

test/e2e/helloworld_test.go

@@ -19,6 +19,7 @@ limitations under the License.
 package e2e
 
 import (
+	"net/http"
 	"strings"
 	"testing"
 
@@ -58,7 +59,12 @@ func TestHelloWorld(t *testing.T) {
 	}
 	domain := route.Status.Domain
 
-	err = test.WaitForEndpointState(clients.Kube, logger, test.Flags.ResolvableDomain, domain, test.MatchesBody(helloWorldExpectedOutput), "HelloWorldServesText")
+	err = test.WaitForEndpointState(
+		clients.Kube,
+		logger,
+		domain,
+		test.Retrying(test.MatchesBody(helloWorldExpectedOutput), http.StatusNotFound),
+		"HelloWorldServesText")
 	if err != nil {
 		t.Fatalf("The endpoint for Route %s at domain %s didn't serve the expected text \"%s\": %v", names.Route, domain, helloWorldExpectedOutput, err)
 	}

test/request.go

@@ -35,6 +35,22 @@ func MatchesAny(_ *spoof.Response) (bool, error) {
 	return true, nil
 }
 
+// Retrying modifies a ResponseChecker to retry certain response codes.
+func Retrying(rc spoof.ResponseChecker, codes ...int) spoof.ResponseChecker {
+	return func(resp *spoof.Response) (bool, error) {
+		for _, code := range codes {
+			if resp.StatusCode == code {
+				// Returning (false, nil) causes SpoofingClient.Poll to retry.
+				// sc.logger.Infof("Retrying for code %v", resp.StatusCode)
+				return false, nil
+			}
+		}
+
+		// If we didn't match any retryable codes, invoke the ResponseChecker that we wrapped.
+		return rc(resp)
+	}
+}
+
 // MatchesBody checks that the *first* response body matches the "expected" body, otherwise failing.
 func MatchesBody(expected string) spoof.ResponseChecker {
 	return func(resp *spoof.Response) (bool, error) {
@@ -65,19 +81,16 @@ func EventuallyMatchesBody(expected string) spoof.ResponseChecker {
 // the domain in the request headers, otherwise it will make the request directly to domain.
 // desc will be used to name the metric that is emitted to track how long it took for the
 // domain to get into the state checked by inState.  Commas in `desc` must be escaped.
-func WaitForEndpointState(kubeClientset *kubernetes.Clientset, logger *zap.SugaredLogger, resolvableDomain bool, domain string, inState spoof.ResponseChecker, desc string) error {
+func WaitForEndpointState(kubeClientset *kubernetes.Clientset, logger *zap.SugaredLogger, domain string, inState spoof.ResponseChecker, desc string) error {
 	metricName := fmt.Sprintf("WaitForEndpointState/%s", desc)
 	_, span := trace.StartSpan(context.Background(), metricName)
 	defer span.End()
 
-	client, err := spoof.New(kubeClientset, logger, domain, resolvableDomain)
+	client, err := spoof.New(kubeClientset, logger, domain, Flags.ResolvableDomain)
 	if err != nil {
 		return err
 	}
 
-	// TODO(#348): The ingress endpoint tends to return 503's and 404's
-	client.RetryCodes = []int{http.StatusServiceUnavailable, http.StatusNotFound}
-
 	req, err := http.NewRequest(http.MethodGet, fmt.Sprintf("http://%s", domain), nil)
 	if err != nil {
 		return err

test/spoof/spoof.go

@@ -68,8 +68,6 @@ type SpoofingClient struct {
 	RequestInterval time.Duration
 	RequestTimeout  time.Duration
 
-	RetryCodes []int
-
 	endpoint string
 	domain   string
 
@@ -170,17 +168,6 @@ func (sc *SpoofingClient) Poll(req *http.Request, inState ResponseChecker) (*Res
 			return true, err
 		}
 
-		// TODO(jonjohnson): This could just be pulled out into a retrying ResponseChecker middleware thing.
-		if resp.StatusCode != http.StatusOK {
-			for _, code := range sc.RetryCodes {
-				if resp.StatusCode == code {
-					sc.logger.Infof("Retrying for code %v", resp.StatusCode)
-					return false, nil
-				}
-			}
-			return true, fmt.Errorf("Status code %d was not a retriable code (%v)", resp.StatusCode, sc.RetryCodes)
-		}
-
 		return inState(resp)
 	})