From 0a121ccfd7bc4da1cbea53ae293570f6e0511eb7 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Fri, 3 Feb 2023 01:46:04 +0100 Subject: [PATCH 1/4] RetrievePayload: improve testability and add tests --- pkg/cvo/updatepayload.go | 34 ++-- pkg/cvo/updatepayload_test.go | 346 ++++++++++++++++++++++++++++++++++ 2 files changed, 369 insertions(+), 11 deletions(-) create mode 100644 pkg/cvo/updatepayload_test.go diff --git a/pkg/cvo/updatepayload.go b/pkg/cvo/updatepayload.go index 186585665b..412269d7ab 100644 --- a/pkg/cvo/updatepayload.go +++ b/pkg/cvo/updatepayload.go @@ -38,14 +38,16 @@ func (optr *Operator) defaultPayloadDir() string { func (optr *Operator) defaultPayloadRetriever() PayloadRetriever { return &payloadRetriever{ - kubeClient: optr.kubeClient, - operatorName: optr.name, - releaseImage: optr.release.Image, - namespace: optr.namespace, - nodeName: optr.nodename, - payloadDir: optr.defaultPayloadDir(), - workingDir: targetUpdatePayloadsDir, - verifier: optr.verifier, + kubeClient: optr.kubeClient, + operatorName: optr.name, + releaseImage: optr.release.Image, + namespace: optr.namespace, + nodeName: optr.nodename, + payloadDir: optr.defaultPayloadDir(), + workingDir: targetUpdatePayloadsDir, + verifier: optr.verifier, + verifyTimeoutOnForce: 2 * time.Minute, + downloadTimeout: 2 * time.Minute, } } @@ -53,6 +55,8 @@ const ( targetUpdatePayloadsDir = "/etc/cvo/updatepayloads" ) +type downloadFunc func(context.Context, configv1.Update) (string, error) + type payloadRetriever struct { // releaseImage and payloadDir are the default payload identifiers - updates that point // to releaseImage will always use the contents of payloadDir @@ -67,7 +71,11 @@ type payloadRetriever struct { operatorName string // verifier guards against invalid remote data being accessed - verifier verify.Interface + verifier verify.Interface + verifyTimeoutOnForce time.Duration + + downloader downloadFunc + downloadTimeout time.Duration } func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1.Update) (PayloadInfo, error) { @@ -94,7 +102,7 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. // if 'force' specified, ensure call to verify payload signature times out well before parent context // to allow time to perform forced update if update.Force { - timeout := time.Minute * 2 + timeout := r.verifyTimeoutOnForce if deadline, deadlineSet := ctx.Deadline(); deadlineSet { timeout = time.Until(deadline) / 2 } @@ -120,9 +128,13 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. info.Verified = true } + if r.downloader == nil { + r.downloader = r.targetUpdatePayloadDir + } + // download the payload to the directory var err error - info.Directory, err = r.targetUpdatePayloadDir(ctx, update) + info.Directory, err = r.downloader(ctx, update) if err != nil { return PayloadInfo{}, &payload.UpdateError{ Reason: "UpdatePayloadRetrievalFailed", diff --git a/pkg/cvo/updatepayload_test.go b/pkg/cvo/updatepayload_test.go new file mode 100644 index 0000000000..437987a6aa --- /dev/null +++ b/pkg/cvo/updatepayload_test.go @@ -0,0 +1,346 @@ +package cvo + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" + configv1 "github.com/openshift/api/config/v1" + "github.com/openshift/library-go/pkg/verify/store" + "golang.org/x/crypto/openpgp" + + "github.com/openshift/cluster-version-operator/pkg/payload" +) + +// EquateErrorMessage reports errors to be equal if both are nil +// or both have the same message +var EquateErrorMessage = cmp.FilterValues(func(x, y interface{}) bool { + _, ok1 := x.(error) + _, ok2 := y.(error) + return ok1 && ok2 +}, cmp.Comparer(func(x, y interface{}) bool { + xe := x.(error) + ye := y.(error) + if xe == nil || ye == nil { + return xe == nil && ye == nil + } + return xe.Error() == ye.Error() +})) + +// mockVerifier implements verify.Verifier +type mockVerifier struct { + t *testing.T + + expectNoVerify bool + expectVerifyDigest string + expectVerifyCancel bool + verifyReturns error +} + +func (m *mockVerifier) Verify(ctx context.Context, releaseDigest string) error { + if m.expectNoVerify { + m.t.Errorf("Unexpected call: Verify(releaseDigest=%s)", releaseDigest) + } + if !m.expectNoVerify && m.expectVerifyDigest != releaseDigest { + m.t.Errorf("Verify() called with unexpected value: %v", releaseDigest) + } + + timeout, timeoutCancel := context.WithTimeout(context.Background(), backstopDuration) + defer timeoutCancel() + + if m.expectVerifyCancel { + select { + case <-ctx.Done(): + case <-timeout.Done(): + m.t.Errorf("Verify() expected to be cancelled by context but it was not") + } + } else { + select { + case <-ctx.Done(): + m.t.Errorf("Unexpected ctx cancel in Verify()") + default: + } + } + + return m.verifyReturns +} +func (m *mockVerifier) Signatures() map[string][][]byte { return nil } +func (m *mockVerifier) Verifiers() map[string]openpgp.EntityList { return nil } +func (m *mockVerifier) AddStore(_ store.Store) {} + +type downloadMocker struct { + expectCancel bool + duration time.Duration + returnLocation string + returnErr error +} + +const ( + // backstopDuration is a maximum duration for which we wait on a tested operation + backstopDuration = 5 * time.Second + // hangs represents a "hanging" operation, always preempted by backstop + hangs = 2 * backstopDuration +) + +func (d *downloadMocker) make(t *testing.T) downloadFunc { + if d == nil { + return func(_ context.Context, update configv1.Update) (string, error) { + t.Errorf("Unexpected call: downloader(, upddate=%v", update) + return "", nil + } + } + return func(ctx context.Context, _ configv1.Update) (string, error) { + backstopCtx, backstopCancel := context.WithTimeout(context.Background(), backstopDuration) + defer backstopCancel() + + downloadCtx, downloadCancel := context.WithTimeout(context.Background(), d.duration) + defer downloadCancel() + + if d.expectCancel { + select { + case <-backstopCtx.Done(): + t.Errorf("downloader: test backstop hit (expected cancel via ctx)") + return "", errors.New("downloader: test backstop hit (expected cancel via ctx)") + case <-downloadCtx.Done(): + t.Errorf("downloader: download finished (expected cancel via ctx)") + return "/some/location", errors.New("downloader: download finished (expected cancel via ctx)") + case <-ctx.Done(): + } + } else { + select { + case <-backstopCtx.Done(): + t.Errorf("downloader: test backstop hit (expected download to finish)") + return "", errors.New("downloader: test backstop hit (expected download to finish)") + case <-ctx.Done(): + t.Errorf("downloader: unexpected ctx cancel (expected download to finish)") + return "", errors.New("downloader: unexpected ctx cancel (expected download to finish)") + case <-downloadCtx.Done(): + } + } + + return d.returnLocation, d.returnErr + } +} + +func TestPayloadRetrieverRetrievePayload(t *testing.T) { + t.Parallel() + testCases := []struct { + name string + verifier *mockVerifier + downloader *downloadMocker + update configv1.Update + ctxTimeout time.Duration + + expected PayloadInfo + expectedErr error + }{ + { + name: "when desired image matches retriever image then return local payload directory", + verifier: &mockVerifier{expectNoVerify: true}, + update: configv1.Update{Image: "releaseImage"}, + expected: PayloadInfo{ + Directory: "/local/payload/dir", + Local: true, + }, + }, + { + name: "when desired image is empty then return error", + verifier: &mockVerifier{expectNoVerify: true}, + update: configv1.Update{}, + expectedErr: errors.New("no payload image has been specified and the contents of the payload cannot be retrieved"), + }, + { + name: "when desired image is tag pullspec and passes verification but fails to download then return error", + verifier: &mockVerifier{expectVerifyDigest: ""}, + downloader: &downloadMocker{returnErr: errors.New("fails to download")}, + update: configv1.Update{Image: "quay.io/openshift-release-dev/ocp-release:failing"}, + expectedErr: errors.New("Unable to download and prepare the update: fails to download"), + }, + { + name: "when desired image is tag pullspec and fails verification then return error", + verifier: &mockVerifier{ + expectVerifyDigest: "", + verifyReturns: errors.New("fails-verification"), + }, + update: configv1.Update{Image: "quay.io/openshift-release-dev/ocp-release:failing"}, + expectedErr: errors.New("The update cannot be verified: fails-verification"), + }, + { + name: "when desired image is sha digest pullspec and passes verification but fails to download then return error", + verifier: &mockVerifier{expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4"}, + downloader: &downloadMocker{returnErr: errors.New("fails to download")}, + update: configv1.Update{Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4"}, + expectedErr: errors.New("Unable to download and prepare the update: fails to download"), + }, + { + name: "when sha digest pullspec image fails verification but update is forced then retrieval proceeds then when download fails then return error", + verifier: &mockVerifier{ + expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + verifyReturns: errors.New("fails-to-verify"), + }, + downloader: &downloadMocker{returnErr: errors.New("fails to download")}, + update: configv1.Update{ + Force: true, + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + expectedErr: errors.New("Unable to download and prepare the update: fails to download"), + }, + { + name: "when sha digest pullspec image is timing out verification with unlimited context and update is forced " + + "then verification times out promptly and retrieval proceeds but download fails then return error", + verifier: &mockVerifier{ + expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + expectVerifyCancel: true, + verifyReturns: errors.New("fails-to-verify"), + }, + downloader: &downloadMocker{returnErr: errors.New("fails to download")}, + update: configv1.Update{ + Force: true, + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + expectedErr: errors.New("Unable to download and prepare the update: fails to download"), + }, + { + name: "when sha digest pullspec image is timing out verification with long deadline context and update is forced " + + "then verification times out promptly, retrieval proceeds but download fails then return error", + verifier: &mockVerifier{ + expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + expectVerifyCancel: true, + verifyReturns: errors.New("fails-to-verify"), + }, + downloader: &downloadMocker{returnErr: errors.New("fails to download")}, + update: configv1.Update{ + Force: true, + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + ctxTimeout: backstopDuration - time.Second, + expectedErr: errors.New("Unable to download and prepare the update: fails to download"), + }, + { + name: "when sha digest pullspec image is timing out verification with long deadline context and update is forced " + + "then verification times out promptly, retrieval proceeds but download times out then return error", + verifier: &mockVerifier{ + expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + expectVerifyCancel: true, + verifyReturns: errors.New("fails-to-verify"), + }, + downloader: &downloadMocker{ + expectCancel: true, + duration: backstopDuration - (2 * time.Second), + returnErr: errors.New("fails to download"), + }, + update: configv1.Update{ + Force: true, + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + ctxTimeout: backstopDuration - time.Second, + expectedErr: errors.New("Unable to download and prepare the update: fails to download"), + }, + { + name: "when sha digest pullspec image fails verification but update is forced then retrieval proceeds and download succeeds then return info with location and verification error", + verifier: &mockVerifier{ + expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + verifyReturns: errors.New("fails-to-verify"), + }, + downloader: &downloadMocker{returnLocation: "/location/of/download"}, + update: configv1.Update{ + Force: true, + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + expected: PayloadInfo{ + Directory: "/location/of/download", + VerificationError: &payload.UpdateError{ + Reason: "ImageVerificationFailed", + Message: `Target release version="" image="quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4" cannot be verified, but continuing anyway because the update was forced: fails-to-verify`, + }, + }, + }, + { + name: "when sha digest pullspec image passes and download hangs then it is terminated and returns error (RHBZ#2090680)", + verifier: &mockVerifier{expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4"}, + downloader: &downloadMocker{ + duration: hangs, + expectCancel: true, + returnErr: errors.New("download was canceled"), + }, + update: configv1.Update{ + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + expectedErr: errors.New("Unable to download and prepare the update: download was canceled"), + }, + { + name: "when sha digest pullspec image fails to verify until timeout then it allows enough time for download and it returns successfully", + verifier: &mockVerifier{ + expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + expectVerifyCancel: true, + verifyReturns: errors.New("fails-to-verify"), + }, + downloader: &downloadMocker{ + duration: 300 * time.Millisecond, + returnLocation: "/location/of/download", + }, + update: configv1.Update{ + Force: true, + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + expected: PayloadInfo{ + Directory: "/location/of/download", + VerificationError: &payload.UpdateError{ + Reason: "ImageVerificationFailed", + Message: `Target release version="" image="quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4" cannot be verified, but continuing anyway because the update was forced: fails-to-verify`, + }, + }, + }, + { + name: "when sha digest pullspec image passes and download succeeds then returns location and no error", + verifier: &mockVerifier{expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4"}, + downloader: &downloadMocker{returnLocation: "/location/of/download"}, + update: configv1.Update{ + Image: "quay.io/openshift-release-dev/ocp-release@sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", + }, + expected: PayloadInfo{ + Directory: "/location/of/download", + Verified: true, + }, + }, + } + for _, tc := range testCases { + tc := tc // prevent parallel closures from sharing a single tc copy + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + retriever := payloadRetriever{ + releaseImage: "releaseImage", + payloadDir: "/local/payload/dir", + verifyTimeoutOnForce: time.Second, + downloadTimeout: time.Second, + } + + if tc.verifier != nil { + tc.verifier.t = t + retriever.verifier = tc.verifier + } + + if tc.downloader != nil { + retriever.downloader = tc.downloader.make(t) + } + + ctx := context.Background() + if tc.ctxTimeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, tc.ctxTimeout) + defer cancel() + } + + actual, err := retriever.RetrievePayload(ctx, tc.update) + if diff := cmp.Diff(tc.expectedErr, err, EquateErrorMessage); diff != "" { + t.Errorf("Returned error differs from expected:\n%s", diff) + } + if diff := cmp.Diff(tc.expected, actual, cmpopts.IgnoreFields(payload.UpdateError{}, "Nested")); err == nil && diff != "" { + t.Errorf("Returned PayloadInfo differs from expected:\n%s", diff) + } + }) + } +} From da31bdf42d759a263d7c210471912c1a7b248ff7 Mon Sep 17 00:00:00 2001 From: Jack Ottofaro Date: Fri, 3 Feb 2023 02:13:13 +0100 Subject: [PATCH 2/4] Bug 2090680: pkg/cvo/updatepayload.go: timeout payload retrieval When we separated payload load from payload apply (#683) the context used for the retrieval changed as well. It went from one that was constrained by syncTimeout (2 -4 minutes) [1] to being the unconstrained shutdownContext [2]. However if "force" is specified we explicitly set a 2 minute timeout in RetrievePayload. This commit creates a new context with a reasonable timeout for RetrievePayload regardless of "force". [1] https://github.com/openshift/cluster-version-operator/blob/57ffa7c610fb92ef4ccd9e9c49e75915e86e9296/pkg/cvo/sync_worker.go#L605 [2] https://github.com/openshift/cluster-version-operator/blob/57ffa7c610fb92ef4ccd9e9c49e75915e86e9296/pkg/cvo/cvo.go#L413 --- pkg/cvo/updatepayload.go | 22 ++++++---------------- pkg/cvo/updatepayload_test.go | 2 +- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/pkg/cvo/updatepayload.go b/pkg/cvo/updatepayload.go index 412269d7ab..b1c41c7180 100644 --- a/pkg/cvo/updatepayload.go +++ b/pkg/cvo/updatepayload.go @@ -97,22 +97,12 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. if index := strings.LastIndex(update.Image, "@"); index != -1 { releaseDigest = update.Image[index+1:] } - verifyCtx := ctx - - // if 'force' specified, ensure call to verify payload signature times out well before parent context - // to allow time to perform forced update - if update.Force { - timeout := r.verifyTimeoutOnForce - if deadline, deadlineSet := ctx.Deadline(); deadlineSet { - timeout = time.Until(deadline) / 2 - } - klog.V(2).Infof("Forced update so reducing payload signature verification timeout to %s", timeout) - var cancel context.CancelFunc - verifyCtx, cancel = context.WithTimeout(ctx, timeout) - defer cancel() - } - if err := r.verifier.Verify(verifyCtx, releaseDigest); err != nil { + // set up a new context with reasonable timeout for signature and payload retrieval + retrieveCtx, cancel := context.WithTimeout(ctx, r.verifyTimeoutOnForce+r.downloadTimeout) + defer cancel() + + if err := r.verifier.Verify(retrieveCtx, releaseDigest); err != nil { vErr := &payload.UpdateError{ Reason: "ImageVerificationFailed", Message: fmt.Sprintf("The update cannot be verified: %v", err), @@ -134,7 +124,7 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. // download the payload to the directory var err error - info.Directory, err = r.downloader(ctx, update) + info.Directory, err = r.downloader(retrieveCtx, update) if err != nil { return PayloadInfo{}, &payload.UpdateError{ Reason: "UpdatePayloadRetrievalFailed", diff --git a/pkg/cvo/updatepayload_test.go b/pkg/cvo/updatepayload_test.go index 437987a6aa..daf3361caf 100644 --- a/pkg/cvo/updatepayload_test.go +++ b/pkg/cvo/updatepayload_test.go @@ -272,7 +272,7 @@ func TestPayloadRetrieverRetrievePayload(t *testing.T) { expectedErr: errors.New("Unable to download and prepare the update: download was canceled"), }, { - name: "when sha digest pullspec image fails to verify until timeout then it allows enough time for download and it returns successfully", + name: "when sha digest pullspec image fails to verify until timeout but is forced then it allows enough time for download and it returns successfully", verifier: &mockVerifier{ expectVerifyDigest: "sha256:08ef16270e643a001454410b22864db6246d782298be267688a4433d83f404f4", expectVerifyCancel: true, From d421ded9a173fe20d721e13f1cbe5067a3311d4e Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Fri, 3 Feb 2023 02:26:20 +0100 Subject: [PATCH 3/4] RetrievePayload: Improve timeouts for corner cases The `RetrievePayload` performs two operations: verification and download. Both can take a non-trivial amount of time to terminate, up to "hanging" where CVO needs to abort the operation. The verification result can be ignored when upgrade is forced. The CVO calls `RetrievePayload` with a context that does not set a deadline, so `RetrievePayload` previously set its own internal deadline, common for both operations. This led to a suboptimal behavior on forced upgrades, where "hanging" verification could eat the whole timeout budget, got cancelled but its result was ignored (because of force). The code tried to proceed with download but that immediately aborts because of the expired context. Improve timeouts in `RetrievePayload` for both input context states: with and without deadline. If the input context sets a deadline, it is respected. If it does not, the default, separate deadlines are applied for both operations. In both cases, the code makes sure the hanging verification never spends the whole budget. When verification terminates fast, the rest of its alloted time is provided to the download operation. --- pkg/cvo/updatepayload.go | 25 +++++++++++++++++-------- pkg/cvo/updatepayload_test.go | 2 ++ 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/pkg/cvo/updatepayload.go b/pkg/cvo/updatepayload.go index b1c41c7180..71a418511a 100644 --- a/pkg/cvo/updatepayload.go +++ b/pkg/cvo/updatepayload.go @@ -90,19 +90,29 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. return PayloadInfo{}, fmt.Errorf("no payload image has been specified and the contents of the payload cannot be retrieved") } - var info PayloadInfo - // verify the provided payload var releaseDigest string if index := strings.LastIndex(update.Image, "@"); index != -1 { releaseDigest = update.Image[index+1:] } - // set up a new context with reasonable timeout for signature and payload retrieval - retrieveCtx, cancel := context.WithTimeout(ctx, r.verifyTimeoutOnForce+r.downloadTimeout) - defer cancel() + var downloadCtx context.Context + var downloadCtxCancel context.CancelFunc + var verifyTimeout time.Duration - if err := r.verifier.Verify(retrieveCtx, releaseDigest); err != nil { + if deadline, ok := ctx.Deadline(); ok { + verifyTimeout = time.Until(deadline) / 2 + downloadCtx = ctx + } else { + verifyTimeout = r.verifyTimeoutOnForce + downloadCtx, downloadCtxCancel = context.WithTimeout(ctx, r.verifyTimeoutOnForce+r.downloadTimeout) + defer downloadCtxCancel() + } + verifyCtx, verifyCtxCancel := context.WithTimeout(ctx, verifyTimeout) + defer verifyCtxCancel() + + var info PayloadInfo + if err := r.verifier.Verify(verifyCtx, releaseDigest); err != nil { vErr := &payload.UpdateError{ Reason: "ImageVerificationFailed", Message: fmt.Sprintf("The update cannot be verified: %v", err), @@ -121,10 +131,9 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. if r.downloader == nil { r.downloader = r.targetUpdatePayloadDir } - // download the payload to the directory var err error - info.Directory, err = r.downloader(retrieveCtx, update) + info.Directory, err = r.downloader(downloadCtx, update) if err != nil { return PayloadInfo{}, &payload.UpdateError{ Reason: "UpdatePayloadRetrievalFailed", diff --git a/pkg/cvo/updatepayload_test.go b/pkg/cvo/updatepayload_test.go index daf3361caf..e20636d70d 100644 --- a/pkg/cvo/updatepayload_test.go +++ b/pkg/cvo/updatepayload_test.go @@ -10,6 +10,8 @@ import ( "github.com/google/go-cmp/cmp/cmpopts" configv1 "github.com/openshift/api/config/v1" "github.com/openshift/library-go/pkg/verify/store" + + //nolint:staticcheck // verify,Verifier from openshift/library-go uses a type from this deprecated package (needs to be addressed there) "golang.org/x/crypto/openpgp" "github.com/openshift/cluster-version-operator/pkg/payload" From ac127d3a5d45f60eb54e5c5acc3711a284728499 Mon Sep 17 00:00:00 2001 From: Petr Muller Date: Tue, 14 Feb 2023 18:35:45 +0100 Subject: [PATCH 4/4] RetrievePayload: improve godocs and refactor (address review) --- pkg/cvo/updatepayload.go | 55 ++++++++++++++++++++++------------- pkg/cvo/updatepayload_test.go | 7 ++--- 2 files changed, 37 insertions(+), 25 deletions(-) diff --git a/pkg/cvo/updatepayload.go b/pkg/cvo/updatepayload.go index 71a418511a..e08ad72c70 100644 --- a/pkg/cvo/updatepayload.go +++ b/pkg/cvo/updatepayload.go @@ -38,16 +38,15 @@ func (optr *Operator) defaultPayloadDir() string { func (optr *Operator) defaultPayloadRetriever() PayloadRetriever { return &payloadRetriever{ - kubeClient: optr.kubeClient, - operatorName: optr.name, - releaseImage: optr.release.Image, - namespace: optr.namespace, - nodeName: optr.nodename, - payloadDir: optr.defaultPayloadDir(), - workingDir: targetUpdatePayloadsDir, - verifier: optr.verifier, - verifyTimeoutOnForce: 2 * time.Minute, - downloadTimeout: 2 * time.Minute, + kubeClient: optr.kubeClient, + operatorName: optr.name, + releaseImage: optr.release.Image, + namespace: optr.namespace, + nodeName: optr.nodename, + payloadDir: optr.defaultPayloadDir(), + workingDir: targetUpdatePayloadsDir, + verifier: optr.verifier, + retrieveTimeout: 4 * time.Minute, } } @@ -55,6 +54,10 @@ const ( targetUpdatePayloadsDir = "/etc/cvo/updatepayloads" ) +// downloadFunc downloads the requested update and returns either a path on the local filesystem +// containing extracted manifests or an error +// The type exists so that tests for payloadRetriever.RetrievePayload can mock this functionality +// by setting payloadRetriever.downloader. type downloadFunc func(context.Context, configv1.Update) (string, error) type payloadRetriever struct { @@ -71,13 +74,23 @@ type payloadRetriever struct { operatorName string // verifier guards against invalid remote data being accessed - verifier verify.Interface - verifyTimeoutOnForce time.Duration + verifier verify.Interface - downloader downloadFunc - downloadTimeout time.Duration + // downloader is called to download the requested update to local filesystem. It should only be + // set to non-nil value in tests. When this is nil, payloadRetriever.targetUpdatePayloadDir + // is called as a downloader. + downloader downloadFunc + + // retrieveTimeout limits the time spent in payloadRetriever.RetrievePayload. This timeout is only + // applied when the context passed into that method is unbounded; otherwise the method respects + // the deadline set in the input context. + retrieveTimeout time.Duration } +// RetrievePayload verifies, downloads and extracts to local filesystem the payload image specified +// by update. If the input context has a deadline, it is respected, otherwise r.retrieveTimeout +// applies. When update.Force is true, the verification is still performed, but the method proceeds +// even when the image cannot be verified successfully. func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1.Update) (PayloadInfo, error) { if r.releaseImage == update.Image { return PayloadInfo{ @@ -97,17 +110,17 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. } var downloadCtx context.Context - var downloadCtxCancel context.CancelFunc - var verifyTimeout time.Duration - - if deadline, ok := ctx.Deadline(); ok { - verifyTimeout = time.Until(deadline) / 2 + downloadDeadline, ok := ctx.Deadline() + if ok { downloadCtx = ctx } else { - verifyTimeout = r.verifyTimeoutOnForce - downloadCtx, downloadCtxCancel = context.WithTimeout(ctx, r.verifyTimeoutOnForce+r.downloadTimeout) + downloadDeadline = time.Now().Add(r.retrieveTimeout) + var downloadCtxCancel context.CancelFunc + downloadCtx, downloadCtxCancel = context.WithDeadline(ctx, downloadDeadline) defer downloadCtxCancel() } + + verifyTimeout := time.Until(downloadDeadline) / 2 verifyCtx, verifyCtxCancel := context.WithTimeout(ctx, verifyTimeout) defer verifyCtxCancel() diff --git a/pkg/cvo/updatepayload_test.go b/pkg/cvo/updatepayload_test.go index e20636d70d..a93b463a7d 100644 --- a/pkg/cvo/updatepayload_test.go +++ b/pkg/cvo/updatepayload_test.go @@ -314,10 +314,9 @@ func TestPayloadRetrieverRetrievePayload(t *testing.T) { t.Run(tc.name, func(t *testing.T) { t.Parallel() retriever := payloadRetriever{ - releaseImage: "releaseImage", - payloadDir: "/local/payload/dir", - verifyTimeoutOnForce: time.Second, - downloadTimeout: time.Second, + releaseImage: "releaseImage", + payloadDir: "/local/payload/dir", + retrieveTimeout: 2 * time.Second, } if tc.verifier != nil {