fix: load gcs credentials and client inside DriverConstructor #4218

katexochen · 2023-12-26T11:54:38Z

Previously the init function would panic with:
Error reading JSON key : open : no such file or directory

milosgajdos

I feel like this code should be inside gcsDriverConstructor:

distribution/registry/storage/driver/gcs/gcs_test.go

Lines 37 to 69 in 012adca

    
           jsonKey, err := os.ReadFile(credentials) 
        
           if err != nil { 
        
           	panic(fmt.Sprintf("Error reading JSON key : %v", err)) 
        
           } 
        
           var ts oauth2.TokenSource 
        
           var email string 
        
           var privateKey []byte 
        
           ts, err = google.DefaultTokenSource(dcontext.Background(), storage.ScopeFullControl) 
        
           if err != nil { 
        
           	// Assume that the file contents are within the environment variable since it exists 
        
           	// but does not contain a valid file path 
        
           	jwtConfig, err := google.JWTConfigFromJSON(jsonKey, storage.ScopeFullControl) 
        
           	if err != nil { 
        
           		panic(fmt.Sprintf("Error reading JWT config : %s", err)) 
        
           	} 
        
           	email = jwtConfig.Email 
        
           	privateKey = jwtConfig.PrivateKey 
        
           	if len(privateKey) == 0 { 
        
           		panic("Error reading JWT config : missing private_key property") 
        
           	} 
        
           	if email == "" { 
        
           		panic("Error reading JWT config : missing client_email property") 
        
           	} 
        
           	ts = jwtConfig.TokenSource(dcontext.Background()) 
        
           } 
        
           gcs, err := storage.NewClient(dcontext.Background(), option.WithCredentialsJSON(jsonKey)) 
        
           if err != nil { 
        
           	panic(fmt.Sprintf("Error initializing gcs client : %v", err)) 
        
           }

We do similar things in S3 driver, where anything that might return error whilst constructing the driver is inside the constructor func:

distribution/registry/storage/driver/s3-aws/s3_test.go

Lines 51 to 131 in 012adca

    
           s3DriverConstructor = func(rootDirectory, storageClass string) (*Driver, error) { 
        
           	encryptBool := false 
        
           	if encrypt != "" { 
        
           		encryptBool, err = strconv.ParseBool(encrypt) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           	} 
        
           	secureBool := true 
        
           	if secure != "" { 
        
           		secureBool, err = strconv.ParseBool(secure) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           	} 
        
           	skipVerifyBool := false 
        
           	if skipVerify != "" { 
        
           		skipVerifyBool, err = strconv.ParseBool(skipVerify) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           	} 
        
           	v4Bool := true 
        
           	if v4Auth != "" { 
        
           		v4Bool, err = strconv.ParseBool(v4Auth) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           	} 
        
           	forcePathStyleBool := true 
        
           	if forcePathStyle != "" { 
        
           		forcePathStyleBool, err = strconv.ParseBool(forcePathStyle) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           	} 
        
           	useDualStackBool := false 
        
           	if useDualStack != "" { 
        
           		useDualStackBool, err = strconv.ParseBool(useDualStack) 
        
           	} 
        
           	accelerateBool := true 
        
           	if accelerate != "" { 
        
           		accelerateBool, err = strconv.ParseBool(accelerate) 
        
           		if err != nil { 
        
           			return nil, err 
        
           		} 
        
           	} 
        
           	parameters := DriverParameters{ 
        
           		accessKey, 
        
           		secretKey, 
        
           		bucket, 
        
           		region, 
        
           		regionEndpoint, 
        
           		forcePathStyleBool, 
        
           		encryptBool, 
        
           		keyID, 
        
           		secureBool, 
        
           		skipVerifyBool, 
        
           		v4Bool, 
        
           		minChunkSize, 
        
           		defaultMultipartCopyChunkSize, 
        
           		defaultMultipartCopyMaxConcurrency, 
        
           		defaultMultipartCopyThresholdSize, 
        
           		rootDirectory, 
        
           		storageClass, 
        
           		driverName + "-test", 
        
           		objectACL, 
        
           		sessionToken, 
        
           		useDualStackBool, 
        
           		accelerateBool, 
        
           		getS3LogLevelFromParam(logLevel), 
        
           	} 
        
           	return New(context.Background(), parameters) 
        
           }

Signed-off-by: Paul Meyer <49727155+katexochen@users.noreply.github.com>

milosgajdos · 2023-12-31T21:40:28Z

PTAL @thaJeztah

milosgajdos · 2024-01-11T11:00:22Z

ping @thaJeztah

milosgajdos · 2024-01-11T12:49:41Z

@corhere mind having a look at this PR?

wy65701436

lgtm

