From c51062191a5d8bb4420d047472d6752c20dd0179 Mon Sep 17 00:00:00 2001 From: Torsten Wunderlich Date: Wed, 30 Oct 2024 09:15:19 +0100 Subject: [PATCH 1/2] enable direct error handling for bundle plugin by returning downloader errors for bundles in manual trigger mode Signed-off-by: Torsten Wunderlich --- cmd/exec_test.go | 27 +++++++++++-- plugins/bundle/errors.go | 34 +++++++++++++++++ plugins/bundle/errors_test.go | 72 +++++++++++++++++++++++++++++++++++ plugins/bundle/plugin.go | 15 ++++++-- plugins/bundle/plugin_test.go | 62 ++++++++++++++++++++++++++++++ 5 files changed, 203 insertions(+), 7 deletions(-) create mode 100644 plugins/bundle/errors.go create mode 100644 plugins/bundle/errors_test.go diff --git a/cmd/exec_test.go b/cmd/exec_test.go index 25f7b46e27..146554d919 100644 --- a/cmd/exec_test.go +++ b/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 { diff --git a/plugins/bundle/errors.go b/plugins/bundle/errors.go new file mode 100644 index 0000000000..4dca13b4cb --- /dev/null +++ b/plugins/bundle/errors.go @@ -0,0 +1,34 @@ +package bundle + +import ( + "errors" + "fmt" +) + +// 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 { + Name string + Err error +} + +func (e Error) Error() string { + return fmt.Sprintf("'%s': %v", e.Name, e.Err) +} + +func (e Error) Unwrap() error { + return e.Err +} diff --git a/plugins/bundle/errors_test.go b/plugins/bundle/errors_test.go new file mode 100644 index 0000000000..888cff852e --- /dev/null +++ b/plugins/bundle/errors_test.go @@ -0,0 +1,72 @@ +package bundle + +import ( + "errors" + "fmt" + "testing" + + "github.com/open-policy-agent/opa/download" +) + +func TestErrors(t *testing.T) { + errs := Errors{ + Error{Name: "foo", Err: fmt.Errorf("foo error")}, + Error{Name: "bar", Err: fmt.Errorf("bar error")}, + } + + expected := "'foo': foo error\n'bar': bar error" + result := errs.Error() + + if result != expected { + t.Errorf("Expected: %v \nbut got: %v", expected, result) + } +} + +func TestUnwrapSlice(t *testing.T) { + fooErr := Error{Name: "foo", Err: fmt.Errorf("foo error")} + barErr := Error{Name: "bar", Err: 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) { + fooErr := Error{Name: "foo", Err: download.HTTPError{StatusCode: 500}} + barErr := Error{Name: "bar", Err: download.HTTPError{StatusCode: 400}} + + errs := Errors{fooErr, barErr} + + // unwrap first bundle.Error + var bundleError Error + if !errors.As(errs, &bundleError) { + t.Fatal("failed to unwrap Error") + } + if bundleError.Error() != fooErr.Error() { + t.Fatalf("expected: %v \ngot: %v", fooErr, bundleError) + } + + // unwrap first HTTPError + var httpError download.HTTPError + if !errors.As(errs, &httpError) { + t.Fatal("failed to unwrap Error") + } + if httpError.Error() != fooErr.Err.Error() { + t.Fatalf("expected: %v \ngot: %v", fooErr.Err, httpError) + } + + // unwrap HTTPError from bundle.Error + if !errors.As(bundleError, &httpError) { + t.Fatal("failed to unwrap HTTPError") + } + if httpError.Error() != fooErr.Err.Error() { + t.Fatalf("expected: %v \nbgot: %v", fooErr.Err, httpError) + } +} diff --git a/plugins/bundle/plugin.go b/plugins/bundle/plugin.go index 53ccde387c..c53b449769 100644 --- a/plugins/bundle/plugin.go +++ b/plugins/bundle/plugin.go @@ -257,6 +257,8 @@ 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 { @@ -264,9 +266,16 @@ func (p *Plugin) Trigger(ctx context.Context) error { } 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 && *p.config.Bundles[name].Trigger == plugins.TriggerManual { + errs = append(errs, Error{Name: name, Err: err}) + } + } + if len(errs) > 0 { + return errs } return nil } diff --git a/plugins/bundle/plugin_test.go b/plugins/bundle/plugin_test.go index 4650689f73..857ac280f4 100644 --- a/plugins/bundle/plugin_test.go +++ b/plugins/bundle/plugin_test.go @@ -6522,6 +6522,68 @@ func TestPluginManualTriggerWithTimeout(t *testing.T) { } } +func TestPluginManualTriggerWithServerError(t *testing.T) { + + 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" + var periodic plugins.TriggerMode = "periodic" + var delay int64 = 10 + polling := download.PollingConfig{MinDelaySeconds: &delay, MaxDelaySeconds: &delay} + + plugin := New(&Config{ + Bundles: map[string]*Source{ + "manual": { + Service: "default", + SizeLimitBytes: int64(bundle.DefaultSizeLimitBytes), + Config: download.Config{Trigger: &manual}, + }, + "periodic": { + Service: "default", + SizeLimitBytes: int64(bundle.DefaultSizeLimitBytes), + Config: download.Config{Trigger: &periodic, Polling: polling}, + }, + }, + }, 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.Name != "manual" { + t.Fatalf("expect error only for bundle with manual trigger mode") + } + } + } else { + t.Fatalf("expected type of error to be %s but got %s", reflect.TypeOf(bundleErrors), reflect.TypeOf(err)) + } +} + func TestGetNormalizedBundleName(t *testing.T) { cases := []struct { input string From 4da831525a55f4d13645a9c6079b7b8f295433f3 Mon Sep 17 00:00:00 2001 From: towunderlich Date: Wed, 6 Nov 2024 17:52:04 +0100 Subject: [PATCH 2/2] Fixed changes request by @ashutosh-narkar (squash before merge) * model Error similar to bundle status Signed-off-by: Torsten Wunderlich --- plugins/bundle/errors.go | 25 ++++++++- plugins/bundle/errors_test.go | 101 +++++++++++++++++++++++++++++----- plugins/bundle/plugin.go | 2 +- plugins/bundle/plugin_test.go | 2 +- 4 files changed, 111 insertions(+), 19 deletions(-) diff --git a/plugins/bundle/errors.go b/plugins/bundle/errors.go index 4dca13b4cb..d612efe75a 100644 --- a/plugins/bundle/errors.go +++ b/plugins/bundle/errors.go @@ -3,6 +3,8 @@ 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. @@ -21,12 +23,29 @@ func (e Errors) Error() string { } type Error struct { - Name string - Err error + 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("'%s': %v", e.Name, e.Err) + return fmt.Sprintf("Bundle name: %s, Code: %s, HTTPCode: %d, Message: %s", e.BundleName, errCode, e.HTTPCode, e.Message) } func (e Error) Unwrap() error { diff --git a/plugins/bundle/errors_test.go b/plugins/bundle/errors_test.go index 888cff852e..9bc106aaeb 100644 --- a/plugins/bundle/errors_test.go +++ b/plugins/bundle/errors_test.go @@ -5,16 +5,17 @@ import ( "fmt" "testing" + "github.com/open-policy-agent/opa/ast" "github.com/open-policy-agent/opa/download" ) func TestErrors(t *testing.T) { errs := Errors{ - Error{Name: "foo", Err: fmt.Errorf("foo error")}, - Error{Name: "bar", Err: fmt.Errorf("bar error")}, + NewBundleError("foo", fmt.Errorf("foo error")), + NewBundleError("bar", fmt.Errorf("bar error")), } - expected := "'foo': foo error\n'bar': 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 { @@ -23,8 +24,8 @@ func TestErrors(t *testing.T) { } func TestUnwrapSlice(t *testing.T) { - fooErr := Error{Name: "foo", Err: fmt.Errorf("foo error")} - barErr := Error{Name: "bar", Err: fmt.Errorf("bar error")} + fooErr := NewBundleError("foo", fmt.Errorf("foo error")) + barErr := NewBundleError("bar", fmt.Errorf("bar error")) errs := Errors{fooErr, barErr} @@ -39,18 +40,19 @@ func TestUnwrapSlice(t *testing.T) { } func TestUnwrap(t *testing.T) { - fooErr := Error{Name: "foo", Err: download.HTTPError{StatusCode: 500}} - barErr := Error{Name: "bar", Err: download.HTTPError{StatusCode: 400}} + 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{fooErr, barErr} + 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() != fooErr.Error() { - t.Fatalf("expected: %v \ngot: %v", fooErr, bundleError) + if bundleError.Error() != serverHTTPError.Error() { + t.Fatalf("expected: %v \ngot: %v", serverHTTPError, bundleError) } // unwrap first HTTPError @@ -58,15 +60,86 @@ func TestUnwrap(t *testing.T) { if !errors.As(errs, &httpError) { t.Fatal("failed to unwrap Error") } - if httpError.Error() != fooErr.Err.Error() { - t.Fatalf("expected: %v \ngot: %v", fooErr.Err, httpError) + 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() != fooErr.Err.Error() { - t.Fatalf("expected: %v \nbgot: %v", fooErr.Err, 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()) } } diff --git a/plugins/bundle/plugin.go b/plugins/bundle/plugin.go index c53b449769..9f7f9b3fac 100644 --- a/plugins/bundle/plugin.go +++ b/plugins/bundle/plugin.go @@ -271,7 +271,7 @@ func (p *Plugin) Trigger(ctx context.Context) error { err := d.Trigger(ctx) // only return errors for TriggerMode manual as periodic bundles will be retried if err != nil && *p.config.Bundles[name].Trigger == plugins.TriggerManual { - errs = append(errs, Error{Name: name, Err: err}) + errs = append(errs, NewBundleError(name, err)) } } if len(errs) > 0 { diff --git a/plugins/bundle/plugin_test.go b/plugins/bundle/plugin_test.go index 857ac280f4..a3d1dbf944 100644 --- a/plugins/bundle/plugin_test.go +++ b/plugins/bundle/plugin_test.go @@ -6575,7 +6575,7 @@ func TestPluginManualTriggerWithServerError(t *testing.T) { t.Fatalf("expected exactly one error, got %d", len(bundleErrors)) } for _, e := range bundleErrors { - if e.Name != "manual" { + if e.BundleName != "manual" { t.Fatalf("expect error only for bundle with manual trigger mode") } }