refactor(gcp): use the shared IsPermissionError and new(expr) instead of local shims

CHANGELOG.md

@@ -126,6 +126,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Changed
 
+- **`gcp`:** region-lookup permission errors are now classified by type only (googleapi 403 or gRPC `PermissionDenied`) through the shared `IsPermissionError`. gRPC `PermissionDenied` is now skipped with a warning like a 403, and an untyped error whose text merely contains "403" and "permission" now propagates instead of being skipped (#6).
 - **`azure`:** managedredis and synapse reservations now get the same generated display name as the other services (`redis-<region>-<sku>-<n>x-<term>-<payment>-...` and `synapse-...`) instead of the fixed `Azure Cache for Redis Reservation - <sku>` / `Synapse SQL Pool Reservation - <sku>`. This also removes the stray space before the idempotency marker on managedredis. Idempotency lookup keys on the tag or the marker suffix, not the base name (#538).
 - **BREAKING `gcp`: Cloud SQL and Memorystore no longer return CUD
   recommendations (#294).** They came from the project-scoped

providers/gcp/recommendations.go

@@ -3,16 +3,13 @@ package gcp
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"os"
 	"sort"
 	"strconv"
-	"strings"
 	"sync"
 
 	"golang.org/x/sync/errgroup"
-	"google.golang.org/api/googleapi"
 	"google.golang.org/api/option"
 
 	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
@@ -72,6 +69,8 @@ type RecommendationsClientAdapter struct {
 	ctx        context.Context
 	projectID  string
 	clientOpts []option.ClientOption
+	// regionsFn replaces the live region lookup; tests only.
+	regionsFn func(context.Context) ([]string, error)
 }
 
 // GetRecommendations retrieves all GCP commitment recommendations across all
@@ -124,7 +123,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p
 		// still works; only GCP recommendations for this account are skipped.
 		// See issue #247. A canceled ctx never reaches this branch (guarded
 		// above), so a genuine 403 is the only thing swallowed here.
-		if isPermissionError(err) {
+		if IsPermissionError(err) {
 			logging.Warnf("GCP account %s: skipping recommendations — insufficient Compute permission to list regions (grant roles/compute.viewer): %v", r.projectID, err)
 			return []common.Recommendation{}, nil
 		}
@@ -295,6 +294,9 @@ func (r *RecommendationsClientAdapter) GetAllRecommendations(ctx context.Context
 
 // getRegions retrieves available GCP regions for the project.
 func (r *RecommendationsClientAdapter) getRegions(ctx context.Context) ([]string, error) {
+	if r.regionsFn != nil {
+		return r.regionsFn(ctx)
+	}
 	// Create a temporary provider to get regions. The local variable is named
 	// p (not provider) to avoid shadowing the imported provider package (10-N3).
 	p := NewProviderWithProject(ctx, r.projectID, r.clientOpts...)
@@ -322,36 +324,3 @@ func shouldIncludeService(params common.RecommendationParams, service common.Ser
 	// Check if this is the requested service
 	return params.Service == service
 }
-
-// isPermissionError returns true when err is a GCP 403 "Required '...' permission"
-// error. These errors arise when the service account is missing an IAM role
-// (e.g. roles/compute.viewer for compute.regions.list). Callers can then log
-// at Warn rather than propagating an error that would be recorded as ERROR by
-// the Lambda handler — see issue #247.
-func isPermissionError(err error) bool {
-	if err == nil {
-		return false
-	}
-	var gapiErr *googleapi.Error
-	if ok := errorAs(err, &gapiErr); ok && gapiErr.Code == 403 {
-		return true
-	}
-	// Fall back to string inspection for wrapped or non-googleapi 403s.
-	msg := err.Error()
-	return strings.Contains(msg, "403") && strings.Contains(msg, "permission")
-}
-
-// errorAs is a thin shim around errors.As so it can be stubbed in tests
-// without importing errors at the call site.
-var errorAs = func(err error, target interface{}) bool {
-	switch t := target.(type) {
-	case **googleapi.Error:
-		var gErr *googleapi.Error
-		if !errors.As(err, &gErr) {
-			return false
-		}
-		*t = gErr
-		return true
-	}
-	return false
-}

providers/gcp/recommendations_test.go

@@ -16,6 +16,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sync/semaphore"
+	"google.golang.org/api/googleapi"
 	"google.golang.org/api/option"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
@@ -464,3 +465,33 @@ func TestGetRecommendations_ExplicitCloudStorageIgnoresOccupiedSemaphore(t *test
 	assert.Zero(t, calls.grpcDials.Load())
 	assert.Zero(t, calls.listRPCs.Load())
 }
+
+func TestGetRecommendations_RegionLookupErrorClassification(t *testing.T) {
+	t.Parallel()
+	tests := []struct {
+		name    string
+		err     error
+		wantErr bool
+	}{
+		{"googleapi 403 is skipped", &googleapi.Error{Code: 403, Message: "Required 'compute.regions.list' permission"}, false},
+		{"grpc PermissionDenied is skipped", status.Error(codes.PermissionDenied, "denied"), false},
+		{"untyped 403 permission string propagates", errors.New("403 permission denied"), true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			adapter := &RecommendationsClientAdapter{
+				projectID: "p",
+				regionsFn: func(context.Context) ([]string, error) { return nil, tt.err },
+			}
+			recs, err := adapter.GetAllRecommendations(context.Background())
+			if tt.wantErr {
+				require.Error(t, err)
+				assert.ErrorContains(t, err, "failed to get regions")
+				return
+			}
+			require.NoError(t, err)
+			assert.Empty(t, recs)
+		})
+	}
+}

providers/gcp/services/computeengine/client.go

@@ -844,23 +844,23 @@ func (c *Client) buildInsertRequest(rec common.Recommendation, opts common.Purch
 
 	// This client supports VCPU+MEMORY commitments; memory Amount is in MB.
 	commitment := &computepb.Commitment{
-		Name:        stringPtr(commitmentName),
-		Plan:        stringPtr(plan),
-		Type:        stringPtr(commitType.String()),
-		Description: stringPtr(description),
+		Name:        new(commitmentName),
+		Plan:        new(plan),
+		Type:        new(commitType.String()),
+		Description: new(description),
 		Resources: []*computepb.ResourceCommitment{
 			{
-				Type:   stringPtr(computepb.ResourceCommitment_VCPU.String()),
-				Amount: int64Ptr(int64(rec.Count)),
+				Type:   new(computepb.ResourceCommitment_VCPU.String()),
+				Amount: new(int64(rec.Count)),
 			},
 			{
 				// GCP's ResourceCommitment.Type enum is VCPU/MEMORY/LOCAL_SSD/
 				// ACCELERATOR; the memory member is "MEMORY" (the Amount is still in
 				// MB). Sending "MEMORY_MB" is an invalid enum value and GCP rejects
 				// the commitments.insert, failing the purchase (issue #1022). Taking
 				// the wire value from the SDK enum keeps that compiler-checked.
-				Type:   stringPtr(computepb.ResourceCommitment_MEMORY.String()),
-				Amount: int64Ptr(memMB),
+				Type:   new(computepb.ResourceCommitment_MEMORY.String()),
+				Amount: new(memMB),
 			},
 		},
 	}
@@ -875,7 +875,7 @@ func (c *Client) buildInsertRequest(rec common.Recommendation, opts common.Purch
 		return nil, "", fmt.Errorf("derive GCP request ID: %w", err)
 	}
 	if requestID != "" {
-		insertReq.RequestId = stringPtr(requestID)
+		insertReq.RequestId = new(requestID)
 		log.Printf("GCP CUD purchase using idempotent request ID for token %s (commitment %s); a re-drive will not double-purchase (issue #654)",
 			common.MaskToken(opts.IdempotencyToken), commitmentName)
 	}
@@ -1459,12 +1459,3 @@ func idempotentCommitmentName(token string) (string, error) {
 }
 
 var idempotencyTokenPattern = regexp.MustCompile(`^[0-9a-f]+$`)
-
-// Helper functions.
-func stringPtr(s string) *string {
-	return &s
-}
-
-func int64Ptr(i int64) *int64 {
-	return &i
-}

providers/gcp/services/computeengine/client_test.go

@@ -376,22 +376,6 @@ func TestComputePricingStructure(t *testing.T) {
 	assert.Equal(t, 50.0, pricing.SavingsPercentage)
 }
 
-func TestStringPtr(t *testing.T) {
-	t.Parallel()
-	s := "test"
-	ptr := stringPtr(s)
-	require.NotNil(t, ptr)
-	assert.Equal(t, "test", *ptr)
-}
-
-func TestInt64Ptr(t *testing.T) {
-	t.Parallel()
-	i := int64(42)
-	ptr := int64Ptr(i)
-	require.NotNil(t, ptr)
-	assert.Equal(t, int64(42), *ptr)
-}
-
 func TestComputeEngineClient_ValidateOffering_NoCredentials(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
@@ -817,25 +801,25 @@ func TestComputeEngineClient_PurchaseCommitment_ReDriveAlreadyExists(t *testing.
 	}{
 		{name: "matching commitment is the earlier purchase", opts: tokenOpts, getStatus: http.StatusOK, wantGets: 1},
 		{name: "different vCPU amount stays a failure", opts: tokenOpts, getStatus: http.StatusOK, wantGets: 1,
-			existing:  func(c *computepb.Commitment) { c.Resources[0].Amount = int64Ptr(4) },
+			existing:  func(c *computepb.Commitment) { c.Resources[0].Amount = new(int64(4)) },
 			wantErrIs: errExistingCommitmentMismatch},
 		{name: "different plan stays a failure", opts: tokenOpts, getStatus: http.StatusOK, wantGets: 1,
-			existing:  func(c *computepb.Commitment) { c.Plan = stringPtr(computepb.Commitment_TWELVE_MONTH.String()) },
+			existing:  func(c *computepb.Commitment) { c.Plan = new(computepb.Commitment_TWELVE_MONTH.String()) },
 			wantErrIs: errExistingCommitmentMismatch},
 		{name: "different type stays a failure", opts: tokenOpts, getStatus: http.StatusOK, wantGets: 1,
-			existing:  func(c *computepb.Commitment) { c.Type = stringPtr(computepb.Commitment_COMPUTE_OPTIMIZED.String()) },
+			existing:  func(c *computepb.Commitment) { c.Type = new(computepb.Commitment_COMPUTE_OPTIMIZED.String()) },
 			wantErrIs: errExistingCommitmentMismatch},
 		{name: "non-409 insert error 400 is not adopted", opts: tokenOpts, insertStatus: http.StatusBadRequest,
 			getStatus: http.StatusOK, wantErr: "insert rejected 400"},
 		{name: "non-409 insert error 500 is not adopted", opts: tokenOpts, insertStatus: http.StatusInternalServerError,
 			getStatus: http.StatusOK, wantErr: "insert rejected 500"},
 		{name: "extra resource stays a failure", opts: tokenOpts, getStatus: http.StatusOK, wantGets: 1,
 			existing: func(c *computepb.Commitment) {
-				c.Resources = append(c.Resources, &computepb.ResourceCommitment{Type: stringPtr("LOCAL_SSD"), Amount: int64Ptr(375)})
+				c.Resources = append(c.Resources, &computepb.ResourceCommitment{Type: new("LOCAL_SSD"), Amount: new(int64(375))})
 			},
 			wantErrIs: errExistingCommitmentMismatch},
 		{name: "matching commitment still CREATING is adopted", opts: tokenOpts, getStatus: http.StatusOK, wantGets: 1,
-			existing: func(c *computepb.Commitment) { c.Status = stringPtr(computepb.Commitment_CREATING.String()) }},
+			existing: func(c *computepb.Commitment) { c.Status = new(computepb.Commitment_CREATING.String()) }},
 		{name: "unreadable existing commitment stays a failure", opts: tokenOpts, getStatus: http.StatusForbidden, wantGets: 1,
 			wantErr: "could not be read to confirm"},
 		{name: "no token keeps the conflict a failure", opts: common.PurchaseOptions{}, getStatus: http.StatusOK,
@@ -847,7 +831,7 @@ func TestComputeEngineClient_PurchaseCommitment_ReDriveAlreadyExists(t *testing.
 			insertReq, name, err := planner.buildInsertRequest(rec, tt.opts)
 			require.NoError(t, err)
 			existing := proto.Clone(insertReq.GetCommitmentResource()).(*computepb.Commitment)
-			existing.Status = stringPtr(computepb.Commitment_NOT_YET_ACTIVE.String())
+			existing.Status = new(computepb.Commitment_NOT_YET_ACTIVE.String())
 			if tt.existing != nil {
 				tt.existing(existing)
 			}

providers/gcp/services/computeengine/dedupe_test.go

@@ -69,7 +69,7 @@ func TestRootCUDPublicPurchasePath(t *testing.T) {
 					svc := &MockCommitmentsService{operation: &MockOperation{}}
 					if existingKind != "" {
 						cud := recentCUD()
-						cud.Type = stringPtr(existingKind)
+						cud.Type = new(existingKind)
 						svc.commitments = []*computepb.Commitment{cud}
 					}
 					client.SetCommitmentsService(svc)
@@ -121,9 +121,9 @@ func TestCUDStoredTypePublicPath(t *testing.T) {
 					client, err := NewClient(ctx, rec.Account, rec.Region)
 					require.NoError(t, err)
 					cud := recentCUD()
-					cud.Type = stringPtr("MEMORY_OPTIMIZED_X4_960_12T")
+					cud.Type = new("MEMORY_OPTIMIZED_X4_960_12T")
 					if sameBucket && valid {
-						cud.Type = stringPtr(want)
+						cud.Type = new(want)
 					}
 					service := &MockCommitmentsService{operation: &MockOperation{}, commitments: []*computepb.Commitment{cud}}
 					client.SetCommitmentsService(service)
@@ -179,11 +179,11 @@ func TestCUDTypeFallbackWithAbsentDetails(t *testing.T) {
 
 func recentCUD() *computepb.Commitment {
 	return &computepb.Commitment{
-		Name: stringPtr("recent-cud"), Type: stringPtr("GENERAL_PURPOSE_N2"),
-		Status: stringPtr("ACTIVE"), StartTimestamp: stringPtr(time.Now().Add(-time.Hour).Format(time.RFC3339)),
+		Name: new("recent-cud"), Type: new("GENERAL_PURPOSE_N2"),
+		Status: new("ACTIVE"), StartTimestamp: new(time.Now().Add(-time.Hour).Format(time.RFC3339)),
 		Resources: []*computepb.ResourceCommitment{
-			{Type: stringPtr("MEMORY"), Amount: int64Ptr(65536)},
-			{Type: stringPtr("VCPU"), Amount: int64Ptr(16)},
+			{Type: new("MEMORY"), Amount: new(int64(65536))},
+			{Type: new("VCPU"), Amount: new(int64(16))},
 		},
 	}
 }
@@ -211,25 +211,25 @@ func TestCUDDuplicatePurchasePath(t *testing.T) {
 		}, 0},
 		{"different family", func(c *computepb.Commitment, r *common.Recommendation) { r.ResourceType = "c3-standard-32" }, 1},
 		{"queued future", func(c *computepb.Commitment, r *common.Recommendation) {
-			c.Status = stringPtr("NOT_YET_ACTIVE")
-			c.StartTimestamp = stringPtr(time.Now().Add(time.Hour).Format(time.RFC3339))
+			c.Status = new("NOT_YET_ACTIVE")
+			c.StartTimestamp = new(time.Now().Add(time.Hour).Format(time.RFC3339))
 		}, 0},
-		{"creating", func(c *computepb.Commitment, r *common.Recommendation) { c.Status = stringPtr("CREATING") }, 0},
-		{"unknown state", func(c *computepb.Commitment, r *common.Recommendation) { c.Status = stringPtr("FUTURE_STATE") }, 0},
-		{"expired", func(c *computepb.Commitment, r *common.Recommendation) { c.Status = stringPtr("EXPIRED") }, 1},
+		{"creating", func(c *computepb.Commitment, r *common.Recommendation) { c.Status = new("CREATING") }, 0},
+		{"unknown state", func(c *computepb.Commitment, r *common.Recommendation) { c.Status = new("FUTURE_STATE") }, 0},
+		{"expired", func(c *computepb.Commitment, r *common.Recommendation) { c.Status = new("EXPIRED") }, 1},
 		{"canceled", func(c *computepb.Commitment, r *common.Recommendation) {
-			c.Status = stringPtr(computepb.Commitment_CANCELLED.String()) //nolint:misspell // SDK identifier
+			c.Status = new(computepb.Commitment_CANCELLED.String()) //nolint:misspell // SDK identifier
 		}, 1},
 		{"stale advice outside window", func(c *computepb.Commitment, r *common.Recommendation) {
-			c.StartTimestamp = stringPtr(time.Now().Add(-48 * time.Hour).Format(time.RFC3339))
+			c.StartTimestamp = new(time.Now().Add(-48 * time.Hour).Format(time.RFC3339))
 			r.Timestamp = time.Now().Add(-72 * time.Hour)
 		}, 1},
 		{"legacy custom N1", func(c *computepb.Commitment, r *common.Recommendation) {
-			c.Type = stringPtr("GENERAL_PURPOSE")
+			c.Type = new("GENERAL_PURPOSE")
 			r.ResourceType = "custom-32-262144"
 		}, 0},
 		{"M1 M2 shared pool", func(c *computepb.Commitment, r *common.Recommendation) {
-			c.Type = stringPtr("MEMORY_OPTIMIZED")
+			c.Type = new("MEMORY_OPTIMIZED")
 			r.ResourceType = "m2-ultramem-208"
 		}, 0},
 	} {
@@ -267,16 +267,16 @@ func TestCUDInventoryRejectsUnusableMetadata(t *testing.T) {
 	for name, mutate := range map[string]func(*computepb.Commitment){
 		"missing name":       func(c *computepb.Commitment) { c.Name = nil },
 		"missing start":      func(c *computepb.Commitment) { c.StartTimestamp = nil },
-		"invalid start":      func(c *computepb.Commitment) { c.StartTimestamp = stringPtr("yesterday") },
-		"unknown family":     func(c *computepb.Commitment) { c.Type = stringPtr("FUTURE_FAMILY") },
-		"undefined family":   func(c *computepb.Commitment) { c.Type = stringPtr("UNDEFINED_TYPE") },
-		"unknown resource":   func(c *computepb.Commitment) { c.Resources[0].Type = stringPtr("FUTURE_RESOURCE") },
-		"undefined resource": func(c *computepb.Commitment) { c.Resources[0].Type = stringPtr("UNDEFINED_TYPE") },
+		"invalid start":      func(c *computepb.Commitment) { c.StartTimestamp = new("yesterday") },
+		"unknown family":     func(c *computepb.Commitment) { c.Type = new("FUTURE_FAMILY") },
+		"undefined family":   func(c *computepb.Commitment) { c.Type = new("UNDEFINED_TYPE") },
+		"unknown resource":   func(c *computepb.Commitment) { c.Resources[0].Type = new("FUTURE_RESOURCE") },
+		"undefined resource": func(c *computepb.Commitment) { c.Resources[0].Type = new("UNDEFINED_TYPE") },
 		"nil resource type":  func(c *computepb.Commitment) { c.Resources[0].Type = nil },
 		"nil memory amount":  func(c *computepb.Commitment) { c.Resources[0].Amount = nil },
 		"nil vcpu amount":    func(c *computepb.Commitment) { c.Resources[1].Amount = nil },
-		"negative amount":    func(c *computepb.Commitment) { c.Resources[1].Amount = int64Ptr(-1) },
-		"zero amount":        func(c *computepb.Commitment) { c.Resources[1].Amount = int64Ptr(0) },
+		"negative amount":    func(c *computepb.Commitment) { c.Resources[1].Amount = new(int64(-1)) },
+		"zero amount":        func(c *computepb.Commitment) { c.Resources[1].Amount = new(int64(0)) },
 		"missing vcpu":       func(c *computepb.Commitment) { c.Resources = c.Resources[:1] },
 		"nil resource":       func(c *computepb.Commitment) { c.Resources[0] = nil },
 		"duplicate vcpu":     func(c *computepb.Commitment) { c.Resources = append(c.Resources, c.Resources[1]) },