From 775f90d398ffb22429c7cab8c3a2a0f6ef799c57 Mon Sep 17 00:00:00 2001 From: Matheus Pimenta Date: Thu, 18 Jun 2026 13:31:40 +0100 Subject: [PATCH] Fix timeout created more than once Signed-off-by: Matheus Pimenta (cherry picked from commit 1879dcbd75e948ff3c2135942ba05cc32fa1c120) --- internal/controller/imagepolicy_controller.go | 9 ++- .../controller/imagepolicy_controller_test.go | 38 +++++++++++++ .../controller/imagerepository_controller.go | 10 ++-- .../imagerepository_controller_test.go | 56 +++++++++++++++++++ internal/registry/options.go | 4 -- 5 files changed, 108 insertions(+), 9 deletions(-) diff --git a/internal/controller/imagepolicy_controller.go b/internal/controller/imagepolicy_controller.go index b5fd9bd6..ec7cc49d 100644 --- a/internal/controller/imagepolicy_controller.go +++ b/internal/controller/imagepolicy_controller.go @@ -459,12 +459,19 @@ func (r *ImagePolicyReconciler) fetchDigest(ctx context.Context, Namespace: obj.GetNamespace(), Operation: cache.OperationReconcile, } + + // The timeout must span both building the auth options and the registry + // request, as the authenticator fetches registry credentials lazily during + // the request. + ctx, cancel := context.WithTimeout(ctx, repo.GetTimeout()) + defer cancel() + opts, err := r.AuthOptionsGetter.GetOptions(ctx, repo, involvedObject) if err != nil { return "", fmt.Errorf("failed to configure authentication options: %w", err) } - desc, err := remote.Head(tagRef, opts...) + desc, err := remote.Head(tagRef, append(opts, remote.WithContext(ctx))...) if err != nil { return "", fmt.Errorf("failed fetching descriptor for %q: %w", tagRef.String(), err) } diff --git a/internal/controller/imagepolicy_controller_test.go b/internal/controller/imagepolicy_controller_test.go index 73a338ae..57fce4fa 100644 --- a/internal/controller/imagepolicy_controller_test.go +++ b/internal/controller/imagepolicy_controller_test.go @@ -829,6 +829,44 @@ func TestImagePolicyReconciler_getImageRepository(t *testing.T) { } } +// TestImagePolicyReconciler_fetchDigest_respectsContext ensures fetchDigest +// propagates the context to the registry request. The timeout that wraps the +// digest fetch (and the lazily-fetched registry credentials) is derived from +// this context, so if it is not passed to the request the fetch would run +// unbounded on a background context. With a cancelled context, the fetch must +// fail rather than silently succeed. +func TestImagePolicyReconciler_fetchDigest_respectsContext(t *testing.T) { + g := NewWithT(t) + + registryServer := test.NewRegistryServer() + defer registryServer.Close() + + imgRepo, _, err := test.LoadImages(registryServer, "foo/bar", []string{"v1.0.0"}) + g.Expect(err).ToNot(HaveOccurred()) + + r := &ImagePolicyReconciler{ + EventRecorder: record.NewFakeRecorder(32), + AuthOptionsGetter: ®istry.AuthOptionsGetter{Client: fake.NewClientBuilder().Build()}, + } + + repo := &imagev1.ImageRepository{} + repo.Spec.Image = imgRepo + + obj := &imagev1.ImagePolicy{} + obj.Name = "test" + obj.Namespace = "default" + + // Cancel the context before fetching. fetchDigest must pass the context to + // the registry request, so the call fails instead of running unbounded on a + // background context. + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err = r.fetchDigest(ctx, repo, obj, "v1.0.0") + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("context canceled")) +} + func TestImagePolicyReconciler_digestReflection(t *testing.T) { registryServer := test.NewRegistryServer() defer registryServer.Close() diff --git a/internal/controller/imagerepository_controller.go b/internal/controller/imagerepository_controller.go index 5d4cc95f..7623415b 100644 --- a/internal/controller/imagerepository_controller.go +++ b/internal/controller/imagerepository_controller.go @@ -278,6 +278,12 @@ func (r *ImageRepositoryReconciler) reconcile(ctx context.Context, sp *patch.Ser Namespace: obj.GetNamespace(), Operation: cache.OperationReconcile, } + + // The timeout must span both building the auth options and the scan, as the + // authenticator fetches registry credentials lazily during the scan. + ctx, cancel := context.WithTimeout(ctx, obj.GetTimeout()) + defer cancel() + opts, err := r.AuthOptionsGetter.GetOptions(ctx, obj, involvedObject) if err != nil { e := fmt.Errorf("failed to configure authentication options: %w", err) @@ -418,10 +424,6 @@ func (r *ImageRepositoryReconciler) shouldScan(ctx context.Context, obj imagev1. // scan performs repository scanning and writes the scanned result in the // internal database and populates the status of the ImageRepository. func (r *ImageRepositoryReconciler) scan(ctx context.Context, obj *imagev1.ImageRepository, ref name.Reference, options []remote.Option) error { - timeout := obj.GetTimeout() - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - options = append(options, remote.WithContext(ctx)) tags, err := remote.List(ref.Context(), options...) diff --git a/internal/controller/imagerepository_controller_test.go b/internal/controller/imagerepository_controller_test.go index 26e470bd..f0510bfc 100644 --- a/internal/controller/imagerepository_controller_test.go +++ b/internal/controller/imagerepository_controller_test.go @@ -419,6 +419,62 @@ func TestImageRepositoryReconciler_scan(t *testing.T) { } } +// deadlineCapturingRoundTripper records whether the request context carried a +// deadline, then delegates to the wrapped RoundTripper. +type deadlineCapturingRoundTripper struct { + rt http.RoundTripper + sawDeadl *bool +} + +func (d *deadlineCapturingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + if _, ok := req.Context().Deadline(); ok { + *d.sawDeadl = true + } + return d.rt.RoundTrip(req) +} + +// TestImageRepositoryReconciler_scan_noOwnTimeout ensures scan does not apply a +// timeout of its own. The timeout must be owned by the caller (reconcile) and +// span the whole operation, so that the lazily-fetched registry credentials are +// not cut short by a second, separate timeout. Given a context without a +// deadline, scan must not introduce one. +func TestImageRepositoryReconciler_scan_noOwnTimeout(t *testing.T) { + g := NewWithT(t) + + registryServer := test.NewRegistryServer() + defer registryServer.Close() + + imgRepo, _, err := test.LoadImages(registryServer, "test-scan-timeout-"+randStringRunes(5), []string{"a"}) + g.Expect(err).ToNot(HaveOccurred()) + + r := ImageRepositoryReconciler{ + EventRecorder: record.NewFakeRecorder(32), + Database: &mockDatabase{}, + patchOptions: getPatchOptions(imageRepositoryOwnedConditions, "irc"), + } + + repo := &imagev1.ImageRepository{} + repo.Spec = imagev1.ImageRepositorySpec{ + Image: imgRepo, + Timeout: &metav1.Duration{Duration: time.Hour}, + } + + ref, err := registry.ParseImageReference(imgRepo, false) + g.Expect(err).ToNot(HaveOccurred()) + + var sawDeadline bool + opts := []remote.Option{ + remote.WithTransport(&deadlineCapturingRoundTripper{rt: http.DefaultTransport, sawDeadl: &sawDeadline}), + } + + // Pass a context without a deadline. If scan adds its own timeout, the + // registry request context will carry a deadline. + err = r.scan(context.Background(), repo, ref, opts) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(sawDeadline).To(BeFalse(), + "scan must not apply its own timeout; the timeout is owned by reconcile and spans the whole scan") +} + func TestSortTagsAndGetLatestTags(t *testing.T) { tests := []struct { name string diff --git a/internal/registry/options.go b/internal/registry/options.go index fe9f7441..d115a641 100644 --- a/internal/registry/options.go +++ b/internal/registry/options.go @@ -53,10 +53,6 @@ type AuthOptionsGetter struct { func (r *AuthOptionsGetter) GetOptions(ctx context.Context, repo *imagev1.ImageRepository, involvedObject *cache.InvolvedObject) ([]remote.Option, error) { - timeout := repo.GetTimeout() - ctx, cancel := context.WithTimeout(ctx, timeout) - defer cancel() - var transportOptions []func(*http.Transport) // Load proxy configuration.