From c891b95d7ab656f03e70a469259f685d75bab769 Mon Sep 17 00:00:00 2001 From: Jordan Barrett Date: Tue, 15 Aug 2023 15:33:11 +0700 Subject: [PATCH] add clienterr package - add tests for loki client errors --- internals/overlord/logstate/clienterr/doc.go | 20 +++++ internals/overlord/logstate/clienterr/err.go | 37 +++++++++ internals/overlord/logstate/gatherer.go | 2 +- .../overlord/logstate/loki/export_test.go | 25 ++++++ internals/overlord/logstate/loki/loki.go | 21 +++-- internals/overlord/logstate/loki/loki_test.go | 81 +++++++++++++++++-- 6 files changed, 172 insertions(+), 14 deletions(-) create mode 100644 internals/overlord/logstate/clienterr/doc.go create mode 100644 internals/overlord/logstate/clienterr/err.go create mode 100644 internals/overlord/logstate/loki/export_test.go diff --git a/internals/overlord/logstate/clienterr/doc.go b/internals/overlord/logstate/clienterr/doc.go new file mode 100644 index 000000000..26524acf4 --- /dev/null +++ b/internals/overlord/logstate/clienterr/doc.go @@ -0,0 +1,20 @@ +// Copyright (c) 2023 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +/* +Package clienterr contains common error types which are recognised by the log +gatherer. logstate.logClient implementations should return these error types +to communicate with the gatherer. +*/ +package clienterr diff --git a/internals/overlord/logstate/clienterr/err.go b/internals/overlord/logstate/clienterr/err.go new file mode 100644 index 000000000..7159b8fba --- /dev/null +++ b/internals/overlord/logstate/clienterr/err.go @@ -0,0 +1,37 @@ +// Copyright (c) 2023 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package clienterr + +import ( + "fmt" + "time" +) + +// Backoff should be returned if the server indicates we are sending too many +// requests (e.g. an HTTP 429 response). +type Backoff struct { + RetryAfter time.Time +} + +func (b Backoff) Error() string { + return fmt.Sprintf("too many requests, retry after %v", b.RetryAfter) +} + +// Timeout should be returned if the client's Flush times out. +type Timeout struct{} + +func (Timeout) Error() string { + return "client flush timed out" +} diff --git a/internals/overlord/logstate/gatherer.go b/internals/overlord/logstate/gatherer.go index cde2bf1d0..15bdd9ca3 100644 --- a/internals/overlord/logstate/gatherer.go +++ b/internals/overlord/logstate/gatherer.go @@ -19,10 +19,10 @@ import ( "fmt" "time" - "github.com/canonical/pebble/internals/overlord/logstate/loki" "gopkg.in/tomb.v2" "github.com/canonical/pebble/internals/logger" + "github.com/canonical/pebble/internals/overlord/logstate/loki" "github.com/canonical/pebble/internals/plan" "github.com/canonical/pebble/internals/servicelog" ) diff --git a/internals/overlord/logstate/loki/export_test.go b/internals/overlord/logstate/loki/export_test.go new file mode 100644 index 000000000..f1616b2f4 --- /dev/null +++ b/internals/overlord/logstate/loki/export_test.go @@ -0,0 +1,25 @@ +// Copyright (c) 2023 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package loki + +import "time" + +func SetRequestTimeout(new time.Duration) (restore func()) { + oldRequestTimeout := requestTimeout + requestTimeout = new + return func() { + requestTimeout = oldRequestTimeout + } +} diff --git a/internals/overlord/logstate/loki/loki.go b/internals/overlord/logstate/loki/loki.go index 9f1b9b36c..55474480c 100644 --- a/internals/overlord/logstate/loki/loki.go +++ b/internals/overlord/logstate/loki/loki.go @@ -30,10 +30,9 @@ import ( "github.com/canonical/pebble/internals/servicelog" ) -const ( - maxRequestSize = 10 - requestTimeout = 10 * time.Second -) +const maxRequestSize = 10 + +var requestTimeout = 10 * time.Second type Client struct { target *plan.LogTarget @@ -90,8 +89,18 @@ func (c *Client) Flush(ctx context.Context) error { resp, err := c.httpClient.Do(httpReq) if err != nil { - return err + return handleHTTPClientErr(err) } + return handleServerResponse(resp) +} + +func handleHTTPClientErr(err error) error { + // req timeout -> timeout + // context cancelled -> timeout + return err +} + +func handleServerResponse(resp *http.Response) error { defer func() { // Drain request body to allow connection reuse // see https://pkg.go.dev/net/http#Response.Body @@ -100,6 +109,8 @@ func (c *Client) Flush(ctx context.Context) error { }() // Check response status code to see if it was successful + // TODO: handle 429 + // TODO: rebase, add logic to gatherer to recognise clienterrs if code := resp.StatusCode; code < 200 || code >= 300 { // Request to Loki failed b, err := io.ReadAll(io.LimitReader(resp.Body, 1024)) diff --git a/internals/overlord/logstate/loki/loki_test.go b/internals/overlord/logstate/loki/loki_test.go index eaef8eeec..cf1d53a25 100644 --- a/internals/overlord/logstate/loki/loki_test.go +++ b/internals/overlord/logstate/loki/loki_test.go @@ -1,24 +1,46 @@ -package loki +// Copyright (c) 2023 Canonical Ltd +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License version 3 as +// published by the Free Software Foundation. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with this program. If not, see . + +package loki_test import ( "context" "encoding/json" + "errors" "io" "net/http" "net/http/httptest" + "testing" "time" . "gopkg.in/check.v1" + "github.com/canonical/pebble/internals/overlord/logstate/clienterr" + "github.com/canonical/pebble/internals/overlord/logstate/loki" "github.com/canonical/pebble/internals/plan" "github.com/canonical/pebble/internals/servicelog" ) -type lokiSuite struct{} +type suite struct{} -var _ = Suite(&lokiSuite{}) +var _ = Suite(&suite{}) -func (*lokiSuite) TestLokiRequest(c *C) { +func Test(t *testing.T) { + TestingT(t) +} + +func (*suite) TestRequest(c *C) { input := []servicelog.Entry{{ Time: time.Date(2023, 12, 31, 12, 34, 50, 0, time.UTC), Service: "svc1", @@ -113,7 +135,7 @@ func (*lokiSuite) TestLokiRequest(c *C) { })) defer server.Close() - client := NewClient(&plan.LogTarget{Location: server.URL}) + client := loki.NewClient(&plan.LogTarget{Location: server.URL}) for _, entry := range input { err := client.Write(context.Background(), entry) c.Assert(err, IsNil) @@ -123,7 +145,7 @@ func (*lokiSuite) TestLokiRequest(c *C) { c.Assert(err, IsNil) } -func (*lokiSuite) TestLokiFlushCancelContext(c *C) { +func (*suite) TestFlushCancelContext(c *C) { serverCtx, killServer := context.WithCancel(context.Background()) server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { select { @@ -135,7 +157,7 @@ func (*lokiSuite) TestLokiFlushCancelContext(c *C) { defer server.Close() defer killServer() - client := NewClient(&plan.LogTarget{Location: server.URL}) + client := loki.NewClient(&plan.LogTarget{Location: server.URL}) err := client.Write(context.Background(), servicelog.Entry{ Time: time.Now(), Service: "svc1", @@ -150,7 +172,7 @@ func (*lokiSuite) TestLokiFlushCancelContext(c *C) { defer cancel() err := client.Flush(ctx) - c.Assert(err, ErrorMatches, ".*context deadline exceeded") + c.Assert(errors.Is(err, clienterr.Timeout{}), Equals, true) close(flushReturned) }() @@ -162,6 +184,49 @@ func (*lokiSuite) TestLokiFlushCancelContext(c *C) { } } +func (*suite) TestServerTimeout(c *C) { + restore := loki.SetRequestTimeout(1 * time.Microsecond) + defer restore() + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + time.Sleep(2 * time.Microsecond) + })) + defer server.Close() + + client := loki.NewClient(&plan.LogTarget{Location: server.URL}) + err := client.Write(context.Background(), servicelog.Entry{ + Time: time.Now(), + Service: "svc1", + Message: "this is a log line\n", + }) + c.Assert(err, IsNil) + + err = client.Flush(context.Background()) + c.Assert(errors.Is(err, clienterr.Timeout{}), Equals, true) +} + +func (*suite) TestTooManyRequests(c *C) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusTooManyRequests) + w.Header().Add("Retry-After", "Tue, 15 Aug 2023 08:49:37 GMT") + })) + defer server.Close() + + client := loki.NewClient(&plan.LogTarget{Location: server.URL}) + err := client.Write(context.Background(), servicelog.Entry{ + Time: time.Now(), + Service: "svc1", + Message: "this is a log line\n", + }) + c.Assert(err, IsNil) + + expectedErr := clienterr.Backoff{ + RetryAfter: time.Date(2023, 8, 15, 8, 49, 37, 0, time.UTC), + } + err = client.Flush(context.Background()) + c.Assert(errors.Is(err, expectedErr), Equals, true) +} + // Strips all extraneous whitespace from JSON func flattenJSON(s string) (string, error) { var v any