From e5cabc545fe46022fa311eba573d375adc12d16a Mon Sep 17 00:00:00 2001 From: Nir Ozery Date: Wed, 29 Nov 2023 20:00:34 +0000 Subject: [PATCH 1/2] Fix: Controller handle Hook errors --- esti/hooks_failure_test.go | 92 +++++++++++++++++++++++++++++--------- pkg/api/controller.go | 25 ++++------- 2 files changed, 81 insertions(+), 36 deletions(-) diff --git a/esti/hooks_failure_test.go b/esti/hooks_failure_test.go index 524abd2de97..0c4c10cba99 100644 --- a/esti/hooks_failure_test.go +++ b/esti/hooks_failure_test.go @@ -2,6 +2,8 @@ package esti import ( "bytes" + "context" + "github.com/google/uuid" "net/http" "testing" "text/template" @@ -27,6 +29,22 @@ hooks: timeout : {{.Timeout}} `)) +var actionPreCreateBranchTmpl = template.Must(template.New("action-pre-create-branch").Parse( + ` +name: Test Create Branch +description: set of checks to verify that branch is good +on: + pre-create-branch: + branches: + - * +hooks: + - id: test_webhook + type: webhook + properties: + url: "{{.URL}}/{{.Path}}" + timeout : {{.Timeout}} +`)) + const hooksTimeout = 2 * time.Second func TestHooksTimeout(t *testing.T) { @@ -34,24 +52,16 @@ func TestHooksTimeout(t *testing.T) { } func TestHooksFail(t *testing.T) { - hookFailToCommit(t, "fail") + t.Run("commit", func(t *testing.T) { + hookFailToCommit(t, "fail") + }) + t.Run("create_branch", func(t *testing.T) { + hookFailToCreateBranch(t, "fail") + }) } -func hookFailToCommit(t *testing.T, path string) { - ctx, logger, repo := setupTest(t) - defer tearDownTest(repo) - const branch = "feature-1" - - logger.WithField("branch", branch).Info("Create branch") - resp, err := client.CreateBranchWithResponse(ctx, repo, apigen.CreateBranchJSONRequestBody{ - Name: branch, - Source: mainBranch, - }) - require.NoError(t, err, "failed to create branch") - require.Equal(t, http.StatusCreated, resp.StatusCode()) - ref := string(resp.Body) - logger.WithField("branchRef", ref).Info("Branch created") - logger.WithField("branch", branch).Info("Upload initial content") +func createAction(t *testing.T, ctx context.Context, repo, branch, path string, tmp *template.Template) { + t.Helper() // render actions based on templates docData := struct { @@ -66,14 +76,56 @@ func hookFailToCommit(t *testing.T, path string) { var doc bytes.Buffer doc.Reset() - err = actionPreCommitTmpl.Execute(&doc, docData) + err := tmp.Execute(&doc, docData) require.NoError(t, err) - preCommitAction := doc.String() - - uploadResp, err := uploadContent(ctx, repo, branch, "_lakefs_actions/testing_pre_commit", preCommitAction) + content := doc.String() + uploadResp, err := uploadContent(ctx, repo, branch, "_lakefs_actions/"+uuid.NewString(), content) require.NoError(t, err) require.Equal(t, http.StatusCreated, uploadResp.StatusCode()) logger.WithField("branch", branch).Info("Commit initial content") +} + +func hookFailToCreateBranch(t *testing.T, path string) { + ctx, logger, repo := setupTest(t) + defer tearDownTest(repo) + const branch = "feature-1" + + logger.WithField("branch", branch).Info("Create branch") + resp, err := client.CreateBranchWithResponse(ctx, repo, apigen.CreateBranchJSONRequestBody{ + Name: branch, + Source: mainBranch, + }) + require.NoError(t, err, "failed to create branch") + require.Equal(t, http.StatusCreated, resp.StatusCode()) + + createAction(t, ctx, repo, branch, path, actionPreCreateBranchTmpl) + + logger.WithField("branch", "test_branch").Info("Create branch - expect failure") + resp, err = client.CreateBranchWithResponse(ctx, repo, apigen.CreateBranchJSONRequestBody{ + Name: "test_branch", + Source: branch, + }) + require.NoError(t, err) + require.Equal(t, http.StatusPreconditionFailed, resp.StatusCode()) +} + +func hookFailToCommit(t *testing.T, path string) { + ctx, logger, repo := setupTest(t) + defer tearDownTest(repo) + const branch = "feature-1" + + logger.WithField("branch", branch).Info("Create branch") + resp, err := client.CreateBranchWithResponse(ctx, repo, apigen.CreateBranchJSONRequestBody{ + Name: branch, + Source: mainBranch, + }) + require.NoError(t, err, "failed to create branch") + require.Equal(t, http.StatusCreated, resp.StatusCode()) + ref := string(resp.Body) + logger.WithField("branchRef", ref).Info("Branch created") + logger.WithField("branch", branch).Info("Upload initial content") + + createAction(t, ctx, repo, branch, path, actionPreCommitTmpl) commitResp, err := client.CommitWithResponse(ctx, repo, branch, &apigen.CommitParams{}, apigen.CommitJSONRequestBody{ Message: "Initial content", diff --git a/pkg/api/controller.go b/pkg/api/controller.go index 1074ba0c018..2f6a25e0e01 100644 --- a/pkg/api/controller.go +++ b/pkg/api/controller.go @@ -2225,6 +2225,14 @@ func (c *Controller) handleAPIErrorCallback(ctx context.Context, w http.Response log := c.Logger.WithContext(ctx).WithError(err) + // Handle Hook Errors + var hookAbortErr *graveler.HookAbortError + if errors.As(err, &hookAbortErr) { + log.WithField("run_id", hookAbortErr.RunID).Warn("aborted by hooks") + cb(w, r, http.StatusPreconditionFailed, err) + return true + } + // order of case is important, more specific errors should be first switch { case errors.Is(err, graveler.ErrLinkAddressNotFound), @@ -2503,15 +2511,6 @@ func (c *Controller) Commit(w http.ResponseWriter, r *http.Request, body apigen. } newCommit, err := c.Catalog.Commit(ctx, repository, branch, body.Message, user.Committer(), metadata, body.Date, params.SourceMetarange) - var hookAbortErr *graveler.HookAbortError - if errors.As(err, &hookAbortErr) { - c.Logger. - WithError(err). - WithField("run_id", hookAbortErr.RunID). - Warn("aborted by hooks") - writeError(w, r, http.StatusPreconditionFailed, err) - return - } if c.handleAPIError(ctx, w, r, err) { return } @@ -4134,13 +4133,7 @@ func (c *Controller) MergeIntoBranch(w http.ResponseWriter, r *http.Request, bod metadata, swag.StringValue(body.Strategy)) - var hookAbortErr *graveler.HookAbortError - switch { - case errors.As(err, &hookAbortErr): - c.Logger.WithError(err).WithField("run_id", hookAbortErr.RunID).Warn("aborted by hooks") - writeError(w, r, http.StatusPreconditionFailed, err) - return - case errors.Is(err, graveler.ErrConflictFound): + if errors.Is(err, graveler.ErrConflictFound) { writeResponse(w, r, http.StatusConflict, apigen.MergeResult{ Reference: reference, }) From 14749f35fd3c3ca1d2dfd2896f1b5e1ec3bf293b Mon Sep 17 00:00:00 2001 From: Nir Ozery Date: Fri, 1 Dec 2023 09:21:22 +0000 Subject: [PATCH 2/2] CR Fixes --- esti/hooks_failure_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/esti/hooks_failure_test.go b/esti/hooks_failure_test.go index 0c4c10cba99..3137db11f64 100644 --- a/esti/hooks_failure_test.go +++ b/esti/hooks_failure_test.go @@ -36,7 +36,6 @@ description: set of checks to verify that branch is good on: pre-create-branch: branches: - - * hooks: - id: test_webhook type: webhook