Update createSpec to use errdefs by dperny · Pull Request #38770 · moby/moby

dperny · 2019-02-21T18:37:56Z

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 errdefs helper 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)

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>

cpuguy83

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.

cpuguy83 · 2019-02-21T18:58:12Z

 	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))


Maybe this should be Conflict?

Agree, will fix.

cpuguy83 · 2019-02-21T19:00:07Z


 	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))


Can you change this to errors.Wrap?

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.

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:")

I think the first one would be clearer (and the InvalidParameter would not be nested deeper)

cpuguy83 · 2019-02-21T19:01:14Z

 	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))


errors.Wrap

cpuguy83 · 2019-02-21T19:02:44Z

-		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))


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.

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.

cpuguy83 · 2019-02-21T19:03:01Z

-		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))


errors.Wrap

codecov · 2019-02-21T21:07:02Z

Codecov Report

❗ No coverage uploaded for pull request base (master@cbb885b). Click here to learn what that means.
The diff coverage is 15.38%.

@@            Coverage Diff            @@
##             master   #38770   +/-   ##
=========================================
  Coverage          ?   36.42%           
=========================================
  Files             ?      613           
  Lines             ?    45847           
  Branches          ?        0           
=========================================
  Hits              ?    16700           
  Misses            ?    26852           
  Partials          ?     2295

thaJeztah · 2019-02-21T21:26:52Z

 )

 func (daemon *Daemon) setupLinkedContainers(container *container.Container) ([]string, error) {
+	// NOTE(dperny): this method has been fixed to return only errdefs errors.


Think we can remove this note (as it's not a "todo") 😅

thaJeztah · 2019-02-21T21:28:35Z

-		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))


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.

thaJeztah · 2019-03-08T10:40:59Z

+		// 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) {


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

logrus.WithFields(logrus.Fields{

"module": "api",

"error_type": fmt.Sprintf("%T", err),

}).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err)

(basically; if we didn't handle the error, it's a server error, so a 500)

thaJeztah · 2019-03-08T10:42:17Z

 	if !IsForbidden(e) {
 		t.Fatalf("expected forbidden error, got: %T", e)
 	}
+	if !IsKnownErrorType(e) {


This looks redundant; we checked if it's a Forbidden error above, and otherwise the test fails (in which case it prints the %T)

right, but this test is to make sure that Forbidden is a known error type.

Not sure I understand; IsForbidden checks that it's a known error type (must be ErrForbidden);

moby/errdefs/is.go

Lines 62 to 66 in 8e610b2

// IsForbidden returns if the passed in error is an ErrForbidden

func IsForbidden(err error) bool {

_, ok := getImplementer(err).(ErrForbidden)

return ok

}

So when we reach this line, we know it's a ErrForbidden, thus a known error-type?

the test isn't for IsForbidden, it's for IsKnownErrorType.

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

case errdefs.IsSystem(err) || errdefs.IsUnknown(err) || errdefs.IsDataLoss(err) || errdefs.IsDeadline(err) || errdefs.IsCancelled(err):

statusCode = http.StatusInternalServerError

default:

statusCode = statusCodeFromGRPCError(err)

if statusCode != http.StatusInternalServerError {

return statusCode

}

if e, ok := err.(causer); ok {

return GetHTTPErrorStatusCode(e.Cause())

}

logrus.WithFields(logrus.Fields{

"module": "api",

"error_type": fmt.Sprintf("%T", err),

}).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err)

}

Yeah, I see that now. I'm gonna change it when I spend some time working on this PR.

GordonTheTurtle added the status/0-triage label Feb 21, 2019

dperny force-pushed the createspec-use-errdefs branch from 001435c to 59c7083 Compare February 21, 2019 18:41

cpuguy83 added status/2-code-review and removed status/0-triage labels Feb 21, 2019

cpuguy83 requested changes Feb 21, 2019

View reviewed changes

thaJeztah reviewed Feb 21, 2019

View reviewed changes

dperny mentioned this pull request Feb 22, 2019

Making it possible to pass Windows credential specs directly to the engine #38777

Merged

GordonTheTurtle assigned thaJeztah Mar 8, 2019

thaJeztah requested changes Mar 8, 2019

View reviewed changes

	logrus.WithFields(logrus.Fields{
	"module": "api",
	"error_type": fmt.Sprintf("%T", err),
	}).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err)

	// IsForbidden returns if the passed in error is an ErrForbidden
	func IsForbidden(err error) bool {
	_, ok := getImplementer(err).(ErrForbidden)
	return ok
	}

	case errdefs.IsSystem(err) \|\| errdefs.IsUnknown(err) \|\| errdefs.IsDataLoss(err) \|\| errdefs.IsDeadline(err) \|\| errdefs.IsCancelled(err):
	statusCode = http.StatusInternalServerError
	default:
	statusCode = statusCodeFromGRPCError(err)
	if statusCode != http.StatusInternalServerError {
	return statusCode
	}

	if e, ok := err.(causer); ok {
	return GetHTTPErrorStatusCode(e.Cause())
	}

	logrus.WithFields(logrus.Fields{
	"module": "api",
	"error_type": fmt.Sprintf("%T", err),
	}).Debugf("FIXME: Got an API for which error does not match any expected type!!!: %+v", err)
	}

Conversation

dperny commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Feb 21, 2019

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dperny commented Feb 21, 2019 •

edited

Loading

cpuguy83 left a comment •

edited

Loading