Adjust retry logic to address 5xx and tests for same.

Signed-off-by: Pushpalanka Jayawardhana <[email protected]>

Author: Pushpalanka

filters/openpolicyagent/openpolicyagent.go

@@ -523,12 +523,6 @@ func (opa *OpenPolicyAgentInstance) Start(ctx context.Context, timeout time.Dura
 	}
 
 	select {
-	case <-done:
-		return nil
-	case err := <-failed:
-		opa.Close(ctx)
-
-		return err
 	case <-ctx.Done():
 		timeoutErr := ctx.Err()
 
@@ -546,6 +540,13 @@ func (opa *OpenPolicyAgentInstance) Start(ctx context.Context, timeout time.Dura
 			return fmt.Errorf("one or more open policy agent plugins failed to start in %v with error: %w", timeout, timeoutErr)
 		}
 		return fmt.Errorf("one or more open policy agent plugins failed to start in %v", timeout)
+
+	case <-done:
+		return nil
+	case err := <-failed:
+		opa.Close(ctx)
+
+		return err
 	}
 }
 
@@ -822,7 +823,7 @@ func (l *QuietLogger) Warn(fmt string, a ...interface{}) {
 	l.target.Warn(fmt, a)
 }
 
-var temporaryErrorHTTPCodes = []int64{
+var temporaryClientErrorHTTPCodes = []int64{
 	429, // too many requests
 	408, // request timeout
 }
@@ -838,10 +839,15 @@ func handleStatusErrors(
 			return
 		}
 		code, err := status.HTTPCode.Int64()
-		if err == nil && code >= 400 && code < 500 && !isTemporaryError(code) {
-			//Fail for error codes in the range 400-500 excluding temporary errors
-			failed <- formatStatusError(prefix, status)
-			return
+		if err == nil {
+			if code >= 400 && code < 500 && !isTemporaryError(code) {
+				// Fail for error codes in the range 400-500 excluding temporary errors
+				failed <- formatStatusError(prefix, status)
+				return
+			} else if code >= 500 {
+				// Do not fail for 5xx errors and keep retrying
+				return
+			}
 		}
 		if err != nil {
 			failed <- formatStatusError(prefix, status)
@@ -851,7 +857,7 @@ func handleStatusErrors(
 }
 
 func isTemporaryError(code int64) bool {
-	for _, tempCode := range temporaryErrorHTTPCodes {
+	for _, tempCode := range temporaryClientErrorHTTPCodes {
 		if code == tempCode {
 			return true
 		}

filters/openpolicyagent/openpolicyagent_test.go

@@ -712,31 +712,39 @@ func TestOpaActivationFailureWithInvalidDiscovery(t *testing.T) {
 }
 
 func TestDiscoveryRetryTemporaryError429(t *testing.T) {
-	testDiscoveryBundleTemporaryError(t, http.StatusTooManyRequests, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
+	testDiscoveryBundleError(t, http.StatusTooManyRequests, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
 }
 
 func TestDiscoveryRetryTemporaryError408(t *testing.T) {
-	testDiscoveryBundleTemporaryError(t, http.StatusRequestTimeout, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
+	testDiscoveryBundleError(t, http.StatusRequestTimeout, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
 }
 
-func TestDiscoveryFailure503BundleError(t *testing.T) {
-	testDiscoveryBundleTemporaryError(t, http.StatusServiceUnavailable, "discovery plugin failed: Name: bundle, Code: bundle_error, Message: server replied with Service Unavailable, HTTPCode: 503")
+func TestDiscoveryRetry5xx(t *testing.T) {
+	testDiscoveryBundleError(t, http.StatusInternalServerError, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
+}
+
+func TestDiscoveryFailure403BundleError(t *testing.T) {
+	testDiscoveryBundleError(t, http.StatusForbidden, "discovery plugin failed: Name: discovery, Code: bundle_error, Message: server replied with Forbidden, HTTPCode: 403")
 }
 
 func TestResourceBundleRetryTemporaryError429(t *testing.T) {
-	testResourceBundleTemporaryError(t, http.StatusTooManyRequests, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
+	testResourceBundleError(t, http.StatusTooManyRequests, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
 }
 
 func TestResourceBundleRetryTemporaryError408(t *testing.T) {
-	testResourceBundleTemporaryError(t, http.StatusRequestTimeout, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
+	testResourceBundleError(t, http.StatusRequestTimeout, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
+}
+
+func TestResourceBundleRetry5xx(t *testing.T) {
+	testResourceBundleError(t, http.StatusInternalServerError, "one or more open policy agent plugins failed to start in 1s with error: context deadline exceeded")
 }
 
-func TestResourceBundleFailure503BundleError(t *testing.T) {
-	testResourceBundleTemporaryError(t, http.StatusServiceUnavailable, "bundle plugin failed: Name: bundle, Code: bundle_error, Message: server replied with Service Unavailable, HTTPCode: 503")
+func TestResourceBundleFailure403BundleError(t *testing.T) {
+	testResourceBundleError(t, http.StatusForbidden, "bundle plugin failed: Name: test, Code: bundle_error, Message: server replied with Forbidden, HTTPCode: 403")
 }
 
 // Helper function to test discovery bundle temporary errors
-func testDiscoveryBundleTemporaryError(t *testing.T, statusCode int, expectedError string) {
+func testDiscoveryBundleError(t *testing.T, statusCode int, expectedError string) {
 	mockDiscoveryBundleServer := mockBundleServerWithStatus("/bundles/discovery", statusCode)
 	defer mockDiscoveryBundleServer.Close()
 
@@ -768,7 +776,7 @@ func testDiscoveryBundleTemporaryError(t *testing.T, statusCode int, expectedErr
 }
 
 // Helper function to test resource bundle temporary errors
-func testResourceBundleTemporaryError(t *testing.T, statusCode int, expectedError string) {
+func testResourceBundleError(t *testing.T, statusCode int, expectedError string) {
 	mockServer := mockBundleServerWithStatus("/bundles/test.tgz", statusCode)
 	defer mockServer.Close()