errdefs: remove dependency on grpc, containerd, and distribution #42624

thaJeztah · 2021-07-12T11:52:49Z

relates to #42623

The errdefs package is used both to generate errors, but also has utilities
to convert errors originating from subsystems into a suitable HTTP error.

The API should always return a suitable HTTP error, and Conversion of errors
originating from subsystems will (generally) only be needed in the API server
(not the client).

This patch prevents the client from depending on grpc, containerd and distribution,
by moving the code to convert those errors to a separate "adapters" package.

The api/server/httputils.GetHTTPErrorStatusCode was un-deprecated, and modified
to include the default set of adapters.

- Description for the changelog

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

thaJeztah · 2021-07-12T11:54:21Z

errdefs/http_helpers.go


-	containerderrors "github.com/containerd/containerd/errdefs"
-	"github.com/docker/distribution/registry/api/errcode"
 	"github.com/sirupsen/logrus"


We should get rid of logrus here

thaJeztah · 2021-07-12T11:55:02Z

@dims @cpuguy83 not sure if this is the most suitable approach; let me know what you think

dims · 2021-07-12T11:58:54Z

@thaJeztah as long as errdefs are not transitively needed from moby/moby/client, we are ok for now. this looks like too much of a change for sure. Let's hold this until we really need this, i'll POC a cadvisor change once #42623 lands and will let you know.

dims · 2021-07-12T12:42:58Z

@thaJeztah bad news ...

go: finding module for package github.com/containerd/containerd/errdefs
github.com/google/cadvisor/container/docker imports
	github.com/docker/docker/client imports
	github.com/docker/docker/errdefs imports
	github.com/containerd/containerd/errdefs

dims · 2021-07-12T12:57:46Z

@thaJeztah one more problem after applying the commit in this PR

github.com/google/cadvisor/container/docker imports
	github.com/docker/docker/client tested by
	github.com/docker/docker/client.test imports
	github.com/docker/docker/api/server/httputils imports
	github.com/docker/docker/errdefs/adapter imports
	github.com/containerd/containerd/errdefs

dims · 2021-07-12T16:09:35Z

@thaJeztah the new commit works well! ( 464f248 ) thanks. i don't see this problem any more

cpuguy83

Having anything other than the base definitions in errdefs seems wrong.

I don't remember the change that moved stuff around, but none of this seems correct and a generic Adapter seems like too much abstraction.

In client/ we should only be outputting for the most part, errors that implement something in errdefs and we should generally only be consuming errors that come across the API.

In api/ we should only be receiving errors that implement something from errdefs and outputing the relavent status code + messages.

thaJeztah · 2021-07-13T19:38:46Z

I don't remember the change that moved stuff around, but none of this seems correct and a generic Adapter seems like too much abstraction.

I think that may have been my doing. We had code to convert from/to errdefs and to/from HTTP status code in various locations, and (from that perspective) it seemed to make sense to have the errdefs package be the canonical place to provide conversions in both directions.

In client/ we should only be outputting for the most part, errors that implement something in errdefs and we should generally only be consuming errors that come across the API.

