feat(buckets): Introduce maxWait timeout to generalized RetrySettings

RetrySettings are generalized and further described, as they also apply to Create's
wait for buckets to become active now.

A generalized maxWaitDuration is introduced for controlling when retries time out
if they take too long.

Update's retry is modified to honor the context being canceled directly - previously
an API call would be attempted and fail if the context times out.

Author: UnseenWizzard

api/clients/buckets/bucket.go

@@ -55,8 +55,9 @@ type listResponse struct {
 }
 
 type retrySettings struct {
-	maxRetries   int
-	waitDuration time.Duration
+	maxRetries           int
+	durationBetweenTries time.Duration
+	maxWaitDuration      time.Duration
 }
 
 type Client struct {
@@ -67,12 +68,20 @@ type Client struct {
 // Option represents a functional Option for the Client.
 type Option func(*Client)
 
-// WithUpdateRetrySettings sets the maximum number of retries as well as duration between retries for Update and Upsert HTTP requests
-func WithUpdateRetrySettings(maxRetries int, waitDuration time.Duration) Option {
+// WithRetrySettings sets the maximum number of retries as well as duration between retries.
+// These settings are honored wherever retries are used in the Client - most notably in Client.Update and Client.Upsert,
+// as well as Client.Create when waiting for a bucket to become available after creation.
+//
+// Parameters:
+//   - maxRetries: maximum amount actions may be retries. (Some actions may ignore this and only honor maxWaitDuration)
+//   - durationBetweenTries: time.Duration to wait between tries.
+//   - maxWaitDuration: maximum time.Duration to wait before retrying is cancelled. If you supply a context.Context with a timeout, the shorter of the two will be honored.
+func WithRetrySettings(maxRetries int, durationBetweenTries time.Duration, maxWaitDuration time.Duration) Option {
 	return func(c *Client) {
 		c.retrySettings = retrySettings{
-			maxRetries:   maxRetries,
-			waitDuration: waitDuration,
+			maxRetries:           maxRetries,
+			durationBetweenTries: durationBetweenTries,
+			maxWaitDuration:      maxWaitDuration,
 		}
 	}
 }
@@ -91,8 +100,9 @@ func NewClient(client *rest.Client, option ...Option) *Client {
 	c := &Client{
 		client: client,
 		retrySettings: retrySettings{
-			maxRetries:   5,
-			waitDuration: time.Second,
+			maxRetries:           5,
+			durationBetweenTries: time.Second,
+			maxWaitDuration:      2 * time.Minute,
 		},
 	}
 
@@ -232,27 +242,34 @@ func (c Client) Update(ctx context.Context, bucketName string, data []byte) (Res
 	}
 
 	// attempt update
+	ctx, cancel := context.WithTimeout(ctx, c.retrySettings.maxWaitDuration)
+	defer cancel()
 	for i := 0; i < c.retrySettings.maxRetries; i++ {
-		logger.V(1).Info(fmt.Sprintf("Trying to update bucket with bucket name %q (%d/%d retries)", bucketName, i+1, c.retrySettings.maxRetries))
+		select {
+		case <-ctx.Done():
+			return Response{}, fmt.Errorf("context cancelled before bucket with bucktName %q became available", bucketName)
+		default:
+			logger.V(1).Info(fmt.Sprintf("Trying to update bucket with bucket name %q (%d/%d retries)", bucketName, i+1, c.retrySettings.maxRetries))
 
-		resp, err = c.getAndUpdate(ctx, bucketName, data)
-		if err != nil {
-			return Response{}, err
-		}
+			resp, err = c.getAndUpdate(ctx, bucketName, data)
+			if err != nil {
+				return Response{}, err
+			}
 
-		if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusBadRequest {
-			return Response{api.Response{
-				StatusCode: resp.StatusCode,
-				Data:       resp.Payload,
-				Request:    resp.RequestInfo,
-			}}, nil
-		}
+			if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusBadRequest {
+				return Response{api.Response{
+					StatusCode: resp.StatusCode,
+					Data:       resp.Payload,
+					Request:    resp.RequestInfo,
+				}}, nil
+			}
 
-		if resp.IsSuccess() {
-			logger.Info(fmt.Sprintf("Updated bucket with bucket name %q", bucketName))
-			return Response{api.Response{StatusCode: resp.StatusCode, Data: resp.Payload, Request: resp.RequestInfo}}, nil
+			if resp.IsSuccess() {
+				logger.Info(fmt.Sprintf("Updated bucket with bucket name %q", bucketName))
+				return Response{api.Response{StatusCode: resp.StatusCode, Data: resp.Payload, Request: resp.RequestInfo}}, nil
+			}
+			time.Sleep(c.retrySettings.durationBetweenTries)
 		}
-		time.Sleep(c.retrySettings.waitDuration)
 	}
 	return Response{
 		Response: api.Response{
@@ -358,7 +375,7 @@ func (c Client) create(ctx context.Context, bucketName string, data []byte) (res
 		return r, nil
 	}
 
-	timoutCtx, cancel := context.WithTimeout(ctx, 1*time.Minute)
+	timoutCtx, cancel := context.WithTimeout(ctx, c.retrySettings.maxWaitDuration)
 	defer cancel() // cancel deadline if awaitBucketReady returns before deadline
 	return c.awaitBucketReady(timoutCtx, bucketName)
 }
@@ -393,7 +410,7 @@ func (c Client) awaitBucketReady(ctx context.Context, bucketName string) (rest.R
 			}
 
 			logger.V(1).Info("Waiting for bucket to become active after creation...")
-			time.Sleep(c.retrySettings.waitDuration)
+			time.Sleep(c.retrySettings.durationBetweenTries)
 		}
 	}
 }

api/clients/buckets/bucket_test.go

@@ -15,6 +15,7 @@
 package buckets_test
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"github.com/dynatrace/dynatrace-configuration-as-code-core/api"
@@ -26,7 +27,9 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
+	"time"
 )
 
 func TestGet(t *testing.T) {
@@ -401,7 +404,7 @@ func TestUpsert(t *testing.T) {
 
 		client := buckets.NewClient(
 			rest.NewClient(server.URL(), server.Client()),
-			buckets.WithUpdateRetrySettings(5, 0))
+			buckets.WithRetrySettings(5, 0, time.Minute))
 		data := []byte("{}")
 
 		ctx := testutils.ContextWithLogger(t)
@@ -427,7 +430,7 @@ func TestUpsert(t *testing.T) {
 
 		client := buckets.NewClient(
 			rest.NewClient(server.URL(), server.Client()),
-			buckets.WithUpdateRetrySettings(5, 0))
+			buckets.WithRetrySettings(5, 0, time.Minute))
 		data := []byte("{}")
 
 		ctx := testutils.ContextWithLogger(t)
@@ -819,7 +822,7 @@ func TestUpdate(t *testing.T) {
 
 		client := buckets.NewClient(
 			rest.NewClient(server.URL(), server.Client()),
-			buckets.WithUpdateRetrySettings(5, 0))
+			buckets.WithRetrySettings(5, 0, time.Minute))
 		data := []byte("{}")
 
 		ctx := testutils.ContextWithLogger(t)
@@ -853,7 +856,7 @@ func TestUpdate(t *testing.T) {
 
 	})
 
-	t.Run("Update fails with conflict only once", func(t *testing.T) {
+	t.Run("Update fails at first, but succeeds after retry", func(t *testing.T) {
 		var firstTry = true
 		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
 			switch req.Method {
@@ -892,6 +895,83 @@ func TestUpdate(t *testing.T) {
 
 		assert.Equal(t, "bucket name", m["bucketName"])
 	})
+
+	t.Run("Update honors context timeout", func(t *testing.T) {
+		var firstTry = true
+		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+			switch req.Method {
+			case http.MethodPost:
+				rw.WriteHeader(http.StatusForbidden)
+				rw.Write([]byte("no, this is an error"))
+			case http.MethodGet:
+				rw.Write([]byte(someBucketResponse))
+			case http.MethodPut:
+				if firstTry {
+					rw.WriteHeader(http.StatusConflict)
+					rw.Write([]byte("conflict"))
+					firstTry = false
+				} else {
+					rw.WriteHeader(http.StatusOK)
+					rw.Write([]byte(someBucketResponse))
+				}
+			default:
+				assert.Failf(t, "unexpected method %q", req.Method)
+			}
+		}))
+		defer server.Close()
+
+		u, _ := url.Parse(server.URL)
+		client := buckets.NewClient(rest.NewClient(u, &http.Client{}),
+			buckets.WithRetrySettings(5, 0, time.Minute)) // maxWaitDuration would allow 1 min
+		data := []byte("{}")
+
+		ctx := testutils.ContextWithLogger(t)
+		ctx, cancel := context.WithTimeout(ctx, 800*time.Microsecond) // context "should" time out after initial GET
+		defer cancel()
+		_, err := client.Update(ctx, "bucket name", data)
+		assert.Error(t, err)
+
+		// if GET happens to be cancelled already we'll get a 'deadline exceeded' from the http client. That's ok too, no need to fail the test.
+		if strings.Contains(err.Error(), "deadline exceeded") {
+			t.Log("context timed out before our logic and http request returned error")
+			return
+		}
+		assert.ErrorContains(t, err, "cancelled")
+	})
+
+	t.Run("Update honors retrySettings maxWaitDuration", func(t *testing.T) {
+		var firstTry = true
+		server := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, req *http.Request) {
+			switch req.Method {
+			case http.MethodPost:
+				rw.WriteHeader(http.StatusForbidden)
+				rw.Write([]byte("no, this is an error"))
+			case http.MethodGet:
+				rw.Write([]byte(someBucketResponse))
+			case http.MethodPut:
+				if firstTry {
+					rw.WriteHeader(http.StatusConflict)
+					rw.Write([]byte("conflict"))
+					firstTry = false
+				} else {
+					rw.WriteHeader(http.StatusOK)
+					rw.Write([]byte(someBucketResponse))
+				}
+			default:
+				assert.Failf(t, "unexpected method %q", req.Method)
+			}
+		}))
+		defer server.Close()
+
+		u, _ := url.Parse(server.URL)
+		client := buckets.NewClient(rest.NewClient(u, &http.Client{}),
+			buckets.WithRetrySettings(5, 0, 0)) // maxWaitDuration should time out immediatly
+		data := []byte("{}")
+
+		ctx := testutils.ContextWithLogger(t)
+		_, err := client.Update(ctx, "bucket name", data)
+		assert.ErrorContains(t, err, "cancelled")
+	})
 }
 
 func TestDecodingBucketResponses(t *testing.T) {