enable direct error handling for bundle plugin by returning downloade…

…r errors for bundles in manual trigger mode

Signed-off-by: Torsten Wunderlich <[email protected]>

cmd/exec_test.go

@@ -3,6 +3,7 @@ package cmd
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -11,6 +12,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/open-policy-agent/opa/ast"
 	"github.com/open-policy-agent/opa/cmd/internal/exec"
 	"github.com/open-policy-agent/opa/internal/file/archive"
 	loggingtest "github.com/open-policy-agent/opa/logging/test"
@@ -321,15 +323,32 @@ main contains "hello" if {
 
 					ctx, cancel := context.WithCancel(context.Background())
 					defer cancel()
-					go func() {
+					go func(expectedErrors []string) {
 						err := runExecWithContext(ctx, params)
 						// Note(philipc): Catch the expected cancellation
 						// errors, allowing unexpected test failures through.
 						if err != context.Canceled {
-							t.Error(err)
-							return
+							var errs ast.Errors
+							if errors.As(err, &errs) {
+								for _, expErr := range expectedErrors {
+									found := false
+									for _, e := range errs {
+										if strings.Contains(e.Error(), expErr) {
+											found = true
+											break
+										}
+									}
+									if !found {
+										t.Errorf("Could not find expected error: %s in %v", expErr, errs)
+										return
+									}
+								}
+							} else {
+								t.Error(err)
+								return
+							}
 						}
-					}()
+					}(tc.expErrs)
 
 					if !test.Eventually(t, 5*time.Second, func() bool {
 						for _, expErr := range tc.expErrs {

plugins/bundle/errors.go

@@ -0,0 +1,53 @@
+package bundle
+
+import (
+	"errors"
+	"fmt"
+
+	"github.com/open-policy-agent/opa/download"
+)
+
+// Errors represents a list of errors that occurred during a bundle load enriched by the bundle name.
+type Errors []Error
+
+func (e Errors) Unwrap() []error {
+	output := make([]error, len(e))
+	for i := range e {
+		output[i] = e[i]
+	}
+	return output
+}
+func (e Errors) Error() string {
+	err := errors.Join(e.Unwrap()...)
+	return err.Error()
+}
+
+type Error struct {
+	BundleName string
+	Code       string
+	HTTPCode   int
+	Message    string
+	Err        error
+}
+
+func NewBundleError(bundleName string, cause error) Error {
+	var (
+		httpError download.HTTPError
+	)
+	switch {
+	case cause == nil:
+		return Error{BundleName: bundleName, Code: "", HTTPCode: -1, Message: "", Err: nil}
+	case errors.As(cause, &httpError):
+		return Error{BundleName: bundleName, Code: errCode, HTTPCode: httpError.StatusCode, Message: httpError.Error(), Err: cause}
+	default:
+		return Error{BundleName: bundleName, Code: errCode, HTTPCode: -1, Message: cause.Error(), Err: cause}
+	}
+}
+
+func (e Error) Error() string {
+	return fmt.Sprintf("Bundle name: %s, Code: %s, HTTPCode: %d, Message: %s", e.BundleName, errCode, e.HTTPCode, e.Message)
+}
+
+func (e Error) Unwrap() error {
+	return e.Err
+}

plugins/bundle/errors_test.go

@@ -0,0 +1,145 @@
+package bundle
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/open-policy-agent/opa/ast"
+	"github.com/open-policy-agent/opa/download"
+)
+
+func TestErrors(t *testing.T) {
+	errs := Errors{
+		NewBundleError("foo", fmt.Errorf("foo error")),
+		NewBundleError("bar", fmt.Errorf("bar error")),
+	}
+
+	expected := "Bundle name: foo, Code: bundle_error, HTTPCode: -1, Message: foo error\nBundle name: bar, Code: bundle_error, HTTPCode: -1, Message: bar error"
+	result := errs.Error()
+
+	if result != expected {
+		t.Errorf("Expected: %v \nbut got: %v", expected, result)
+	}
+}
+
+func TestUnwrapSlice(t *testing.T) {
+	fooErr := NewBundleError("foo", fmt.Errorf("foo error"))
+	barErr := NewBundleError("bar", fmt.Errorf("bar error"))
+
+	errs := Errors{fooErr, barErr}
+
+	result := errs.Unwrap()
+
+	if result[0].Error() != fooErr.Error() {
+		t.Fatalf("expected %v \nbut got: %v", fooErr, result[0])
+	}
+	if result[1].Error() != barErr.Error() {
+		t.Fatalf("expected %v \nbut got: %v", barErr, result[1])
+	}
+}
+
+func TestUnwrap(t *testing.T) {
+	serverHTTPError := NewBundleError("server", download.HTTPError{StatusCode: 500})
+	clientHTTPError := NewBundleError("client", download.HTTPError{StatusCode: 400})
+	astErrors := ast.Errors{ast.NewError(ast.ParseErr, ast.NewLocation(nil, "foo.rego", 100, 2), "blarg")}
+
+	errs := Errors{serverHTTPError, clientHTTPError, NewBundleError("ast", astErrors)}
+
+	// unwrap first bundle.Error
+	var bundleError Error
+	if !errors.As(errs, &bundleError) {
+		t.Fatal("failed to unwrap Error")
+	}
+	if bundleError.Error() != serverHTTPError.Error() {
+		t.Fatalf("expected: %v \ngot: %v", serverHTTPError, bundleError)
+	}
+
+	// unwrap first HTTPError
+	var httpError download.HTTPError
+	if !errors.As(errs, &httpError) {
+		t.Fatal("failed to unwrap Error")
+	}
+	if httpError.Error() != serverHTTPError.Err.Error() {
+		t.Fatalf("expected: %v \ngot: %v", serverHTTPError.Err, httpError)
+	}
+
+	// unwrap HTTPError from bundle.Error
+	if !errors.As(bundleError, &httpError) {
+		t.Fatal("failed to unwrap HTTPError")
+	}
+	if httpError.Error() != serverHTTPError.Err.Error() {
+		t.Fatalf("expected: %v \nbgot: %v", serverHTTPError.Err, httpError)
+	}
+
+	var unwrappedAstErrors ast.Errors
+	if !errors.As(errs, &unwrappedAstErrors) {
+		t.Fatal("failed to unwrap ast.Errors")
+	}
+	if unwrappedAstErrors.Error() != astErrors.Error() {
+		t.Fatalf("expected: %v \ngot: %v", astErrors, unwrappedAstErrors)
+	}
+}
+
+func TestHTTPErrorWrapping(t *testing.T) {
+	err := download.HTTPError{StatusCode: 500}
+	bundleErr := NewBundleError("foo", err)
+
+	if bundleErr.BundleName != "foo" {
+		t.Fatalf("BundleName: expected: %v \ngot: %v", "foo", bundleErr.BundleName)
+	}
+	if bundleErr.HTTPCode != err.StatusCode {
+		t.Fatalf("HTTPCode: expected: %v \ngot: %v", err.StatusCode, bundleErr.HTTPCode)
+	}
+	if bundleErr.Message != err.Error() {
+		t.Fatalf("Message: expected: %v \ngot: %v", err.Error(), bundleErr.Message)
+	}
+	if bundleErr.Code != errCode {
+		t.Fatalf("Code: expected: %v \ngot: %v", errCode, bundleErr.Code)
+	}
+	if bundleErr.Err != err {
+		t.Fatalf("Err: expected: %v \ngot: %v", err, bundleErr.Err)
+	}
+}
+
+func TestASTErrorsWrapping(t *testing.T) {
+	err := ast.Errors{ast.NewError(ast.ParseErr, ast.NewLocation(nil, "foo.rego", 100, 2), "blarg")}
+	bundleErr := NewBundleError("foo", err)
+
+	if bundleErr.BundleName != "foo" {
+		t.Fatalf("BundleName: expected: %v \ngot: %v", "foo", bundleErr.BundleName)
+	}
+	if bundleErr.HTTPCode != -1 {
+		t.Fatalf("HTTPCode: expected: %v \ngot: %v", -1, bundleErr.HTTPCode)
+	}
+	if bundleErr.Message != err.Error() {
+		t.Fatalf("Message: expected: %v \ngot: %v", err.Error(), bundleErr.Message)
+	}
+	if bundleErr.Code != errCode {
+		t.Fatalf("Code: expected: %v \ngot: %v", errCode, bundleErr.Code)
+	}
+	if bundleErr.Err.Error() != err.Error() {
+		t.Fatalf("Err: expected: %v \ngot: %v", err.Error(), bundleErr.Err.Error())
+	}
+}
+
+func TestGenericErrorWrapping(t *testing.T) {
+	err := fmt.Errorf("foo error")
+	bundleErr := NewBundleError("foo", err)
+
+	if bundleErr.BundleName != "foo" {
+		t.Fatalf("BundleName: expected: %v \ngot: %v", "foo", bundleErr.BundleName)
+	}
+	if bundleErr.HTTPCode != -1 {
+		t.Fatalf("HTTPCode: expected: %v \ngot: %v", -1, bundleErr.HTTPCode)
+	}
+	if bundleErr.Message != err.Error() {
+		t.Fatalf("Message: expected: %v \ngot: %v", err.Error(), bundleErr.Message)
+	}
+	if bundleErr.Code != errCode {
+		t.Fatalf("Code: expected: %v \ngot: %v", errCode, bundleErr.Code)
+	}
+	if bundleErr.Err.Error() != err.Error() {
+		t.Fatalf("Err: expected: %v \ngot: %v", err.Error(), bundleErr.Err.Error())
+	}
+}

plugins/bundle/plugin.go

@@ -257,16 +257,29 @@ func (p *Plugin) Loaders() map[string]Loader {
 
 // Trigger triggers a bundle download on all configured bundles.
 func (p *Plugin) Trigger(ctx context.Context) error {
+	var errs Errors
+
 	p.mtx.Lock()
 	downloaders := map[string]Loader{}
 	for name, dl := range p.downloaders {
 		downloaders[name] = dl
 	}
 	p.mtx.Unlock()
 
-	for _, d := range downloaders {
-		// plugin callback will log the trigger error and include it in the bundle status
-		_ = d.Trigger(ctx)
+	for name, d := range downloaders {
+		// plugin callback will also log the trigger error and include it in the bundle status
+		err := d.Trigger(ctx)
+
+		// only return errors for TriggerMode manual as periodic bundles will be retried
+		if err != nil {
+			trigger := p.Config().Bundles[name].Trigger
+			if trigger != nil && *trigger == plugins.TriggerManual {
+				errs = append(errs, NewBundleError(name, err))
+			}
+		}
+	}
+	if len(errs) > 0 {
+		return errs
 	}
 	return nil
 }

plugins/bundle/plugin_test.go

@@ -6609,6 +6609,61 @@ func TestPluginManualTriggerWithTimeout(t *testing.T) {
 	}
 }
 
+func TestPluginManualTriggerWithServerError(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
+	defer cancel()
+
+	s := httptest.NewServer(http.HandlerFunc(func(resp http.ResponseWriter, _ *http.Request) {
+		resp.WriteHeader(500)
+	}))
+
+	// setup plugin pointing at fake server
+	manager := getTestManagerWithOpts([]byte(fmt.Sprintf(`{
+		"services": {
+			"default": {
+				"url": %q
+			}
+		}
+	}`, s.URL)))
+
+	var manual plugins.TriggerMode = "manual"
+
+	plugin := New(&Config{
+		Bundles: map[string]*Source{
+			"test": {
+				Service:        "default",
+				SizeLimitBytes: int64(bundle.DefaultSizeLimitBytes),
+				Config:         download.Config{Trigger: &manual},
+			},
+		},
+	}, manager)
+
+	err := plugin.Start(ctx)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// manually trigger bundle download
+	err = plugin.Trigger(ctx)
+
+	plugin.Stop(ctx)
+
+	var bundleErrors Errors
+	if errors.As(err, &bundleErrors) {
+		if len(bundleErrors) != 1 {
+			t.Fatalf("expected exactly one error, got %d", len(bundleErrors))
+		}
+		for _, e := range bundleErrors {
+			if e.BundleName != "test" {
+				t.Fatalf("expected error for bundle 'test' but got '%s'", e.BundleName)
+			}
+		}
+	} else {
+		t.Fatalf("expected type of error to be %s but got %s", reflect.TypeOf(bundleErrors), reflect.TypeOf(err))
+	}
+}
+
 // Warning: This test modifies package variables, and as
 // a result, cannot be run in parallel with other tests.
 func TestGetNormalizedBundleName(t *testing.T) {