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.

Sign up for GitHub

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

Jump to bottom

fix(edge): parse the response body with failed request [EE-7244] #661

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

fix(edge): parse the response body with failed request [EE-7244] #661

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
04af5ac
fix(edge): parse response body with failed request
oscarzhou-portainer Jun 13, 2024
ef4569b
fix(edge): add parsed response body error log
oscarzhou-portainer Jun 14, 2024
6ac038f
fix(edge): update error log for async polling
oscarzhou-portainer Jun 17, 2024
385914a
fix(edge): update error package
oscarzhou-portainer Jun 17, 2024
a9c97e8
chore: update portainer package
oscarzhou-portainer Jun 17, 2024
46c81c1
chore: fix lint error
oscarzhou-portainer Jun 17, 2024
9426755
fix(edge/poll): update logging format
oscarzhou-portainer Jul 8, 2024
55b9929
fix(edge): improve the log struct
oscarzhou-portainer Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 17 additions & 29 deletions edge/client/error_response.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,40 +2,28 @@ package client

import (
"encoding/json"
"errors"
"net/http"

httperror "github.com/portainer/portainer/pkg/libhttp/error"
"github.com/rs/zerolog/log"
)

type errorData struct {
Details string
Message string
}

func parseError(resp *http.Response) *errorData {
errorData := &errorData{}

err := json.NewDecoder(resp.Body).Decode(&errorData)
if err != nil {
log.Debug().CallerSkipFrame(1).
func logPollingError(resp *http.Response, ctxMsg, errMsg string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should mix logging with error handling.

var respErr httperror.HandlerError
if err := json.NewDecoder(resp.Body).Decode(&respErr); err != nil {
log.
Error().
Err(err).
Int("status_code", resp.StatusCode).
Msg("failed to decode error response")

return nil
Str("context", ctxMsg).
Int("response_code", resp.StatusCode).
Msg("PollClient failed to decode server response")
}

return errorData
}

func logError(resp *http.Response, errorData *errorData) {
if errorData == nil {
return
}

log.Debug().CallerSkipFrame(1).
Str("error_response_message", errorData.Message).
Str("error_response_details", errorData.Details).
Int("status_code", resp.StatusCode).
Msg("poll request failure")
log.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we use CallerSkipFrame() here as before?

Error().Err(respErr.Err).
Str("context", ctxMsg).
Str("response message", respErr.Message).
Int("status code", respErr.StatusCode).
Msg(errMsg)
return errors.New(errMsg)
}
9 changes: 1 addition & 8 deletions edge/client/portainer_edge_async_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,14 +415,7 @@ func (client *PortainerAsyncClient) executeAsyncRequest(payload AsyncRequest, po
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
errorData := parseError(resp)
logError(resp, errorData)

if errorData != nil {
return nil, errors.New(errorData.Message + ": " + errorData.Details)
}

return nil, errors.New("short poll request failed")
return nil, logPollingError(resp, "AsyncEdgeAgentExecuteAsyncRequest", fmt.Sprintf("AsyncEdgeAgent [%d] failed to execute async request", client.getEndpointIDFn()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please try to avoid using Sprintf() for this purpose, it's much better to use the structured logging functions that zlog provides, in this case something like .Int("endpoint_id", endpointID).

}

var asyncResponse AsyncResponse
Expand Down
39 changes: 8 additions & 31 deletions edge/client/portainer_edge_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,6 @@ type NonOkResponseError struct {
msg string
}

func newNonOkResponseError(msg string) *NonOkResponseError {
return &NonOkResponseError{msg: msg}
}

Comment on lines -54 to -57
Copy link
Contributor

Choose a reason for hiding this comment

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

You cannot remove this, we rely on that error type in other parts of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The type was not removed, only the constructor function was removed, because I found that it is not used.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is technically true but the constructor function is not used anymore because you removed it in this PR. 🤣

You can't do that, we need to return that error because we depend on it in other places, please see:

agent/edge/poll.go

Lines 217 to 225 in 676d62a

environmentStatus, err := service.portainerClient.GetEnvironmentStatus()
if err != nil {
var nonOkError *client.NonOkResponseError
if errors.As(err, &nonOkError) {
service.edgeManager.SetEndpointID(0)
}
return err
}

func (e *NonOkResponseError) Error() string {
return e.msg
}
Expand Down Expand Up @@ -121,9 +117,7 @@ func (client *PortainerEdgeClient) GetEnvironmentID() (portainer.EndpointID, err
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
log.Debug().Int("response_code", resp.StatusCode).Msg("global key request failure")

return 0, errors.New("global key request failed")
return 0, logPollingError(resp, "EdgeAgentGetEnvironmentID", fmt.Sprintf("EdgeAgent [%d] failed to request global key", client.getEndpointIDFn()))
}

var responseData globalKeyResponse
Expand Down Expand Up @@ -166,14 +160,7 @@ func (client *PortainerEdgeClient) GetEnvironmentStatus(flags ...string) (*PollS
}

if resp.StatusCode != http.StatusOK {
errorData := parseError(resp)
logError(resp, errorData)

if errorData != nil {
return nil, newNonOkResponseError(errorData.Message + ": " + errorData.Details)
}

return nil, newNonOkResponseError("short poll request failed")
return nil, logPollingError(resp, "EdgeAgentGetEnvironmentStatus", fmt.Sprintf("EdgeAgent [%d] failed to request edge environment status", client.getEndpointIDFn()))
}

var responseData PollStatusResponse
Expand Down Expand Up @@ -208,9 +195,7 @@ func (client *PortainerEdgeClient) GetEdgeStackConfig(edgeStackID int, version *
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
log.Error().Int("response_code", resp.StatusCode).Msg("GetEdgeStackConfig operation failed")

return nil, errors.New("GetEdgeStackConfig operation failed")
return nil, logPollingError(resp, "EdgeAgentGetEdgeStackConfig", fmt.Sprintf("EdgeAgent [%d] failed to request edge stack config", client.getEndpointIDFn()))
}

var data edge.StackPayload
Expand Down Expand Up @@ -265,9 +250,7 @@ func (client *PortainerEdgeClient) SetEdgeStackStatus(
resp.Body.Close()

if resp.StatusCode != http.StatusOK {
log.Error().Int("response_code", resp.StatusCode).Msg("SetEdgeStackStatus operation failed")

return errors.New("SetEdgeStackStatus operation failed")
return logPollingError(resp, "EdgeAgentSetEdgeStackStatus", fmt.Sprintf("EdgeAgent [%d] failed to set edge stack status", client.getEndpointIDFn()))
}

return nil
Expand Down Expand Up @@ -302,9 +285,7 @@ func (client *PortainerEdgeClient) SetEdgeJobStatus(edgeJobStatus agent.EdgeJobS
resp.Body.Close()

if resp.StatusCode != http.StatusOK {
log.Error().Int("response_code", resp.StatusCode).Msg("SetEdgeJobStatus operation failed")

return errors.New("SetEdgeJobStatus operation failed")
return logPollingError(resp, "EdgeAgentSetEdgeJobStatus", fmt.Sprintf("EdgeAgent [%d] failed to set edge job status", client.getEndpointIDFn()))
}

return nil
Expand All @@ -326,13 +307,11 @@ func (client *PortainerEdgeClient) GetEdgeConfig(id EdgeConfigID) (*EdgeConfig,
}

if resp.StatusCode != http.StatusOK {
log.Error().Int("response_code", resp.StatusCode).Msg("GetEdgeConfig operation failed")

if resp.StatusCode == http.StatusForbidden {
return nil, errors.New("GetEdgeConfig operation forbidden")
return nil, logPollingError(resp, "EdgeAgentGetEdgeConfig", fmt.Sprintf("EdgeAgent [%d] is forbidden to get the info of edge config [%d]", client.getEndpointIDFn(), id))
}

return nil, errors.New("GetEdgeConfig operation failed")
return nil, logPollingError(resp, "EdgeAgentGetEdgeConfig", fmt.Sprintf("EdgeAgent [%d] failed to get the info of edge config [%d]", client.getEndpointIDFn(), id))
}

var data EdgeConfig
Expand Down Expand Up @@ -362,9 +341,7 @@ func (client *PortainerEdgeClient) SetEdgeConfigState(id EdgeConfigID, state Edg
resp.Body.Close()

if resp.StatusCode != http.StatusOK {
log.Error().Int("edge_config_id", int(id)).Stringer("state", state).Int("response_code", resp.StatusCode).Msg("SetEdgeConfigState operation failed")

return errors.New("SetEdgeConfigState operation failed")
return logPollingError(resp, "EdgeAgentSetEdgeConfigState", fmt.Sprintf("EdgeAgent [%d] failed to set the state [%s] to edge config [%d]", client.getEndpointIDFn(), state, id))
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ require (
github.com/mitchellh/mapstructure v1.5.0
github.com/opencontainers/image-spec v1.1.0-rc5
github.com/pkg/errors v0.9.1
github.com/portainer/portainer v0.6.1-0.20240430014408-a9ead542b3aa
github.com/portainer/portainer v0.6.1-0.20240616212454-be9d3285e178
github.com/rs/zerolog v1.29.0
github.com/stretchr/testify v1.9.0
github.com/wI2L/jsondiff v0.2.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,8 +281,8 @@ github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM=
github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4=
github.com/portainer/portainer v0.6.1-0.20240430014408-a9ead542b3aa h1:gfKGsoMclwChTqxX4eCfIFo7b4ncHUdHdY7yQwi7HO8=
github.com/portainer/portainer v0.6.1-0.20240430014408-a9ead542b3aa/go.mod h1:AeF9ey0EZ44IK7+kuwCFwcmfY12PfCwmJs57r9STj6s=
github.com/portainer/portainer v0.6.1-0.20240616212454-be9d3285e178 h1:U/Wg0rjTySv2Sa2u8TVPXNyLE+9Ypx/fGRJSiqGrNac=
github.com/portainer/portainer v0.6.1-0.20240616212454-be9d3285e178/go.mod h1:AeF9ey0EZ44IK7+kuwCFwcmfY12PfCwmJs57r9STj6s=
github.com/posener/complete v1.1.1/go.mod h1:em0nMJCgc9GFtwrmVmEMR/ZL6WyhyjMBndrE9hABlRI=
github.com/prometheus/client_golang v0.9.2/go.mod h1:OsXs2jCmiKlQ1lTBmv21f2mNfw4xf/QclQDMrYNZzcM=
github.com/prometheus/client_model v0.0.0-20180712105110-5c3871d89910/go.mod h1:MbSGuTsp3dbXC40dX6PRTWyKYBIrTGTE9sqQNg2J8bo=
Expand Down
Loading