[image-builder] Use req.GetUseRetryClient() instead of feature flag resolved on startup

Author: geropl

components/image-builder-mk3/cmd/run.go

@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"github.com/gitpod-io/gitpod/common-go/baseserver"
-	"github.com/gitpod-io/gitpod/common-go/experiments"
 
 	"github.com/gitpod-io/gitpod/common-go/log"
 	"github.com/gitpod-io/gitpod/image-builder/api"
@@ -41,12 +40,7 @@ var runCmd = &cobra.Command{
 		span, ctx := opentracing.StartSpanFromContext(ctx, "/cmd/Run")
 		defer span.Finish()
 
-		// Check feature flag for retry client
-		experimentsClient := experiments.NewClient()
-		useRetryClient := experimentsClient.GetBoolValue(ctx, "imagebuilder_retry_resolve", false, experiments.Attributes{})
-		log.WithField("useRetryClient", useRetryClient).Info("Feature flag imagebuilder_retry_resolve checked")
-
-		service, err := orchestrator.NewOrchestratingBuilder(cfg.Orchestrator, useRetryClient)
+		service, err := orchestrator.NewOrchestratingBuilder(cfg.Orchestrator)
 		if err != nil {
 			log.Fatal(err)
 		}

components/image-builder-mk3/pkg/orchestrator/orchestrator.go

@@ -11,6 +11,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net/http"
 	"os"
 	"path/filepath"
 	"sort"
@@ -56,7 +57,7 @@ const (
 )
 
 // NewOrchestratingBuilder creates a new orchestrating image builder
-func NewOrchestratingBuilder(cfg config.Configuration, useRetryClient bool) (res *Orchestrator, err error) {
+func NewOrchestratingBuilder(cfg config.Configuration) (res *Orchestrator, err error) {
 	var authentication auth.CompositeAuth
 	if cfg.PullSecretFile != "" {
 		fn := cfg.PullSecretFile
@@ -106,17 +107,7 @@ func NewOrchestratingBuilder(cfg config.Configuration, useRetryClient bool) (res
 		wsman = wsmanapi.NewWorkspaceManagerClient(conn)
 	}
 
-	// Create RefResolver with conditional retry client based on feature flag
-	var refResolver resolve.DockerRefResolver
-	if useRetryClient {
-		refResolver = &resolve.StandaloneRefResolver{
-			Client: NewRetryTimeoutClient(),
-		}
-		log.Info("Using retry timeout client for Docker registry requests")
-	} else {
-		refResolver = &resolve.StandaloneRefResolver{}
-		log.Info("Using standard client for Docker registry requests")
-	}
+	retryResolveClient := NewRetryTimeoutClient()
 
 	o := &Orchestrator{
 		Config: cfg,
@@ -125,7 +116,9 @@ func NewOrchestratingBuilder(cfg config.Configuration, useRetryClient bool) (res
 			BaseImageRepository:      cfg.BaseImageRepository,
 			WorkspaceImageRepository: cfg.WorkspaceImageRepository,
 		},
-		RefResolver: refResolver,
+		RefResolver: &resolve.StandaloneRefResolver{},
+
+		retryResolveClient: retryResolveClient,
 
 		wsman:         wsman,
 		buildListener: make(map[string]map[buildListener]struct{}),
@@ -145,6 +138,8 @@ type Orchestrator struct {
 	AuthResolver auth.Resolver
 	RefResolver  resolve.DockerRefResolver
 
+	retryResolveClient *http.Client
+
 	wsman wsmanapi.WorkspaceManagerClient
 
 	buildListener map[string]map[buildListener]struct{}
@@ -173,7 +168,7 @@ func (o *Orchestrator) ResolveBaseImage(ctx context.Context, req *protocol.Resol
 
 	reqauth := o.AuthResolver.ResolveRequestAuth(ctx, req.Auth)
 
-	refstr, err := o.getAbsoluteImageRef(ctx, req.Ref, reqauth)
+	refstr, err := o.getAbsoluteImageRef(ctx, req.Ref, reqauth, req.GetUseRetryClient())
 	if err != nil {
 		return nil, err
 	}
@@ -190,7 +185,8 @@ func (o *Orchestrator) ResolveWorkspaceImage(ctx context.Context, req *protocol.
 	tracing.LogRequestSafe(span, req)
 
 	reqauth := o.AuthResolver.ResolveRequestAuth(ctx, req.Auth)
-	baseref, err := o.getBaseImageRef(ctx, req.Source, reqauth)
+	useRetryClient := req.GetUseRetryClient()
+	baseref, err := o.getBaseImageRef(ctx, req.Source, reqauth, useRetryClient)
 	if _, ok := status.FromError(err); err != nil && ok {
 		return nil, err
 	}
@@ -209,7 +205,7 @@ func (o *Orchestrator) ResolveWorkspaceImage(ctx context.Context, req *protocol.
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "cannot get workspace image authentication: %v", err)
 	}
-	exists, err := o.checkImageExists(ctx, refstr, auth)
+	exists, err := o.checkImageExists(ctx, refstr, auth, useRetryClient)
 	if err != nil {
 		return nil, status.Errorf(codes.Internal, "cannot resolve workspace image: %s", err.Error())
 	}
@@ -240,8 +236,8 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 
 	// resolve build request authentication
 	reqauth := o.AuthResolver.ResolveRequestAuth(ctx, req.Auth)
-
-	log.WithField("forceRebuild", req.GetForceRebuild()).WithField("baseImageNameResolved", req.BaseImageNameResolved).Info("build request")
+	useRetryClient := req.GetUseRetryClient()
+	log.WithField("forceRebuild", req.GetForceRebuild()).WithField("baseImageNameResolved", req.BaseImageNameResolved).WithField("useRetryClient", useRetryClient).Info("build request")
 
 	// resolve to ref to baseImageNameResolved (if it exists)
 	if req.BaseImageNameResolved != "" && !req.GetForceRebuild() {
@@ -261,7 +257,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 		}
 
 		// check if needs build -> early return
-		exists, err := o.checkImageExists(ctx, wsrefstr, wsrefAuth)
+		exists, err := o.checkImageExists(ctx, wsrefstr, wsrefAuth, useRetryClient)
 		if err != nil {
 			return status.Errorf(codes.Internal, "cannot check if image is already built: %q", err)
 		}
@@ -276,7 +272,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 			}
 			return nil
 		}
-		baseref, err := o.getAbsoluteImageRef(ctx, req.BaseImageNameResolved, reqauth)
+		baseref, err := o.getAbsoluteImageRef(ctx, req.BaseImageNameResolved, reqauth, useRetryClient)
 		if err == nil {
 			req.Source.From = &protocol.BuildSource_Ref{
 				Ref: &protocol.BuildSourceReference{
@@ -287,7 +283,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 	}
 
 	log.Info("falling through to old way of building")
-	baseref, err := o.getBaseImageRef(ctx, req.Source, reqauth)
+	baseref, err := o.getBaseImageRef(ctx, req.Source, reqauth, useRetryClient)
 	if _, ok := status.FromError(err); err != nil && ok {
 		log.WithError(err).Error("gRPC status error")
 		return err
@@ -307,7 +303,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 	}
 
 	// check if needs build -> early return
-	exists, err := o.checkImageExists(ctx, wsrefstr, wsrefAuth)
+	exists, err := o.checkImageExists(ctx, wsrefstr, wsrefAuth, req.GetUseRetryClient())
 	if err != nil {
 		return status.Errorf(codes.Internal, "cannot check if image is already built: %q", err)
 	}
@@ -476,7 +472,7 @@ func (o *Orchestrator) Build(req *protocol.BuildRequest, resp protocol.ImageBuil
 		// "cannot pull from reg.gitpod.io" error message. Instead the image-build should fail properly.
 		// To do this, we resolve the built image afterwards to ensure it was actually built.
 		if update.Status == protocol.BuildStatus_done_success {
-			exists, err := o.checkImageExists(ctx, wsrefstr, wsrefAuth)
+			exists, err := o.checkImageExists(ctx, wsrefstr, wsrefAuth, useRetryClient)
 			if err != nil {
 				update.Status = protocol.BuildStatus_done_failure
 				update.Message = fmt.Sprintf("cannot check if workspace image exists after the build: %v", err)
@@ -593,12 +589,12 @@ func (o *Orchestrator) ListBuilds(ctx context.Context, req *protocol.ListBuildsR
 	return &protocol.ListBuildsResponse{Builds: res}, nil
 }
 
-func (o *Orchestrator) checkImageExists(ctx context.Context, ref string, authentication *auth.Authentication) (exists bool, err error) {
+func (o *Orchestrator) checkImageExists(ctx context.Context, ref string, authentication *auth.Authentication, useRetryClient bool) (exists bool, err error) {
 	span, ctx := opentracing.StartSpanFromContext(ctx, "checkImageExists")
 	defer tracing.FinishSpan(span, &err)
 	span.SetTag("ref", ref)
 
-	_, err = o.RefResolver.Resolve(ctx, ref, resolve.WithAuthentication(authentication))
+	_, err = o.RefResolver.Resolve(ctx, ref, resolve.WithAuthentication(authentication), o.withRetryIfEnabled(useRetryClient))
 	if errors.Is(err, resolve.ErrNotFound) {
 		return false, nil
 	}
@@ -613,18 +609,19 @@ func (o *Orchestrator) checkImageExists(ctx context.Context, ref string, authent
 }
 
 // getAbsoluteImageRef returns the "digest" form of an image, i.e. contains no mutable image tags
-func (o *Orchestrator) getAbsoluteImageRef(ctx context.Context, ref string, allowedAuth auth.AllowedAuthFor) (res string, err error) {
-	span, ctx := opentracing.StartSpanFromContext(ctx, "getAbsoluteImageRef")
+func (o *Orchestrator) getAbsoluteImageRef(ctx context.Context, ref string, allowedAuth auth.AllowedAuthFor, useRetryClient bool) (res string, err error) {
+	span, ctx := opentracing.StartSpanFromContext(ctx, "getAbsoluteImageRefWithResolver")
 	defer tracing.FinishSpan(span, &err)
 	span.LogKV("ref", ref)
+	span.LogKV("useRetryClient", useRetryClient)
 
-	log.WithField("ref", ref).Debug("getAbsoluteImageRef")
+	log.WithField("ref", ref).WithField("useRetryClient", useRetryClient).Debug("getAbsoluteImageRefWithResolver")
 	auth, err := allowedAuth.GetAuthFor(ctx, o.Auth, ref)
 	if err != nil {
 		return "", status.Errorf(codes.InvalidArgument, "cannt resolve base image ref: %v", err)
 	}
 
-	ref, err = o.RefResolver.Resolve(ctx, ref, resolve.WithAuthentication(auth))
+	ref, err = o.RefResolver.Resolve(ctx, ref, resolve.WithAuthentication(auth), o.withRetryIfEnabled(useRetryClient))
 	if errors.Is(err, resolve.ErrNotFound) {
 		return "", status.Error(codes.NotFound, "cannot resolve image")
 	}
@@ -645,13 +642,20 @@ func (o *Orchestrator) getAbsoluteImageRef(ctx context.Context, ref string, allo
 	return ref, nil
 }
 
-func (o *Orchestrator) getBaseImageRef(ctx context.Context, bs *protocol.BuildSource, allowedAuth auth.AllowedAuthFor) (res string, err error) {
+func (o *Orchestrator) withRetryIfEnabled(useRetryClient bool) resolve.DockerRefResolverOption {
+	if useRetryClient {
+		return resolve.WithHttpClient(o.retryResolveClient)
+	}
+	return resolve.WithHttpClient(nil)
+}
+
+func (o *Orchestrator) getBaseImageRef(ctx context.Context, bs *protocol.BuildSource, allowedAuth auth.AllowedAuthFor, useRetryClient bool) (res string, err error) {
 	span, ctx := opentracing.StartSpanFromContext(ctx, "getBaseImageRef")
 	defer tracing.FinishSpan(span, &err)
 
 	switch src := bs.From.(type) {
 	case *protocol.BuildSource_Ref:
-		return o.getAbsoluteImageRef(ctx, src.Ref.Ref, allowedAuth)
+		return o.getAbsoluteImageRef(ctx, src.Ref.Ref, allowedAuth, useRetryClient)
 
 	case *protocol.BuildSource_File:
 		manifest := map[string]string{

components/image-builder-mk3/pkg/orchestrator/orchestrator_test.go

@@ -138,7 +138,7 @@ func TestBuild(t *testing.T) {
 				BaseImageRepository:      "registry/base",
 				WorkspaceImageRepository: "registry/workspace",
 				BuilderImage:             "builder-image",
-			}, false)
+			})
 			if err != nil {
 				t.Fatal(err)
 			}
@@ -188,7 +188,7 @@ func TestResolveBaseImage(t *testing.T) {
 				WorkspaceManager: config.WorkspaceManagerConfig{
 					Client: wsmock.NewMockWorkspaceManagerClient(ctrl),
 				},
-			}, false)
+			})
 			if err != nil {
 				t.Fatal(err)
 			}

components/image-builder-mk3/pkg/resolve/resolve.go

@@ -46,7 +46,6 @@ var (
 // StandaloneRefResolver can resolve image references without a Docker daemon
 type StandaloneRefResolver struct {
 	ResolverFactory func() remotes.Resolver
-	Client          *http.Client
 }
 
 // Resolve resolves a mutable Docker tag to its absolute digest form by asking the corresponding Docker registry
@@ -74,8 +73,8 @@ func (sr *StandaloneRefResolver) Resolve(ctx context.Context, ref string, opts .
 			}))),
 		}
 
-		if sr.Client != nil {
-			registryOpts = append(registryOpts, dockerremote.WithClient(sr.Client))
+		if options.Client != nil {
+			registryOpts = append(registryOpts, dockerremote.WithClient(options.Client))
 		}
 
 		r = dockerremote.NewResolver(dockerremote.ResolverOptions{
@@ -164,7 +163,8 @@ func (sr *StandaloneRefResolver) Resolve(ctx context.Context, ref string, opts .
 }
 
 type opts struct {
-	Auth *auth.Authentication
+	Auth   *auth.Authentication
+	Client *http.Client
 }
 
 // DockerRefResolverOption configures reference resolution
@@ -181,6 +181,16 @@ func WithAuthentication(auth *auth.Authentication) DockerRefResolverOption {
 	}
 }
 
+// WithHttpClient sets the HTTP client to use for making requests to the Docker registry.
+func WithHttpClient(client *http.Client) DockerRefResolverOption {
+	return func(o *opts) {
+		if client == nil {
+			log.Debug("WithHttpClient - client was nil")
+		}
+		o.Client = client
+	}
+}
+
 func getOptions(o []DockerRefResolverOption) *opts {
 	var res opts
 	for _, opt := range o {