Update createSpec to use errdefs #38770
Conversation
Updates the createSpec method, for both Windows and Linux systems, to return all errors in terms of errdefs. Makes easy changes in dependencies of those methods as well, if bringing them to use all errdefs errors was trivial. Includes comments annotating why each errdef type was chosen for each particular error. Signed-off-by: Drew Erny <drew.erny@docker.com>
001435c to
59c7083
Compare
There was a problem hiding this comment.
Thanks for following up.
Some general comments.
In some cases it seems improper to make assumptions about the errors returned by a lower level function unless it's clear (e.g. validateUserInput would by definition always be an InvaldParam error).
Otherwise we end up with the same risk as we have for the caller of createSpec assuming a System error.
| for linkAlias, child := range children { | ||
| if !child.IsRunning() { | ||
| return nil, fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias) | ||
| return nil, errdefs.InvalidParameter(fmt.Errorf("Cannot link to a non running container: %s AS %s", child.Name, linkAlias)) |
There was a problem hiding this comment.
Maybe this should be Conflict?
|
|
||
| if err := setResources(&s, c.HostConfig.Resources); err != nil { | ||
| return nil, fmt.Errorf("linux runtime spec resources: %v", err) | ||
| return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec resources: %v", err)) |
There was a problem hiding this comment.
Can you change this to errors.Wrap?
There was a problem hiding this comment.
Yeah, I realize I discovered that errors.Wrap works with errdefs, but I forgot to go back and change the ones I'd already written.
There was a problem hiding this comment.
To be clear, do you think this:
errdefs.InvalidParameter(errors.Wrap(err, "linux runtime spec resources:")or this:
errors.Wrap(errdefs.InvalidParameter(err), "linux runtime spec resources:")There was a problem hiding this comment.
I think the first one would be clearer (and the InvalidParameter would not be nested deeper)
| if err := setUser(&s, c); err != nil { | ||
| return nil, fmt.Errorf("linux spec user: %v", err) | ||
| // setUser fails if you specify a bad user, so it's an InvalidParameter | ||
| return nil, errdefs.InvalidParameter(fmt.Errorf("linux spec user: %v", err)) |
| return nil, fmt.Errorf("linux runtime spec rlimits: %v", err) | ||
| // as of this writing, setRlimits doesn't return any errors at all, so | ||
| // we return error type Unknown if it happens. | ||
| return nil, errdefs.Unknown(fmt.Errorf("linux runtime spec rlimits: %v", err)) |
There was a problem hiding this comment.
errors.Wrap
But we should maybe not use errdefs here, in case setRLimits wants to use errors int he future? Or if it's really not returning errors, maybe the method definition should be updated.
There was a problem hiding this comment.
I could do either. I suppose it would be the ideal to enforce that if setRLimits ever did return errors, they would have to be of errdefs types.
| return nil, fmt.Errorf("linux runtime spec devices: %v", err) | ||
| // The errors resulting from devices are generally more of a | ||
| // user-configured-it-wrong error, so return InvalidParameter | ||
| return nil, errdefs.InvalidParameter(fmt.Errorf("linux runtime spec devices: %v", err)) |
Codecov Report
@@ Coverage Diff @@
## master #38770 +/- ##
=========================================
Coverage ? 36.42%
=========================================
Files ? 613
Lines ? 45847
Branches ? 0
=========================================
Hits ? 16700
Misses ? 26852
Partials ? 2295 |
| ) | ||
|
|
||
| func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]string, error) { | ||
| // NOTE(dperny): this method has been fixed to return only errdefs errors. |
There was a problem hiding this comment.
Think we can remove this note (as it's not a "todo") 😅
| return nil, fmt.Errorf("linux spec capabilities: %v", err) | ||
| // SetCapabilities returned no error in this revision, so if it fails, | ||
| // we don't know why | ||
| return nil, errdefs.Unknown(fmt.Errorf("linux spec capabilities: %v", err)) |
There was a problem hiding this comment.
I see there's some more fmt.Errorf('s here (and below); should all ideally be changed to errors.Wrap so that the original error doesn't get lost.
| // have changed. To ensure that, no matter what, we get a valid error | ||
| // type, we will match on all known error types, and then wrap this | ||
| // error in "Unknown" if it is none of them | ||
| if !errdefs.IsKnownErrorType(err) { |
There was a problem hiding this comment.
Wondering if we need this, as we already log errors that don't have a type assigned, and treat them as a 500;
moby/api/server/httputils/errors.go
Lines 62 to 65 in 72b0b03
(basically; if we didn't handle the error, it's a server error, so a 500)
| if !IsForbidden(e) { | ||
| t.Fatalf("expected forbidden error, got: %T", e) | ||
| } | ||
| if !IsKnownErrorType(e) { |
There was a problem hiding this comment.
This looks redundant; we checked if it's a Forbidden error above, and otherwise the test fails (in which case it prints the %T)
There was a problem hiding this comment.
right, but this test is to make sure that Forbidden is a known error type.
There was a problem hiding this comment.
Not sure I understand; IsForbidden checks that it's a known error type (must be ErrForbidden);
Lines 62 to 66 in 8e610b2
So when we reach this line, we know it's a ErrForbidden, thus a known error-type?
There was a problem hiding this comment.
the test isn't for IsForbidden, it's for IsKnownErrorType.
There was a problem hiding this comment.
If it's to test if IsKnownErrorType is handling all error-types, it should be a separate test. This test is to test if IsForbidden is working as expected.
But I'm still not sure what the IsKnownErrorType utility is solving (see my comment above: https://github.com/moby/moby/pull/38770/files#r263735447)
By wrapping "unknown error types" as an errdefs.Unknown, we're actually masking unhandled errors, and won't be logging them as errors that have not been mapped;
moby/api/server/httputils/errors.go
Lines 50 to 66 in 72b0b03
There was a problem hiding this comment.
Yeah, I see that now. I'm gonna change it when I spend some time working on this PR.
Signed-off-by: Drew Erny drew.erny@docker.com
- What I did
Updates the createSpec method, for both Windows and Linux systems, to return all errors in terms of errdefs. Makes easy changes in dependencies of those methods as well, if bringing them to use all errdefs errors was trivial. Includes comments annotating why each errdef type was chosen for each particular error.
/cc @cpuguy83, done as promised in exchange for #38632 being accepted.
- How I did it
Carefully step through createSpec and its dependencies, and wrapped each error in an
errdefshelper function before it was returned- How to verify it
You sort of have to review it by hand. This function is really bad in terms of complexity, so a comprehensive test case to cover it would be prohibitively difficult to write.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)