So currently the client uses errdefs to convert HTTP status codes returned by the API back to errdefs errors (added in #38689). I think that makes sense (I recall string matching in various code using the client because it returned no information whatsoever).

In api/ we should only be receiving errors that implement something from errdefs and outputing the relavent status code + messages.

So conversion from other subsystems was added over time;

gRPC errors conversion related to SwarmKit; choose rpc code to determine status code #32122
conversion for "distribution", to prevent it returning a 500 error Fix error 500 on distribution endpoint #37534
same for "containerd" errors returning a 500 Fix error handling of incorrect --platform values #39527

For those above, perhaps they should live in a client specific to each (e.g. libcontainerd/client for containerd ?) and convert to our errdef errors?

fredericdalleau · 2021-08-27T16:21:11Z

api/server/router/container/container_routes.go

 		conn, _, errHijack := hijacker.Hijack()
 		if errHijack == nil {
-			statusCode := errdefs.GetHTTPErrorStatusCode(err)
+			statusCode := errdefs.GetHTTPErrorStatusCode(err, adapter.All()...)


Is there any reason why the adapters are not hidden inside GetHTTPErrorStatusCode? other than that LGTM

dims · 2021-10-23T11:35:15Z

@thaJeztah let's please reach consensus on this one (with a hopefully quick tag/release!). this is holding up a long-dependency-update-chain in kubernetes.

allows cadvisor to reduce footprint of docker/docker in its deps.
which then allows us to update containerd + containerd-api (from 1.6 branch) + moby/moby + cadvisor into kubernetes/kubernetes.

The errdefs package is used both to generate errors, but also has utilities to convert errors originating from subsystems into a suitable HTTP error. The API should always return a suitable HTTP error, and Conversion of errors originating from subsystems will (generally) only be needed in the API server (not the client). This patch prevents the client from depending on grpc, containerd and distribution, by moving the code to convert those errors to a separate "adapters" package. The api/server/httputils.GetHTTPErrorStatusCode was un-deprecated, and modified to include the default set of adapters. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Unfortunately, the previous change didn't produce the desired result, because the client uses httputils in a test: github.com/google/cadvisor/container/docker imports github.com/docker/docker/client tested by github.com/docker/docker/client.test imports github.com/docker/docker/api/server/httputils imports github.com/docker/docker/errdefs/adapter imports github.com/containerd/containerd/errdefs Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

thaJeztah · 2022-03-21T11:20:16Z

I had a draft alternative to this one, which does not include the Adapter interface, and just moves everything together into a package that's only used by the API server; I just pushed it as an alternative PR; #43399 - hopefully that's able to unblock this.

thaJeztah · 2022-03-22T10:06:20Z

Replaced by #43399, which was just merged 👍

dims · 2022-03-22T11:45:06Z

thanks a ton @thaJeztah

thaJeztah added area/api API status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jul 12, 2021

thaJeztah commented Jul 12, 2021

View reviewed changes

thaJeztah force-pushed the errdefs_reduce_client_deps branch from ac75bbd to 464f248 Compare July 12, 2021 15:16

cpuguy83 requested changes Jul 13, 2021

View reviewed changes

dims mentioned this pull request Jul 27, 2021

Depend only on containerd API and avoid vendoring unnecessary things from containerd google/cadvisor#2908

Merged

fredericdalleau reviewed Aug 27, 2021

View reviewed changes

thaJeztah force-pushed the errdefs_reduce_client_deps branch from 464f248 to 1efa7ca Compare March 6, 2022 09:50

thaJeztah added this to the 21.xx milestone Mar 6, 2022

thaJeztah force-pushed the errdefs_reduce_client_deps branch from 1efa7ca to 5cdb258 Compare March 6, 2022 17:06

thaJeztah added 2 commits March 20, 2022 19:48

thaJeztah force-pushed the errdefs_reduce_client_deps branch from 5cdb258 to c44b11e Compare March 20, 2022 18:49

thaJeztah mentioned this pull request Mar 21, 2022

errdefs: move GetHTTPErrorStatusCode to api/server/httpstatus #43399

Merged

thaJeztah closed this in #43399 Mar 22, 2022

thaJeztah deleted the errdefs_reduce_client_deps branch March 22, 2022 10:06

errdefs: remove dependency on grpc, containerd, and distribution #42624

errdefs: remove dependency on grpc, containerd, and distribution #42624

Uh oh!

Conversation

thaJeztah commented Jul 12, 2021

Uh oh!

thaJeztah Jul 12, 2021

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 12, 2021

Uh oh!

dims commented Jul 12, 2021

Uh oh!

dims commented Jul 12, 2021

Uh oh!

dims commented Jul 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dims commented Jul 12, 2021

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jul 13, 2021

Uh oh!

fredericdalleau Aug 27, 2021

Choose a reason for hiding this comment

Uh oh!

dims commented Oct 23, 2021

Uh oh!

thaJeztah commented Mar 21, 2022

Uh oh!

thaJeztah commented Mar 22, 2022

Uh oh!

dims commented Mar 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dims commented Jul 12, 2021 •

edited

Loading