Skip to content

Log unexpected responses#4523

Merged
estesp merged 1 commit intocontainerd:masterfrom
errordeveloper:master
Sep 3, 2020
Merged

Log unexpected responses#4523
estesp merged 1 commit intocontainerd:masterfrom
errordeveloper:master

Conversation

@errordeveloper
Copy link
Copy Markdown
Contributor

This accomplishes a few long-standing TODO items, but also helps users in showing exact registry error messages.

I was stuck for a while trying to debug docker/buildx#300 without this, not realising the responses had pretty clear error messages attached.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Sep 2, 2020

Build succeeded.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do we feel about the readall here for an unexpected response? This could be tricky as we could get a large response and OOM the daemon/machine.

I think for this to be secure, we need to have some type of upper bound on this.

Copy link
Copy Markdown
Contributor Author

@errordeveloper errordeveloper Sep 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure myself, this is just what I had, I was wondering if this could be conditioned on the log-level somehow, but even then it could be too expensive...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given this is just to log, seems 64kb is way more than enough? like this maybe: https://github.com/containerd/containerd/blob/master/remotes/docker/auth/fetch.go#L55

@errordeveloper
Copy link
Copy Markdown
Contributor Author

@estesp @crosbymichael I figured reusing newUnexpectedStatusErr would be a good idea, so I refactored things a little - PTAL :)

@errordeveloper errordeveloper force-pushed the master branch 2 times, most recently from c455e6b to 68aa7f6 Compare September 3, 2020 13:50
This accomplishes a few long-standing TODO items, but also helps users
in showing exact registry error messages

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Sep 3, 2020

Build succeeded.

Copy link
Copy Markdown
Member

@crosbymichael crosbymichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@errordeveloper
Copy link
Copy Markdown
Contributor Author

Thanks @crosbymichael! Looks like CI is failing, am I right that it's unrelated to the change?

https://github.com/containerd/containerd/pull/4523/checks?check_run_id=1067134149

Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp merged commit a5c6381 into containerd:master Sep 3, 2020
@errordeveloper
Copy link
Copy Markdown
Contributor Author

thanks a lot @estesp!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants