Merge pull request #900 from fluxcd/fix-timeout

Fix timeout created more than once

Author: matheuscscp

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)
 	}

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: &registry.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()

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...)

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

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.