From e6a4137e3b957d760bf7c05a21379d6ed36b157b Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Thu, 8 Jun 2023 18:07:35 +0300 Subject: [PATCH 01/15] properly set version Signed-off-by: Gabriel Adrian Samfira --- scripts/build-static.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/build-static.sh b/scripts/build-static.sh index 70cc4114..debcd5e4 100755 --- a/scripts/build-static.sh +++ b/scripts/build-static.sh @@ -11,11 +11,11 @@ USER_ID=${USER_ID:-$UID} 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)" . +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 --tags --match='v[0-9]*' --dirty --always)" . # 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)" . +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 --tags --match='v[0-9]*' --dirty --always)" . # GOOS=windows CGO_ENABLED=0 go build -mod vendor -o $BIN_DIR/garm-cli.exe -ldflags "-s -w -X garm/cmd/garm-cli/cmd.Version=$(git describe --always --dirty)" . chown $USER_ID:$USER_GROUP -R "$BIN_DIR" From 06745eb88a6ae97ea7589a6bf023764bd59b93cb Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Sat, 10 Jun 2023 16:46:02 +0300 Subject: [PATCH 02/15] Validate the result returned by providers Providers may return only 3 possible statuses: * InstanceRunning * InstanceError * InstanceStopped Every other status is reserved for the controller to set. Provider responses will be split from the instance response in a future commit. Signed-off-by: Gabriel Adrian Samfira --- runner/providers/common/common.go | 15 +++++++++++++++ runner/providers/external/external.go | 25 ++++++++++++++++++------- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/runner/providers/common/common.go b/runner/providers/common/common.go index 41b50027..dfa49f0d 100644 --- a/runner/providers/common/common.go +++ b/runner/providers/common/common.go @@ -35,6 +35,7 @@ const ( RunnerActive RunnerStatus = "active" ) +// IsValidStatus checks if the given status is valid. func IsValidStatus(status InstanceStatus) bool { switch status { case InstanceRunning, InstanceError, InstancePendingCreate, @@ -46,3 +47,17 @@ func IsValidStatus(status InstanceStatus) bool { return false } } + +// IsProviderValidStatus checks if the given status is valid for the provider. +// A provider should only return a status indicating that the instance is in a +// lifecycle state that it can influence. The sole purpose of a provider is to +// manage the lifecycle of an instance. Statuses that indicate an instance should +// be created or removed, will be set by the controller. +func IsValidProviderStatus(status InstanceStatus) bool { + switch status { + case InstanceRunning, InstanceError, InstanceStopped: + return true + default: + return false + } +} diff --git a/runner/providers/external/external.go b/runner/providers/external/external.go index c8054da9..1c2ec1f2 100644 --- a/runner/providers/external/external.go +++ b/runner/providers/external/external.go @@ -44,21 +44,21 @@ type external struct { execPath string } -func (e *external) validateCreateResult(inst params.Instance) error { +func (e *external) validateResult(inst params.Instance) error { if inst.ProviderID == "" { - return garmErrors.NewProviderError("missing provider ID after create call") + return garmErrors.NewProviderError("missing provider ID") } if inst.Name == "" { - return garmErrors.NewProviderError("missing instance name after create call") + return garmErrors.NewProviderError("missing instance name") } if inst.OSName == "" || inst.OSArch == "" || inst.OSType == "" { // we can still function without this info (I think) - log.Printf("WARNING: missing OS information after create call") + log.Printf("WARNING: missing OS information") } - if !providerCommon.IsValidStatus(inst.Status) { - return garmErrors.NewProviderError("invalid status returned (%s) after create call", inst.Status) + if !providerCommon.IsValidProviderStatus(inst.Status) { + return garmErrors.NewProviderError("invalid status returned (%s)", inst.Status) } return nil @@ -88,7 +88,7 @@ func (e *external) CreateInstance(ctx context.Context, bootstrapParams params.Bo return params.Instance{}, garmErrors.NewProviderError("failed to decode response from binary: %s", err) } - if err := e.validateCreateResult(param); err != nil { + if err := e.validateResult(param); err != nil { return params.Instance{}, garmErrors.NewProviderError("failed to validate result: %s", err) } @@ -137,6 +137,11 @@ func (e *external) GetInstance(ctx context.Context, instance string) (params.Ins if err := json.Unmarshal(out, ¶m); err != nil { return params.Instance{}, garmErrors.NewProviderError("failed to decode response from binary: %s", err) } + + if err := e.validateResult(param); err != nil { + return params.Instance{}, garmErrors.NewProviderError("failed to validate result: %s", err) + } + return param, nil } @@ -158,6 +163,12 @@ func (e *external) ListInstances(ctx context.Context, poolID string) ([]params.I if err := json.Unmarshal(out, ¶m); err != nil { return []params.Instance{}, garmErrors.NewProviderError("failed to decode response from binary: %s", err) } + + for _, inst := range param { + if err := e.validateResult(inst); err != nil { + return []params.Instance{}, garmErrors.NewProviderError("failed to validate result: %s", err) + } + } return param, nil } From e3065d6951e756081307a38d28246c77a8d276ea Mon Sep 17 00:00:00 2001 From: Lea Waller Date: Sun, 11 Jun 2023 17:30:16 +0200 Subject: [PATCH 03/15] Allow installing additional packages in lxd --- params/params.go | 3 ++- runner/providers/lxd/lxd.go | 1 + runner/providers/lxd/specs.go | 3 ++- util/util.go | 3 +++ 4 files changed, 8 insertions(+), 2 deletions(-) diff --git a/params/params.go b/params/params.go index 0e07f3a1..b93952fb 100644 --- a/params/params.go +++ b/params/params.go @@ -222,7 +222,8 @@ type BootstrapInstance struct { } type UserDataOptions struct { - DisableUpdatesOnBoot bool `json:"disable_updates_on_boot"` + DisableUpdatesOnBoot bool `json:"disable_updates_on_boot"` + ExtraPackages []string `json:"extra_packages"` } type Tag struct { diff --git a/runner/providers/lxd/lxd.go b/runner/providers/lxd/lxd.go index dec6361f..270073a2 100644 --- a/runner/providers/lxd/lxd.go +++ b/runner/providers/lxd/lxd.go @@ -236,6 +236,7 @@ func (l *LXD) getCreateInstanceArgs(bootstrapParams params.BootstrapInstance, sp } bootstrapParams.UserDataOptions.DisableUpdatesOnBoot = specs.DisableUpdates + bootstrapParams.UserDataOptions.ExtraPackages = specs.ExtraPackages cloudCfg, err := util.GetCloudConfig(bootstrapParams, tools, bootstrapParams.Name) if err != nil { return api.InstancesPost{}, errors.Wrap(err, "generating cloud-config") diff --git a/runner/providers/lxd/specs.go b/runner/providers/lxd/specs.go index 21470c11..202473b7 100644 --- a/runner/providers/lxd/specs.go +++ b/runner/providers/lxd/specs.go @@ -22,7 +22,8 @@ import ( ) type extraSpecs struct { - DisableUpdates bool `json:"disable_updates"` + DisableUpdates bool `json:"disable_updates"` + ExtraPackages []string `json:"extra_packages"` } func parseExtraSpecsFromBootstrapParams(bootstrapParams params.BootstrapInstance) (extraSpecs, error) { diff --git a/util/util.go b/util/util.go index 35b50ee2..eddbb1a1 100644 --- a/util/util.go +++ b/util/util.go @@ -271,6 +271,9 @@ func GetCloudConfig(bootstrapParams params.BootstrapInstance, tools github.Runne cloudCfg.PackageUpgrade = false cloudCfg.Packages = []string{} } + for _, pkg := range bootstrapParams.UserDataOptions.ExtraPackages { + cloudCfg.AddPackage(pkg) + } cloudCfg.AddSSHKey(bootstrapParams.SSHKeys...) cloudCfg.AddFile(installScript, "/install_runner.sh", "root:root", "755") From 636164fb18f8bc5338b7194a9041a2bed46c5884 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 13 Jun 2023 13:24:00 +0300 Subject: [PATCH 04/15] Exclude a temp cmd used for testing Signed-off-by: Gabriel Adrian Samfira --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index a8b112e6..c4a5d98b 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ bin/ # Dependency directories (remove the comment below to include it) # vendor/ .vscode +cmd/temp From 44c7ac1b7b61dfe1a9ead802228d2be80f92f8ea Mon Sep 17 00:00:00 2001 From: Mihaela Balutoiu Date: Thu, 8 Jun 2023 16:46:32 +0300 Subject: [PATCH 05/15] Add more `users.go` unit tests Signed-off-by: Mihaela Balutoiu --- database/sql/users_test.go | 46 +++++++++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 3 deletions(-) diff --git a/database/sql/users_test.go b/database/sql/users_test.go index 80970ef3..961f2ed3 100644 --- a/database/sql/users_test.go +++ b/database/sql/users_test.go @@ -26,9 +26,9 @@ import ( ) type UserTestFixtures struct { - Users []params.User - NewUserParams params.NewUserParams - AdminContext context.Context + Users []params.User + NewUserParams params.NewUserParams + UpdateUserParams params.UpdateUserParams } type UserTestSuite struct { @@ -65,6 +65,7 @@ func (s *UserTestSuite) SetupTest() { } // setup test fixtures + var enabled bool fixtures := &UserTestFixtures{ Users: users, NewUserParams: params.NewUserParams{ @@ -73,6 +74,11 @@ func (s *UserTestSuite) SetupTest() { FullName: "test-fullname", Password: "test-password", }, + UpdateUserParams: params.UpdateUserParams{ + FullName: "test-update-fullname", + Password: "test-update-password", + Enabled: &enabled, + }, } s.Fixtures = fixtures } @@ -120,6 +126,24 @@ func (s *UserTestSuite) TestCreateUserEmailAlreadyExist() { s.Require().Equal(("email already exists"), err.Error()) } +func (s *UserTestSuite) TestHasAdminUserNoAdmin() { + hasAdmin := s.Store.HasAdminUser(context.Background()) + + // initially, we don't have any admin users in the store + s.Require().False(hasAdmin) +} + +func (s *UserTestSuite) TestHasAdminUser() { + // create an admin user + s.Fixtures.NewUserParams.IsAdmin = true + _, err := s.Store.CreateUser(context.Background(), s.Fixtures.NewUserParams) + s.Require().Nil(err) + + // check again if the store has any admin users + hasAdmin := s.Store.HasAdminUser(context.Background()) + s.Require().True(hasAdmin) +} + func (s *UserTestSuite) TestGetUser() { user, err := s.Store.GetUser(context.Background(), s.Fixtures.Users[0].Username) @@ -154,6 +178,22 @@ func (s *UserTestSuite) TestGetUserByIDNotFound() { s.Require().Equal("fetching user: not found", err.Error()) } +func (s *UserTestSuite) TestUpdateUser() { + user, err := s.Store.UpdateUser(context.Background(), s.Fixtures.Users[0].Username, s.Fixtures.UpdateUserParams) + + s.Require().Nil(err) + s.Require().Equal(s.Fixtures.UpdateUserParams.FullName, user.FullName) + s.Require().Equal(s.Fixtures.UpdateUserParams.Password, user.Password) + s.Require().Equal(*s.Fixtures.UpdateUserParams.Enabled, user.Enabled) +} + +func (s *UserTestSuite) TestUpdateUserNotFound() { + _, err := s.Store.UpdateUser(context.Background(), "dummy-user", s.Fixtures.UpdateUserParams) + + s.Require().NotNil(err) + s.Require().Equal("fetching user: not found", err.Error()) +} + func TestUserTestSuite(t *testing.T) { suite.Run(t, new(UserTestSuite)) } From 0e637c10e3e598e891299a25de208fe9429f9b40 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 13 Jun 2023 21:01:40 +0300 Subject: [PATCH 06/15] Remove failed runner and some retries * When a runner fails to set up the github agent, we reap it after the pool timeout is reached. * add a retry in the userdata when configuring the runner agent Signed-off-by: Gabriel Adrian Samfira --- cloudconfig/templates.go | 28 ++++++++++++++++++++++++++- runner/pool/pool.go | 42 ++++++++++++++++++++++++++++------------ 2 files changed, 57 insertions(+), 13 deletions(-) diff --git a/cloudconfig/templates.go b/cloudconfig/templates.go index e088d2cd..15638fab 100644 --- a/cloudconfig/templates.go +++ b/cloudconfig/templates.go @@ -124,7 +124,33 @@ fi sendStatus "configuring runner" -sudo -u {{ .RunnerUsername }} -- ./config.sh --unattended --url "{{ .RepoURL }}" --token "$GITHUB_TOKEN" $RUNNER_GROUP_OPT --name "{{ .RunnerName }}" --labels "{{ .RunnerLabels }}" --ephemeral || fail "failed to configure runner" +set +e +attempt=1 +while true; do + ERROUT=$(mktemp) + sudo -u {{ .RunnerUsername }} -- ./config.sh --unattended --url "{{ .RepoURL }}" --token "$GITHUB_TOKEN" $RUNNER_GROUP_OPT --name "{{ .RunnerName }}" --labels "{{ .RunnerLabels }}" --ephemeral 2>$ERROUT + if [ $? -eq 0 ]; then + rm $ERROUT || true + break + fi + LAST_ERR=$(cat $ERROUT) + echo "$LAST_ERR" + + # if the runner is already configured, remove it and try again. In the past configuring a runner + # managed to register it but timed out later, resulting in an error. + sudo -u {{ .RunnerUsername }} -- ./config.sh remove --token "$GITHUB_TOKEN" || true + + if [ $attempt -gt 5 ];then + rm $ERROUT || true + fail "failed to configure runner: $LAST_ERR" + fi + + sendStatus "failed to configure runner (attempt $attempt): $LAST_ERR" + attempt=$((attempt+1)) + rm $ERROUT || true + sleep 5 +done +set -e sendStatus "installing runner service" ./svc.sh install {{ .RunnerUsername }} || fail "failed to install service" diff --git a/runner/pool/pool.go b/runner/pool/pool.go index dce4e741..e46bd4f0 100644 --- a/runner/pool/pool.go +++ b/runner/pool/pool.go @@ -365,26 +365,44 @@ func (r *basePoolManager) reapTimedOutRunners(runners []*github.Runner) error { return errors.Wrap(err, "fetching instances from db") } - runnerNames := map[string]bool{} + runnersByName := map[string]*github.Runner{} for _, run := range runners { if !r.isManagedRunner(labelsFromRunner(run)) { log.Printf("runner %s is not managed by a pool belonging to %s", *run.Name, r.helper.String()) continue } - runnerNames[*run.Name] = true + runnersByName[*run.Name] = run } for _, instance := range dbInstances { - if ok := runnerNames[instance.Name]; !ok { - pool, err := r.store.GetPoolByID(r.ctx, instance.PoolID) - if err != nil { - return errors.Wrap(err, "fetching instance pool info") - } - if time.Since(instance.UpdatedAt).Minutes() < float64(pool.RunnerTimeout()) { - continue - } - log.Printf("reaping instance %s due to timeout", instance.Name) - if err := r.setInstanceStatus(instance.Name, providerCommon.InstancePendingDelete, nil); err != nil { + pool, err := r.store.GetPoolByID(r.ctx, instance.PoolID) + if err != nil { + return errors.Wrap(err, "fetching instance pool info") + } + if time.Since(instance.UpdatedAt).Minutes() < float64(pool.RunnerTimeout()) { + continue + } + + // There are 2 cases (currently) where we consider a runner as timed out: + // * The runner never joined github within the pool timeout + // * The runner managed to join github, but the setup process failed later and the runner + // never started on the instance. + // + // There are several steps in the user data that sets up the runner: + // * Download and unarchive the runner from github (or used the cached version) + // * Configure runner (connects to github). At this point the runner is seen as offline. + // * Install the service + // * Set SELinux context (if SELinux is enabled) + // * Start the service (if successful, the runner will transition to "online") + // * Get the runner ID + // + // If we fail getting the runner ID after it's started, garm will set the runner status to "failed", + // even though, technically the runner is online and fully functional. This is why we check here for + // both the runner status as reported by GitHub and the runner status as reported by the provider. + // If the runner is "offline" and marked as "failed", it should be safe to reap it. + if runner, ok := runnersByName[instance.Name]; !ok || (runner.GetStatus() == "offline" && instance.RunnerStatus == providerCommon.RunnerFailed) { + log.Printf("reaping timed-out/failed runner %s", instance.Name) + if err := r.ForceDeleteRunner(instance); err != nil { log.Printf("failed to update runner %s status", instance.Name) return errors.Wrap(err, "updating runner") } From 08bf2bf5a1afcc3836307ec6a13457320cdda377 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 13 Jun 2023 21:27:49 +0300 Subject: [PATCH 07/15] Make spacing consistent & add some status updates Convert the templates.go file to contain tabs instead of spaces consistently. Add some status messages when installing. Signed-off-by: Gabriel Adrian Samfira --- cloudconfig/templates.go | 272 ++++++++++++++++++++------------------- 1 file changed, 137 insertions(+), 135 deletions(-) diff --git a/cloudconfig/templates.go b/cloudconfig/templates.go index 15638fab..bfffe1fc 100644 --- a/cloudconfig/templates.go +++ b/cloudconfig/templates.go @@ -63,41 +63,41 @@ function fail() { # This will echo the version number in the filename. Given a file name like: actions-runner-osx-x64-2.299.1.tar.gz # this will output: 2.299.1 function getRunnerVersion() { - FILENAME="{{ .FileName }}" - [[ $FILENAME =~ ([0-9]+\.[0-9]+\.[0-9+]) ]] - echo $BASH_REMATCH + FILENAME="{{ .FileName }}" + [[ $FILENAME =~ ([0-9]+\.[0-9]+\.[0-9+]) ]] + echo $BASH_REMATCH } function getCachedToolsPath() { - CACHED_RUNNER="/opt/cache/actions-runner/latest" - if [ -d "$CACHED_RUNNER" ];then - echo "$CACHED_RUNNER" - return 0 - fi + CACHED_RUNNER="/opt/cache/actions-runner/latest" + if [ -d "$CACHED_RUNNER" ];then + echo "$CACHED_RUNNER" + return 0 + fi - VERSION=$(getRunnerVersion) - if [ -z "$VERSION" ]; then - return 0 - fi + VERSION=$(getRunnerVersion) + if [ -z "$VERSION" ]; then + return 0 + fi - CACHED_RUNNER="/opt/cache/actions-runner/$VERSION" - if [ -d "$CACHED_RUNNER" ];then - echo "$CACHED_RUNNER" - return 0 - fi - return 0 + CACHED_RUNNER="/opt/cache/actions-runner/$VERSION" + if [ -d "$CACHED_RUNNER" ];then + echo "$CACHED_RUNNER" + return 0 + fi + return 0 } function downloadAndExtractRunner() { - sendStatus "downloading tools from {{ .DownloadURL }}" - if [ ! -z "{{ .TempDownloadToken }}" ]; then + sendStatus "downloading tools from {{ .DownloadURL }}" + if [ ! -z "{{ .TempDownloadToken }}" ]; then TEMP_TOKEN="Authorization: Bearer {{ .TempDownloadToken }}" - fi - curl -L -H "${TEMP_TOKEN}" -o "/home/{{ .RunnerUsername }}/{{ .FileName }}" "{{ .DownloadURL }}" || fail "failed to download tools" - mkdir -p /home/runner/actions-runner || fail "failed to create actions-runner folder" - sendStatus "extracting runner" - tar xf "/home/{{ .RunnerUsername }}/{{ .FileName }}" -C /home/{{ .RunnerUsername }}/actions-runner/ || fail "failed to extract runner" - chown {{ .RunnerUsername }}:{{ .RunnerGroup }} -R /home/{{ .RunnerUsername }}/actions-runner/ || fail "failed to change owner" + fi + curl -L -H "${TEMP_TOKEN}" -o "/home/{{ .RunnerUsername }}/{{ .FileName }}" "{{ .DownloadURL }}" || fail "failed to download tools" + mkdir -p /home/runner/actions-runner || fail "failed to create actions-runner folder" + sendStatus "extracting runner" + tar xf "/home/{{ .RunnerUsername }}/{{ .FileName }}" -C /home/{{ .RunnerUsername }}/actions-runner/ || fail "failed to extract runner" + chown {{ .RunnerUsername }}:{{ .RunnerGroup }} -R /home/{{ .RunnerUsername }}/actions-runner/ || fail "failed to change owner" } TEMP_TOKEN="" @@ -107,19 +107,20 @@ GH_RUNNER_GROUP="{{.GitHubRunnerGroup}}" # if it holds a value, it will be part of the command. RUNNER_GROUP_OPT="" if [ ! -z $GH_RUNNER_GROUP ];then - RUNNER_GROUP_OPT="--runnergroup=$GH_RUNNER_GROUP" + RUNNER_GROUP_OPT="--runnergroup=$GH_RUNNER_GROUP" fi CACHED_RUNNER=$(getCachedToolsPath) if [ -z "$CACHED_RUNNER" ];then - downloadAndExtractRunner - sendStatus "installing dependencies" - cd /home/{{ .RunnerUsername }}/actions-runner - sudo ./bin/installdependencies.sh || fail "failed to install dependencies" + downloadAndExtractRunner + sendStatus "installing dependencies" + cd /home/{{ .RunnerUsername }}/actions-runner + sudo ./bin/installdependencies.sh || fail "failed to install dependencies" else - sudo cp -a "$CACHED_RUNNER" "/home/{{ .RunnerUsername }}/actions-runner" - cd /home/{{ .RunnerUsername }}/actions-runner - chown {{ .RunnerUsername }}:{{ .RunnerGroup }} -R "/home/{{ .RunnerUsername }}/actions-runner" || fail "failed to change owner" + sendStatus "using cached runner found in $CACHED_RUNNER" + sudo cp -a "$CACHED_RUNNER" "/home/{{ .RunnerUsername }}/actions-runner" + cd /home/{{ .RunnerUsername }}/actions-runner + chown {{ .RunnerUsername }}:{{ .RunnerGroup }} -R "/home/{{ .RunnerUsername }}/actions-runner" || fail "failed to change owner" fi @@ -131,6 +132,7 @@ while true; do sudo -u {{ .RunnerUsername }} -- ./config.sh --unattended --url "{{ .RepoURL }}" --token "$GITHUB_TOKEN" $RUNNER_GROUP_OPT --name "{{ .RunnerName }}" --labels "{{ .RunnerLabels }}" --ephemeral 2>$ERROUT if [ $? -eq 0 ]; then rm $ERROUT || true + sendStatus "runner successfully configured after $attempt attempt(s)" break fi LAST_ERR=$(cat $ERROUT) @@ -145,7 +147,7 @@ while true; do fail "failed to configure runner: $LAST_ERR" fi - sendStatus "failed to configure runner (attempt $attempt): $LAST_ERR" + sendStatus "failed to configure runner (attempt $attempt): $LAST_ERR (retrying in 5 seconds)" attempt=$((attempt+1)) rm $ERROUT || true sleep 5 @@ -156,8 +158,8 @@ sendStatus "installing runner service" ./svc.sh install {{ .RunnerUsername }} || fail "failed to install service" if [ -e "/sys/fs/selinux" ];then - sudo chcon -h user_u:object_r:bin_t /home/runner/ || fail "failed to change selinux context" - sudo chcon -R -h {{ .RunnerUsername }}:object_r:bin_t /home/runner/* || fail "failed to change selinux context" + sudo chcon -h user_u:object_r:bin_t /home/runner/ || fail "failed to change selinux context" + sudo chcon -R -h {{ .RunnerUsername }}:object_r:bin_t /home/runner/* || fail "failed to change selinux context" fi sendStatus "starting service" @@ -182,105 +184,105 @@ Param( $ErrorActionPreference="Stop" function Invoke-FastWebRequest { - [CmdletBinding()] - Param( - [Parameter(Mandatory=$True,ValueFromPipeline=$true,Position=0)] - [System.Uri]$Uri, - [Parameter(Position=1)] - [string]$OutFile, - [Hashtable]$Headers=@{}, - [switch]$SkipIntegrityCheck=$false - ) - PROCESS - { - if(!([System.Management.Automation.PSTypeName]'System.Net.Http.HttpClient').Type) - { - $assembly = [System.Reflection.Assembly]::LoadWithPartialName("System.Net.Http") - } + [CmdletBinding()] + Param( + [Parameter(Mandatory=$True,ValueFromPipeline=$true,Position=0)] + [System.Uri]$Uri, + [Parameter(Position=1)] + [string]$OutFile, + [Hashtable]$Headers=@{}, + [switch]$SkipIntegrityCheck=$false + ) + PROCESS + { + if(!([System.Management.Automation.PSTypeName]'System.Net.Http.HttpClient').Type) + { + $assembly = [System.Reflection.Assembly]::LoadWithPartialName("System.Net.Http") + } - if(!$OutFile) { - $OutFile = $Uri.PathAndQuery.Substring($Uri.PathAndQuery.LastIndexOf("/") + 1) - if(!$OutFile) { - throw "The ""OutFile"" parameter needs to be specified" - } - } + if(!$OutFile) { + $OutFile = $Uri.PathAndQuery.Substring($Uri.PathAndQuery.LastIndexOf("/") + 1) + if(!$OutFile) { + throw "The ""OutFile"" parameter needs to be specified" + } + } - $fragment = $Uri.Fragment.Trim('#') - if ($fragment) { - $details = $fragment.Split("=") - $algorithm = $details[0] - $hash = $details[1] - } + $fragment = $Uri.Fragment.Trim('#') + if ($fragment) { + $details = $fragment.Split("=") + $algorithm = $details[0] + $hash = $details[1] + } - if (!$SkipIntegrityCheck -and $fragment -and (Test-Path $OutFile)) { - try { - return (Test-FileIntegrity -File $OutFile -Algorithm $algorithm -ExpectedHash $hash) - } catch { - Remove-Item $OutFile - } - } + if (!$SkipIntegrityCheck -and $fragment -and (Test-Path $OutFile)) { + try { + return (Test-FileIntegrity -File $OutFile -Algorithm $algorithm -ExpectedHash $hash) + } catch { + Remove-Item $OutFile + } + } - $client = new-object System.Net.Http.HttpClient - foreach ($k in $Headers.Keys){ - $client.DefaultRequestHeaders.Add($k, $Headers[$k]) - } - $task = $client.GetStreamAsync($Uri) - $response = $task.Result - if($task.IsFaulted) { - $msg = "Request for URL '{0}' is faulted. Task status: {1}." -f @($Uri, $task.Status) - if($task.Exception) { - $msg += "Exception details: {0}" -f @($task.Exception) - } - Throw $msg - } - $outStream = New-Object IO.FileStream $OutFile, Create, Write, None + $client = new-object System.Net.Http.HttpClient + foreach ($k in $Headers.Keys){ + $client.DefaultRequestHeaders.Add($k, $Headers[$k]) + } + $task = $client.GetStreamAsync($Uri) + $response = $task.Result + if($task.IsFaulted) { + $msg = "Request for URL '{0}' is faulted. Task status: {1}." -f @($Uri, $task.Status) + if($task.Exception) { + $msg += "Exception details: {0}" -f @($task.Exception) + } + Throw $msg + } + $outStream = New-Object IO.FileStream $OutFile, Create, Write, None - try { - $totRead = 0 - $buffer = New-Object Byte[] 1MB - while (($read = $response.Read($buffer, 0, $buffer.Length)) -gt 0) { - $totRead += $read - $outStream.Write($buffer, 0, $read); - } - } - finally { - $outStream.Close() - } - if(!$SkipIntegrityCheck -and $fragment) { - Test-FileIntegrity -File $OutFile -Algorithm $algorithm -ExpectedHash $hash - } - } + try { + $totRead = 0 + $buffer = New-Object Byte[] 1MB + while (($read = $response.Read($buffer, 0, $buffer.Length)) -gt 0) { + $totRead += $read + $outStream.Write($buffer, 0, $read); + } + } + finally { + $outStream.Close() + } + if(!$SkipIntegrityCheck -and $fragment) { + Test-FileIntegrity -File $OutFile -Algorithm $algorithm -ExpectedHash $hash + } + } } function Import-Certificate() { - [CmdletBinding()] - param ( - [parameter(Mandatory=$true)] - [string]$CertificatePath, - [parameter(Mandatory=$true)] - [System.Security.Cryptography.X509Certificates.StoreLocation]$StoreLocation="LocalMachine", - [parameter(Mandatory=$true)] - [System.Security.Cryptography.X509Certificates.StoreName]$StoreName="TrustedPublisher" - ) - PROCESS - { - $store = New-Object System.Security.Cryptography.X509Certificates.X509Store( - $StoreName, $StoreLocation) - $store.Open([System.Security.Cryptography.X509Certificates.OpenFlags]::ReadWrite) - $cert = New-Object System.Security.Cryptography.X509Certificates.X509Certificate2( - $CertificatePath) - $store.Add($cert) - } + [CmdletBinding()] + param ( + [parameter(Mandatory=$true)] + [string]$CertificatePath, + [parameter(Mandatory=$true)] + [System.Security.Cryptography.X509Certificates.StoreLocation]$StoreLocation="LocalMachine", + [parameter(Mandatory=$true)] + [System.Security.Cryptography.X509Certificates.StoreName]$StoreName="TrustedPublisher" + ) + PROCESS + { + $store = New-Object System.Security.Cryptography.X509Certificates.X509Store( + $StoreName, $StoreLocation) + $store.Open([System.Security.Cryptography.X509Certificates.OpenFlags]::ReadWrite) + $cert = New-Object System.Security.Cryptography.X509Certificates.X509Certificate2( + $CertificatePath) + $store.Add($cert) + } } function Invoke-APICall() { [CmdletBinding()] - param ( - [parameter(Mandatory=$true)] - [object]$Payload, + param ( + [parameter(Mandatory=$true)] + [object]$Payload, [parameter(Mandatory=$true)] [string]$CallbackURL - ) + ) PROCESS{ Invoke-WebRequest -UseBasicParsing -Method Post -Headers @{"Accept"="application/json"; "Authorization"="Bearer $Token"} -Uri $CallbackURL -Body (ConvertTo-Json $Payload) | Out-Null } @@ -288,12 +290,12 @@ function Invoke-APICall() { function Update-GarmStatus() { [CmdletBinding()] - param ( - [parameter(Mandatory=$true)] - [string]$Message, + param ( + [parameter(Mandatory=$true)] + [string]$Message, [parameter(Mandatory=$true)] [string]$CallbackURL - ) + ) PROCESS{ $body = @{ "status"="installing" @@ -305,14 +307,14 @@ function Update-GarmStatus() { function Invoke-GarmSuccess() { [CmdletBinding()] - param ( - [parameter(Mandatory=$true)] - [string]$Message, + param ( [parameter(Mandatory=$true)] - [int64]$AgentID, + [string]$Message, + [parameter(Mandatory=$true)] + [int64]$AgentID, [parameter(Mandatory=$true)] [string]$CallbackURL - ) + ) PROCESS{ $body = @{ "status"="idle" @@ -325,12 +327,12 @@ function Invoke-GarmSuccess() { function Invoke-GarmFailure() { [CmdletBinding()] - param ( - [parameter(Mandatory=$true)] - [string]$Message, + param ( + [parameter(Mandatory=$true)] + [string]$Message, [parameter(Mandatory=$true)] [string]$CallbackURL - ) + ) PROCESS{ $body = @{ "status"="failed" From 9ed9e9eec580c1adf464fcb15028a677466f7392 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 13 Jun 2023 22:37:20 +0300 Subject: [PATCH 08/15] Add retries for curl calls Signed-off-by: Gabriel Adrian Samfira --- cloudconfig/templates.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cloudconfig/templates.go b/cloudconfig/templates.go index bfffe1fc..e7c7e7db 100644 --- a/cloudconfig/templates.go +++ b/cloudconfig/templates.go @@ -36,11 +36,11 @@ if [ -z "$METADATA_URL" ];then echo "no token is available and METADATA_URL is not set" exit 1 fi -GITHUB_TOKEN=$(curl --fail -s -X GET -H 'Accept: application/json' -H "Authorization: Bearer ${BEARER_TOKEN}" "${METADATA_URL}/runner-registration-token/") +GITHUB_TOKEN=$(curl --retry 5 --retry-max-time 5 --fail -s -X GET -H 'Accept: application/json' -H "Authorization: Bearer ${BEARER_TOKEN}" "${METADATA_URL}/runner-registration-token/") function call() { PAYLOAD="$1" - curl --fail -s -X POST -d "${PAYLOAD}" -H 'Accept: application/json' -H "Authorization: Bearer ${BEARER_TOKEN}" "${CALLBACK_URL}" || echo "failed to call home: exit code ($?)" + curl --retry 5 --retry-max-time 5 --retry-all-errors --fail -s -X POST -d "${PAYLOAD}" -H 'Accept: application/json' -H "Authorization: Bearer ${BEARER_TOKEN}" "${CALLBACK_URL}" || echo "failed to call home: exit code ($?)" } function sendStatus() { @@ -93,7 +93,7 @@ function downloadAndExtractRunner() { if [ ! -z "{{ .TempDownloadToken }}" ]; then TEMP_TOKEN="Authorization: Bearer {{ .TempDownloadToken }}" fi - curl -L -H "${TEMP_TOKEN}" -o "/home/{{ .RunnerUsername }}/{{ .FileName }}" "{{ .DownloadURL }}" || fail "failed to download tools" + curl --retry 5 --retry-max-time 5 --retry-all-errors --fail -L -H "${TEMP_TOKEN}" -o "/home/{{ .RunnerUsername }}/{{ .FileName }}" "{{ .DownloadURL }}" || fail "failed to download tools" mkdir -p /home/runner/actions-runner || fail "failed to create actions-runner folder" sendStatus "extracting runner" tar xf "/home/{{ .RunnerUsername }}/{{ .FileName }}" -C /home/{{ .RunnerUsername }}/actions-runner/ || fail "failed to extract runner" From 9e18d5d7a9465c987337c19116a33bb5057ace51 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 13 Jun 2023 23:47:34 +0300 Subject: [PATCH 09/15] Add build target to Makefile Add a build target to the Makefile that allows building a non static version if both garm and garm-cli. This is useful when you just need to build the binaries locally and make sure the version is properly set. Signed-off-by: Gabriel Adrian Samfira --- Makefile | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Makefile b/Makefile index 72fd651b..be922aae 100644 --- a/Makefile +++ b/Makefile @@ -6,6 +6,7 @@ USER_ID=$(shell ((docker --version | grep -q podman) && echo "0" || id -u)) USER_GROUP=$(shell ((docker --version | grep -q podman) && echo "0" || id -g)) ROOTDIR=$(dir $(abspath $(lastword $(MAKEFILE_LIST)))) GOPATH ?= $(shell go env GOPATH) +VERSION ?= $(shell git describe --tags --match='v[0-9]*' --dirty --always) GO ?= go @@ -18,6 +19,13 @@ build-static: docker run --rm -e USER_ID=$(USER_ID) -e USER_GROUP=$(USER_GROUP) -v $(PWD):/build/garm:z $(IMAGE_TAG) /build-static.sh @echo Binaries are available in $(PWD)/bin +build: + @echo Building garm ${VERSION} + $(shell mkdir -p ./bin) + @$(GO) build -ldflags "-s -w -X main.Version=${VERSION}" -tags osusergo,netgo,sqlite_omit_load_extension -o bin/garm ./cmd/garm + @$(GO) build -ldflags "-s -w -X github.com/cloudbase/garm/cmd/garm-cli/cmd.Version=${VERSION}" -tags osusergo,netgo,sqlite_omit_load_extension -o bin/garm-cli ./cmd/garm-cli + @echo Binaries are available in $(PWD)/bin + install: @$(GO) install -tags osusergo,netgo,sqlite_omit_load_extension ./... @echo Binaries available in ${GOPATH} From a33b15c6c943f248f07e7c930f2a4c2ac303e9b3 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Tue, 13 Jun 2023 23:50:33 +0300 Subject: [PATCH 10/15] Remove install target Signed-off-by: Gabriel Adrian Samfira --- Makefile | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/Makefile b/Makefile index be922aae..fbd2dbf2 100644 --- a/Makefile +++ b/Makefile @@ -10,7 +10,7 @@ VERSION ?= $(shell git describe --tags --match='v[0-9]*' --dirty --always) GO ?= go -default: install +default: build .PHONY : build-static test install-lint-deps lint go-test fmt fmtcheck verify-vendor verify build-static: @@ -26,10 +26,6 @@ build: @$(GO) build -ldflags "-s -w -X github.com/cloudbase/garm/cmd/garm-cli/cmd.Version=${VERSION}" -tags osusergo,netgo,sqlite_omit_load_extension -o bin/garm-cli ./cmd/garm-cli @echo Binaries are available in $(PWD)/bin -install: - @$(GO) install -tags osusergo,netgo,sqlite_omit_load_extension ./... - @echo Binaries available in ${GOPATH} - test: verify go-test install-lint-deps: From e1ad300f795139d938c8980060ef747ecdcea3c9 Mon Sep 17 00:00:00 2001 From: Mihaela Balutoiu Date: Mon, 12 Jun 2023 16:34:29 +0300 Subject: [PATCH 11/15] Add test cases for the `runner/pools.go` Signed-off-by: Mihaela Balutoiu --- runner/pools_test.go | 223 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 223 insertions(+) create mode 100644 runner/pools_test.go diff --git a/runner/pools_test.go b/runner/pools_test.go new file mode 100644 index 00000000..c19a7858 --- /dev/null +++ b/runner/pools_test.go @@ -0,0 +1,223 @@ +// Copyright 2022 Cloudbase Solutions SRL +// +// Licensed under the Apache License, Version 2.0 (the "License"); you may +// not use this file except in compliance with the License. You may obtain +// a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, WITHOUT +// WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the +// License for the specific language governing permissions and limitations +// under the License. + +package runner + +import ( + "context" + "fmt" + "testing" + + "github.com/cloudbase/garm/auth" + "github.com/cloudbase/garm/config" + "github.com/cloudbase/garm/database" + dbCommon "github.com/cloudbase/garm/database/common" + runnerErrors "github.com/cloudbase/garm/errors" + garmTesting "github.com/cloudbase/garm/internal/testing" + "github.com/cloudbase/garm/params" + "github.com/cloudbase/garm/runner/common" + "github.com/stretchr/testify/suite" +) + +type PoolTestFixtures struct { + AdminContext context.Context + Store dbCommon.Store + Pools []params.Pool + Providers map[string]common.Provider + Credentials map[string]config.Github + CreateInstanceParams params.CreateInstanceParams + UpdatePoolParams params.UpdatePoolParams +} + +type PoolTestSuite struct { + suite.Suite + Fixtures *PoolTestFixtures + Runner *Runner +} + +func (s *PoolTestSuite) SetupTest() { + adminCtx := auth.GetAdminContext() + + // create testing sqlite database + dbCfg := garmTesting.GetTestSqliteDBConfig(s.T()) + db, err := database.NewDatabase(adminCtx, dbCfg) + if err != nil { + s.FailNow(fmt.Sprintf("failed to create db connection: %s", err)) + } + + // create an organization for testing purposes + org, err := db.CreateOrganization(context.Background(), "test-org", "test-creds", "test-webhookSecret") + if err != nil { + s.FailNow(fmt.Sprintf("failed to create org: %s", err)) + } + + // create some pool objects in the database, for testing purposes + orgPools := []params.Pool{} + for i := 1; i <= 3; i++ { + pool, err := db.CreateOrganizationPool( + context.Background(), + org.ID, + params.CreatePoolParams{ + ProviderName: "test-provider", + MaxRunners: 4, + MinIdleRunners: 2, + Image: fmt.Sprintf("test-image-%d", i), + Flavor: "test-flavor", + OSType: "linux", + Tags: []string{"self-hosted", "amd64", "linux"}, + RunnerBootstrapTimeout: 0, + }, + ) + if err != nil { + s.FailNow(fmt.Sprintf("cannot create org pool: %v", err)) + } + orgPools = append(orgPools, pool) + } + + // setup test fixtures + var maxRunners uint = 40 + var minIdleRunners uint = 20 + fixtures := &PoolTestFixtures{ + AdminContext: adminCtx, + Store: db, + Pools: orgPools, + UpdatePoolParams: params.UpdatePoolParams{ + MaxRunners: &maxRunners, + MinIdleRunners: &minIdleRunners, + Image: "test-images-updated", + Flavor: "test-flavor-updated", + }, + CreateInstanceParams: params.CreateInstanceParams{ + Name: "test-instance-name", + OSType: "linux", + }, + } + s.Fixtures = fixtures + + // setup test runner + runner := &Runner{ + providers: fixtures.Providers, + credentials: fixtures.Credentials, + store: fixtures.Store, + ctx: fixtures.AdminContext, + } + s.Runner = runner +} + +func (s *PoolTestSuite) TestListAllPools() { + // call tested function + pools, err := s.Runner.ListAllPools(s.Fixtures.AdminContext) + + // assertions + s.Require().Nil(err) + garmTesting.EqualDBEntityID(s.T(), s.Fixtures.Pools, pools) +} + +func (s *PoolTestSuite) TestListAllPoolsErrUnauthorized() { + _, err := s.Runner.ListAllPools(context.Background()) + + s.Require().NotNil(err) + s.Require().Equal(runnerErrors.ErrUnauthorized, err) +} + +func (s *PoolTestSuite) TestGetPoolByID() { + pool, err := s.Runner.GetPoolByID(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID) + + s.Require().Nil(err) + s.Require().Equal(s.Fixtures.Pools[0].ID, pool.ID) +} + +func (s *PoolTestSuite) TestGetPoolByIDErrUnauthorized() { + _, err := s.Runner.GetPoolByID(context.Background(), "dummy-pool-id") + + s.Require().NotNil(err) + s.Require().Equal(runnerErrors.ErrUnauthorized, err) +} + +func (s *PoolTestSuite) TestGetPoolByIDNotFound() { + err := s.Fixtures.Store.DeletePoolByID(context.Background(), s.Fixtures.Pools[0].ID) + + s.Require().Nil(err) + _, err = s.Runner.GetPoolByID(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID) + s.Require().NotNil(err) + s.Require().Equal("fetching pool: fetching pool by ID: not found", err.Error()) +} + +func (s *PoolTestSuite) TestDeletePoolByID() { + err := s.Runner.DeletePoolByID(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID) + + s.Require().Nil(err) + _, err = s.Fixtures.Store.GetPoolByID(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID) + s.Require().NotNil(err) + s.Require().Equal("fetching pool by ID: not found", err.Error()) +} + +func (s *PoolTestSuite) TestDeletePoolByIDErrUnauthorized() { + err := s.Runner.DeletePoolByID(context.Background(), "dummy-pool-id") + + s.Require().NotNil(err) + s.Require().Equal(runnerErrors.ErrUnauthorized, err) +} + +func (s *PoolTestSuite) TestDeletePoolByIDRunnersFailed() { + _, err := s.Fixtures.Store.CreateInstance(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID, s.Fixtures.CreateInstanceParams) + if err != nil { + s.FailNow(fmt.Sprintf("cannot create instance: %s", err)) + } + + err = s.Runner.DeletePoolByID(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID) + s.Require().NotNil(err) + s.Require().Equal(runnerErrors.NewBadRequestError("pool has runners"), err) +} + +func (s *PoolTestSuite) TestUpdatePoolByIDErrUnauthorized() { + _, err := s.Runner.UpdatePoolByID(context.Background(), "dummy-pool-id", s.Fixtures.UpdatePoolParams) + + s.Require().NotNil(err) + s.Require().Equal(runnerErrors.ErrUnauthorized, err) +} + +func (s *PoolTestSuite) TestTestUpdatePoolByIDInvalidPoolID() { + _, err := s.Runner.UpdatePoolByID(s.Fixtures.AdminContext, "dummy-pool-id", s.Fixtures.UpdatePoolParams) + + s.Require().NotNil(err) + s.Require().Equal("fetching pool: fetching pool by ID: parsing id: invalid request", err.Error()) +} + +func (s *PoolTestSuite) TestTestUpdatePoolByIDRunnerBootstrapTimeoutFailed() { + // this is already created in `SetupTest()` + var RunnerBootstrapTimeout uint = 0 + s.Fixtures.UpdatePoolParams.RunnerBootstrapTimeout = &RunnerBootstrapTimeout + + _, err := s.Runner.UpdatePoolByID(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID, s.Fixtures.UpdatePoolParams) + + s.Require().NotNil(err) + s.Require().Equal(runnerErrors.NewBadRequestError("runner_bootstrap_timeout cannot be 0"), err) +} + +func (s *PoolTestSuite) TestTestUpdatePoolByIDMinIdleGreaterThanMax() { + var maxRunners uint = 10 + var minIdleRunners uint = 11 + s.Fixtures.UpdatePoolParams.MaxRunners = &maxRunners + s.Fixtures.UpdatePoolParams.MinIdleRunners = &minIdleRunners + + _, err := s.Runner.UpdatePoolByID(s.Fixtures.AdminContext, s.Fixtures.Pools[0].ID, s.Fixtures.UpdatePoolParams) + + s.Require().NotNil(err) + s.Require().Equal(runnerErrors.NewBadRequestError("min_idle_runners cannot be larger than max_runners"), err) +} + +func TestPoolTestSuite(t *testing.T) { + suite.Run(t, new(PoolTestSuite)) +} From d3402003330f2cb8a52a98a4c28c873bc02e65cb Mon Sep 17 00:00:00 2001 From: Mihaela Balutoiu Date: Thu, 8 Jun 2023 16:46:32 +0300 Subject: [PATCH 12/15] Add more test cases for the `/database/sql/users.go` Add more coverage for the `users.go` using the SQL database interactions. Signed-off-by: Mihaela Balutoiu --- database/sql/users_test.go | 83 +++++++++++++++++++++++++++++++++++++- 1 file changed, 81 insertions(+), 2 deletions(-) diff --git a/database/sql/users_test.go b/database/sql/users_test.go index 961f2ed3..37105cb6 100644 --- a/database/sql/users_test.go +++ b/database/sql/users_test.go @@ -16,25 +16,40 @@ package sql import ( "context" + "flag" "fmt" + "regexp" "testing" dbCommon "github.com/cloudbase/garm/database/common" garmTesting "github.com/cloudbase/garm/internal/testing" "github.com/cloudbase/garm/params" "github.com/stretchr/testify/suite" + "gopkg.in/DATA-DOG/go-sqlmock.v1" + "gorm.io/driver/mysql" + "gorm.io/gorm" + "gorm.io/gorm/logger" ) type UserTestFixtures struct { Users []params.User NewUserParams params.NewUserParams UpdateUserParams params.UpdateUserParams + SQLMock sqlmock.Sqlmock } type UserTestSuite struct { suite.Suite - Store dbCommon.Store - Fixtures *UserTestFixtures + Store dbCommon.Store + StoreSQLMocked *sqlDatabase + Fixtures *UserTestFixtures +} + +func (s *UserTestSuite) assertSQLMockExpectations() { + err := s.Fixtures.SQLMock.ExpectationsWereMet() + if err != nil { + s.FailNow(fmt.Sprintf("failed to meet sqlmock expectations, got error: %v", err)) + } } func (s *UserTestSuite) SetupTest() { @@ -64,6 +79,29 @@ func (s *UserTestSuite) SetupTest() { users = append(users, user) } + // create store with mocked sql connection + sqlDB, sqlMock, err := sqlmock.New() + if err != nil { + s.FailNow(fmt.Sprintf("failed to run 'sqlmock.New()', got error: %v", err)) + } + s.T().Cleanup(func() { sqlDB.Close() }) + mysqlConfig := mysql.Config{ + Conn: sqlDB, + SkipInitializeWithVersion: true, + } + gormConfig := &gorm.Config{} + if flag.Lookup("test.v").Value.String() == "false" { + gormConfig.Logger = logger.Default.LogMode(logger.Silent) + } + gormConn, err := gorm.Open(mysql.New(mysqlConfig), gormConfig) + if err != nil { + s.FailNow(fmt.Sprintf("fail to open gorm connection: %v", err)) + } + s.StoreSQLMocked = &sqlDatabase{ + conn: gormConn, + cfg: garmTesting.GetTestSqliteDBConfig(s.T()), + } + // setup test fixtures var enabled bool fixtures := &UserTestFixtures{ @@ -79,6 +117,7 @@ func (s *UserTestSuite) SetupTest() { Password: "test-update-password", Enabled: &enabled, }, + SQLMock: sqlMock, } s.Fixtures = fixtures } @@ -126,6 +165,28 @@ func (s *UserTestSuite) TestCreateUserEmailAlreadyExist() { s.Require().Equal(("email already exists"), err.Error()) } +func (s *UserTestSuite) TestCreateUserDBCreateErr() { + s.Fixtures.SQLMock. + ExpectQuery(regexp.QuoteMeta("SELECT * FROM `users` WHERE username = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT 1")). + WithArgs(s.Fixtures.NewUserParams.Username). + WillReturnRows(sqlmock.NewRows([]string{"id"})) + s.Fixtures.SQLMock. + ExpectQuery(regexp.QuoteMeta("SELECT * FROM `users` WHERE email = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT 1")). + WithArgs(s.Fixtures.NewUserParams.Email). + WillReturnRows(sqlmock.NewRows([]string{"id"})) + s.Fixtures.SQLMock.ExpectBegin() + s.Fixtures.SQLMock. + ExpectExec("INSERT INTO `users`"). + WillReturnError(fmt.Errorf("creating user mock error")) + s.Fixtures.SQLMock.ExpectRollback() + + _, err := s.StoreSQLMocked.CreateUser(context.Background(), s.Fixtures.NewUserParams) + + s.assertSQLMockExpectations() + s.Require().NotNil(err) + s.Require().Equal("creating user: creating user mock error", err.Error()) +} + func (s *UserTestSuite) TestHasAdminUserNoAdmin() { hasAdmin := s.Store.HasAdminUser(context.Background()) @@ -194,6 +255,24 @@ func (s *UserTestSuite) TestUpdateUserNotFound() { s.Require().Equal("fetching user: not found", err.Error()) } +func (s *UserTestSuite) TestUpdateUserDBSaveErr() { + s.Fixtures.SQLMock. + ExpectQuery(regexp.QuoteMeta("SELECT * FROM `users` WHERE username = ? AND `users`.`deleted_at` IS NULL ORDER BY `users`.`id` LIMIT 1")). + WithArgs(s.Fixtures.Users[0].ID). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow(s.Fixtures.Users[0].ID)) + s.Fixtures.SQLMock.ExpectBegin() + s.Fixtures.SQLMock. + ExpectExec(("UPDATE `users` SET")). + WillReturnError(fmt.Errorf("saving user mock error")) + s.Fixtures.SQLMock.ExpectRollback() + + _, err := s.StoreSQLMocked.UpdateUser(context.Background(), s.Fixtures.Users[0].ID, s.Fixtures.UpdateUserParams) + + s.assertSQLMockExpectations() + s.Require().NotNil(err) + s.Require().Equal("saving user: saving user mock error", err.Error()) +} + func TestUserTestSuite(t *testing.T) { suite.Run(t, new(UserTestSuite)) } From 7ac24553792a3d94d54aa901666752a0afa17443 Mon Sep 17 00:00:00 2001 From: Mihaela Balutoiu Date: Thu, 15 Jun 2023 14:11:49 +0300 Subject: [PATCH 13/15] Fix `runner/pools.go` typo Signed-off-by: Mihaela Balutoiu --- runner/pools.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/runner/pools.go b/runner/pools.go index f45dec35..f6561b68 100644 --- a/runner/pools.go +++ b/runner/pools.go @@ -67,7 +67,7 @@ func (r *Runner) DeletePoolByID(ctx context.Context, poolID string) error { } if err := r.store.DeletePoolByID(ctx, poolID); err != nil { - return errors.Wrap(err, "fetching pool") + return errors.Wrap(err, "deleting pool") } return nil } From 3a5c939612f98da77e00b12a91e730dc98cdac16 Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Fri, 16 Jun 2023 12:00:32 +0300 Subject: [PATCH 14/15] Add slack link Signed-off-by: Gabriel Adrian Samfira --- README.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/README.md b/README.md index f4ba063e..360f1ca2 100644 --- a/README.md +++ b/README.md @@ -10,6 +10,12 @@ The goal of ```garm``` is to be simple to set up, simple to configure and simple Garm supports creating pools on either GitHub itself or on your own deployment of [GitHub Enterprise Server](https://docs.github.com/en/enterprise-server@3.5/admin/overview/about-github-enterprise-server). For instructions on how to use ```garm``` with GHE, see the [credentials](/doc/github_credentials.md) section of the documentation. +## Join us on slack + +Whether you're running into issues or just want to drop by and say "hi", feel free to [join us on slack](https://communityinviter.com/apps/garm-hq/garm). + +[![slack](https://img.shields.io/badge/slack-garm-brightgreen.svg?logo=slack)](https://communityinviter.com/apps/garm-hq/garm) + ## Installing ## Build from source From 8f31db1f6794b230bdf65046451e4c4d227a81db Mon Sep 17 00:00:00 2001 From: Gabriel Adrian Samfira Date: Fri, 16 Jun 2023 17:36:27 +0300 Subject: [PATCH 15/15] Reword title Signed-off-by: Gabriel Adrian Samfira --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 360f1ca2..6553e1aa 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# GitHub Actions Runners Manager (garm) +# GitHub Actions Runner Manager (garm) [![Go Tests](https://github.com/cloudbase/garm/actions/workflows/go-tests.yml/badge.svg)](https://github.com/cloudbase/garm/actions/workflows/go-tests.yml)