Fix: Controller handle Hook errors (#7081)

* Fix: Controller handle Hook errors

* CR Fixes

esti/hooks_failure_test.go

@@ -2,6 +2,8 @@ package esti
 
 import (
 	"bytes"
+	"context"
+	"github.com/google/uuid"
 	"net/http"
 	"testing"
 	"text/template"
@@ -27,31 +29,38 @@ 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) {
 	hookFailToCommit(t, "timeout")
 }
 
 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 +75,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",

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
 	}
@@ -4135,13 +4134,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,
 		})