Repository: deislabs/ratify
Pull request #745 (2023-03-28 00:13:19)

refactor: switch retry client to native oras client
# Description

## What this PR does / why we need it:
As of ORAS 2.0, there is now a built in http retry client. This PR switches from Hashicorps' retryable http client to ORAS's built in one. This will set the foundation for defining a custom `Predicate` for metrics collection in a subsequent PR.

Also deletes the `http_retry_logger` adapter from `logr` to `logrus` since we no longer use the library.

## Type of change

Please delete options that are not relevant.

- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)
- [ ] Helm Chart Change (any edit/addition/update that is necessary for changes merged to the `main` branch)
- [ ] This change requires a documentation update

# How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Please also list any relevant details for your test configuration

- [ ] Test A
- [ ] Test B

# Checklist:

- [ ] Does the affected code have corresponding tests?
- [ ] Are the changes documented, not just with inline documentation, but also with conceptual documentation such as an overview of a new feature, or task-based documentation like a tutorial? Consider if this change should be announced on your project blog.
- [ ] Does this introduce breaking changes that would require an announcement or bumping the major version?
- [ ] Do all new files have appropriate license header?

# Post Merge Requirements
- [ ] MAINTAINERS: manually trigger the "Publish Package" workflow after merging any PR that indicates `Helm Chart Change`


