HCP Packer Buckets: Change UpsertBucket to call GetBucket (#13059)

* Update UpsertBucket to first call GetBucket, this will allow bucket level role based authentication, as CreateBucket uses project level auth

* Fix one incorrect test failure message

internal/hcp/api/mock_service.go

@@ -19,7 +19,7 @@ import (
 // Upon calling a service method a boolean is set to true to indicate that a method has been called.
 // To skip the setting of these booleans set TrackCalledServiceMethods to false; defaults to true in NewMockPackerClientService().
 type MockPackerClientService struct {
-	CreateBucketCalled, UpdateBucketCalled, BucketAlreadyExist                   bool
+	CreateBucketCalled, UpdateBucketCalled, GetBucketCalled, BucketNotFound      bool
 	CreateVersionCalled, GetVersionCalled, VersionAlreadyExist, VersionCompleted bool
 	CreateBuildCalled, UpdateBuildCalled, ListBuildsCalled, BuildAlreadyDone     bool
 	TrackCalledServiceMethods                                                    bool
@@ -55,14 +55,6 @@ func (svc *MockPackerClientService) PackerServiceCreateBucket(
 	params *hcpPackerService.PackerServiceCreateBucketParams, _ runtime.ClientAuthInfoWriter,
 	opts ...hcpPackerService.ClientOption,
 ) (*hcpPackerService.PackerServiceCreateBucketOK, error) {
-
-	if svc.BucketAlreadyExist {
-		return nil, status.Error(
-			codes.AlreadyExists,
-			fmt.Sprintf("Code:%d %s", codes.AlreadyExists, codes.AlreadyExists.String()),
-		)
-	}
-
 	if params.Body.Name == "" {
 		return nil, errors.New("no bucket name was passed in")
 	}
@@ -84,6 +76,19 @@ func (svc *MockPackerClientService) PackerServiceCreateBucket(
 	return ok, nil
 }
 
+func (svc *MockPackerClientService) PackerServiceGetBucket(
+	params *hcpPackerService.PackerServiceGetBucketParams, _ runtime.ClientAuthInfoWriter,
+	opts ...hcpPackerService.ClientOption,
+) (*hcpPackerService.PackerServiceGetBucketOK, error) {
+	if svc.TrackCalledServiceMethods {
+		svc.GetBucketCalled = true
+	}
+	if svc.BucketNotFound {
+		return nil, status.Error(codes.NotFound, fmt.Sprintf("Code:%d %s", codes.NotFound, codes.NotFound.String()))
+	}
+	return hcpPackerService.NewPackerServiceGetBucketOK(), nil
+}
+
 func (svc *MockPackerClientService) PackerServiceUpdateBucket(
 	params *hcpPackerService.PackerServiceUpdateBucketParams, _ runtime.ClientAuthInfoWriter,
 	opts ...hcpPackerService.ClientOption,

internal/hcp/api/service_bucket.go

@@ -36,20 +36,24 @@ func (c *Client) DeleteBucket(
 	return c.Packer.PackerServiceDeleteBucket(deleteBktParams, nil)
 }
 
-// UpsertBucket tries to create a bucket on a HCP Packer Registry. If the bucket exists it will
-// handle the error and update the bucket with the provided details.
+// UpsertBucket will create or update a bucket. It calls GetBucket first, if the bucket is not found it creates that bucket
+// If GetBucket succeeded we then call UpdateBucket description and bucket labels in case they've changed.
+// GetBucket is used instead of CreateBucket since users with bucket level access to specific existing buckets can not create new buckets.
 func (c *Client) UpsertBucket(
 	ctx context.Context, bucketName, bucketDescription string, bucketLabels map[string]string,
 ) error {
 
-	// Create bucket if exist we continue as is, eventually we want to treat this like an upsert.
-	_, err := c.CreateBucket(ctx, bucketName, bucketDescription, bucketLabels)
-	if err != nil && !CheckErrorCode(err, codes.AlreadyExists) {
-		return err
-	}
+	getParams := hcpPackerService.NewPackerServiceGetBucketParamsWithContext(ctx)
+	getParams.LocationOrganizationID = c.OrganizationID
+	getParams.LocationProjectID = c.ProjectID
+	getParams.BucketName = bucketName
 
-	if err == nil {
-		return nil
+	_, err := c.Packer.PackerServiceGetBucket(getParams, nil)
+	if err != nil {
+		if CheckErrorCode(err, codes.NotFound) {
+			_, err = c.CreateBucket(ctx, bucketName, bucketDescription, bucketLabels)
+		}
+		return err
 	}
 
 	params := hcpPackerService.NewPackerServiceUpdateBucketParamsWithContext(ctx)

internal/hcp/registry/types.bucket.go

@@ -122,7 +122,6 @@ func (bucket *Bucket) Initialize(
 	}
 
 	bucket.Destination = fmt.Sprintf("%s/%s", bucket.client.OrganizationID, bucket.client.ProjectID)
-
 	err := bucket.client.UpsertBucket(ctx, bucket.Name, bucket.Description, bucket.BucketLabels)
 	if err != nil {
 		return fmt.Errorf("failed to initialize bucket %q: %w", bucket.Name, err)

internal/hcp/registry/types.bucket_service_test.go

@@ -13,6 +13,7 @@ import (
 
 func TestInitialize_NewBucketNewVersion(t *testing.T) {
 	mockService := hcpPackerAPI.NewMockPackerClientService()
+	mockService.BucketNotFound = true
 
 	b := &Bucket{
 		Name: "TestBucket",
@@ -34,10 +35,15 @@ func TestInitialize_NewBucketNewVersion(t *testing.T) {
 		t.Errorf("unexpected failure: %v", err)
 	}
 
+	if !mockService.GetBucketCalled {
+		t.Errorf("expected a call to GetBucket but it didn't happen")
+	}
 	if !mockService.CreateBucketCalled {
 		t.Errorf("expected a call to CreateBucket but it didn't happen")
 	}
-
+	if mockService.UpdateBucketCalled {
+		t.Errorf("unexpected call to UpdateBucket")
+	}
 	if !mockService.CreateVersionCalled {
 		t.Errorf("expected a call to CreateVersion but it didn't happen")
 	}
@@ -65,8 +71,6 @@ func TestInitialize_NewBucketNewVersion(t *testing.T) {
 
 func TestInitialize_UnsetTemplateTypeError(t *testing.T) {
 	mockService := hcpPackerAPI.NewMockPackerClientService()
-	mockService.BucketAlreadyExist = true
-
 	b := &Bucket{
 		Name: "TestBucket",
 		client: &hcpPackerAPI.Client{
@@ -90,7 +94,6 @@ func TestInitialize_UnsetTemplateTypeError(t *testing.T) {
 
 func TestInitialize_ExistingBucketNewVersion(t *testing.T) {
 	mockService := hcpPackerAPI.NewMockPackerClientService()
-	mockService.BucketAlreadyExist = true
 
 	b := &Bucket{
 		Name: "TestBucket",
@@ -110,6 +113,12 @@ func TestInitialize_ExistingBucketNewVersion(t *testing.T) {
 	if err != nil {
 		t.Errorf("unexpected failure: %v", err)
 	}
+	if !mockService.GetBucketCalled {
+		t.Errorf("expected call to GetBucket but it didn't happen")
+	}
+	if mockService.CreateBucketCalled {
+		t.Errorf("unexpected call to CreateBucket")
+	}
 
 	if !mockService.UpdateBucketCalled {
 		t.Errorf("expected call to UpdateBucket but it didn't happen")
@@ -143,7 +152,6 @@ func TestInitialize_ExistingBucketNewVersion(t *testing.T) {
 
 func TestInitialize_ExistingBucketExistingVersion(t *testing.T) {
 	mockService := hcpPackerAPI.NewMockPackerClientService()
-	mockService.BucketAlreadyExist = true
 	mockService.VersionAlreadyExist = true
 
 	b := &Bucket{
@@ -175,20 +183,26 @@ func TestInitialize_ExistingBucketExistingVersion(t *testing.T) {
 		t.Errorf("unexpected call to CreateBucket")
 	}
 
+	if !mockService.GetBucketCalled {
+		t.Errorf("expected call to GetBucket but it didn't happen")
+	}
+	if mockService.CreateBucketCalled {
+		t.Errorf("unexpected call to CreateBucket")
+	}
 	if !mockService.UpdateBucketCalled {
 		t.Errorf("expected call to UpdateBucket but it didn't happen")
 	}
 
 	if mockService.CreateVersionCalled {
-		t.Errorf("unexpected a call to CreateVersion")
+		t.Errorf("unexpected call to CreateVersion")
 	}
 
 	if !mockService.GetVersionCalled {
 		t.Errorf("expected a call to GetVersion but it didn't happen")
 	}
 
 	if mockService.CreateBuildCalled {
-		t.Errorf("unexpected a call to CreateBuild")
+		t.Errorf("unexpected call to CreateBuild")
 	}
 
 	if b.Version.ID != "version-id" {
@@ -212,7 +226,6 @@ func TestInitialize_ExistingBucketExistingVersion(t *testing.T) {
 
 func TestInitialize_ExistingBucketCompleteVersion(t *testing.T) {
 	mockService := hcpPackerAPI.NewMockPackerClientService()
-	mockService.BucketAlreadyExist = true
 	mockService.VersionAlreadyExist = true
 	mockService.VersionCompleted = true
 	mockService.BuildAlreadyDone = true
@@ -238,6 +251,15 @@ func TestInitialize_ExistingBucketCompleteVersion(t *testing.T) {
 		t.Errorf("unexpected failure: %v", err)
 	}
 
+	if !mockService.GetBucketCalled {
+		t.Errorf("expected call to GetBucket, but it didn't happen")
+	}
+	if !mockService.UpdateBucketCalled {
+		t.Errorf("expected call to UpdateBucket, but it didn't happen")
+	}
+	if mockService.CreateBucketCalled {
+		t.Errorf("unexpected call to CreateBucket")
+	}
 	if mockService.CreateVersionCalled {
 		t.Errorf("unexpected call to CreateVersion")
 	}
@@ -257,7 +279,6 @@ func TestInitialize_ExistingBucketCompleteVersion(t *testing.T) {
 
 func TestUpdateBuildStatus(t *testing.T) {
 	mockService := hcpPackerAPI.NewMockPackerClientService()
-	mockService.BucketAlreadyExist = true
 	mockService.VersionAlreadyExist = true
 
 	b := &Bucket{
@@ -310,9 +331,7 @@ func TestUpdateBuildStatus(t *testing.T) {
 
 func TestUpdateBuildStatus_DONENoImages(t *testing.T) {
 	mockService := hcpPackerAPI.NewMockPackerClientService()
-	mockService.BucketAlreadyExist = true
 	mockService.VersionAlreadyExist = true
-
 	b := &Bucket{
 		Name: "TestBucket",
 		client: &hcpPackerAPI.Client{

internal/hcp/registry/types.bucket_test.go

@@ -291,7 +291,6 @@ func TestBucket_PopulateVersion(t *testing.T) {
 			t.Setenv("HCP_PACKER_BUILD_FINGERPRINT", "test-run-"+strconv.Itoa(i))
 
 			mockService := hcpPackerAPI.NewMockPackerClientService()
-			mockService.BucketAlreadyExist = true
 			mockService.VersionAlreadyExist = true
 			mockService.BuildAlreadyDone = tt.buildCompleted