tsh: ignore empty or non-existing config files #11495

marcoandredinis · 2022-03-28T09:54:50Z

If the newly created config.yaml didnt exist we would load the default
values and continue the flow
However, we were not resetting the err's value and would have an ERROR
message at the end and an invalid exit code

Most of the commands would reset that variable to the output of the
command's execution
One of them was not: tsh version
The version command has no return value, so the program would execute
as expected until the last statement: trace.Wrap(error) which would
re-use the err variable whose value is the result of the loadConfig
method.

We could either reset the errs value inside the PrintVersion switch
case block or reset it right after we check for IsNotFound.

We ended up picking the first option as it seems cleaner

 # before
$ make full > /dev/null; build/tsh version
Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18
ERROR: open /home/marco/.tsh/config/config.yaml: no such file or directory

 # after
$ make full > /dev/null; build/tsh version
Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18

r0mant

@marcoandredinis Good catch! Have one comment.

r0mant · 2022-03-28T15:26:42Z

tool/tsh/tsh.go

 	confOptions, err := loadConfig(cf.HomePath)
-	if err != nil && !trace.IsNotFound(err) {
+	if trace.IsNotFound(err) {
+		err = nil
+	}


Hm, I wonder if it would be a little cleaner interface if we made loadConfig return an empty config in case the config file does not exist. Then the caller won't have to worry about handling the not found error. cc @lxea

That makes sense to me

r0mant

@marcoandredinis Also, please include test coverage with the change. If you take my suggestion, I think you can just write a test for loadConfig and make sure it doesn't return error if the file isn't found.

marcoandredinis · 2022-03-29T09:24:26Z

Should we also get a valid config if file is empty? 🤔

r0mant

Should we also get a valid config if file is empty?

@marcoandredinis If yaml decoder fails with empty file, I'd return an empty config in that case also, yes. Otherwise lgtm.

r0mant

Have some comments after the latest changes.

r0mant · 2022-03-29T17:18:45Z

tool/tsh/tshconfig.go

+	if err != nil {
+		return nil, trace.ConvertSystemError(err)
+	}
+	if len(bs) == 0 {


Nit: I'd use bytes.TrimSpace before comparing the length just in case.

I ended up removing the empty/only spaces checks
yaml.Unmarshal handles that just fine

We initially added the check because we were decoding using yaml.NewDecoder(configFile).Decode(&cfg) and that requires at least one valid node: https://pkg.go.dev/gopkg.in/yaml.v2#Decoder.Decode
However, the Unmarshal is fine with empty input

r0mant · 2022-03-29T17:19:00Z

tool/tsh/tshconfig.go

 	cfg := TshConfig{}
-	err = yaml.NewDecoder(configFile).Decode(&cfg)
-	return &cfg, trace.Wrap(err)
+	if yaml.Unmarshal(bs, &cfg); err != nil {


Suggested change

if yaml.Unmarshal(bs, &cfg); err != nil {

if err := yaml.Unmarshal(bs, &cfg); err != nil {

r0mant · 2022-03-29T17:19:48Z

tool/tsh/tshconfig.go

-	confPath := filepath.Join(profile.FullProfilePath(homePath), tshConfigPath)
-	configFile, err := os.Open(confPath)
+func loadConfig(fullConfigPath string) (*TshConfig, error) {
+	emptyConfig := &TshConfig{}


Nit: I think that usually you'd just return &TshConfig{}, nil inline, so you don't really need additional emptyConfig variable.

r0mant · 2022-03-29T17:21:07Z

tool/tsh/tshconfig.go

 		return nil, trace.ConvertSystemError(err)
 	}
 	defer configFile.Close()
+
+	bs, err := io.ReadAll(configFile)


Why not just use os.ReadFile instead of Open+ReadAll then?

lxea

Thanks!

r0mant

@marcoandredinis Let's merge and backport to branch/v9 please.

If the newly created config.yaml doesn't exist we load the default values and continue the flow However, we were not resetting the `err`'s value and would have an ERROR message at the end and an invalid exit code Most of the commands would reset that variable to the output of the command's execution One of them was not: `tsh version` The version command has not return value, so the program would execute as expected until the last statement: `trace.Wrap(error)` which would re-use the `err` variable whose value is the result of the `loadConfig` method. We could clean up the `err`'s value or return no error at all and an empty config when the file doesn't exist We ended up picking the latter option as it seems cleaner ``` # before $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ERROR: open /home/marco/.tsh/config/config.yaml: no such file or directory # after $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ```

yaml.Unmarshal handles empty files just fine, no need to do prior checks for empty or space only files

If the newly created config.yaml didnt exist we would load the default values and continue the flow However, we were not resetting the `err`'s value and would have an ERROR message at the end and an invalid exit code Most of the commands would reset that variable to the output of the command's execution One of them was not: `tsh version` The version command has no return value, so the program would execute as expected until the last statement: `trace.Wrap(error)` which would re-use the `err` variable whose value is the result of the `loadConfig` method. We could either reset the `err`s value inside the `PrintVersion` switch case block or reset it right after we check for `IsNotFound`. We ended up picking the first option as it seems cleaner ``` # before $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ERROR: open /home/marco/.tsh/config/config.yaml: no such file or directory # after $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ```

zmb3 · 2022-04-05T13:26:42Z

@marcoandredinis Looks like this needs to go to branch/v8 as well.

marcoandredinis · 2022-04-05T13:49:18Z

@zmb3 Indeed
On it

If the newly created config.yaml didnt exist we would load the default values and continue the flow However, we were not resetting the `err`'s value and would have an ERROR message at the end and an invalid exit code Most of the commands would reset that variable to the output of the command's execution One of them was not: `tsh version` The version command has no return value, so the program would execute as expected until the last statement: `trace.Wrap(error)` which would re-use the `err` variable whose value is the result of the `loadConfig` method. We could either reset the `err`s value inside the `PrintVersion` switch case block or reset it right after we check for `IsNotFound`. We ended up picking the first option as it seems cleaner ``` # before $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ERROR: open /home/marco/.tsh/config/config.yaml: no such file or directory # after $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ```

* tsh: ignore empty or non-existing config files (#11495) If the newly created config.yaml didnt exist we would load the default values and continue the flow However, we were not resetting the `err`'s value and would have an ERROR message at the end and an invalid exit code Most of the commands would reset that variable to the output of the command's execution One of them was not: `tsh version` The version command has no return value, so the program would execute as expected until the last statement: `trace.Wrap(error)` which would re-use the `err` variable whose value is the result of the `loadConfig` method. We could either reset the `err`s value inside the `PrintVersion` switch case block or reset it right after we check for `IsNotFound`. We ended up picking the first option as it seems cleaner ``` # before $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ERROR: open /home/marco/.tsh/config/config.yaml: no such file or directory # after $ make full > /dev/null; build/tsh version Teleport v10.0.0-dev git:v8.0.0-alpha.1-899-g335adf1f4 go1.18 ``` * fix tsh config test (#11603) check for error when writing to a file

github-actions bot added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label Mar 28, 2022

github-actions bot requested review from r0mant and russjones March 28, 2022 09:55

r0mant reviewed Mar 28, 2022

View reviewed changes

r0mant mentioned this pull request Mar 28, 2022

tsh version ends with error in v9.0.2 #11513

Closed

r0mant reviewed Mar 29, 2022

View reviewed changes

marcoandredinis force-pushed the marcoandredinis/tsh_unset_error_on_config_notfound branch from 727fb99 to df62670 Compare March 29, 2022 09:15

r0mant approved these changes Mar 29, 2022

View reviewed changes

marcoandredinis force-pushed the marcoandredinis/tsh_unset_error_on_config_notfound branch from 80f5d81 to ade6f70 Compare March 29, 2022 15:52

marcoandredinis requested a review from lxea March 29, 2022 16:59

marcoandredinis force-pushed the marcoandredinis/tsh_unset_error_on_config_notfound branch from a8d5f6a to 85cdae7 Compare March 29, 2022 17:01

marcoandredinis enabled auto-merge (squash) March 29, 2022 17:05

r0mant requested changes Mar 29, 2022

View reviewed changes

marcoandredinis changed the title ~~tsh: don't propagate error on config notfound~~ tsh: ignore empty or non-existing config files Mar 30, 2022

lxea approved these changes Mar 30, 2022

View reviewed changes

r0mant approved these changes Mar 30, 2022

View reviewed changes

marcoandredinis added 4 commits March 30, 2022 16:04

return empty config when file is empty

f9e7fc5

dont assume EOF is always empty file

ee0bfc7

fix err handling in yaml unmarshal

ee119d6

yaml.Unmarshal handles empty files just fine, no need to do prior checks for empty or space only files

marcoandredinis force-pushed the marcoandredinis/tsh_unset_error_on_config_notfound branch from bddd3e0 to ee119d6 Compare March 30, 2022 15:04

marcoandredinis disabled auto-merge March 30, 2022 15:04

marcoandredinis merged commit 83a32f4 into master Mar 30, 2022

marcoandredinis deleted the marcoandredinis/tsh_unset_error_on_config_notfound branch March 30, 2022 15:58

marcoandredinis mentioned this pull request Mar 30, 2022

[v9] Backport: tsh: ignore empty or non-existing config files (#11495) #11571

Merged

lxea mentioned this pull request Mar 31, 2022

Add a .tsh/config file, add support for configuring custom http headers from the config file #10336

Merged

marcoandredinis mentioned this pull request Apr 5, 2022

Backport tsh: ignore empty or non-existing config files #11727

Merged

webvictim mentioned this pull request Apr 19, 2022

opened in error #12068

Closed

webvictim mentioned this pull request Jun 8, 2022

Opened in error #13285

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tsh: ignore empty or non-existing config files #11495

tsh: ignore empty or non-existing config files #11495

marcoandredinis commented Mar 28, 2022 •

edited

Loading

r0mant left a comment

r0mant Mar 28, 2022

lxea Mar 28, 2022

r0mant left a comment

marcoandredinis commented Mar 29, 2022

r0mant left a comment

r0mant left a comment

r0mant Mar 29, 2022

marcoandredinis Mar 30, 2022

r0mant Mar 29, 2022

r0mant Mar 29, 2022

r0mant Mar 29, 2022

lxea left a comment

r0mant left a comment

zmb3 commented Apr 5, 2022

marcoandredinis commented Apr 5, 2022

	if yaml.Unmarshal(bs, &cfg); err != nil {
	if err := yaml.Unmarshal(bs, &cfg); err != nil {

tsh: ignore empty or non-existing config files #11495

tsh: ignore empty or non-existing config files #11495

Conversation

marcoandredinis commented Mar 28, 2022 • edited Loading

r0mant left a comment

Choose a reason for hiding this comment

r0mant Mar 28, 2022

Choose a reason for hiding this comment

lxea Mar 28, 2022

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

marcoandredinis commented Mar 29, 2022

r0mant left a comment

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

r0mant Mar 29, 2022

Choose a reason for hiding this comment

marcoandredinis Mar 30, 2022

Choose a reason for hiding this comment

r0mant Mar 29, 2022

Choose a reason for hiding this comment

r0mant Mar 29, 2022

Choose a reason for hiding this comment

r0mant Mar 29, 2022

Choose a reason for hiding this comment

lxea left a comment

Choose a reason for hiding this comment

r0mant left a comment

Choose a reason for hiding this comment

zmb3 commented Apr 5, 2022

marcoandredinis commented Apr 5, 2022

marcoandredinis commented Mar 28, 2022 •

edited

Loading