Skip to content

Commit

Permalink
fix: webhook updates (#454)
Browse files Browse the repository at this point in the history 
Fixes a couple of issues with webhooks. Upon "connecting" a workspace
with a repo, OTF checks to see if a webhook is already configured for
the repo, and if so, it checks for any discrepancies in its
configuration, and if there are discrepancies then it updates the
webhook configuration to bring into sync with what OTF has. However,
that code had a bug where even though the configuration was ok it would
flag up a discrepancy and so the webhook would always be updated.

Secondly, the Github client code for updating a webhook would neglect to
set all the required parameters. Specifically, it neglected to set the
"content-type" of the webhook payload, and thus Github would revert to
its default, `form`, whereas OTF requires it to be set to `json`.

This fix does two things:
1. Don't check for discrepancies in webhook config; instead **always**
update the webhook whenever a workspace (or module) is connected to a
repo. This also ensures any out-of-band alterations to the webhook
configuration on the repo are reverted.
2. Fix the Github client code to set the content-type when updating a
webhook.
  • Loading branch information
@leg100
leg100 authored May 24, 2023
1 parent e5a2f01 commit 9b411ce
Show file tree
Hide file tree
Showing 16 changed files with 419 additions and 260 deletions.
144 changes: 71 additions & 73 deletions internal/cloud/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,86 +4,84 @@ import (
"context"
)

type Client interface {
GetUser(ctx context.Context) (*User, error)
// ListRepositories lists repositories accessible to the current user.
ListRepositories(ctx context.Context, opts ListRepositoriesOptions) ([]string, error)
GetRepository(ctx context.Context, identifier string) (string, error)
// GetRepoTarball retrieves a .tar.gz tarball of a git repository
GetRepoTarball(ctx context.Context, opts GetRepoTarballOptions) ([]byte, error)
// CreateWebhook creates a webhook on the cloud provider, returning the
// provider's unique ID for the webhook.
CreateWebhook(ctx context.Context, opts CreateWebhookOptions) (string, error)
UpdateWebhook(ctx context.Context, opts UpdateWebhookOptions) error
GetWebhook(ctx context.Context, opts GetWebhookOptions) (Webhook, error)
DeleteWebhook(ctx context.Context, opts DeleteWebhookOptions) error
SetStatus(ctx context.Context, opts SetStatusOptions) error
// ListTags lists git tags on a repository. Each tag should be prefixed with
// 'tags/'.
ListTags(ctx context.Context, opts ListTagsOptions) ([]string, error)
}
type (
Client interface {
GetUser(ctx context.Context) (*User, error)
// ListRepositories lists repositories accessible to the current user.
ListRepositories(ctx context.Context, opts ListRepositoriesOptions) ([]string, error)
GetRepository(ctx context.Context, identifier string) (string, error)
// GetRepoTarball retrieves a .tar.gz tarball of a git repository
GetRepoTarball(ctx context.Context, opts GetRepoTarballOptions) ([]byte, error)
// CreateWebhook creates a webhook on the cloud provider, returning the
// provider's unique ID for the webhook.
CreateWebhook(ctx context.Context, opts CreateWebhookOptions) (string, error)
UpdateWebhook(ctx context.Context, id string, opts UpdateWebhookOptions) error
GetWebhook(ctx context.Context, opts GetWebhookOptions) (Webhook, error)
DeleteWebhook(ctx context.Context, opts DeleteWebhookOptions) error
SetStatus(ctx context.Context, opts SetStatusOptions) error
// ListTags lists git tags on a repository. Each tag should be prefixed with
// 'tags/'.
ListTags(ctx context.Context, opts ListTagsOptions) ([]string, error)
}

// ClientOptions are options for constructing a cloud client
type ClientOptions struct {
Hostname string
SkipTLSVerification bool
Credentials
}
// ClientOptions are options for constructing a cloud client
ClientOptions struct {
Hostname string
SkipTLSVerification bool
Credentials
}

type GetRepoTarballOptions struct {
Repo string // repo identifier, <owner>/<repo>
Ref *string // branch/tag/SHA ref, nil means default branch
}
GetRepoTarballOptions struct {
Repo string // repo identifier, <owner>/<repo>
Ref *string // branch/tag/SHA ref, nil means default branch
}

type ListRepositoriesOptions struct {
PageSize int
}
ListRepositoriesOptions struct {
PageSize int
}

// ListTagsOptions are options for listing tags on a vcs repository
type ListTagsOptions struct {
Repo string // repo identifier, <owner>/<repo>
Prefix string // only list tags that start with this string
}
// ListTagsOptions are options for listing tags on a vcs repository
ListTagsOptions struct {
Repo string // repo identifier, <owner>/<repo>
Prefix string // only list tags that start with this string
}

// Webhook is a cloud's configuration for a webhook.
type Webhook struct {
ID string // cloud's webhook ID
Repo string // identifier is <repo_owner>/<repo_name>
Events []VCSEventType
Endpoint string // the OTF URL that receives events
}
// Webhook is a cloud's configuration for a webhook.
Webhook struct {
ID string // cloud's webhook ID
Repo string // identifier is <repo_owner>/<repo_name>
Events []VCSEventType
Endpoint string // the OTF URL that receives events
}

type CreateWebhookOptions struct {
Repo string // repo identifier, <owner>/<repo>
Secret string // secret string for generating signature
Endpoint string // otf's external-facing host[:port]
Events []VCSEventType
}
CreateWebhookOptions struct {
Repo string // repo identifier, <owner>/<repo>
Secret string // secret string for generating signature
Endpoint string // otf's external-facing host[:port]
Events []VCSEventType
}

type UpdateWebhookOptions struct {
ID string // vcs' webhook ID
UpdateWebhookOptions CreateWebhookOptions

CreateWebhookOptions
}
// GetWebhookOptions are options for retrieving a webhook.
GetWebhookOptions struct {
Repo string // Repository identifier, <owner>/<repo>
ID string // vcs' webhook ID
}

// GetWebhookOptions are options for retrieving a webhook.
type GetWebhookOptions struct {
Repo string // Repository identifier, <owner>/<repo>
ID string // vcs' webhook ID
}
// DeleteWebhookOptions are options for deleting a webhook.
DeleteWebhookOptions struct {
Repo string // Repository identifier, <owner>/<repo>
ID string // vcs' webhook ID
}

// DeleteWebhookOptions are options for deleting a webhook.
type DeleteWebhookOptions struct {
Repo string // Repository identifier, <owner>/<repo>
ID string // vcs' webhook ID
}

// SetStatusOptions are options for setting a status on a VCS repo
type SetStatusOptions struct {
Workspace string // workspace name
Repo string // <owner>/<repo>
Ref string // git ref
Status VCSStatus
TargetURL string
Description string
}
// SetStatusOptions are options for setting a status on a VCS repo
SetStatusOptions struct {
Workspace string // workspace name
Repo string // <owner>/<repo>
Ref string // git ref
Status VCSStatus
TargetURL string
Description string
}
)
22 changes: 17 additions & 5 deletions internal/github/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package github
import (
"context"
"crypto/tls"
"errors"
"fmt"
"net/http"
"os"
Expand Down Expand Up @@ -248,13 +249,13 @@ func (g *Client) CreateWebhook(ctx context.Context, opts cloud.CreateWebhookOpti
return strconv.FormatInt(hook.GetID(), 10), nil
}

func (g *Client) UpdateWebhook(ctx context.Context, opts cloud.UpdateWebhookOptions) error {
func (g *Client) UpdateWebhook(ctx context.Context, id string, opts cloud.UpdateWebhookOptions) error {
owner, name, found := strings.Cut(opts.Repo, "/")
if !found {
return fmt.Errorf("malformed identifier: %s", opts.Repo)
}

intID, err := strconv.ParseInt(opts.ID, 10, 64)
intID, err := strconv.ParseInt(id, 10, 64)
if err != nil {
return err
}
Expand All @@ -272,8 +273,9 @@ func (g *Client) UpdateWebhook(ctx context.Context, opts cloud.UpdateWebhookOpti
_, _, err = g.client.Repositories.EditHook(ctx, owner, name, intID, &github.Hook{
Events: events,
Config: map[string]any{
"url": opts.Endpoint,
"secret": opts.Secret,
"url": opts.Endpoint,
"secret": opts.Secret,
"content_type": "json",
},
Active: internal.Bool(true),
})
Expand Down Expand Up @@ -312,11 +314,21 @@ func (g *Client) GetWebhook(ctx context.Context, opts cloud.GetWebhookOptions) (
}
}

// extracting OTF endpoint from github's config map is a bit of work...
rawEndpoint, ok := hook.Config["url"]
if !ok {
return cloud.Webhook{}, errors.New("missing url")
}
endpoint, ok := rawEndpoint.(string)
if !ok {
return cloud.Webhook{}, errors.New("url is not a string")
}

return cloud.Webhook{
ID: strconv.FormatInt(hook.GetID(), 10),
Repo: opts.Repo,
Events: events,
Endpoint: hook.GetURL(),
Endpoint: endpoint,
}, nil
}

Expand Down
9 changes: 5 additions & 4 deletions internal/github/test_server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,14 +161,13 @@ func NewTestServer(t *testing.T, opts ...TestServerOption) (*TestServer, cloud.C
// docs.github.com/en/rest/webhooks/repos#create-a-repository-webhook
mux.HandleFunc("/api/v3/repos/"+*srv.repo+"/hooks", func(w http.ResponseWriter, r *http.Request) {
// retrieve the webhook url
type options struct {
var opts struct {
Events []string `json:"events"`
Config struct {
URL string `json:"url"`
Secret string `json:"secret"`
} `json:"config"`
}
var opts options
if err := json.NewDecoder(r.Body).Decode(&opts); err != nil {
http.Error(w, err.Error(), http.StatusUnprocessableEntity)
return
Expand All @@ -190,11 +189,13 @@ func NewTestServer(t *testing.T, opts ...TestServerOption) (*TestServer, cloud.C
// https://docs.github.com/en/free-pro-team@latest/rest/reference/repos/#delete-a-repository-webhook
mux.HandleFunc("/api/v3/repos/"+*srv.repo+"/hooks/123", func(w http.ResponseWriter, r *http.Request) {
switch r.Method {
case "GET":
case "PATCH", "GET":
hook := github.Hook{
ID: internal.Int64(123),
Events: srv.HookEvents,
URL: srv.HookEndpoint,
Config: map[string]any{
"url": srv.HookEndpoint,
},
}
out, err := json.Marshal(hook)
require.NoError(t, err)
Expand Down
6 changes: 3 additions & 3 deletions internal/gitlab/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,8 +195,8 @@ func (g *Client) CreateWebhook(ctx context.Context, opts cloud.CreateWebhookOpti
return strconv.Itoa(hook.ID), nil
}

func (g *Client) UpdateWebhook(ctx context.Context, opts cloud.UpdateWebhookOptions) error {
id, err := strconv.Atoi(opts.ID)
func (g *Client) UpdateWebhook(ctx context.Context, id string, opts cloud.UpdateWebhookOptions) error {
intID, err := strconv.Atoi(id)
if err != nil {
return err
}
Expand All @@ -215,7 +215,7 @@ func (g *Client) UpdateWebhook(ctx context.Context, opts cloud.UpdateWebhookOpti
}
}

_, _, err = g.client.Projects.EditProjectHook(opts.Repo, id, editOpts)
_, _, err = g.client.Projects.EditProjectHook(opts.Repo, intID, editOpts)
if err != nil {
return err
}
Expand Down
44 changes: 27 additions & 17 deletions internal/repo/db.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,20 @@ import (
"github.com/leg100/otf/internal/sql/pggen"
)

// pgdb is the repo database on postgres
type pgdb struct {
internal.DB
factory
}
type (
// pgdb is the repo database on postgres
pgdb struct {
internal.DB
factory
}
hookRow struct {
WebhookID pgtype.UUID `json:"webhook_id"`
VCSID pgtype.Text `json:"vcs_id"`
Secret pgtype.Text `json:"secret"`
Identifier pgtype.Text `json:"identifier"`
Cloud pgtype.Text `json:"cloud"`
}
)

func newPGDB(db internal.DB, f factory) *pgdb {
return &pgdb{db, f}
Expand All @@ -24,7 +33,7 @@ func newPGDB(db internal.DB, f factory) *pgdb {
// getOrCreate gets a hook if it exists or creates it if it does not. Should be
// called within a tx to avoid concurrent access causing unpredictible results.
func (db *pgdb) getOrCreateHook(ctx context.Context, hook *hook) (*hook, error) {
result, err := db.FindWebhooksByRepo(ctx, sql.String(hook.identifier), sql.String(hook.cloud))
result, err := db.FindWebhookByRepo(ctx, sql.String(hook.identifier), sql.String(hook.cloud))
if err != nil {
return nil, sql.Error(err)
}
Expand Down Expand Up @@ -60,6 +69,14 @@ func (db *pgdb) getHookByID(ctx context.Context, id uuid.UUID) (*hook, error) {
return db.unmarshal(hookRow(result))
}

func (db *pgdb) getHookByIDForUpdate(ctx context.Context, id uuid.UUID) (*hook, error) {
result, err := db.FindWebhookByIDForUpdate(ctx, sql.UUID(id))
if err != nil {
return nil, sql.Error(err)
}
return db.unmarshal(hookRow(result))
}

func (db *pgdb) updateHookCloudID(ctx context.Context, id uuid.UUID, cloudID string) error {
_, err := db.UpdateWebhookVCSID(ctx, sql.String(cloudID), sql.UUID(id))
if err != nil {
Expand Down Expand Up @@ -122,25 +139,18 @@ func (db *pgdb) deleteHook(ctx context.Context, id uuid.UUID) (*hook, error) {
return db.unmarshal(hookRow(result))
}

// lock webhooks table within a transaction, providing a callback within which
// caller can use the transaction.
// lock webhooks table within a transaction, preventing anything else from
// updating the table. Provides a callback within which caller can use the
// transaction.
func (db *pgdb) lock(ctx context.Context, callback func(*pgdb) error) error {
return db.Tx(ctx, func(tx internal.DB) error {
if _, err := tx.Exec(ctx, "LOCK webhooks"); err != nil {
if _, err := tx.Exec(ctx, "LOCK TABLE tags IN EXCLUSIVE MODE"); err != nil {
return err
}
return callback(newPGDB(tx, db.factory))
})
}

type hookRow struct {
WebhookID pgtype.UUID `json:"webhook_id"`
VCSID pgtype.Text `json:"vcs_id"`
Secret pgtype.Text `json:"secret"`
Identifier pgtype.Text `json:"identifier"`
Cloud pgtype.Text `json:"cloud"`
}

func (db *pgdb) unmarshal(row hookRow) (*hook, error) {
opts := newHookOpts{
id: internal.UUID(row.WebhookID.Bytes),
Expand Down
Loading

0 comments on commit 9b411ce

Please sign in to comment.