diff --git a/changes/25759-illegal-argument-errors b/changes/25759-illegal-argument-errors new file mode 100644 index 0000000000..f9b6ca55ed --- /dev/null +++ b/changes/25759-illegal-argument-errors @@ -0,0 +1 @@ +Illegal argument errors will no longer be logged at the ERROR level on the server. Since these are client errors, they will be logged at the DEBUG level instead. This will reduce the amount of noise in the server logs and help debugging other issues. diff --git a/server/contexts/logging/logging.go b/server/contexts/logging/logging.go index 056f6a81ad..846e7d23a6 100644 --- a/server/contexts/logging/logging.go +++ b/server/contexts/logging/logging.go @@ -53,14 +53,6 @@ func WithNoUser(ctx context.Context) context.Context { return ctx } -// WithNoError returns a context with logging.SkipError set to true so error won't be logged at level=error -func WithNoError(ctx context.Context) context.Context { - if logCtx, ok := FromContext(ctx); ok { - logCtx.SetSkipError() - } - return ctx -} - // WithExtras returns a context with logging.Extras set as the values provided func WithExtras(ctx context.Context, extras ...interface{}) context.Context { if logCtx, ok := FromContext(ctx); ok { @@ -87,7 +79,6 @@ type LoggingContext struct { Extras []interface{} SkipUser bool ForceLevel func(kitlog.Logger) kitlog.Logger - SkipError bool } func (l *LoggingContext) SetForceLevel(level func(kitlog.Logger) kitlog.Logger) { @@ -108,12 +99,6 @@ func (l *LoggingContext) SetSkipUser() { l.SkipUser = true } -func (l *LoggingContext) SetSkipError() { - l.l.Lock() - defer l.l.Unlock() - l.SkipError = true -} - func (l *LoggingContext) SetStartTime() { l.l.Lock() defer l.l.Unlock() @@ -132,7 +117,7 @@ func (l *LoggingContext) Log(ctx context.Context, logger kitlog.Logger) { defer l.l.Unlock() switch { - case len(l.Errs) > 0 && !l.SkipError: + case l.setLevelError(): logger = level.Error(logger) case l.ForceLevel != nil: logger = l.ForceLevel(logger) @@ -208,3 +193,18 @@ func (l *LoggingContext) Log(ctx context.Context, logger kitlog.Logger) { _ = logger.Log(keyvals...) } + +func (l *LoggingContext) setLevelError() bool { + if len(l.Errs) == 0 { + return false + } + + if len(l.Errs) == 1 { + var ew fleet.ErrWithIsClientError + if errors.As(l.Errs[0], &ew) && ew.IsClientError() { + return false + } + } + + return true +} diff --git a/server/docs/patterns.md b/server/docs/patterns.md index ef71aca09f..0ac33d423f 100644 --- a/server/docs/patterns.md +++ b/server/docs/patterns.md @@ -18,6 +18,8 @@ Validate API inputs and return a 4XX status code if invalid. If you did not do a Inputs corresponding to sortable or indexed DB fields should be preprocessed (trim spaces, normalize Unicode, etc.). Use utility method `fleet.Preprocess(input string) string`. [Backend sync where discussed](https://us-65885.app.gong.io/call?id=4055688254267958899). +Invalid inputs should NOT log a server error. Server errors should be reserved for unexpected/serious issues. [`InvalidArgumentError` implements `IsClientError`](https://github.com/fleetdm/fleet/blob/529f4ed725117d99d668318aad23c9e1575fa7ee/server/fleet/errors.go#L134) method to indicate that it is a client error. [Backend sync where discussed](https://us-65885.app.gong.io/call?id=6515110653090875786&highlights=%5B%7B%22type%22%3A%22SHARE%22%2C%22from%22%3A340%2C%22to%22%3A1578%7D%5D). + ### JSON unmarshaling `PATCH` API calls often need to distinguish between a field being set to `null` and a field not being present in the JSON. Use the structs from `optjson` package to handle this. [Backend sync where discussed](https://us-65885.app.gong.io/call?id=4055688254267958899). [JSON unmarshaling article and example](https://victoronsoftware.com/posts/go-json-unmarshal/). diff --git a/server/fleet/errors.go b/server/fleet/errors.go index 77098617bc..8dc616bdb5 100644 --- a/server/fleet/errors.go +++ b/server/fleet/errors.go @@ -63,6 +63,14 @@ type ErrWithRetryAfter interface { RetryAfter() int } +// ErrWithIsClientError is an interface for errors that explicitly specify +// whether they are client errors or not. By default, errors are treated as +// server errors. +type ErrWithIsClientError interface { + error + IsClientError() bool +} + type invalidArgWithStatusError struct { InvalidArgumentError code int @@ -102,7 +110,7 @@ func (e *ErrorWithUUID) UUID() string { } // InvalidArgumentError is the error returned when invalid data is presented to -// a service method. +// a service method. It is a client error. type InvalidArgumentError struct { Errors []InvalidArgument @@ -123,6 +131,10 @@ func NewInvalidArgumentError(name, reason string) *InvalidArgumentError { return &invalid } +func (e InvalidArgumentError) IsClientError() bool { + return true +} + func (e *InvalidArgumentError) Append(name, reason string) { e.Errors = append(e.Errors, InvalidArgument{ name: name, diff --git a/server/service/apple_mdm.go b/server/service/apple_mdm.go index 0d2965c997..3b277fcf98 100644 --- a/server/service/apple_mdm.go +++ b/server/service/apple_mdm.go @@ -2378,8 +2378,6 @@ func bootstrapPackageMetadataEndpoint(ctx context.Context, request interface{}, meta, err := svc.GetMDMAppleBootstrapPackageMetadata(ctx, req.TeamID, req.ForUpdate) switch { case fleet.IsNotFound(err): - // Don't log this response as error -- it's expected to happen when the bootstrap package is missing, which is a common case. - logging.WithNoError(ctx) return bootstrapPackageMetadataResponse{Err: fleet.NewInvalidArgumentError("team_id", "bootstrap package for this team does not exist").WithStatus(http.StatusNotFound)}, nil case err != nil: