image/docker: use unified configfile search for cert directories#746
image/docker: use unified configfile search for cert directories#746jankaluza wants to merge 2 commits intocontainers:mainfrom
Conversation
Switch `dockerCertDir` to use the new `configfile.ContainersResourceDirs` for resolving certificate directories. Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
github-actions
bot
added
storage
| for _, dir := range candidates { | ||
| info, err := os.Stat(dir) | ||
| if err != nil { | ||
| if errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrPermission) { |
There was a problem hiding this comment.
why do we skip permission errors?
There was a problem hiding this comment.
I thought it is a good idea to fallback to next directory if we cannot access some directory. In my opinion, this function should find valid directory which we can read files from.
Thinking about it now, maybe showing a warning in the ErrPermission case would be good thing to do. Or do you think this condition should be removed?
| // userConfigPathForResourceDirs is a test hook for ContainersResourceDirs. | ||
| userConfigPathForResourceDirs = UserConfigPath |
There was a problem hiding this comment.
it is not clear to me why do you need this? The other test does not need it? you can just sentenv the XDG_CONFIG_HOME dir?
There was a problem hiding this comment.
This was the best way I found out to let the userConfigPath fail with an error for the user config path resolution failure is ignored test. I admit I'm not the go expert, so maybe there is better way to do that.
In that test, I want to test that even if userConfigPath returns an error, other directories are evaluated. This behavior is aligned with the original code and I wanted to keep it.
I therefore needed to find a way how to monkey-patch the userConfigPath to generate an error so I can test its handling. Maybe I'm just trying to come up with Python solution in go lang :-).
| // | ||
| // The search covers, where configured (listed here from lowest to highest precedence. | ||
| // It can be extended with additional absolute directories via extraDirs (lowest precedence). | ||
| func ContainersResourceDirs(conf *Directory) ([]string, error) { |
There was a problem hiding this comment.
API wise I am not to sure we just want to return the list here?
certs.d just needs the first match starting with home, etc, /usr... so would it not be more logical to pass in the name we search as argument and the return a signle full path and exit early?
There was a problem hiding this comment.
I wanted to make it more generic and let the caller to decide what to do with these directories. But if you think we can just return single directory, I'm also fine changing it.
| func ContainersResourceDirs(conf *Directory) ([]string, error) { | ||
| candidates := make([]string, 0, 7+len(conf.ExtraDirs)) | ||
|
|
||
| userConfig, _ := userConfigPathForResourceDirs() |
There was a problem hiding this comment.
error should not be silently ignored
There was a problem hiding this comment.
This is aligned with the old code - it used homedir.Get() which ignores the error the same way:
func Get() string {
homedir, _ := unshare.HomeDir()
return homedir
}
I can add warning here in the ContainersResourceDirs so the error message about missing homedir is logged. I believe we still want to try other directories if homedir cannot be found, so this error should not be fatal.
| path string | ||
| absolute bool | ||
| var perHostCertDirs = []string{ | ||
| etcDir + "/docker/certs.d", |
There was a problem hiding this comment.
@mtrmac Do we still want this path? API wise it seems rather ugly to define that search order with that additional path.
There was a problem hiding this comment.
That's a good point and it would make the API cleaner definitely.
There was a problem hiding this comment.
We do need it. It’s never been deprecated by us, and it’s the one place users can use to affect both consumers.
See e.g. https://github.com/search?q=repo%3Aopenshift%2Fmachine-config-operator%20%2Fdocker%2Fcerts.d&type=code for a minimal proof.
|
@Luap99 , thanks for the review :-). I will change the code once we finish discussions for all the suggested changes. |
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
(Just a random note for now)
| return "", err | ||
| } | ||
| return fullCertDirPath, nil | ||
| return "", nil |
There was a problem hiding this comment.
Hum, the code is not actually ready to consume "". I guess we never ran into that.
There was a problem hiding this comment.
Well, the code works, but it is not as good as it could be. The previous code returned non-existing directory while the current code returns "" in this case. I think "" is cleaner, because returning non-existing directory looks weird to me.
The directory is then passed to tlsclientconfig.SetupCertificates(certDir, tlsClientConfig) as certDir. This function tries to read that empty dir and returns nil error in case it does not exist.
But I will add check for empty certDir to not even call SetupCertificates. Tell me if you want me to go back to old behavior.
Signed-off-by: Jan Kaluza <jkaluza@redhat.com>
3 participants
Switch
dockerCertDirto use the newconfigfile.ContainersResourceDirsfor resolving certificate directories.