From 4569b60f09ce1c436d4e0f42b2d47a4a2ecebdd5 Mon Sep 17 00:00:00 2001 From: Brad Rydzewski Date: Fri, 5 May 2017 18:59:37 +0200 Subject: [PATCH 1/3] persist and compare yaml for gating --- drone/server/server.go | 1 + model/build.go | 1 + model/config.go | 34 +++--- model/repo.go | 1 + model/settings.go | 24 ++++ router/middleware/config.go | 4 +- router/middleware/session/user.go | 2 +- server/build.go | 16 ++- server/hook.go | 42 ++++++- server/login.go | 4 +- server/rpc.go | 5 +- store/datastore/config.go | 29 +++++ store/datastore/config_test.go | 106 ++++++++++++++++++ store/datastore/ddl/mysql/16.sql | 21 ++++ store/datastore/ddl/postgres/16.sql | 21 ++++ store/datastore/ddl/sqlite3/16.sql | 21 ++++ store/datastore/sql/postgres/files/config.sql | 22 ++++ store/datastore/sql/postgres/sql_gen.go | 25 +++++ store/datastore/sql/sqlite/files/config.sql | 22 ++++ store/datastore/sql/sqlite/sql_gen.go | 25 +++++ store/store.go | 5 + 21 files changed, 395 insertions(+), 36 deletions(-) create mode 100644 model/settings.go create mode 100644 store/datastore/config.go create mode 100644 store/datastore/config_test.go create mode 100644 store/datastore/ddl/mysql/16.sql create mode 100644 store/datastore/ddl/postgres/16.sql create mode 100644 store/datastore/ddl/sqlite3/16.sql create mode 100644 store/datastore/sql/postgres/files/config.sql create mode 100644 store/datastore/sql/sqlite/files/config.sql diff --git a/drone/server/server.go b/drone/server/server.go index a2eb5fd41..9993e1a2d 100644 --- a/drone/server/server.go +++ b/drone/server/server.go @@ -391,6 +391,7 @@ func setupEvilGlobals(c *cli.Context, v store.Store) { // storage droneserver.Config.Storage.Files = v + droneserver.Config.Storage.Config = v // services droneserver.Config.Services.Queue = setupQueue(c, v) diff --git a/model/build.go b/model/build.go index f718598e1..3ed1e3f9b 100644 --- a/model/build.go +++ b/model/build.go @@ -4,6 +4,7 @@ package model type Build struct { ID int64 `json:"id" meddler:"build_id,pk"` RepoID int64 `json:"-" meddler:"build_repo_id"` + ConfigID int64 `json:"-" meddler:"build_config_id"` Number int `json:"number" meddler:"build_number"` Parent int `json:"parent" meddler:"build_parent"` Event string `json:"event" meddler:"build_event"` diff --git a/model/config.go b/model/config.go index 76c3f7d49..291341781 100644 --- a/model/config.go +++ b/model/config.go @@ -1,24 +1,18 @@ package model -// Config defines system configuration parameters. +// ConfigStore persists pipeline configuration to storage. +type ConfigStore interface { + ConfigLoad(int64) (*Config, error) + ConfigFind(*Repo, string) (*Config, error) + ConfigUpdate(*Config) error + ConfigInsert(*Config) error +} + +// Config represents a pipeline configuration. type Config struct { - Open bool // Enables open registration - Secret string // Secret token used to authenticate agents - Admins map[string]bool // Administrative users - Orgs map[string]bool // Organization whitelist -} - -// IsAdmin returns true if the user is a member of the administrator list. -func (c *Config) IsAdmin(user *User) bool { - return c.Admins[user.Login] -} - -// IsMember returns true if the user is a member of the whitelisted teams. -func (c *Config) IsMember(teams []*Team) bool { - for _, team := range teams { - if c.Orgs[team.Login] { - return true - } - } - return false + ID int64 `json:"-" meddler:"config_id,pk"` + RepoID int64 `json:"-" meddler:"config_repo_id"` + Data string `json:"data" meddler:"config_data"` + Hash string `json:"hash" meddler:"config_hash"` + Approved bool `json:"approved" meddler:"config_approved"` } diff --git a/model/repo.go b/model/repo.go index c650e5dd9..2ced3d599 100644 --- a/model/repo.go +++ b/model/repo.go @@ -26,6 +26,7 @@ type Repo struct { IsTrusted bool `json:"trusted" meddler:"repo_trusted"` IsStarred bool `json:"starred,omitempty" meddler:"-"` IsGated bool `json:"gated" meddler:"repo_gated"` + IsGatedConf bool `json:"gated_conf" meddler:"repo_gated_conf"` AllowPull bool `json:"allow_pr" meddler:"repo_allow_pr"` AllowPush bool `json:"allow_push" meddler:"repo_allow_push"` AllowDeploy bool `json:"allow_deploys" meddler:"repo_allow_deploys"` diff --git a/model/settings.go b/model/settings.go new file mode 100644 index 000000000..b3722655f --- /dev/null +++ b/model/settings.go @@ -0,0 +1,24 @@ +package model + +// Settings defines system configuration parameters. +type Settings struct { + Open bool // Enables open registration + Secret string // Secret token used to authenticate agents + Admins map[string]bool // Administrative users + Orgs map[string]bool // Organization whitelist +} + +// IsAdmin returns true if the user is a member of the administrator list. +func (c *Settings) IsAdmin(user *User) bool { + return c.Admins[user.Login] +} + +// IsMember returns true if the user is a member of the whitelisted teams. +func (c *Settings) IsMember(teams []*Team) bool { + for _, team := range teams { + if c.Orgs[team.Login] { + return true + } + } + return false +} diff --git a/router/middleware/config.go b/router/middleware/config.go index 77a0217f5..67f798ecf 100644 --- a/router/middleware/config.go +++ b/router/middleware/config.go @@ -19,8 +19,8 @@ func Config(cli *cli.Context) gin.HandlerFunc { } // helper function to create the configuration from the CLI context. -func setupConfig(c *cli.Context) *model.Config { - return &model.Config{ +func setupConfig(c *cli.Context) *model.Settings { + return &model.Settings{ Open: c.Bool("open"), Secret: c.String("agent-secret"), Admins: sliceToMap2(c.StringSlice("admin")), diff --git a/router/middleware/session/user.go b/router/middleware/session/user.go index c1c0e09b4..f965014a6 100644 --- a/router/middleware/session/user.go +++ b/router/middleware/session/user.go @@ -45,7 +45,7 @@ func SetUser() gin.HandlerFunc { }) if err == nil { confv := c.MustGet("config") - if conf, ok := confv.(*model.Config); ok { + if conf, ok := confv.(*model.Settings); ok { user.Admin = conf.IsAdmin(user) } c.Set("user", user) diff --git a/server/build.go b/server/build.go index 7aaf95d58..b362f2bb5 100644 --- a/server/build.go +++ b/server/build.go @@ -174,12 +174,16 @@ func PostApproval(c *gin.Context) { // // fetch the build file from the database - raw, err := remote_.File(user, repo, build, repo.Config) + conf, err := Config.Storage.Config.ConfigLoad(build.ConfigID) if err != nil { logrus.Errorf("failure to get build config for %s. %s", repo.FullName, err) c.AbortWithError(404, err) return } + if !conf.Approved { + conf.Approved = true + Config.Storage.Config.ConfigUpdate(conf) + } netrc, err := remote_.Netrc(user, repo) if err != nil { @@ -222,7 +226,7 @@ func PostApproval(c *gin.Context) { Secs: secs, Regs: regs, Link: httputil.GetURL(c.Request), - Yaml: string(raw), + Yaml: conf.Data, } items, err := b.Build() if err != nil { @@ -394,12 +398,16 @@ func PostBuild(c *gin.Context) { } // fetch the .drone.yml file from the database - raw, err := remote_.File(user, repo, build, repo.Config) + conf, err := Config.Storage.Config.ConfigLoad(build.ConfigID) if err != nil { logrus.Errorf("failure to get build config for %s. %s", repo.FullName, err) c.AbortWithError(404, err) return } + if !conf.Approved { + conf.Approved = true + Config.Storage.Config.ConfigUpdate(conf) + } netrc, err := remote_.Netrc(user, repo) if err != nil { @@ -493,7 +501,7 @@ func PostBuild(c *gin.Context) { Secs: secs, Regs: regs, Link: httputil.GetURL(c.Request), - Yaml: string(raw), + Yaml: conf.Data, } // TODO inject environment varibles !!!!!! buildParams items, err := b.Build() diff --git a/server/hook.go b/server/hook.go index d065a742a..7d5f8a943 100644 --- a/server/hook.go +++ b/server/hook.go @@ -2,6 +2,7 @@ package server import ( "context" + "crypto/sha256" "encoding/json" "fmt" "regexp" @@ -131,12 +132,38 @@ func PostHook(c *gin.Context) { } // fetch the build file from the database - raw, err := remote_.File(user, repo, build, repo.Config) + confb, err := remote_.File(user, repo, build, repo.Config) if err != nil { logrus.Errorf("failure to get build config for %s. %s", repo.FullName, err) c.AbortWithError(404, err) return } + sha := shasum(confb) + conf, err := Config.Storage.Config.ConfigFind(repo, sha) + if err != nil { + conf = &model.Config{ + RepoID: repo.ID, + Data: string(confb), + Hash: sha, + Approved: false, + } + if user.Login == repo.Owner || build.Event != model.EventPull { + conf.Approved = true + } + err = Config.Storage.Config.ConfigInsert(conf) + if err != nil { + logrus.Errorf("failure to persist config for %s. %s", repo.FullName, err) + c.AbortWithError(500, err) + return + } + } + if !conf.Approved { + if user.Login == repo.Owner || build.Event != model.EventPull || !repo.IsGatedConf { + conf.Approved = true + Config.Storage.Config.ConfigUpdate(conf) + } + } + build.ConfigID = conf.ID netrc, err := remote_.Netrc(user, repo) if err != nil { @@ -145,7 +172,7 @@ func PostHook(c *gin.Context) { } // verify the branches can be built vs skipped - branches, err := yaml.ParseBytes(raw) + branches, err := yaml.ParseString(conf.Data) if err == nil { if !branches.Branches.Match(build.Branch) && build.Event != model.EventTag && build.Event != model.EventDeploy { c.String(200, "Branch does not match restrictions defined in yaml") @@ -168,7 +195,7 @@ func PostHook(c *gin.Context) { build.Verified = true build.Status = model.StatusPending - if repo.IsGated { + if repo.IsGated || repo.IsGatedConf { allowed, _ := Config.Services.Senders.SenderAllowed(user, repo, build) if !allowed { build.Status = model.StatusBlocked @@ -212,7 +239,7 @@ func PostHook(c *gin.Context) { Secs: secs, Regs: regs, Link: httputil.GetURL(c.Request), - Yaml: string(raw), + Yaml: conf.Data, } items, err := b.Build() if err != nil { @@ -442,7 +469,7 @@ func (b *builder) Build() ([]*buildItem, error) { linter.WithTrusted(b.Repo.IsTrusted), ).Lint(parsed) if lerr != nil { - return nil, err + return nil, lerr } var registries []compiler.Registry @@ -511,3 +538,8 @@ func (b *builder) Build() ([]*buildItem, error) { return items, nil } + +func shasum(raw []byte) string { + sum := sha256.Sum256(raw) + return fmt.Sprintf("%x", sum) +} diff --git a/server/login.go b/server/login.go index 9e951d786..fcc654edc 100644 --- a/server/login.go +++ b/server/login.go @@ -161,7 +161,7 @@ type tokenPayload struct { } // ToConfig returns the config from the Context -func ToConfig(c *gin.Context) *model.Config { +func ToConfig(c *gin.Context) *model.Settings { v := c.MustGet("config") - return v.(*model.Config) + return v.(*model.Settings) } diff --git a/server/rpc.go b/server/rpc.go index 0e5feed39..702d8ecab 100644 --- a/server/rpc.go +++ b/server/rpc.go @@ -41,8 +41,9 @@ var Config = struct { // Repos model.RepoStore // Builds model.BuildStore // Logs model.LogStore - Files model.FileStore - Procs model.ProcStore + Config model.ConfigStore + Files model.FileStore + Procs model.ProcStore // Registries model.RegistryStore // Secrets model.SecretStore } diff --git a/store/datastore/config.go b/store/datastore/config.go new file mode 100644 index 000000000..ad4309249 --- /dev/null +++ b/store/datastore/config.go @@ -0,0 +1,29 @@ +package datastore + +import ( + "github.com/drone/drone/model" + "github.com/drone/drone/store/datastore/sql" + "github.com/russross/meddler" +) + +func (db *datastore) ConfigLoad(id int64) (*model.Config, error) { + stmt := sql.Lookup(db.driver, "config-find-repo-id") + conf := new(model.Config) + err := meddler.QueryRow(db, conf, stmt, id) + return conf, err +} + +func (db *datastore) ConfigFind(repo *model.Repo, hash string) (*model.Config, error) { + stmt := sql.Lookup(db.driver, "config-find-repo-hash") + conf := new(model.Config) + err := meddler.QueryRow(db, conf, stmt, repo.ID, hash) + return conf, err +} + +func (db *datastore) ConfigUpdate(config *model.Config) error { + return meddler.Update(db, "config", config) +} + +func (db *datastore) ConfigInsert(config *model.Config) error { + return meddler.Insert(db, "config", config) +} diff --git a/store/datastore/config_test.go b/store/datastore/config_test.go new file mode 100644 index 000000000..bc1082c02 --- /dev/null +++ b/store/datastore/config_test.go @@ -0,0 +1,106 @@ +package datastore + +import ( + "testing" + + "github.com/drone/drone/model" +) + +func TestConfig(t *testing.T) { + s := newTest() + defer func() { + s.Exec("delete from config") + s.Close() + }() + + var ( + data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]" + hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" + ) + + if err := s.ConfigInsert( + &model.Config{ + RepoID: 2, + Data: data, + Hash: hash, + Approved: false, + }, + ); err != nil { + t.Errorf("Unexpected error: insert config: %s", err) + return + } + + config, err := s.ConfigFind(&model.Repo{ID: 2}, hash) + if err != nil { + t.Error(err) + return + } + if got, want := config.ID, int64(1); got != want { + t.Errorf("Want config id %d, got %d", want, got) + } + if got, want := config.RepoID, int64(2); got != want { + t.Errorf("Want config repo id %d, got %d", want, got) + } + if got, want := config.Data, data; got != want { + t.Errorf("Want config data %s, got %s", want, got) + } + if got, want := config.Hash, hash; got != want { + t.Errorf("Want config hash %s, got %s", want, got) + } + if got, want := config.Approved, false; got != want { + t.Errorf("Want config approved %v, got %v", want, got) + } + + config.Approved = true + err = s.ConfigUpdate(config) + if err != nil { + t.Errorf("Want config updated, got error %q", err) + return + } + + updated, err := s.ConfigFind(&model.Repo{ID: 2}, hash) + if err != nil { + t.Errorf("Want config find, got error %q", err) + return + } + if got, want := updated.Approved, true; got != want { + t.Errorf("Want config approved updated %v, got %v", want, got) + } +} + +// +// func TestConfigIndexes(t *testing.T) { +// s := newTest() +// defer func() { +// s.Exec("delete from config") +// s.Close() +// }() +// +// if err := s.FileCreate( +// &model.File{ +// BuildID: 1, +// ProcID: 1, +// Name: "hello.txt", +// Size: 11, +// Mime: "text/plain", +// }, +// bytes.NewBufferString("hello world"), +// ); err != nil { +// t.Errorf("Unexpected error: insert file: %s", err) +// return +// } +// +// // fail due to duplicate file name +// if err := s.FileCreate( +// &model.File{ +// BuildID: 1, +// ProcID: 1, +// Name: "hello.txt", +// Mime: "text/plain", +// Size: 11, +// }, +// bytes.NewBufferString("hello world"), +// ); err == nil { +// t.Errorf("Unexpected error: dupliate pid") +// } +// } diff --git a/store/datastore/ddl/mysql/16.sql b/store/datastore/ddl/mysql/16.sql new file mode 100644 index 000000000..1636663e5 --- /dev/null +++ b/store/datastore/ddl/mysql/16.sql @@ -0,0 +1,21 @@ +-- +migrate Up + +CREATE TABLE config ( + config_id INTEGER PRIMARY KEY AUTO_INCREMENT +,config_repo_id INTEGER +,config_hash VARCHAR(250) +,config_data MEDIUMBLOB +,config_approved BOOLEAN + +,UNIQUE(config_hash, config_repo_id) +); + +ALTER TABLE builds ADD COLUMN build_config_id INTEGER; +UPDATE builds set build_config_id = 0; + +ALTER TABLE repos ADD COLUMN repo_gated_conf BOOLEAN; +UPDATE repos SET repo_gated_conf = 0; + +-- +migrate Down + +DROP TABLE config; diff --git a/store/datastore/ddl/postgres/16.sql b/store/datastore/ddl/postgres/16.sql new file mode 100644 index 000000000..e15397c81 --- /dev/null +++ b/store/datastore/ddl/postgres/16.sql @@ -0,0 +1,21 @@ +-- +migrate Up + +CREATE TABLE config ( + config_id SERIAL PRIMARY KEY +,config_repo_id INTEGER +,config_hash VARCHAR(250) +,config_data BYTEA +,config_approved BOOLEAN + +,UNIQUE(config_hash, config_repo_id) +); + +ALTER TABLE builds ADD COLUMN build_config_id INTEGER; +UPDATE builds set build_config_id = 0; + +ALTER TABLE repos ADD COLUMN repo_gated_conf BOOLEAN; +UPDATE repos SET repo_gated_conf = 0; + +-- +migrate Down + +DROP TABLE config; diff --git a/store/datastore/ddl/sqlite3/16.sql b/store/datastore/ddl/sqlite3/16.sql new file mode 100644 index 000000000..316229444 --- /dev/null +++ b/store/datastore/ddl/sqlite3/16.sql @@ -0,0 +1,21 @@ +-- +migrate Up + +CREATE TABLE config ( + config_id INTEGER PRIMARY KEY AUTOINCREMENT +,config_repo_id INTEGER +,config_hash TEXT +,config_data BLOB +,config_approved BOOLEAN + +,UNIQUE(config_hash, config_repo_id) +); + +ALTER TABLE builds ADD COLUMN build_config_id INTEGER; +UPDATE builds set build_config_id = 0; + +ALTER TABLE repos ADD COLUMN repo_gated_conf BOOLEAN; +UPDATE repos SET repo_gated_conf = 0; + +-- +migrate Down + +DROP TABLE config; diff --git a/store/datastore/sql/postgres/files/config.sql b/store/datastore/sql/postgres/files/config.sql new file mode 100644 index 000000000..0385c27ce --- /dev/null +++ b/store/datastore/sql/postgres/files/config.sql @@ -0,0 +1,22 @@ +-- name: config-find-id + +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_id = $1 + +-- name: config-find-repo-hash + +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_repo_id = $1 + AND config_hash = $2 diff --git a/store/datastore/sql/postgres/sql_gen.go b/store/datastore/sql/postgres/sql_gen.go index 08be6c7da..c27fa554f 100644 --- a/store/datastore/sql/postgres/sql_gen.go +++ b/store/datastore/sql/postgres/sql_gen.go @@ -6,6 +6,8 @@ func Lookup(name string) string { } var index = map[string]string{ + "config-find-id": configFindId, + "config-find-repo-hash": configFindRepoHash, "count-users": countUsers, "count-repos": countRepos, "count-builds": countBuilds, @@ -33,6 +35,29 @@ var index = map[string]string{ "task-delete": taskDelete, } +var configFindId = ` +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_id = $1 +` + +var configFindRepoHash = ` +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_repo_id = $1 + AND config_hash = $2 +` + var countUsers = ` SELECT reltuples FROM pg_class WHERE relname = 'users'; diff --git a/store/datastore/sql/sqlite/files/config.sql b/store/datastore/sql/sqlite/files/config.sql new file mode 100644 index 000000000..dd07a4f1a --- /dev/null +++ b/store/datastore/sql/sqlite/files/config.sql @@ -0,0 +1,22 @@ +-- name: config-find-id + +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_id = ? + +-- name: config-find-repo-hash + +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_repo_id = ? + AND config_hash = ? diff --git a/store/datastore/sql/sqlite/sql_gen.go b/store/datastore/sql/sqlite/sql_gen.go index c2e082dd8..15cc0e7b6 100644 --- a/store/datastore/sql/sqlite/sql_gen.go +++ b/store/datastore/sql/sqlite/sql_gen.go @@ -6,6 +6,8 @@ func Lookup(name string) string { } var index = map[string]string{ + "config-find-id": configFindId, + "config-find-repo-hash": configFindRepoHash, "count-users": countUsers, "count-repos": countRepos, "count-builds": countBuilds, @@ -33,6 +35,29 @@ var index = map[string]string{ "task-delete": taskDelete, } +var configFindId = ` +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_id = ? +` + +var configFindRepoHash = ` +SELECT + config_id +,config_repo_id +,config_hash +,config_data +,config_approved +FROM config +WHERE config_repo_id = ? + AND config_hash = ? +` + var countUsers = ` SELECT count(1) FROM users diff --git a/store/store.go b/store/store.go index 4ba358323..18a177822 100644 --- a/store/store.go +++ b/store/store.go @@ -92,6 +92,11 @@ type Store interface { // new functions // + ConfigLoad(int64) (*model.Config, error) + ConfigFind(*model.Repo, string) (*model.Config, error) + ConfigUpdate(*model.Config) error + ConfigInsert(*model.Config) error + SenderFind(*model.Repo, string) (*model.Sender, error) SenderList(*model.Repo) ([]*model.Sender, error) SenderCreate(*model.Sender) error From 4aac0bc4d6d50cd364c459fd791725582a2a67ff Mon Sep 17 00:00:00 2001 From: Brad Rydzewski Date: Fri, 5 May 2017 19:13:40 +0200 Subject: [PATCH 2/3] re-use gated logic --- model/repo.go | 1 - model/sender.go | 2 +- plugins/sender/builtin.go | 4 ++-- plugins/sender/plugin.go | 2 +- server/hook.go | 8 ++++---- store/datastore/ddl/mysql/16.sql | 3 --- store/datastore/ddl/postgres/16.sql | 3 --- store/datastore/ddl/sqlite3/16.sql | 3 --- 8 files changed, 8 insertions(+), 18 deletions(-) diff --git a/model/repo.go b/model/repo.go index 2ced3d599..c650e5dd9 100644 --- a/model/repo.go +++ b/model/repo.go @@ -26,7 +26,6 @@ type Repo struct { IsTrusted bool `json:"trusted" meddler:"repo_trusted"` IsStarred bool `json:"starred,omitempty" meddler:"-"` IsGated bool `json:"gated" meddler:"repo_gated"` - IsGatedConf bool `json:"gated_conf" meddler:"repo_gated_conf"` AllowPull bool `json:"allow_pr" meddler:"repo_allow_pr"` AllowPush bool `json:"allow_push" meddler:"repo_allow_push"` AllowDeploy bool `json:"allow_deploys" meddler:"repo_allow_deploys"` diff --git a/model/sender.go b/model/sender.go index 0f8fab0b8..7655067c7 100644 --- a/model/sender.go +++ b/model/sender.go @@ -1,7 +1,7 @@ package model type SenderService interface { - SenderAllowed(*User, *Repo, *Build) (bool, error) + SenderAllowed(*User, *Repo, *Build, *Config) (bool, error) SenderCreate(*Repo, *Sender) error SenderUpdate(*Repo, *Sender) error SenderDelete(*Repo, string) error diff --git a/plugins/sender/builtin.go b/plugins/sender/builtin.go index 3cc1f1650..835877c90 100644 --- a/plugins/sender/builtin.go +++ b/plugins/sender/builtin.go @@ -13,8 +13,8 @@ func New(store model.SenderStore) model.SenderService { return &builtin{store} } -func (b *builtin) SenderAllowed(user *model.User, repo *model.Repo, build *model.Build) (bool, error) { - if repo.IsPrivate == false && build.Event == model.EventPull && build.Sender != user.Login { +func (b *builtin) SenderAllowed(user *model.User, repo *model.Repo, build *model.Build, conf *model.Config) (bool, error) { + if !conf.Approved { sender, err := b.store.SenderFind(repo, build.Sender) if err != nil || sender.Block { return false, nil diff --git a/plugins/sender/plugin.go b/plugins/sender/plugin.go index 2be5638e2..ffd19802c 100644 --- a/plugins/sender/plugin.go +++ b/plugins/sender/plugin.go @@ -16,7 +16,7 @@ func NewRemote(endpoint string) model.SenderService { return &plugin{endpoint} } -func (p *plugin) SenderAllowed(user *model.User, repo *model.Repo, build *model.Build) (bool, error) { +func (p *plugin) SenderAllowed(user *model.User, repo *model.Repo, build *model.Build, conf *model.Config) (bool, error) { path := fmt.Sprintf("%s/senders/%s/%s/%s/verify", p.endpoint, repo.Owner, repo.Name, build.Sender) err := internal.Send("POST", path, build, nil) if err != nil { diff --git a/server/hook.go b/server/hook.go index 7d5f8a943..e8fb52aa9 100644 --- a/server/hook.go +++ b/server/hook.go @@ -147,7 +147,7 @@ func PostHook(c *gin.Context) { Hash: sha, Approved: false, } - if user.Login == repo.Owner || build.Event != model.EventPull { + if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false { conf.Approved = true } err = Config.Storage.Config.ConfigInsert(conf) @@ -158,7 +158,7 @@ func PostHook(c *gin.Context) { } } if !conf.Approved { - if user.Login == repo.Owner || build.Event != model.EventPull || !repo.IsGatedConf { + if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false { conf.Approved = true Config.Storage.Config.ConfigUpdate(conf) } @@ -195,8 +195,8 @@ func PostHook(c *gin.Context) { build.Verified = true build.Status = model.StatusPending - if repo.IsGated || repo.IsGatedConf { - allowed, _ := Config.Services.Senders.SenderAllowed(user, repo, build) + if repo.IsGated { + allowed, _ := Config.Services.Senders.SenderAllowed(user, repo, build, conf) if !allowed { build.Status = model.StatusBlocked } diff --git a/store/datastore/ddl/mysql/16.sql b/store/datastore/ddl/mysql/16.sql index 1636663e5..8a29b1fb1 100644 --- a/store/datastore/ddl/mysql/16.sql +++ b/store/datastore/ddl/mysql/16.sql @@ -13,9 +13,6 @@ CREATE TABLE config ( ALTER TABLE builds ADD COLUMN build_config_id INTEGER; UPDATE builds set build_config_id = 0; -ALTER TABLE repos ADD COLUMN repo_gated_conf BOOLEAN; -UPDATE repos SET repo_gated_conf = 0; - -- +migrate Down DROP TABLE config; diff --git a/store/datastore/ddl/postgres/16.sql b/store/datastore/ddl/postgres/16.sql index e15397c81..e8ed77ef9 100644 --- a/store/datastore/ddl/postgres/16.sql +++ b/store/datastore/ddl/postgres/16.sql @@ -13,9 +13,6 @@ CREATE TABLE config ( ALTER TABLE builds ADD COLUMN build_config_id INTEGER; UPDATE builds set build_config_id = 0; -ALTER TABLE repos ADD COLUMN repo_gated_conf BOOLEAN; -UPDATE repos SET repo_gated_conf = 0; - -- +migrate Down DROP TABLE config; diff --git a/store/datastore/ddl/sqlite3/16.sql b/store/datastore/ddl/sqlite3/16.sql index 316229444..88351bf70 100644 --- a/store/datastore/ddl/sqlite3/16.sql +++ b/store/datastore/ddl/sqlite3/16.sql @@ -13,9 +13,6 @@ CREATE TABLE config ( ALTER TABLE builds ADD COLUMN build_config_id INTEGER; UPDATE builds set build_config_id = 0; -ALTER TABLE repos ADD COLUMN repo_gated_conf BOOLEAN; -UPDATE repos SET repo_gated_conf = 0; - -- +migrate Down DROP TABLE config; From 3a64aa4cf2fe9b071369eb7bddcedf0c58113977 Mon Sep 17 00:00:00 2001 From: Brad Rydzewski Date: Fri, 5 May 2017 20:05:42 +0200 Subject: [PATCH 3/3] simplify gating logic --- drone/server/server.go | 2 +- model/config.go | 13 +- plugins/sender/builtin.go | 14 +- server/build.go | 8 - server/hook.go | 18 +-- store/datastore/config.go | 18 ++- store/datastore/config_test.go | 151 +++++++++++------- store/datastore/ddl/mysql/16.sql | 1 - store/datastore/ddl/postgres/16.sql | 1 - store/datastore/ddl/sqlite3/16.sql | 1 - store/datastore/sql/postgres/files/config.sql | 10 +- store/datastore/sql/postgres/sql_gen.go | 11 +- store/datastore/sql/sqlite/files/config.sql | 10 +- store/datastore/sql/sqlite/sql_gen.go | 11 +- store/store.go | 4 +- 15 files changed, 168 insertions(+), 105 deletions(-) diff --git a/drone/server/server.go b/drone/server/server.go index 9993e1a2d..25ac5204f 100644 --- a/drone/server/server.go +++ b/drone/server/server.go @@ -400,7 +400,7 @@ func setupEvilGlobals(c *cli.Context, v store.Store) { droneserver.Config.Services.Pubsub.Create(context.Background(), "topic/events") droneserver.Config.Services.Registries = registry.New(v) droneserver.Config.Services.Secrets = secrets.New(v) - droneserver.Config.Services.Senders = sender.New(v) + droneserver.Config.Services.Senders = sender.New(v, v) if endpoint := c.String("registry-service"); endpoint != "" { droneserver.Config.Services.Registries = registry.NewRemote(endpoint) } diff --git a/model/config.go b/model/config.go index 291341781..572947596 100644 --- a/model/config.go +++ b/model/config.go @@ -4,15 +4,14 @@ package model type ConfigStore interface { ConfigLoad(int64) (*Config, error) ConfigFind(*Repo, string) (*Config, error) - ConfigUpdate(*Config) error - ConfigInsert(*Config) error + ConfigFindApproved(*Config) (bool, error) + ConfigCreate(*Config) error } // Config represents a pipeline configuration. type Config struct { - ID int64 `json:"-" meddler:"config_id,pk"` - RepoID int64 `json:"-" meddler:"config_repo_id"` - Data string `json:"data" meddler:"config_data"` - Hash string `json:"hash" meddler:"config_hash"` - Approved bool `json:"approved" meddler:"config_approved"` + ID int64 `json:"-" meddler:"config_id,pk"` + RepoID int64 `json:"-" meddler:"config_repo_id"` + Data string `json:"data" meddler:"config_data"` + Hash string `json:"hash" meddler:"config_hash"` } diff --git a/plugins/sender/builtin.go b/plugins/sender/builtin.go index 835877c90..577c1bf2e 100644 --- a/plugins/sender/builtin.go +++ b/plugins/sender/builtin.go @@ -6,15 +6,23 @@ import ( type builtin struct { store model.SenderStore + conf model.ConfigStore } // New returns a new local gating service. -func New(store model.SenderStore) model.SenderService { - return &builtin{store} +func New(store model.SenderStore, conf model.ConfigStore) model.SenderService { + return &builtin{store, conf} } func (b *builtin) SenderAllowed(user *model.User, repo *model.Repo, build *model.Build, conf *model.Config) (bool, error) { - if !conf.Approved { + if build.Event == model.EventPull && build.Sender != user.Login { + // check to see if the configuration has already been used in an + // existing build. If yes it is considered approved. + if ok, _ := b.conf.ConfigFindApproved(conf); ok { + return true, nil + } + // else check to see if the configuration is sent from a user + // account that is a repositroy approver themselves. sender, err := b.store.SenderFind(repo, build.Sender) if err != nil || sender.Block { return false, nil diff --git a/server/build.go b/server/build.go index b362f2bb5..f2da48e00 100644 --- a/server/build.go +++ b/server/build.go @@ -180,10 +180,6 @@ func PostApproval(c *gin.Context) { c.AbortWithError(404, err) return } - if !conf.Approved { - conf.Approved = true - Config.Storage.Config.ConfigUpdate(conf) - } netrc, err := remote_.Netrc(user, repo) if err != nil { @@ -404,10 +400,6 @@ func PostBuild(c *gin.Context) { c.AbortWithError(404, err) return } - if !conf.Approved { - conf.Approved = true - Config.Storage.Config.ConfigUpdate(conf) - } netrc, err := remote_.Netrc(user, repo) if err != nil { diff --git a/server/hook.go b/server/hook.go index e8fb52aa9..73cfa7474 100644 --- a/server/hook.go +++ b/server/hook.go @@ -142,27 +142,17 @@ func PostHook(c *gin.Context) { conf, err := Config.Storage.Config.ConfigFind(repo, sha) if err != nil { conf = &model.Config{ - RepoID: repo.ID, - Data: string(confb), - Hash: sha, - Approved: false, + RepoID: repo.ID, + Data: string(confb), + Hash: sha, } - if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false { - conf.Approved = true - } - err = Config.Storage.Config.ConfigInsert(conf) + err = Config.Storage.Config.ConfigCreate(conf) if err != nil { logrus.Errorf("failure to persist config for %s. %s", repo.FullName, err) c.AbortWithError(500, err) return } } - if !conf.Approved { - if user.Login == repo.Owner || build.Event != model.EventPull || repo.IsGated == false { - conf.Approved = true - Config.Storage.Config.ConfigUpdate(conf) - } - } build.ConfigID = conf.ID netrc, err := remote_.Netrc(user, repo) diff --git a/store/datastore/config.go b/store/datastore/config.go index ad4309249..ca6d6f24a 100644 --- a/store/datastore/config.go +++ b/store/datastore/config.go @@ -1,13 +1,15 @@ package datastore import ( + gosql "database/sql" + "github.com/drone/drone/model" "github.com/drone/drone/store/datastore/sql" "github.com/russross/meddler" ) func (db *datastore) ConfigLoad(id int64) (*model.Config, error) { - stmt := sql.Lookup(db.driver, "config-find-repo-id") + stmt := sql.Lookup(db.driver, "config-find-id") conf := new(model.Config) err := meddler.QueryRow(db, conf, stmt, id) return conf, err @@ -20,10 +22,18 @@ func (db *datastore) ConfigFind(repo *model.Repo, hash string) (*model.Config, e return conf, err } -func (db *datastore) ConfigUpdate(config *model.Config) error { - return meddler.Update(db, "config", config) +func (db *datastore) ConfigFindApproved(config *model.Config) (bool, error) { + var dest int64 + stmt := sql.Lookup(db.driver, "config-find-approved") + err := db.DB.QueryRow(stmt, config.RepoID, config.ID).Scan(&dest) + if err == gosql.ErrNoRows { + return false, nil + } else if err != nil { + return false, err + } + return true, nil } -func (db *datastore) ConfigInsert(config *model.Config) error { +func (db *datastore) ConfigCreate(config *model.Config) error { return meddler.Insert(db, "config", config) } diff --git a/store/datastore/config_test.go b/store/datastore/config_test.go index bc1082c02..9586fed0a 100644 --- a/store/datastore/config_test.go +++ b/store/datastore/config_test.go @@ -18,12 +18,11 @@ func TestConfig(t *testing.T) { hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" ) - if err := s.ConfigInsert( + if err := s.ConfigCreate( &model.Config{ - RepoID: 2, - Data: data, - Hash: hash, - Approved: false, + RepoID: 2, + Data: data, + Hash: hash, }, ); err != nil { t.Errorf("Unexpected error: insert config: %s", err) @@ -47,60 +46,102 @@ func TestConfig(t *testing.T) { if got, want := config.Hash, hash; got != want { t.Errorf("Want config hash %s, got %s", want, got) } - if got, want := config.Approved, false; got != want { - t.Errorf("Want config approved %v, got %v", want, got) - } - config.Approved = true - err = s.ConfigUpdate(config) + loaded, err := s.ConfigLoad(config.ID) if err != nil { - t.Errorf("Want config updated, got error %q", err) + t.Errorf("Want config by id, got error %q", err) return } - - updated, err := s.ConfigFind(&model.Repo{ID: 2}, hash) - if err != nil { - t.Errorf("Want config find, got error %q", err) - return - } - if got, want := updated.Approved, true; got != want { - t.Errorf("Want config approved updated %v, got %v", want, got) + if got, want := loaded.ID, config.ID; got != want { + t.Errorf("Want config by id %d, got %d", want, got) } } -// -// func TestConfigIndexes(t *testing.T) { -// s := newTest() -// defer func() { -// s.Exec("delete from config") -// s.Close() -// }() -// -// if err := s.FileCreate( -// &model.File{ -// BuildID: 1, -// ProcID: 1, -// Name: "hello.txt", -// Size: 11, -// Mime: "text/plain", -// }, -// bytes.NewBufferString("hello world"), -// ); err != nil { -// t.Errorf("Unexpected error: insert file: %s", err) -// return -// } -// -// // fail due to duplicate file name -// if err := s.FileCreate( -// &model.File{ -// BuildID: 1, -// ProcID: 1, -// Name: "hello.txt", -// Mime: "text/plain", -// Size: 11, -// }, -// bytes.NewBufferString("hello world"), -// ); err == nil { -// t.Errorf("Unexpected error: dupliate pid") -// } -// } +func TestConfigApproved(t *testing.T) { + s := newTest() + defer func() { + s.Exec("delete from config") + s.Exec("delete from builds") + s.Close() + }() + + var ( + data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]" + hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" + conf = &model.Config{ + RepoID: 1, + Data: data, + Hash: hash, + } + ) + + if err := s.ConfigCreate(conf); err != nil { + t.Errorf("Unexpected error: insert config: %s", err) + return + } + + s.CreateBuild(&model.Build{ + RepoID: 1, + ConfigID: conf.ID, + Status: model.StatusBlocked, + Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac", + }) + s.CreateBuild(&model.Build{ + RepoID: 1, + ConfigID: conf.ID, + Status: model.StatusPending, + Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac", + }) + + if ok, _ := s.ConfigFindApproved(conf); ok == true { + t.Errorf("Want config not approved, when blocked or pending") + return + } + + s.CreateBuild(&model.Build{ + RepoID: 1, + ConfigID: conf.ID, + Status: model.StatusRunning, + Commit: "85f8c029b902ed9400bc600bac301a0aadb144ac", + }) + + if ok, _ := s.ConfigFindApproved(conf); ok == false { + t.Errorf("Want config approved, when running.") + return + } +} + +func TestConfigIndexes(t *testing.T) { + s := newTest() + defer func() { + s.Exec("delete from config") + s.Close() + }() + + var ( + data = "pipeline: [ { image: golang, commands: [ go build, go test ] } ]" + hash = "8d8647c9aa90d893bfb79dddbe901f03e258588121e5202632f8ae5738590b26" + ) + + if err := s.ConfigCreate( + &model.Config{ + RepoID: 2, + Data: data, + Hash: hash, + }, + ); err != nil { + t.Errorf("Unexpected error: insert config: %s", err) + return + } + + // fail due to duplicate sha + if err := s.ConfigCreate( + &model.Config{ + RepoID: 2, + Data: data, + Hash: hash, + }, + ); err == nil { + t.Errorf("Unexpected error: dupliate sha") + } +} diff --git a/store/datastore/ddl/mysql/16.sql b/store/datastore/ddl/mysql/16.sql index 8a29b1fb1..d1a660d80 100644 --- a/store/datastore/ddl/mysql/16.sql +++ b/store/datastore/ddl/mysql/16.sql @@ -5,7 +5,6 @@ CREATE TABLE config ( ,config_repo_id INTEGER ,config_hash VARCHAR(250) ,config_data MEDIUMBLOB -,config_approved BOOLEAN ,UNIQUE(config_hash, config_repo_id) ); diff --git a/store/datastore/ddl/postgres/16.sql b/store/datastore/ddl/postgres/16.sql index e8ed77ef9..63dbf86a0 100644 --- a/store/datastore/ddl/postgres/16.sql +++ b/store/datastore/ddl/postgres/16.sql @@ -5,7 +5,6 @@ CREATE TABLE config ( ,config_repo_id INTEGER ,config_hash VARCHAR(250) ,config_data BYTEA -,config_approved BOOLEAN ,UNIQUE(config_hash, config_repo_id) ); diff --git a/store/datastore/ddl/sqlite3/16.sql b/store/datastore/ddl/sqlite3/16.sql index 88351bf70..fc6129f49 100644 --- a/store/datastore/ddl/sqlite3/16.sql +++ b/store/datastore/ddl/sqlite3/16.sql @@ -5,7 +5,6 @@ CREATE TABLE config ( ,config_repo_id INTEGER ,config_hash TEXT ,config_data BLOB -,config_approved BOOLEAN ,UNIQUE(config_hash, config_repo_id) ); diff --git a/store/datastore/sql/postgres/files/config.sql b/store/datastore/sql/postgres/files/config.sql index 0385c27ce..2dc954384 100644 --- a/store/datastore/sql/postgres/files/config.sql +++ b/store/datastore/sql/postgres/files/config.sql @@ -5,7 +5,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = $1 @@ -16,7 +15,14 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = $1 AND config_hash = $2 + +-- name: config-find-approved + +SELECT build_id FROM builds +WHERE build_repo_id = $1 +AND build_config_id = $2 +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 diff --git a/store/datastore/sql/postgres/sql_gen.go b/store/datastore/sql/postgres/sql_gen.go index c27fa554f..24f37a610 100644 --- a/store/datastore/sql/postgres/sql_gen.go +++ b/store/datastore/sql/postgres/sql_gen.go @@ -8,6 +8,7 @@ func Lookup(name string) string { var index = map[string]string{ "config-find-id": configFindId, "config-find-repo-hash": configFindRepoHash, + "config-find-approved": configFindApproved, "count-users": countUsers, "count-repos": countRepos, "count-builds": countBuilds, @@ -41,7 +42,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = $1 ` @@ -52,12 +52,19 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = $1 AND config_hash = $2 ` +var configFindApproved = ` +SELECT build_id FROM builds +WHERE build_repo_id = $1 +AND build_config_id = $2 +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 +` + var countUsers = ` SELECT reltuples FROM pg_class WHERE relname = 'users'; diff --git a/store/datastore/sql/sqlite/files/config.sql b/store/datastore/sql/sqlite/files/config.sql index dd07a4f1a..8f29bd5d1 100644 --- a/store/datastore/sql/sqlite/files/config.sql +++ b/store/datastore/sql/sqlite/files/config.sql @@ -5,7 +5,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = ? @@ -16,7 +15,14 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = ? AND config_hash = ? + +-- name: config-find-approved + +SELECT build_id FROM builds +WHERE build_repo_id = ? +AND build_config_id = ? +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 diff --git a/store/datastore/sql/sqlite/sql_gen.go b/store/datastore/sql/sqlite/sql_gen.go index 15cc0e7b6..1a06db44e 100644 --- a/store/datastore/sql/sqlite/sql_gen.go +++ b/store/datastore/sql/sqlite/sql_gen.go @@ -8,6 +8,7 @@ func Lookup(name string) string { var index = map[string]string{ "config-find-id": configFindId, "config-find-repo-hash": configFindRepoHash, + "config-find-approved": configFindApproved, "count-users": countUsers, "count-repos": countRepos, "count-builds": countBuilds, @@ -41,7 +42,6 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_id = ? ` @@ -52,12 +52,19 @@ SELECT ,config_repo_id ,config_hash ,config_data -,config_approved FROM config WHERE config_repo_id = ? AND config_hash = ? ` +var configFindApproved = ` +SELECT build_id FROM builds +WHERE build_repo_id = ? +AND build_config_id = ? +AND build_status NOT IN ('blocked', 'pending') +LIMIT 1 +` + var countUsers = ` SELECT count(1) FROM users diff --git a/store/store.go b/store/store.go index 18a177822..1949bf823 100644 --- a/store/store.go +++ b/store/store.go @@ -94,8 +94,8 @@ type Store interface { ConfigLoad(int64) (*model.Config, error) ConfigFind(*model.Repo, string) (*model.Config, error) - ConfigUpdate(*model.Config) error - ConfigInsert(*model.Config) error + ConfigFindApproved(*model.Config) (bool, error) + ConfigCreate(*model.Config) error SenderFind(*model.Repo, string) (*model.Sender, error) SenderList(*model.Repo) ([]*model.Sender, error)