Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add http response to log #709

Merged
merged 1 commit into from
Sep 27, 2019
Merged

add http response to log #709

merged 1 commit into from
Sep 27, 2019

Conversation

QiWang19
Copy link
Collaborator

fix containers/podman#3884
Add http response message log to show server-side error message.

Signed-off-by: Qi Wang [email protected]

Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

Ideally a usable error message should be returned to the caller, instead of the single ErrUnauthorizedForCredentials constant.

OTOH that would be an ABI break…

docker/docker_client.go Outdated Show resolved Hide resolved
docker/docker_client.go Outdated Show resolved Hide resolved
@QiWang19 QiWang19 force-pushed the resplog branch 4 times, most recently from dfffa35 to cefef03 Compare September 26, 2019 19:57
docker/docker_client.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM pending tests.

Thanks!

docker/docker_client.go Outdated Show resolved Hide resolved
fix containers/podman#3884
Add http response message log to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
@mtrmac mtrmac merged commit 0fcdae1 into containers:master Sep 27, 2019
@@ -533,6 +533,8 @@ func (c *dockerClient) getBearerToken(ctx context.Context, challenge challenge,
defer res.Body.Close()
switch res.StatusCode {
case http.StatusUnauthorized:
err := client.HandleErrorResponse(res)
logrus.Debugf("Server response when trying to obtain an access token: \n%q", err.Error())
return nil, ErrUnauthorizedForCredentials
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

what does break API mean?

return nil, errors.Wrapf(err, "Server response when trying to obtain an access token")

Copy link
Member

Choose a reason for hiding this comment

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

In current version Buildah and Podman can just look at the return code and compare it to ErrUnauthorizedForCredentials
If we wrap the error with the message, then podman and buildah will no longer get ErrUnauthorizedForCredentials back, they would have to take the error returned and change it to
if errors.Cause(err) == ErrUnauthorizedForCredentials

Any time the users of a library have to change their code, then you have broken ABI, if the callers do not have to change their code, then ABI remained the same.

https://en.wikipedia.org/wiki/Application_binary_interface

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, Thanks, PTAL #719

QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 1, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 1, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 1, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 2, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 2, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 3, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 3, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 4, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 22, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 22, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
QiWang19 added a commit to QiWang19/image that referenced this pull request Oct 24, 2019
follow containers#709
return and wrap http response message to show server-side error message.

Signed-off-by: Qi Wang <[email protected]>
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.

login doesn't display server-side error message
3 participants