cli/config: improve handling of errors #5077

thaJeztah · 2024-05-16T15:49:52Z

Improving some parts of how we handle failures when loading the CLI configuration file. This relates to #5075, but does not yet fix that issue (only handling some additional errors);

docker should fail if config file is invalid #5075

Changes in this PR should not likely cause a change in behavior (so could be included in a patch release); follow-up changes are still needed to handle various scenarios where we should hard-fail, but there's many different related code-paths, so this still needs work.

cli/config: Load(): improve GoDoc

cli/config: add test for dangling symlink for config-file

This may need further discussion, but we currently handle dangling
symlinks gracefully, so let's add a test for this, and verify that
we don't replace symlinks with a file.

cli/config: TestLoadDefaultConfigFile: check that no warnings are printed

Loading the config should print no warnings on a successful load.

cli/config: do not discard permission errors when loading config-file

When attempting to load a config-file that exists, but is not accessible for
the current user, we should not discard the error.

This patch makes sure that the error is returned by Load(), but does not yet
change LoadDefaultConfigFile, as this requires a change in signature.

- Description for the changelog

Print a warning when the CLI does not have permissions to read the configuration file.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2024-05-17T08:13:03Z

Ah 🙈 probably testing won't work if it's running as root;

59.75 === FAIL: cli/config TestLoadNoPermissions (0.00s)
59.75     config_test.go:79: assertion failed: error is nil, not "permission denied" (os.ErrPermission)
59.75 
59.75 === FAIL: cli/config TestLoadDefaultConfigFile/permission_error (0.00s)
59.75     config_test.go:377: assertion failed: string "" does not contain "WARNING:"
59.75     config_test.go:378: assertion failed: string "" does not contain "permission denied"

vvoland

LGTM (once CI is green)

thaJeztah · 2024-05-21T09:25:22Z

cli/config/config_test.go

+ if runtime.GOOS != "windows" {
+ skip.If(t, os.Getuid() == 0, "cannot test permission denied when running as root")
+ }


I tried if I could make the tests run as non-root, but ran into some issues, so for now, skipping this tests when running as root.

krissetto

overal LGTM, with some godoc nits/questions

krissetto · 2024-05-21T12:01:26Z

cli/config/config.go

@@ -84,8 +84,14 @@ func LoadFromReader(configData io.Reader) (*configfile.ConfigFile, error) {
 return &configFile, err
 }

-// Load reads the configuration files in the given directory, and sets up
-// the auth config information and returns values.
+// Load reads the configuration file ([ConfigFileName]) from the given directory.


what is ([ConfigFileName]) here?

They're "doc links"; if you use brackets and put a reference to a type, then GoDoc renders them as links.

You can test those using pkgsite (go install golang.org/x/pkgsite/cmd/pkgsite@latest), and then browse the docs (it's a bit fiddly on the CLI because of go modules, so I just ran it in a container);

Ah, good to know! I didn't think that having just the type name would be enough for the linking to occur

krissetto · 2024-05-21T12:02:34Z

cli/config/config.go

-// Load reads the configuration files in the given directory, and sets up
-// the auth config information and returns values.
+// Load reads the configuration file ([ConfigFileName]) from the given directory.
+// If no directory is given, it uses the default [Dir]. A [configfile.ConfigFile]


nit: [*configfile.ConfigFile] here since it's a pointer?

Oh! Looks like that works as well; I thought GoDoc links wouldn't accept that (so requiring to manually construct a link), but it looks like it does;

krissetto · 2024-05-21T12:03:26Z

cli/config/config.go

- // than it doesn't exist then stop
- return configFile, nil
+ // Any other error happening when failing to read the file must be returned.
+ return configFile, errors.Wrap(err, "loading config file")


nit: maybe errors.Wrapf(err, "loading config file: %s: ", filename) like below so the errors have a consistent format?

I left that out, because os.Open returns a os.PathError / fs.PathError; https://pkg.go.dev/os#PathError and those already include the path, which would be open /no/such/file: no such file or directory, so manually adding the filename would now have the filename in the error twice.

krissetto · 2024-05-21T12:05:10Z

cli/config/config.go

 }
 return configFile, err
 }

 // LoadDefaultConfigFile attempts to load the default config file and returns
-// an initialized ConfigFile struct if none is found.
+// an initialized ConfigFile struct if none is found or when failing to load


I'm not sure about godoc best practices when returning pointers to structs, should we specify that here as well or only in the function signature?

Yeah, I left it out for now; I think we should consider deprecating this function, or at least provide an alternative that does return the error.

I tried to slightly rephrase the GoDoc to make it more clear that it returns a reference, and that it initialises a default struct if it fails to load / find a config-file.

codecov-commenter · 2024-05-22T08:27:27Z

