Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Controller handle Hook errors #7081

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Fix: Controller handle Hook errors #7081

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e5cabc5
Fix: Controller handle Hook errors
N-o-Z Nov 29, 2023
14749f3
CR Fixes
N-o-Z Dec 1, 2023
202ca1c
Merge remote-tracking branch 'origin/master' into fix/handle-hook-err…
N-o-Z Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
91 changes: 71 additions & 20 deletions esti/hooks_failure_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ package esti

import (
"bytes"
"context"
"github.com/google/uuid"
"net/http"
"testing"
"text/template"
Expand All @@ -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 {
Expand All @@ -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",
Expand Down
25 changes: 9 additions & 16 deletions pkg/api/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Comment on lines +2229 to +2234
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HookAbortErr can hold (wrapped by graveler.ErrPreconditionFailed), so the default handler implementation will return http.StatusPreconditionFailed.
All is left is the logging which I think we should keep in all places we trigger hooks and not in the central location.

My two concerns are:

  • logging related to the operation, today we capture run_id tomorrow we will need the full content (ex: source branch and etc)
  • the downside in having central error location is we can't map where this error is being used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. HookAbortErr already wraps the underlying error from the hook execution.
  2. Logging is coupled with checking that we actually had a HookAbortErr which brings us back to exactly the same situation where we were - missing handling because we forgot to add the same code block in another operation that uses hooks


// order of case is important, more specific errors should be first
switch {
case errors.Is(err, graveler.ErrLinkAddressNotFound),
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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,
})
Expand Down
Loading