milosgajdos reviewed Dec 27, 2023

View reviewed changes

katexochen force-pushed the gcs/skipinit branch from f3ea6b8 to 09bbf18 Compare December 27, 2023 10:21

fix: load gcs credentials and client inside DriverConstructor

5bd7f25

Signed-off-by: Paul Meyer <49727155+katexochen@users.noreply.github.com>

katexochen force-pushed the gcs/skipinit branch from 09bbf18 to 5bd7f25 Compare December 27, 2023 10:22

katexochen changed the title ~~fix: skip init of gcs tests when tests are skipped~~ fix: load gcs credentials and client inside DriverConstructor Dec 27, 2023

katexochen requested a review from milosgajdos December 27, 2023 10:23

milosgajdos approved these changes Dec 27, 2023

View reviewed changes

milosgajdos requested review from corhere, thaJeztah and wy65701436 December 27, 2023 11:11

milosgajdos added the area/storage/gcs label Dec 31, 2023

milosgajdos mentioned this pull request Jan 11, 2024

test: Skip init if GCS envs are missing #4251

Closed

wy65701436 approved these changes Jan 12, 2024

View reviewed changes

wy65701436 merged commit 14366a2 into distribution:main Jan 12, 2024

katexochen deleted the gcs/skipinit branch January 12, 2024 10:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: load gcs credentials and client inside DriverConstructor #4218

fix: load gcs credentials and client inside DriverConstructor #4218

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	jsonKey, err := os.ReadFile(credentials)
	if err != nil {
	panic(fmt.Sprintf("Error reading JSON key : %v", err))
	}

	var ts oauth2.TokenSource
	var email string
	var privateKey []byte

	ts, err = google.DefaultTokenSource(dcontext.Background(), storage.ScopeFullControl)
	if err != nil {
	// Assume that the file contents are within the environment variable since it exists
	// but does not contain a valid file path
	jwtConfig, err := google.JWTConfigFromJSON(jsonKey, storage.ScopeFullControl)
	if err != nil {
	panic(fmt.Sprintf("Error reading JWT config : %s", err))
	}
	email = jwtConfig.Email
	privateKey = jwtConfig.PrivateKey
	if len(privateKey) == 0 {
	panic("Error reading JWT config : missing private_key property")
	}
	if email == "" {
	panic("Error reading JWT config : missing client_email property")
	}
	ts = jwtConfig.TokenSource(dcontext.Background())
	}

	gcs, err := storage.NewClient(dcontext.Background(), option.WithCredentialsJSON(jsonKey))
	if err != nil {
	panic(fmt.Sprintf("Error initializing gcs client : %v", err))
	}

	s3DriverConstructor = func(rootDirectory, storageClass string) (*Driver, error) {
	encryptBool := false
	if encrypt != "" {
	encryptBool, err = strconv.ParseBool(encrypt)
	if err != nil {
	return nil, err
	}
	}

	secureBool := true
	if secure != "" {
	secureBool, err = strconv.ParseBool(secure)
	if err != nil {
	return nil, err
	}
	}

	skipVerifyBool := false
	if skipVerify != "" {
	skipVerifyBool, err = strconv.ParseBool(skipVerify)
	if err != nil {
	return nil, err
	}
	}

	v4Bool := true
	if v4Auth != "" {
	v4Bool, err = strconv.ParseBool(v4Auth)
	if err != nil {
	return nil, err
	}
	}
	forcePathStyleBool := true
	if forcePathStyle != "" {
	forcePathStyleBool, err = strconv.ParseBool(forcePathStyle)
	if err != nil {
	return nil, err
	}
	}

	useDualStackBool := false
	if useDualStack != "" {
	useDualStackBool, err = strconv.ParseBool(useDualStack)
	}

	accelerateBool := true
	if accelerate != "" {
	accelerateBool, err = strconv.ParseBool(accelerate)
	if err != nil {
	return nil, err
	}
	}

	parameters := DriverParameters{
	accessKey,
	secretKey,
	bucket,
	region,
	regionEndpoint,
	forcePathStyleBool,
	encryptBool,
	keyID,
	secureBool,
	skipVerifyBool,
	v4Bool,
	minChunkSize,
	defaultMultipartCopyChunkSize,
	defaultMultipartCopyMaxConcurrency,
	defaultMultipartCopyThresholdSize,
	rootDirectory,
	storageClass,
	driverName + "-test",
	objectACL,
	sessionToken,
	useDualStackBool,
	accelerateBool,
	getS3LogLevelFromParam(logLevel),
	}

	return New(context.Background(), parameters)
	}

fix: load gcs credentials and client inside DriverConstructor #4218

fix: load gcs credentials and client inside DriverConstructor #4218

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!