codecov[bot]: ## [Codecov](https://codecov.io/gh/deislabs/ratify/pull/745?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
Patch coverage: **`40.00`**% and project coverage change: **`-0.17`** :warning:
> Comparison is base [(`198e453`)](https://codecov.io/gh/deislabs/ratify/commit/198e45355c755843920cefb3b7d286a394c00605?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 50.12% compared to head [(`3f85f16`)](https://codecov.io/gh/deislabs/ratify/pull/745?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) 49.95%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #745      +/-   ##
==========================================
- Coverage   50.12%   49.95%   -0.17%     
==========================================
  Files          57       56       -1     
  Lines        3274     3265       -9     
==========================================
- Hits         1641     1631      -10     
  Misses       1458     1458              
- Partials      175      176       +1     
```


| [Impacted Files](https://codecov.io/gh/deislabs/ratify/pull/745?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [pkg/referrerstore/oras/oras.go](https://codecov.io/gh/deislabs/ratify/pull/745?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cGtnL3JlZmVycmVyc3RvcmUvb3Jhcy9vcmFzLmdv) | `42.12% <40.00%> (-3.27%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

</details>

[:umbrella: View full report in Codecov by Sentry](https://codecov.io/gh/deislabs/ratify/pull/745?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).


Patch:
diff --git a/go.mod b/go.mod
index f41fc98..9fe75bd 100644
--- a/go.mod
+++ b/go.mod
@@ -22,7 +22,6 @@ require (
 	github.com/golang/protobuf v1.5.3
 	github.com/google/go-containerregistry v0.12.1
 	github.com/gorilla/mux v1.8.0
-	github.com/hashicorp/go-retryablehttp v0.7.2
 	github.com/notaryproject/notation-core-go v1.0.0-rc.2
 	github.com/notaryproject/notation-go v1.0.0-rc.3
 	github.com/open-policy-agent/frameworks/constraint v0.0.0-20220627162905-95c012350402
@@ -77,6 +76,7 @@ require (
 	github.com/google/go-github/v45 v45.2.0 // indirect
 	github.com/google/go-querystring v1.1.0 // indirect
 	github.com/hashicorp/go-cleanhttp v0.5.2 // indirect
+	github.com/hashicorp/go-retryablehttp v0.7.2 // indirect
 	github.com/mattn/go-isatty v0.0.17 // indirect
 	github.com/miekg/pkcs11 v1.1.1 // indirect
 	github.com/mozillazg/docker-credential-acr-helper v0.3.0 // indirect
diff --git a/pkg/referrerstore/oras/oras.go b/pkg/referrerstore/oras/oras.go
index af08385..905f77a 100644
--- a/pkg/referrerstore/oras/oras.go
+++ b/pkg/referrerstore/oras/oras.go
@@ -36,6 +36,7 @@ import (
 	"oras.land/oras-go/v2/registry/remote"
 	"oras.land/oras-go/v2/registry/remote/auth"
 	"oras.land/oras-go/v2/registry/remote/errcode"
+	"oras.land/oras-go/v2/registry/remote/retry"
 
 	ratifyconfig "github.com/deislabs/ratify/config"
 	"github.com/deislabs/ratify/pkg/common"
@@ -48,8 +49,6 @@ import (
 	"github.com/deislabs/ratify/pkg/referrerstore"
 	"github.com/deislabs/ratify/pkg/referrerstore/config"
 	"github.com/deislabs/ratify/pkg/referrerstore/factory"
-	"github.com/deislabs/ratify/pkg/referrerstore/utils"
-	retryablehttp "github.com/hashicorp/go-retryablehttp"
 	"github.com/opencontainers/go-digest"
 	"github.com/sirupsen/logrus"
 )
@@ -58,9 +57,9 @@ const (
 	HttpMaxIdleConns                      = 100
 	HttpMaxConnsPerHost                   = 100
 	HttpMaxIdleConnsPerHost               = 100
-	HttpRetryMax                          = 10
-	HttpRetryDurationMin    time.Duration = 100 * time.Millisecond
-	HttpRetryDurationMax    time.Duration = 200 * time.Millisecond
+	HttpRetryMax                          = 5
+	HttpRetryDurationMin    time.Duration = 200 * time.Millisecond
+	HttpRetryDurationMax    time.Duration = 1750 * time.Millisecond
 )
 
 const (
@@ -146,26 +145,25 @@ func createBaseStore(version string, storeConfig config.StorePluginConfig) (*ora
 		return nil, fmt.Errorf("could not create local oras cache at path %s: %w", conf.LocalCachePath, err)
 	}
 
+	customRetryPolicy := func() retry.Policy {
+		return &retry.GenericPolicy{
+			Retryable: retry.DefaultPredicate,
+			Backoff:   retry.DefaultBackoff,
+			MinWait:   HttpRetryDurationMin,
+			MaxWait:   HttpRetryDurationMax,
+			MaxRetry:  HttpRetryMax,
+		}
+	}
+
 	// define the http client for TLS enabled
-	secureRetryClient := retryablehttp.NewClient()
-	secureRetryClient.RetryMax = HttpRetryMax
-	secureRetryClient.Backoff = retryablehttp.LinearJitterBackoff
-	secureRetryClient.RetryWaitMin = HttpRetryDurationMin
-	secureRetryClient.RetryWaitMax = HttpRetryDurationMax
-	secureRetryClient.Logger = utils.HttpRetryLogger{}
 	secureTransport := http.DefaultTransport.(*http.Transport).Clone()
 	secureTransport.MaxIdleConns = HttpMaxIdleConns
 	secureTransport.MaxConnsPerHost = HttpMaxConnsPerHost
 	secureTransport.MaxIdleConnsPerHost = HttpMaxIdleConnsPerHost
-	secureRetryClient.HTTPClient.Transport = secureTransport
+	secureRetryTransport := retry.NewTransport(secureTransport)
+	secureRetryTransport.Policy = customRetryPolicy
 
 	// define the http client for TLS disabled
-	insecureRetryClient := retryablehttp.NewClient()
-	insecureRetryClient.RetryMax = HttpRetryMax
-	insecureRetryClient.Backoff = retryablehttp.LinearJitterBackoff
-	insecureRetryClient.RetryWaitMin = HttpRetryDurationMin
-	insecureRetryClient.RetryWaitMax = HttpRetryDurationMax
-	insecureRetryClient.Logger = utils.HttpRetryLogger{}
 	insecureTransport := http.DefaultTransport.(*http.Transport).Clone()
 	insecureTransport.MaxIdleConns = HttpMaxIdleConns
 	insecureTransport.MaxConnsPerHost = HttpMaxConnsPerHost
@@ -174,14 +172,15 @@ func createBaseStore(version string, storeConfig config.StorePluginConfig) (*ora
 	insecureTransport.TLSClientConfig = &tls.Config{
 		InsecureSkipVerify: true,
 	}
-	insecureRetryClient.HTTPClient.Transport = insecureTransport
+	insecureRetryTransport := retry.NewTransport(insecureTransport)
+	insecureRetryTransport.Policy = customRetryPolicy
 
 	return &orasStore{config: &conf,
 		rawConfig:          config.StoreConfig{Version: version, Store: storeConfig},
 		localCache:         localRegistry,
 		authProvider:       authenticationProvider,
-		httpClient:         secureRetryClient.StandardClient(),
-		httpClientInsecure: insecureRetryClient.StandardClient(),
+		httpClient:         &http.Client{Transport: secureRetryTransport},
+		httpClientInsecure: &http.Client{Transport: insecureRetryTransport},
 		createRepository:   createDefaultRepository}, nil
 }
 
diff --git a/pkg/referrerstore/utils/http_retry_logger.go b/pkg/referrerstore/utils/http_retry_logger.go
deleted file mode 100644
index 14f3021..0000000
--- a/pkg/referrerstore/utils/http_retry_logger.go
+++ /dev/null
@@ -1,42 +0,0 @@
-/*
-Copyright The Ratify Authors.
-Licensed under the Apache License, Version 2.0 (the "License");
-you may not use this file except in compliance with the License.
-You may obtain a copy of the License at
-
-http://www.apache.org/licenses/LICENSE-2.0
-
-Unless required by applicable law or agreed to in writing, software
-distributed under the License is distributed on an "AS IS" BASIS,
-WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
-See the License for the specific language governing permissions and
-limitations under the License.
-*/
-
-package utils
-
-import (
-	"github.com/hashicorp/go-retryablehttp"
-	"github.com/sirupsen/logrus"
-)
-
-// NOTE: This Logger is an adapter for logrus used by Retryable HTTP client in ORAS
-type HttpRetryLogger struct {
-	retryablehttp.LeveledLogger
-}
-
-func (l HttpRetryLogger) Error(msg string, keysAndValues ...interface{}) {
-	logrus.Errorln(append([]interface{}{msg}, keysAndValues...)...)
-}
-
-func (l HttpRetryLogger) Info(msg string, keysAndValues ...interface{}) {
-	logrus.Infoln(append([]interface{}{msg}, keysAndValues...)...)
-}
-
-func (l HttpRetryLogger) Debug(msg string, keysAndValues ...interface{}) {
-	logrus.Debugln(append([]interface{}{msg}, keysAndValues...)...)
-}
-
-func (l HttpRetryLogger) Warn(msg string, keysAndValues ...interface{}) {
-	logrus.Warnln(append([]interface{}{msg}, keysAndValues...)...)
-}


Test patch:
diff --git a/pkg/referrerstore/oras/oras_test.go b/pkg/referrerstore/oras/oras_test.go
index 7bdac5e..07f6d2d 100644
--- a/pkg/referrerstore/oras/oras_test.go
+++ b/pkg/referrerstore/oras/oras_test.go
@@ -19,8 +19,12 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"reflect"
 	"testing"
 	"time"
@@ -32,6 +36,7 @@ import (
 	"github.com/opencontainers/go-digest"
 	oci "github.com/opencontainers/image-spec/specs-go/v1"
 	"oras.land/oras-go/v2/registry"
+	"oras.land/oras-go/v2/registry/remote/errcode"
 )
 
 const inputOriginalPath = "localhost:5000/net-monitor:v0"
@@ -470,3 +475,52 @@ func TestORASGetBlobContent_NotCachedDesc(t *testing.T) {
 		t.Fatalf("expected content %s, got %s", expectedContent, content)
 	}
 }
+
+// Test_ORASRetryClient tests that the retry client retries on 429 for specified number of retries
+func Test_ORASRetryClient(t *testing.T) {
+	ctx := context.Background()
+	// Create a test server
+	count := 0
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		switch {
+		case r.Method == http.MethodHead && r.URL.Path == "/v2/test/manifests/latest":
+			w.WriteHeader(http.StatusTooManyRequests)
+			count++
+			return
+		default:
+			w.WriteHeader(http.StatusForbidden)
+		}
+		w.WriteHeader(http.StatusTooManyRequests)
+	}))
+	defer ts.Close()
+
+	conf := config.StorePluginConfig{
+		"name":    "oras",
+		"useHttp": true,
+	}
+	store, err := createBaseStore("1.0.0", conf)
+	if err != nil {
+		t.Fatalf("failed to create oras store: %v", err)
+	}
+	uri, err := url.Parse(ts.URL)
+	if err != nil {
+		t.Fatalf("invalid test http server: %v", err)
+	}
+	_, err = store.GetSubjectDescriptor(ctx, common.Reference{
+		Original: uri.Host + "/test:latest",
+		Tag:      "latest",
+		Path:     uri.Host + "/test",
+	})
+	if err != nil {
+		var ec errcode.Error
+		if errors.As(err, &ec) && (ec.Code != fmt.Sprint(http.StatusTooManyRequests)) {
+			t.Fatalf("expected error code %d, got %s", http.StatusTooManyRequests, ec.Code)
+		}
+	}
+
+	// Verify that the retry client was used by checking the number of retries
+	// The retry client will retry 5 times, so we expect 6 total requests
+	if count != 6 {
+		t.Fatalf("expected 6 retries, got %d", count)
+	}
+}