From afb1d3139461249acfe0bd4cbe5d8c022b165f3c Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Thu, 7 Jul 2022 16:48:00 +0000 Subject: [PATCH] Slight cleanup * added interface for the github client. This will help mocking it out for testing. * removed some unused code * moved some code around Signed-off-by: Gabriel Adrian Samfira --- apiserver/controllers/controllers.go | 4 ++ auth/auth.go | 8 +-- database/sql/sql.go | 32 ++++++++-- runner/common/util.go | 30 ++++++++++ runner/pool/organization.go | 10 ++-- runner/pool/{common.go => pool.go} | 73 +++++++++-------------- runner/pool/repository.go | 10 ++-- runner/runner.go | 40 ------------- scripts/build-static.sh | 1 + util/util.go | 87 +++++++--------------------- 10 files changed, 123 insertions(+), 172 deletions(-) create mode 100644 runner/common/util.go rename runner/pool/{common.go => pool.go} (97%) mode change 100644 => 100755 scripts/build-static.sh diff --git a/apiserver/controllers/controllers.go b/apiserver/controllers/controllers.go index 94375a47..07c8b001 100644 --- a/apiserver/controllers/controllers.go +++ b/apiserver/controllers/controllers.go @@ -55,6 +55,8 @@ func handleError(w http.ResponseWriter, err error) { case *gErrors.UnauthorizedError: w.WriteHeader(http.StatusUnauthorized) apiErr.Error = "Not Authorized" + // Don't include details on 401 errors. + apiErr.Details = "" case *gErrors.BadRequestError: w.WriteHeader(http.StatusBadRequest) apiErr.Error = "Bad Request" @@ -64,6 +66,8 @@ func handleError(w http.ResponseWriter, err error) { default: w.WriteHeader(http.StatusInternalServerError) apiErr.Error = "Server error" + // Don't include details on server error. + apiErr.Details = "" } json.NewEncoder(w).Encode(apiErr) diff --git a/auth/auth.go b/auth/auth.go index 3a8b819f..5795791a 100644 --- a/auth/auth.go +++ b/auth/auth.go @@ -118,22 +118,18 @@ func (a *Authenticator) InitController(ctx context.Context, param params.NewUser } func (a *Authenticator) AuthenticateUser(ctx context.Context, info params.PasswordLoginParams) (context.Context, error) { - if info.Username == "" { - return ctx, runnerErrors.ErrUnauthorized - } - - if info.Password == "" { + if info.Username == "" || info.Password == "" { return ctx, runnerErrors.ErrUnauthorized } user, err := a.store.GetUser(ctx, info.Username) - if err != nil { if errors.Is(err, runnerErrors.ErrNotFound) { return ctx, runnerErrors.ErrUnauthorized } return ctx, errors.Wrap(err, "authenticating") } + if !user.Enabled { return ctx, runnerErrors.ErrUnauthorized } diff --git a/database/sql/sql.go b/database/sql/sql.go index 6959a750..d5d6ee31 100644 --- a/database/sql/sql.go +++ b/database/sql/sql.go @@ -16,16 +16,40 @@ package sql import ( "context" - "garm/config" - "garm/database/common" - "garm/util" "github.com/pkg/errors" + "gorm.io/driver/mysql" + "gorm.io/driver/sqlite" "gorm.io/gorm" + + "garm/config" + "garm/database/common" ) +// newDBConn returns a new gorm db connection, given the config +func newDBConn(dbCfg config.Database) (conn *gorm.DB, err error) { + dbType, connURI, err := dbCfg.GormParams() + if err != nil { + return nil, errors.Wrap(err, "getting DB URI string") + } + switch dbType { + case config.MySQLBackend: + conn, err = gorm.Open(mysql.Open(connURI), &gorm.Config{}) + case config.SQLiteBackend: + conn, err = gorm.Open(sqlite.Open(connURI), &gorm.Config{}) + } + if err != nil { + return nil, errors.Wrap(err, "connecting to database") + } + + if dbCfg.Debug { + conn = conn.Debug() + } + return conn, nil +} + func NewSQLDatabase(ctx context.Context, cfg config.Database) (common.Store, error) { - conn, err := util.NewDBConn(cfg) + conn, err := newDBConn(cfg) if err != nil { return nil, errors.Wrap(err, "creating DB connection") } diff --git a/runner/common/util.go b/runner/common/util.go new file mode 100644 index 00000000..2304543d --- /dev/null +++ b/runner/common/util.go @@ -0,0 +1,30 @@ +package common + +import ( + "context" + + "github.com/google/go-github/v43/github" +) + +// GithubClient that describes the minimum list of functions we need to interact with github. +// Allows for easier testing. +type GithubClient interface { + // ListRunners lists all runners within a repository. + ListRunners(ctx context.Context, owner, repo string, opts *github.ListOptions) (*github.Runners, *github.Response, error) + // ListRunnerApplicationDownloads returns a list of github runner application downloads for the + // various supported operating systems and architectures. + ListRunnerApplicationDownloads(ctx context.Context, owner, repo string) ([]*github.RunnerApplicationDownload, *github.Response, error) + // RemoveRunner removes one runner from a repository. + RemoveRunner(ctx context.Context, owner, repo string, runnerID int64) (*github.Response, error) + // CreateRegistrationToken creates a runner registration token for one repository. + CreateRegistrationToken(ctx context.Context, owner, repo string) (*github.RegistrationToken, *github.Response, error) + // ListOrganizationRunners lists all runners within an organization. + ListOrganizationRunners(ctx context.Context, owner string, opts *github.ListOptions) (*github.Runners, *github.Response, error) + // ListOrganizationRunnerApplicationDownloads returns a list of github runner application downloads for the + // various supported operating systems and architectures. + ListOrganizationRunnerApplicationDownloads(ctx context.Context, owner string) ([]*github.RunnerApplicationDownload, *github.Response, error) + // RemoveOrganizationRunner removes one github runner from an organization. + RemoveOrganizationRunner(ctx context.Context, owner string, runnerID int64) (*github.Response, error) + // CreateOrganizationRegistrationToken creates a runner registration token for an organization. + CreateOrganizationRegistrationToken(ctx context.Context, owner string) (*github.RegistrationToken, *github.Response, error) +} diff --git a/runner/pool/organization.go b/runner/pool/organization.go index 067c535f..00fa1686 100644 --- a/runner/pool/organization.go +++ b/runner/pool/organization.go @@ -62,7 +62,7 @@ func NewOrganizationPoolManager(ctx context.Context, cfg params.Organization, pr type organization struct { cfg params.Organization ctx context.Context - ghcli *github.Client + ghcli common.GithubClient id string store dbCommon.Store @@ -89,7 +89,7 @@ func (r *organization) GetGithubToken() string { } func (r *organization) GetGithubRunners() ([]*github.Runner, error) { - runners, _, err := r.ghcli.Actions.ListOrganizationRunners(r.ctx, r.cfg.Name, nil) + runners, _, err := r.ghcli.ListOrganizationRunners(r.ctx, r.cfg.Name, nil) if err != nil { return nil, errors.Wrap(err, "fetching runners") } @@ -100,7 +100,7 @@ func (r *organization) GetGithubRunners() ([]*github.Runner, error) { func (r *organization) FetchTools() ([]*github.RunnerApplicationDownload, error) { r.mux.Lock() defer r.mux.Unlock() - tools, _, err := r.ghcli.Actions.ListOrganizationRunnerApplicationDownloads(r.ctx, r.cfg.Name) + tools, _, err := r.ghcli.ListOrganizationRunnerApplicationDownloads(r.ctx, r.cfg.Name) if err != nil { return nil, errors.Wrap(err, "fetching runner tools") } @@ -113,7 +113,7 @@ func (r *organization) FetchDbInstances() ([]params.Instance, error) { } func (r *organization) RemoveGithubRunner(runnerID int64) (*github.Response, error) { - return r.ghcli.Actions.RemoveOrganizationRunner(r.ctx, r.cfg.Name, runnerID) + return r.ghcli.RemoveOrganizationRunner(r.ctx, r.cfg.Name, runnerID) } func (r *organization) ListPools() ([]params.Pool, error) { @@ -133,7 +133,7 @@ func (r *organization) JwtToken() string { } func (r *organization) GetGithubRegistrationToken() (string, error) { - tk, _, err := r.ghcli.Actions.CreateOrganizationRegistrationToken(r.ctx, r.cfg.Name) + tk, _, err := r.ghcli.CreateOrganizationRegistrationToken(r.ctx, r.cfg.Name) if err != nil { return "", errors.Wrap(err, "creating runner token") diff --git a/runner/pool/common.go b/runner/pool/pool.go similarity index 97% rename from runner/pool/common.go rename to runner/pool/pool.go index 40c449fe..54e735e3 100644 --- a/runner/pool/common.go +++ b/runner/pool/pool.go @@ -237,17 +237,11 @@ func (r *basePool) fetchInstance(runnerName string) (params.Instance, error) { } func (r *basePool) setInstanceRunnerStatus(job params.WorkflowJob, status providerCommon.RunnerStatus) error { - runner, err := r.fetchInstance(job.WorkflowJob.RunnerName) - if err != nil { - return errors.Wrap(err, "fetching instance") - } - updateParams := params.UpdateInstanceParams{ RunnerStatus: status, } - log.Printf("setting runner status for %s to %s", runner.Name, status) - if _, err := r.store.UpdateInstance(r.ctx, runner.ID, updateParams); err != nil { + if err := r.updateInstance(job.WorkflowJob.RunnerName, updateParams); err != nil { return errors.Wrap(err, "updating runner state") } return nil @@ -609,13 +603,12 @@ func (r *basePool) retryFailedInstancesForOnePool(pool params.Pool) { continue } - // cleanup the failed instance from provider. If we have a provider ID, we use that - // for cleanup. Otherwise, attempt to pass in the name of the instance to the provider, in an - // attempt to cleanup the failed machine. // NOTE(gabriel-samfira): this is done in parallel. If there are many failed instances // this has the potential to create many API requests to the target provider. // TODO(gabriel-samfira): implement request throttling. if instance.ProviderID == "" && instance.Name == "" { + // This really should not happen, but no harm in being extra paranoid. The name is set + // when creating a db entity for the runner, so we should at least have a name. return } // TODO(gabriel-samfira): Incrementing CreateAttempt should be done within a transaction. @@ -695,16 +688,6 @@ func (r *basePool) deleteInstanceFromProvider(instance params.Instance) error { return nil } -func (r *basePool) poolIDFromLabels(labels []*github.RunnerLabels) (string, error) { - for _, lbl := range labels { - if strings.HasPrefix(*lbl.Name, poolIDLabelprefix) { - labelName := *lbl.Name - return labelName[len(poolIDLabelprefix):], nil - } - } - return "", runnerErrors.ErrNotFound -} - func (r *basePool) deletePendingInstances() { instances, err := r.helper.FetchDbInstances() if err != nil { @@ -743,31 +726,6 @@ func (r *basePool) deletePendingInstances() { } } -func (r *basePool) ForceDeleteRunner(runner params.Instance) error { - if runner.AgentID != 0 { - resp, err := r.helper.RemoveGithubRunner(runner.AgentID) - if err != nil { - if resp != nil { - switch resp.StatusCode { - case http.StatusUnprocessableEntity: - return errors.Wrapf(runnerErrors.ErrUnprocessable, "removing runner: %q", err) - case http.StatusNotFound: - return errors.Wrapf(runnerErrors.ErrNotFound, "removing runner: %q", err) - default: - return errors.Wrap(err, "removing runner") - } - } - return errors.Wrap(err, "removing runner") - } - } - - if err := r.setInstanceStatus(runner.Name, providerCommon.InstancePendingDelete, nil); err != nil { - log.Printf("failed to update runner %s status", runner.Name) - return errors.Wrap(err, "updating runner") - } - return nil -} - func (r *basePool) addPendingInstances() { // TODO: filter instances by status. instances, err := r.helper.FetchDbInstances() @@ -879,3 +837,28 @@ func (r *basePool) WebhookSecret() string { func (r *basePool) ID() string { return r.helper.ID() } + +func (r *basePool) ForceDeleteRunner(runner params.Instance) error { + if runner.AgentID != 0 { + resp, err := r.helper.RemoveGithubRunner(runner.AgentID) + if err != nil { + if resp != nil { + switch resp.StatusCode { + case http.StatusUnprocessableEntity: + return errors.Wrapf(runnerErrors.ErrUnprocessable, "removing runner: %q", err) + case http.StatusNotFound: + return errors.Wrapf(runnerErrors.ErrNotFound, "removing runner: %q", err) + default: + return errors.Wrap(err, "removing runner") + } + } + return errors.Wrap(err, "removing runner") + } + } + + if err := r.setInstanceStatus(runner.Name, providerCommon.InstancePendingDelete, nil); err != nil { + log.Printf("failed to update runner %s status", runner.Name) + return errors.Wrap(err, "updating runner") + } + return nil +} diff --git a/runner/pool/repository.go b/runner/pool/repository.go index 3c12d5d6..1c2d80e2 100644 --- a/runner/pool/repository.go +++ b/runner/pool/repository.go @@ -64,7 +64,7 @@ var _ poolHelper = &repository{} type repository struct { cfg params.Repository ctx context.Context - ghcli *github.Client + ghcli common.GithubClient id string store dbCommon.Store @@ -91,7 +91,7 @@ func (r *repository) GetGithubToken() string { } func (r *repository) GetGithubRunners() ([]*github.Runner, error) { - runners, _, err := r.ghcli.Actions.ListRunners(r.ctx, r.cfg.Owner, r.cfg.Name, nil) + runners, _, err := r.ghcli.ListRunners(r.ctx, r.cfg.Owner, r.cfg.Name, nil) if err != nil { return nil, errors.Wrap(err, "fetching runners") } @@ -102,7 +102,7 @@ func (r *repository) GetGithubRunners() ([]*github.Runner, error) { func (r *repository) FetchTools() ([]*github.RunnerApplicationDownload, error) { r.mux.Lock() defer r.mux.Unlock() - tools, _, err := r.ghcli.Actions.ListRunnerApplicationDownloads(r.ctx, r.cfg.Owner, r.cfg.Name) + tools, _, err := r.ghcli.ListRunnerApplicationDownloads(r.ctx, r.cfg.Owner, r.cfg.Name) if err != nil { return nil, errors.Wrap(err, "fetching runner tools") } @@ -115,7 +115,7 @@ func (r *repository) FetchDbInstances() ([]params.Instance, error) { } func (r *repository) RemoveGithubRunner(runnerID int64) (*github.Response, error) { - return r.ghcli.Actions.RemoveRunner(r.ctx, r.cfg.Owner, r.cfg.Name, runnerID) + return r.ghcli.RemoveRunner(r.ctx, r.cfg.Owner, r.cfg.Name, runnerID) } func (r *repository) ListPools() ([]params.Pool, error) { @@ -135,7 +135,7 @@ func (r *repository) JwtToken() string { } func (r *repository) GetGithubRegistrationToken() (string, error) { - tk, _, err := r.ghcli.Actions.CreateRegistrationToken(r.ctx, r.cfg.Owner, r.cfg.Name) + tk, _, err := r.ghcli.CreateRegistrationToken(r.ctx, r.cfg.Owner, r.cfg.Name) if err != nil { return "", errors.Wrap(err, "creating runner token") diff --git a/runner/runner.go b/runner/runner.go index f9e5acfd..804ffaf1 100644 --- a/runner/runner.go +++ b/runner/runner.go @@ -25,7 +25,6 @@ import ( "hash" "io/ioutil" "log" - "os" "path/filepath" "strings" "sync" @@ -78,10 +77,6 @@ func NewRunner(ctx context.Context, cfg config.Config) (*Runner, error) { credentials: creds, } - if err := runner.ensureSSHKeys(); err != nil { - return nil, errors.Wrap(err, "ensuring SSH keys") - } - if err := runner.loadReposAndOrgs(); err != nil { return nil, errors.Wrap(err, "loading pool managers") } @@ -405,41 +400,6 @@ func (r *Runner) sshPubKey() ([]byte, error) { return key, nil } -func (r *Runner) ensureSSHKeys() error { - sshDir := r.sshDir() - - if _, err := os.Stat(sshDir); err != nil { - if !errors.Is(err, os.ErrNotExist) { - return errors.Wrapf(err, "checking SSH dir %s", sshDir) - } - if err := os.MkdirAll(sshDir, 0o700); err != nil { - return errors.Wrapf(err, "creating ssh dir %s", sshDir) - } - } - - privKeyFile := r.sshKeyPath() - pubKeyFile := r.sshPubKeyPath() - - if _, err := os.Stat(privKeyFile); err == nil { - return nil - } - - pubKey, privKey, err := util.GenerateSSHKeyPair() - if err != nil { - errors.Wrap(err, "generating keypair") - } - - if err := ioutil.WriteFile(privKeyFile, privKey, 0o600); err != nil { - return errors.Wrap(err, "writing private key") - } - - if err := ioutil.WriteFile(pubKeyFile, pubKey, 0o600); err != nil { - return errors.Wrap(err, "writing public key") - } - - return nil -} - func (r *Runner) appendTagsToCreatePoolParams(param params.CreatePoolParams) (params.CreatePoolParams, error) { if err := param.Validate(); err != nil { return params.CreatePoolParams{}, errors.Wrapf(runnerErrors.ErrBadRequest, "validating params: %s", err) diff --git a/scripts/build-static.sh b/scripts/build-static.sh old mode 100644 new mode 100755 index 00ce6302..6aa5aca5 --- a/scripts/build-static.sh +++ b/scripts/build-static.sh @@ -12,6 +12,7 @@ USER_GROUP=${USER_GROUP:-$(id -g)} cd $GARM_SOURCE/cmd/garm go build -mod vendor -o $BIN_DIR/garm -tags osusergo,netgo,sqlite_omit_load_extension -ldflags "-linkmode external -extldflags '-static' -s -w -X main.Version=$(git describe --always --dirty)" . +GOOS=windows CC=x86_64-w64-mingw32-cc go build -mod vendor -o $BIN_DIR/garm.exe -tags osusergo,netgo,sqlite_omit_load_extension -ldflags "-s -w -X main.Version=$(git describe --always --dirty)" . cd $GARM_SOURCE/cmd/garm-cli go build -mod vendor -o $BIN_DIR/garm-cli -tags osusergo,netgo -ldflags "-linkmode external -extldflags '-static' -s -w -X garm/cmd/garm-cli/cmd.Version=$(git describe --always --dirty)" . diff --git a/util/util.go b/util/util.go index b09f3237..993beace 100644 --- a/util/util.go +++ b/util/util.go @@ -15,15 +15,11 @@ package util import ( - "bytes" "context" "crypto/aes" "crypto/cipher" "crypto/rand" - "crypto/rsa" - "crypto/x509" "encoding/base64" - "encoding/pem" "fmt" "io" "io/ioutil" @@ -37,16 +33,13 @@ import ( "garm/config" runnerErrors "garm/errors" "garm/params" + "garm/runner/common" "github.com/google/go-github/v43/github" "github.com/pkg/errors" "golang.org/x/crypto/bcrypt" - "golang.org/x/crypto/ssh" "golang.org/x/oauth2" lumberjack "gopkg.in/natefinch/lumberjack.v2" - "gorm.io/driver/mysql" - "gorm.io/driver/sqlite" - "gorm.io/gorm" ) const alphanumeric = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" @@ -56,15 +49,23 @@ var rxEmail = regexp.MustCompile("^[a-zA-Z0-9.!#$%&'*+\\/=?^_`{|}~-]+@[a-zA-Z0-9 var ( OSToOSTypeMap map[string]config.OSType = map[string]config.OSType{ - "ubuntu": config.Linux, - "rhel": config.Linux, - "centos": config.Linux, - "suse": config.Linux, - "fedora": config.Linux, - "debian": config.Linux, - "flatcar": config.Linux, - "gentoo": config.Linux, - "windows": config.Windows, + "almalinux": config.Linux, + "alma": config.Linux, + "alpine": config.Linux, + "archlinux": config.Linux, + "arch": config.Linux, + "centos": config.Linux, + "ubuntu": config.Linux, + "rhel": config.Linux, + "suse": config.Linux, + "opensuse": config.Linux, + "fedora": config.Linux, + "debian": config.Linux, + "flatcar": config.Linux, + "gentoo": config.Linux, + "rockylinux": config.Linux, + "rocky": config.Linux, + "windows": config.Windows, } githubArchMapping map[string]string = map[string]string{ @@ -151,31 +152,6 @@ func ConvertFileToBase64(file string) (string, error) { return base64.StdEncoding.EncodeToString(bytes), nil } -// GenerateSSHKeyPair generates a private/public key-pair. -// Shamlessly copied from: https://stackoverflow.com/questions/21151714/go-generate-an-ssh-public-key -func GenerateSSHKeyPair() (pubKey, privKey []byte, err error) { - privateKey, err := rsa.GenerateKey(rand.Reader, 4096) - if err != nil { - return nil, nil, err - } - - // generate and write private key as PEM - var privKeyBuf bytes.Buffer - - privateKeyPEM := &pem.Block{Type: "RSA PRIVATE KEY", Bytes: x509.MarshalPKCS1PrivateKey(privateKey)} - if err := pem.Encode(&privKeyBuf, privateKeyPEM); err != nil { - return nil, nil, err - } - - // generate and write public key - pub, err := ssh.NewPublicKey(&privateKey.PublicKey) - if err != nil { - return nil, nil, err - } - - return ssh.MarshalAuthorizedKey(pub), privKeyBuf.Bytes(), nil -} - func OSToOSType(os string) (config.OSType, error) { osType, ok := OSToOSTypeMap[strings.ToLower(os)] if !ok { @@ -184,7 +160,7 @@ func OSToOSType(os string) (config.OSType, error) { return osType, nil } -func GithubClient(ctx context.Context, token string) (*github.Client, error) { +func GithubClient(ctx context.Context, token string) (common.GithubClient, error) { ts := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: token}, ) @@ -193,7 +169,7 @@ func GithubClient(ctx context.Context, token string) (*github.Client, error) { ghClient := github.NewClient(tc) - return ghClient, nil + return ghClient.Actions, nil } func GetCloudConfig(bootstrapParams params.BootstrapInstance, tools github.RunnerApplicationDownload, runnerName string) (string, error) { @@ -229,28 +205,6 @@ func GetCloudConfig(bootstrapParams params.BootstrapInstance, tools github.Runne return asStr, nil } -// NewDBConn returns a new gorm db connection, given the config -func NewDBConn(dbCfg config.Database) (conn *gorm.DB, err error) { - dbType, connURI, err := dbCfg.GormParams() - if err != nil { - return nil, errors.Wrap(err, "getting DB URI string") - } - switch dbType { - case config.MySQLBackend: - conn, err = gorm.Open(mysql.Open(connURI), &gorm.Config{}) - case config.SQLiteBackend: - conn, err = gorm.Open(sqlite.Open(connURI), &gorm.Config{}) - } - if err != nil { - return nil, errors.Wrap(err, "connecting to database") - } - - if dbCfg.Debug { - conn = conn.Debug() - } - return conn, nil -} - // GetRandomString returns a secure random string func GetRandomString(n int) (string, error) { data := make([]byte, n) @@ -322,7 +276,6 @@ func Aes256DecodeString(target []byte, passphrase string) (string, error) { func PaswsordToBcrypt(password string) (string, error) { hashedPassword, err := bcrypt.GenerateFromPassword([]byte(password), bcrypt.DefaultCost) if err != nil { - // TODO: make this a fatal error, that should return a 500 error to user return "", fmt.Errorf("failed to hash password") } return string(hashedPassword), nil