From c523032c8649206d6a14297fc214b6ee8a4406e9 Mon Sep 17 00:00:00 2001 From: mihaelabalutoiu Date: Wed, 20 Jul 2022 21:13:05 +0300 Subject: [PATCH 1/3] Fix NewConfig function Move the default setting before validating the config, otherwise the code will never run. This happens because the `Validate()` function will always fail when `config.Default.ConfigDir` is empty. --- config/config.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/config.go b/config/config.go index 487c52c5..5b9d9a1f 100644 --- a/config/config.go +++ b/config/config.go @@ -100,12 +100,12 @@ func NewConfig(cfgFile string) (*Config, error) { if _, err := toml.DecodeFile(cfgFile, &config); err != nil { return nil, errors.Wrap(err, "decoding toml") } - if err := config.Validate(); err != nil { - return nil, errors.Wrap(err, "validating config") - } if config.Default.ConfigDir == "" { config.Default.ConfigDir = DefaultConfigDir } + if err := config.Validate(); err != nil { + return nil, errors.Wrap(err, "validating config") + } return &config, nil } From 795b7ebb566e6ae503d73c4c958cd2477b5691ea Mon Sep 17 00:00:00 2001 From: mihaelabalutoiu Date: Tue, 26 Jul 2022 13:43:25 +0300 Subject: [PATCH 2/3] Add missing assertion --- config/config_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/config/config_test.go b/config/config_test.go index c8e4cd9a..464bb8ef 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -294,6 +294,7 @@ func TestAPITLSconfig(t *testing.T) { // Any error in the TLSConfig should return an error here. cfg.TLSConfig = TLSConfig{} tlsCfg, err = cfg.APITLSConfig() + require.Nil(t, tlsCfg) require.NotNil(t, err) require.EqualError(t, err, "missing crt or key") From 229946a4c007ef996edd0806ef1b238d1c649811 Mon Sep 17 00:00:00 2001 From: mihaelabalutoiu Date: Wed, 20 Jul 2022 21:39:28 +0300 Subject: [PATCH 3/3] Add unit tests for full coverage to the `NewConfig` function --- config/config.go | 7 +-- config/config_test.go | 58 ++++++++++++++++++++++ config/testdata/test-empty-config-dir.toml | 22 ++++++++ config/testdata/test-invalid-config.toml | 2 + config/testdata/test-valid-config.toml | 22 ++++++++ 5 files changed, 108 insertions(+), 3 deletions(-) create mode 100644 config/testdata/test-empty-config-dir.toml create mode 100644 config/testdata/test-invalid-config.toml create mode 100644 config/testdata/test-valid-config.toml diff --git a/config/config.go b/config/config.go index 5b9d9a1f..49bc4080 100644 --- a/config/config.go +++ b/config/config.go @@ -52,9 +52,6 @@ const ( // DefaultConfigFilePath is the default path on disk to the garm // configuration file. DefaultConfigFilePath = "/etc/garm/config.toml" - // DefaultConfigDir is the default path on disk to the config dir. The config - // file will probably be in the same folder, but it is not mandatory. - DefaultConfigDir = "/etc/garm" // DefaultUser is the default username that should exist on the instances. DefaultUser = "runner" @@ -73,6 +70,10 @@ const ( ) var ( + // DefaultConfigDir is the default path on disk to the config dir. The config + // file will probably be in the same folder, but it is not mandatory. + DefaultConfigDir = "/etc/garm" + // DefaultUserGroups are the groups the default user will be part of. DefaultUserGroups = []string{ "sudo", "adm", "cdrom", "dialout", diff --git a/config/config_test.go b/config/config_test.go index 464bb8ef..009fd8c8 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -655,3 +655,61 @@ func TestTimeToLiveDuration(t *testing.T) { require.NotNil(t, err) require.EqualError(t, err, "time: unknown unit \"d\" in duration \"2d\"") } + +func TestNewConfig(t *testing.T) { + cfg, err := NewConfig("testdata/test-valid-config.toml") + require.Nil(t, err) + require.NotNil(t, cfg) + require.Equal(t, "https://garm.example.com/", cfg.Default.CallbackURL) + require.Equal(t, "./testdata/config_dir", cfg.Default.ConfigDir) + require.Equal(t, "0.0.0.0", cfg.APIServer.Bind) + require.Equal(t, 9998, cfg.APIServer.Port) + require.Equal(t, false, cfg.APIServer.UseTLS) + require.Equal(t, DBBackendType("mysql"), cfg.Database.DbBackend) + require.Equal(t, "bocyasicgatEtenOubwonIbsudNutDom", cfg.Database.Passphrase) + require.Equal(t, "test", cfg.Database.MySQL.Username) + require.Equal(t, "test", cfg.Database.MySQL.Password) + require.Equal(t, "127.0.0.1", cfg.Database.MySQL.Hostname) + require.Equal(t, "garm", cfg.Database.MySQL.DatabaseName) + require.Equal(t, "bocyasicgatEtenOubwonIbsudNutDom", cfg.JWTAuth.Secret) + require.Equal(t, timeToLive("48h"), cfg.JWTAuth.TimeToLive) +} + +func TestNewConfigEmptyConfigDir(t *testing.T) { + dirPath, err := ioutil.TempDir("", "garm-config-test") + if err != nil { + t.Fatalf("failed to create temporary directory: %s", err) + } + defer os.RemoveAll(dirPath) + DefaultConfigDir = dirPath + + cfg, err := NewConfig("testdata/test-empty-config-dir.toml") + require.Nil(t, err) + require.NotNil(t, cfg) + require.Equal(t, cfg.Default.ConfigDir, dirPath) + require.Equal(t, "https://garm.example.com/", cfg.Default.CallbackURL) + require.Equal(t, "0.0.0.0", cfg.APIServer.Bind) + require.Equal(t, 9998, cfg.APIServer.Port) + require.Equal(t, false, cfg.APIServer.UseTLS) + require.Equal(t, DBBackendType("mysql"), cfg.Database.DbBackend) + require.Equal(t, "test", cfg.Database.MySQL.Username) + require.Equal(t, "test", cfg.Database.MySQL.Password) + require.Equal(t, "127.0.0.1", cfg.Database.MySQL.Hostname) + require.Equal(t, "garm", cfg.Database.MySQL.DatabaseName) + require.Equal(t, "bocyasicgatEtenOubwonIbsudNutDom", cfg.JWTAuth.Secret) + require.Equal(t, timeToLive("48h"), cfg.JWTAuth.TimeToLive) +} + +func TestNewConfigInvalidTomlPath(t *testing.T) { + cfg, err := NewConfig("this is not a file path") + require.Nil(t, cfg) + require.NotNil(t, err) + require.Regexp(t, "decoding toml", err.Error()) +} + +func TestNewConfigInvalidConfig(t *testing.T) { + cfg, err := NewConfig("testdata/test-invalid-config.toml") + require.Nil(t, cfg) + require.NotNil(t, err) + require.Regexp(t, "validating config", err.Error()) +} diff --git a/config/testdata/test-empty-config-dir.toml b/config/testdata/test-empty-config-dir.toml new file mode 100644 index 00000000..30f36e7a --- /dev/null +++ b/config/testdata/test-empty-config-dir.toml @@ -0,0 +1,22 @@ +[default] + callback_url = "https://garm.example.com/" + config_dir = "" + +[apiserver] + bind = "0.0.0.0" + port = 9998 + use_tls = false + +[database] + backend = "mysql" + passphrase = "bocyasicgatEtenOubwonIbsudNutDom" + + [database.mysql] + username = "test" + password = "test" + hostname = "127.0.0.1" + database = "garm" + +[jwt_auth] + secret = "bocyasicgatEtenOubwonIbsudNutDom" + time_to_live = "48h" diff --git a/config/testdata/test-invalid-config.toml b/config/testdata/test-invalid-config.toml new file mode 100644 index 00000000..2f62acf2 --- /dev/null +++ b/config/testdata/test-invalid-config.toml @@ -0,0 +1,2 @@ +[apiserver] + port = -2 # this is considered an invalid port for garm diff --git a/config/testdata/test-valid-config.toml b/config/testdata/test-valid-config.toml new file mode 100644 index 00000000..5885f90f --- /dev/null +++ b/config/testdata/test-valid-config.toml @@ -0,0 +1,22 @@ +[default] + callback_url = "https://garm.example.com/" + config_dir = "./testdata/config_dir" + +[apiserver] + bind = "0.0.0.0" + port = 9998 + use_tls = false + +[database] + backend = "mysql" + passphrase = "bocyasicgatEtenOubwonIbsudNutDom" + + [database.mysql] + username = "test" + password = "test" + hostname = "127.0.0.1" + database = "garm" + +[jwt_auth] + secret = "bocyasicgatEtenOubwonIbsudNutDom" + time_to_live = "48h"