Codecov Report

Attention: Patch coverage is 66.66667% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 61.34%. Comparing base (57a1180) to head (c102e29).

❗ Current head c102e29 differs from pull request most recent head dc75e9e

Please upload reports for the commit dc75e9e to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5077      +/-   ##
==========================================
+ Coverage   61.32%   61.34%   +0.02%     
==========================================
  Files         298      298              
  Lines       20706    20706              
==========================================
+ Hits        12698    12703       +5     
+ Misses       7106     7103       -3     
+ Partials      902      900       -2

Signed-off-by: Sebastiaan van Stijn <[email protected]>

This may need further discussion, but we currently handle dangling symlinks gracefully, so let's add a test for this, and verify that we don't replace symlinks with a file. Signed-off-by: Sebastiaan van Stijn <[email protected]>

…nted Loading the config should print no warnings on a successful load. Signed-off-by: Sebastiaan van Stijn <[email protected]>

When attempting to load a config-file that exists, but is not accessible for the current user, we should not discard the error. This patch makes sure that the error is returned by Load(), but does not yet change LoadDefaultConfigFile, as this requires a change in signature. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah · 2024-05-22T08:35:13Z

@krissetto I made some changes; PTAL if it LGTY

krissetto · 2024-05-22T12:01:47Z

@krissetto I made some changes; PTAL if it LGTY

looking good @thaJeztah 😎

thaJeztah · 2024-05-22T12:39:45Z

Let me bring this one in; I'll try to spend some time on fixing the actual handling related to #5075, but taking it small steps at a time.

thaJeztah added kind/enhancement status/2-code-review labels May 16, 2024

thaJeztah added this to the 27.0.0 milestone May 16, 2024

thaJeztah self-assigned this May 16, 2024

thaJeztah force-pushed the config_error_handling branch from 06e9312 to 604c531 Compare May 16, 2024 15:57

vvoland approved these changes May 17, 2024

View reviewed changes

thaJeztah force-pushed the config_error_handling branch from 604c531 to 4aab3ef Compare May 21, 2024 09:13

thaJeztah commented May 21, 2024

View reviewed changes

thaJeztah requested a review from krissetto May 21, 2024 11:56

krissetto approved these changes May 21, 2024

View reviewed changes

thaJeztah force-pushed the config_error_handling branch from 4aab3ef to c102e29 Compare May 22, 2024 08:25

thaJeztah added 4 commits May 22, 2024 10:31

cli/config: Load(), LoadDefaultConfigFile(): improve GoDoc

426fb2f

Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/config: add test for dangling symlink for config-file

de91207

This may need further discussion, but we currently handle dangling symlinks gracefully, so let's add a test for this, and verify that we don't replace symlinks with a file. Signed-off-by: Sebastiaan van Stijn <[email protected]>

cli/config: TestLoadDefaultConfigFile: check that no warnings are pri…

c80adf4

…nted Loading the config should print no warnings on a successful load. Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah force-pushed the config_error_handling branch from c102e29 to dc75e9e Compare May 22, 2024 08:34

thaJeztah merged commit 421e3b5 into docker:master May 22, 2024
88 checks passed

thaJeztah deleted the config_error_handling branch May 22, 2024 12:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli/config: improve handling of errors #5077

cli/config: improve handling of errors #5077

thaJeztah commented May 16, 2024

thaJeztah commented May 17, 2024

vvoland left a comment •

edited

thaJeztah May 21, 2024

krissetto left a comment

krissetto May 21, 2024

thaJeztah May 21, 2024

krissetto May 21, 2024

krissetto May 21, 2024

thaJeztah May 22, 2024

krissetto May 21, 2024

thaJeztah May 21, 2024 •

edited

krissetto May 21, 2024

thaJeztah May 21, 2024

thaJeztah May 22, 2024

codecov-commenter commented May 22, 2024 •

edited

thaJeztah commented May 22, 2024

krissetto commented May 22, 2024

thaJeztah commented May 22, 2024

cli/config: improve handling of errors #5077

cli/config: improve handling of errors #5077

Conversation

thaJeztah commented May 16, 2024

cli/config: Load(): improve GoDoc

cli/config: add test for dangling symlink for config-file

cli/config: TestLoadDefaultConfigFile: check that no warnings are printed

cli/config: do not discard permission errors when loading config-file

thaJeztah commented May 17, 2024

vvoland left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krissetto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thaJeztah May 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 22, 2024 • edited

Codecov Report

thaJeztah commented May 22, 2024

krissetto commented May 22, 2024

thaJeztah commented May 22, 2024

vvoland left a comment •

edited

thaJeztah May 21, 2024 •

edited

codecov-commenter commented May 22, 2024 •

edited