cli-plugins: Fix searching inaccessible directories

[vvoland]

• fixes: Permission issue on /usr/local/libexec prevents using plugins from /usr/libexec #5643

Fix a case where one inaccessible plugin search path stops the whole search and prevents latter paths from being scanned.

Remove a preliminary Stat call that verifies whether path is an actual directory and is accessible.
It's unneeded and doesn't actually check whether the directory can be listed or not.
os.ReadDir will fail in such case anyway, so just attempt to do that and ignore any encountered error, instead of erroring out the whole plugin candidate listing.

- What I did

- How I did it

- How to verify it
TestListPluginCandidatesInaccesibleDir

- Description for the changelog

Fix inaccessible plugins paths preventing plugins from being detected.

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

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.52%. Comparing base (682cf57) to head (fcd94fe).
Report is 11 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5651      +/-   ##
==========================================
+ Coverage   59.49%   59.52%   +0.03%     
==========================================
  Files         346      346              
  Lines       29371    29359      -12     
==========================================
+ Hits        17474    17476       +2     
+ Misses      10922    10912      -10     
+ Partials      975      971       -4     

[thaJeztah]

The only thing I was considering (but perhaps it's not a real issue) is if we need to distinguish default ("try -> try next") paths and path(s) that are explicitly configured in ~/.docker/config.json as "additional paths" (cliPluginsExtraDirs).

My train of thought there is that if it's an explicit configuration, then failing to traverse the location (perhaps except for "not exist") could be considered a hard failure.

Happy to hear thoughts on that though!

[vvoland]

The attempt to use a plugin which isn't accessible will result in a hard failure anyway. Erroring out during plugin scan would prevent the CLI from running in the non-plugin usage.

[thaJeztah]

Yeah, perhaps I'm looking for issues that aren't important, but mostly considering if it should be completely silent. Here's the order of preference in which paths are considered;

cli/cli-plugins/manager/manager.go

Lines 52 to 62 in 41fba28

	// getPluginDirs returns the platform-specific locations to search for plugins
	// in order of preference.
	//
	// Plugin-discovery is performed in the following order of preference:
	//
	// 1. The "cli-plugins" directory inside the CLIs [config.Path] (usually "~/.docker/cli-plugins").
	// 2. Additional plugin directories as configured through [ConfigFile.CLIPluginsExtraDirs].
	// 3. Platform-specific defaultSystemPluginDirs.
	//
	// [ConfigFile.CLIPluginsExtraDirs]: https://pkg.go.dev/github.com/docker/[email protected]+incompatible/cli/config/configfile#ConfigFile.CLIPluginsExtraDirs
	func getPluginDirs(cfg *configfile.ConfigFile) ([]string, error) {

Based on that, the custom path is intended to have a higher priority than the system-wide installation paths. Which could mean "the cli is configured to use these overridden versions". On Docker Desktop, this could be some of the additional plugins shipping with it, but perhaps it's a situation where (say) the system-wide version is provided by the distro you're running and has a vulnerability, so the intent is to use a fixed version through one of the higher-priority paths. If we silently ignore that we aren't able to use that path, it means we're silently falling back to either an older (perhaps vulnerable) version, or missing extra plugins that were supposed to be available.

For the system-wide paths, perhaps that's OK (it's a system-wide path, and current user isn't in the right group to use it), so even for those, I somewhat wonder if we should have some warning. Not sure if anything documents expected permissions though, or at least I couldn't find that in the FHS (Filesystem Hierarchy Standard) docs.

Quick check on some machines showed me that they are accessible;

# CentOS, Fedora machine

ls -ld /usr/libexec /usr/libexec/docker /usr/libexec/docker/cli-plugins /usr/local/libexec /usr/local/libexec/docker /usr/local/libexec/docker/cli-plugins
ls: cannot access '/usr/local/libexec/docker': No such file or directory
ls: cannot access '/usr/local/libexec/docker/cli-plugins': No such file or directory
drwxr-xr-x. 26 root root 4096 Nov 25 10:52 /usr/libexec
drwxr-xr-x.  3 root root   44 Nov 25 10:53 /usr/libexec/docker
drwxr-xr-x.  2 root root   49 Nov 25 10:52 /usr/libexec/docker/cli-plugins
drwxr-xr-x.  2 root root    6 Aug  9  2021 /usr/local/libexec

# Ubuntu machine
ls -ld /usr/libexec /usr/libexec/docker /usr/libexec/docker/cli-plugins /usr/local/libexec /usr/local/libexec/docker /usr/local/libexec/docker/cli-plugins
ls: cannot access '/usr/local/libexec': No such file or directory
ls: cannot access '/usr/local/libexec/docker': No such file or directory
ls: cannot access '/usr/local/libexec/docker/cli-plugins': No such file or directory
drwxr-xr-x 12 root root 4096 Nov 19 16:02 /usr/libexec
drwxr-xr-x  3 root root 4096 Nov 19 16:02 /usr/libexec/docker
drwxr-xr-x  2 root root 4096 Nov 19 16:02 /usr/libexec/docker/cli-plugins

[thaJeztah]

LGTM

let's discuss if we want to surface these errors as a warning for a follow-up (perhaps we could do as part of the getPluginDirs function, as that would be the place where we collect paths to consider).

[thaJeztah]

Opened a ticket for discussion;

• [rfc] should the CLI warn if a plugin-directory is not accessible? #5656

[laurazard]

LGTM

Since this is quite a bit of a special case, I think we can fix it now and discuss what we think the behaviour should be later (re: should it be an error?).

[thaJeztah]

Yup; let's bring this one in for now 👍