Prefer partial success to failure #126
Conversation
```shell
$ SSL_CERT_FILE="notexist" target/debug/examples/google
thread 'main' panicked at examples/google.rs:7:58:
could not load platform certs: Custom { kind: NotFound, error: "could not load certs from dir notexist: No such file or directory (os error 2)" }
```
Note "from dir notexist". In this case `path.is_file()` is false
(because it doesn't exist) but that doesn't imply it was being
read as a directory.
If we read some certs from a file, but fail to read some from a directory (or vv.) then return what we have. This is good because it restores the crate's previous behaviour. It also matches what (for example) golang crypto.x509 does. This is bad because it hides a failure, which would be confusing if a user relied on reading from _both_ a file and directory at the same time. Would someone do that? The follow commit steps towards ameliorating this, slightly.
ctz
force-pushed
the
jbp-best-effort-errors
branch
from
9a9936a to
16b2dfc
Compare
There was a problem hiding this comment.
If we're going to do this, should we also try to be more forgiving inside load_certs_from_dir()? What about the various implementations of load_certs()? I think both could skip over some error cases.
Are we trying to maintain backwards compatibility or making the library entirely best effort? And if it's the latter, wouldn't it still be better to make the API explicit about this by returning retrieved certificates and errors so that the application can decide how strict it wants to be?
| } | ||
|
|
||
| // promote first error if we have no certs to return | ||
| if let (Some(error), []) = (first_error, certs.as_slice()) { |
There was a problem hiding this comment.
Just to call this out: is the 0 vs 1 boundary the right one? It is different from the previous approach -- it would be "more" backwards-compatible if we just swallowed the error from load_pem_certs_from_dir(), I suppose?
There was a problem hiding this comment.
Not quite grasping what this comment is saying, could your reword? Is it whether we return the first or last error? Or about the preexisting behaviour where the crate can return Ok([])?
I'm not sure I follow what you're thinking here, the only public API surface is |
|
So if we follow #128 (comment), which I think we should, I am minded to remove the tracing stuff here (the feature, dependency, and calls), on the basis that:
|
That sounds sensible to me 👍 |
|
Yup, makes sense. |
ctz
force-pushed
the
jbp-best-effort-errors
branch
from
16b2dfc to
c2be86c
Compare
|
Dropped that commit. |
|
Added up top, please review. |
|
Looks great to me! |
|
LGTM too, thanks! |
fixes #124
Draft release notes:
If loading certificates from a directory fails, only raise that error if no certificates were successfully loaded from a file. See ”Permission denied“ since v0.7.1 #124.
In 0.8.0 we plan to alter the API to give the caller full error details, so they can choose the level of success they require; see Yield all the errors #128.