From 0f60be3ffced5185ce24ac4d563ef1745b80e94f Mon Sep 17 00:00:00 2001 From: Kegan Dougal Date: Mon, 22 Aug 2016 12:42:48 +0100 Subject: [PATCH] Create delta webhooks in GithubService.Register() Remove PostRegister() as it isn't used for anything anymore. Add TODO marker for guarding against race conditions. --- src/github.com/matrix-org/go-neb/api.go | 54 ++++--- .../matrix-org/go-neb/services/echo/echo.go | 9 +- .../go-neb/services/github/github.go | 151 ++++++++---------- .../matrix-org/go-neb/services/jira/jira.go | 5 +- .../matrix-org/go-neb/types/types.go | 3 +- src/github.com/matrix-org/go-neb/util/util.go | 35 ++++ 6 files changed, 141 insertions(+), 116 deletions(-) create mode 100644 src/github.com/matrix-org/go-neb/util/util.go diff --git a/src/github.com/matrix-org/go-neb/api.go b/src/github.com/matrix-org/go-neb/api.go index 33ac64a..e1168e5 100644 --- a/src/github.com/matrix-org/go-neb/api.go +++ b/src/github.com/matrix-org/go-neb/api.go @@ -211,29 +211,19 @@ func (s *configureServiceHandler) OnIncomingRequest(req *http.Request) (interfac return nil, &errors.HTTPError{nil, "Unsupported Method", 405} } - var body struct { - ID string - Type string - UserID string - Config json.RawMessage - } - if err := json.NewDecoder(req.Body).Decode(&body); err != nil { - return nil, &errors.HTTPError{err, "Error parsing request JSON", 400} + service, httpErr := s.createService(req) + if httpErr != nil { + return nil, httpErr } - if body.ID == "" || body.Type == "" || body.UserID == "" || body.Config == nil { - return nil, &errors.HTTPError{ - nil, `Must supply an "ID", a "Type", a "UserID" and a "Config"`, 400, - } - } + // TODO mutex lock keyed off service ID - service, err := types.CreateService(body.ID, body.Type, body.UserID, body.Config) - if err != nil { - return nil, &errors.HTTPError{err, "Error parsing config JSON", 400} + old, err := s.db.LoadService(service.ServiceID()) + if err != nil && err != sql.ErrNoRows { + return nil, &errors.HTTPError{err, "Error loading old service", 500} } - err = service.Register() - if err != nil { + if err = service.Register(old); err != nil { return nil, &errors.HTTPError{err, "Failed to register service: " + err.Error(), 500} } @@ -247,14 +237,38 @@ func (s *configureServiceHandler) OnIncomingRequest(req *http.Request) (interfac return nil, &errors.HTTPError{err, "Error storing service", 500} } - service.PostRegister(oldService) + // TODO mutex unlock keyed off service ID return &struct { ID string Type string OldConfig types.Service NewConfig types.Service - }{body.ID, body.Type, oldService, service}, nil + }{service.ServiceID(), service.ServiceType(), oldService, service}, nil +} + +func (s *configureServiceHandler) createService(req *http.Request) (types.Service, *errors.HTTPError) { + var body struct { + ID string + Type string + UserID string + Config json.RawMessage + } + if err := json.NewDecoder(req.Body).Decode(&body); err != nil { + return nil, &errors.HTTPError{err, "Error parsing request JSON", 400} + } + + if body.ID == "" || body.Type == "" || body.UserID == "" || body.Config == nil { + return nil, &errors.HTTPError{ + nil, `Must supply an "ID", a "Type", a "UserID" and a "Config"`, 400, + } + } + + service, err := types.CreateService(body.ID, body.Type, body.UserID, body.Config) + if err != nil { + return nil, &errors.HTTPError{err, "Error parsing config JSON", 400} + } + return service, nil } type getServiceHandler struct { diff --git a/src/github.com/matrix-org/go-neb/services/echo/echo.go b/src/github.com/matrix-org/go-neb/services/echo/echo.go index dfac071..3e0d77a 100644 --- a/src/github.com/matrix-org/go-neb/services/echo/echo.go +++ b/src/github.com/matrix-org/go-neb/services/echo/echo.go @@ -13,11 +13,10 @@ type echoService struct { serviceUserID string } -func (e *echoService) ServiceUserID() string { return e.serviceUserID } -func (e *echoService) ServiceID() string { return e.id } -func (e *echoService) ServiceType() string { return "echo" } -func (e *echoService) Register() error { return nil } -func (e *echoService) PostRegister(old types.Service) {} +func (e *echoService) ServiceUserID() string { return e.serviceUserID } +func (e *echoService) ServiceID() string { return e.id } +func (e *echoService) ServiceType() string { return "echo" } +func (e *echoService) Register(oldService types.Service) error { return nil } func (e *echoService) Plugin(roomID string) plugin.Plugin { return plugin.Plugin{ Commands: []plugin.Command{ diff --git a/src/github.com/matrix-org/go-neb/services/github/github.go b/src/github.com/matrix-org/go-neb/services/github/github.go index ff60f99..de2efe0 100644 --- a/src/github.com/matrix-org/go-neb/services/github/github.go +++ b/src/github.com/matrix-org/go-neb/services/github/github.go @@ -11,6 +11,7 @@ import ( "github.com/matrix-org/go-neb/services/github/client" "github.com/matrix-org/go-neb/services/github/webhook" "github.com/matrix-org/go-neb/types" + "github.com/matrix-org/go-neb/util" "net/http" "regexp" "sort" @@ -203,7 +204,7 @@ func (s *githubService) OnReceiveWebhook(w http.ResponseWriter, req *http.Reques // // Hooks can get out of sync if a user manually deletes a hook in the Github UI. In this case, toggling the repo configuration will // force NEB to recreate the hook. -func (s *githubService) Register() error { +func (s *githubService) Register(oldService types.Service) error { if s.RealmID == "" { return fmt.Errorf("RealmID is required") } @@ -225,108 +226,88 @@ func (s *githubService) Register() error { "User %s does not have a Github auth session with realm %s.", s.ClientUserID, realm.ID()) } // Make sure they have specified some webhooks (it makes no sense otherwise) - - // Work out - - } - - log.Infof("%+v", s) - - return nil -} - -func (s *githubService) PostRegister(oldService types.Service) { - // PostRegister handles creating/destroying webhooks, which is only valid if this service - // is configured on behalf of a client. - if s.ClientUserID == "" { - if len(s.Rooms) != 0 { - log.WithFields(log.Fields{ - "Rooms": s.Rooms, - }).Error("Empty ClientUserID but a webhook config was supplied.") + reposForWebhooks := s.repoList() + if len(reposForWebhooks) == 0 { + return fmt.Errorf("No repos for webhooks specified") } - return - } - cli := s.githubClientFor(s.ClientUserID, false) - if cli == nil { - log.Errorf("PostRegister: %s does not have a github session", s.ClientUserID) - return - } - - if oldService != nil { + // Fetch the old service list and work out the difference between the two. + var oldRepos []string old, ok := oldService.(*githubService) if !ok { - log.Error("PostRegister: Provided old service is not of type GithubService") - return + log.WithFields(log.Fields{ + "service_id": oldService.ServiceID(), + "service_type": oldService.ServiceType(), + }).Print("Cannot case old github service to GithubService") + // non-fatal though, we'll just make the hooks + } else { + oldRepos = old.repoList() } - // Don't spam github webhook requests if we can help it. - if sameRepos(s, old) { - log.Print("PostRegister: old and new services have the same repo set. Nooping.") - return + // Add the repos in the new service but not the old service + newRepos, _ := util.Difference(reposForWebhooks, oldRepos) + for _, r := range newRepos { + logger := log.WithField("repo", r) + err := s.createHook(cli, r) + if err != nil { + logger.WithError(err).Error("Failed to create webhook") + return err + } + logger.Info("Created webhook") } - - // TODO: We should be adding webhooks in Register() then removing old hooks in PostRegister() - // - // By doing both operations in PostRegister(), if some of the requests fail we can end up in - // an inconsistent state. It is a lot simpler and easy to reason about this way though, so - // for now it will do. - - // remove any existing webhooks this service created on the user's behalf - modifyWebhooks(old, cli, true) } - // make new webhooks according to service config - modifyWebhooks(s, cli, false) + log.Infof("%+v", s) + + return nil } -func modifyWebhooks(s *githubService, cli *github.Client, removeHooks bool) { - ownerRepoSet := make(map[string]bool) - for _, roomCfg := range s.Rooms { - for ownerRepo := range roomCfg.Repos { - // sanity check that it looks like 'owner/repo' as we'll split on / later +func (s *githubService) repoList() []string { + var repos []string + if s.Rooms == nil { + return repos + } + for _, roomConfig := range s.Rooms { + for ownerRepo := range roomConfig.Repos { if strings.Count(ownerRepo, "/") != 1 { - log.WithField("owner_repo", ownerRepo).Print("Bad owner/repo value.") + log.WithField("repo", ownerRepo).Error("Bad owner/repo key in config") continue } - ownerRepoSet[ownerRepo] = true - } - } - - for ownerRepo := range ownerRepoSet { - o := strings.Split(ownerRepo, "/") - owner := o[0] - repo := o[1] - logger := log.WithFields(log.Fields{ - "owner": owner, - "repo": repo, - }) - if removeHooks { - removeHook(logger, cli, owner, repo, s.webhookEndpointURL) - } else { - // make a hook for all GH events since we'll filter it when we receive webhook requests - name := "web" // https://developer.github.com/v3/repos/hooks/#create-a-hook - cfg := map[string]interface{}{ - "content_type": "json", - "url": s.webhookEndpointURL, - } - if s.SecretToken != "" { - cfg["secret"] = s.SecretToken + exists := false + for _, r := range repos { + if r == ownerRepo { + exists = true + break + } } - events := []string{"push", "pull_request", "issues", "issue_comment", "pull_request_review_comment"} - _, _, err := cli.Repositories.CreateHook(owner, repo, &github.Hook{ - Name: &name, - Config: cfg, - Events: events, - }) - if err != nil { - logger.WithError(err).Print("Failed to create webhook") - // continue as others may succeed - } else { - logger.WithField("endpoint", s.webhookEndpointURL).Print("Created hook with endpoint") + if !exists { + repos = append(repos, ownerRepo) } } } + return repos +} + +func (s *githubService) createHook(cli *github.Client, ownerRepo string) error { + o := strings.Split(ownerRepo, "/") + owner := o[0] + repo := o[1] + // make a hook for all GH events since we'll filter it when we receive webhook requests + name := "web" // https://developer.github.com/v3/repos/hooks/#create-a-hook + cfg := map[string]interface{}{ + "content_type": "json", + "url": s.webhookEndpointURL, + } + if s.SecretToken != "" { + cfg["secret"] = s.SecretToken + } + events := []string{"push", "pull_request", "issues", "issue_comment", "pull_request_review_comment"} + _, _, err := cli.Repositories.CreateHook(owner, repo, &github.Hook{ + Name: &name, + Config: cfg, + Events: events, + }) + return err } func sameRepos(a *githubService, b *githubService) bool { diff --git a/src/github.com/matrix-org/go-neb/services/jira/jira.go b/src/github.com/matrix-org/go-neb/services/jira/jira.go index 94a8fce..ed400f4 100644 --- a/src/github.com/matrix-org/go-neb/services/jira/jira.go +++ b/src/github.com/matrix-org/go-neb/services/jira/jira.go @@ -41,7 +41,7 @@ type jiraService struct { func (s *jiraService) ServiceUserID() string { return s.serviceUserID } func (s *jiraService) ServiceID() string { return s.id } func (s *jiraService) ServiceType() string { return "jira" } -func (s *jiraService) Register() error { +func (s *jiraService) Register(oldService types.Service) error { // We only ever make 1 JIRA webhook which listens for all projects and then filter // on receive. So we simply need to know if we need to make a webhook or not. We // need to do this for each unique realm. @@ -61,9 +61,6 @@ func (s *jiraService) Register() error { } return nil } -func (s *jiraService) PostRegister(old types.Service) { - // TODO: We don't remove old JIRA webhooks for now. Let the admin sort it out. -} func (s *jiraService) cmdJiraCreate(roomID, userID string, args []string) (interface{}, error) { // E.g jira create PROJ "Issue title" "Issue desc" diff --git a/src/github.com/matrix-org/go-neb/types/types.go b/src/github.com/matrix-org/go-neb/types/types.go index f58eb61..dadea80 100644 --- a/src/github.com/matrix-org/go-neb/types/types.go +++ b/src/github.com/matrix-org/go-neb/types/types.go @@ -38,8 +38,7 @@ type Service interface { ServiceType() string Plugin(roomID string) plugin.Plugin OnReceiveWebhook(w http.ResponseWriter, req *http.Request, cli *matrix.Client) - Register() error - PostRegister(oldService Service) + Register(oldService Service) error } var baseURL = "" diff --git a/src/github.com/matrix-org/go-neb/util/util.go b/src/github.com/matrix-org/go-neb/util/util.go new file mode 100644 index 0000000..d8b59c7 --- /dev/null +++ b/src/github.com/matrix-org/go-neb/util/util.go @@ -0,0 +1,35 @@ +package util + +import ( + "sort" +) + +// Difference returns the elements that are only in the first list and +// the elements that are only in the second. As a side-effect this sorts +// the input lists in-place. +func Difference(a, b []string) (onlyA, onlyB []string) { + sort.Strings(a) + sort.Strings(b) + for { + if len(b) == 0 { + onlyA = append(onlyA, a...) + return + } + if len(a) == 0 { + onlyB = append(onlyB, b...) + return + } + xA := a[0] + xB := b[0] + if xA < xB { + onlyA = append(onlyA, xA) + a = a[1:] + } else if xA > xB { + onlyB = append(onlyB, xB) + b = b[1:] + } else { + a = a[1:] + b = b[1:] + } + } +}