Merge 83ece20 into blathers/backport-release-23.2-135565

cockroachdb · Nov 18, 2024 · 0fa99b8 · 0fa99b8
2 parents 5674996 + 83ece20
commit 0fa99b8
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 37 deletions.
diff --git a/pkg/cloud/external_storage.go b/pkg/cloud/external_storage.go
@@ -123,6 +123,10 @@ type SQLConnI interface {
 // ErrFileDoesNotExist is a sentinel error for indicating that a specified
 // bucket/object/key/file (depending on storage terminology) does not exist.
 // This error is raised by the ReadFile method.
+//
+// On GCS, a non-existent bucket returns a 403 forbidden, so a not found is
+// only returned if the bucket exists and the user has the permissions needed
+// to interact with the bucket.
 var ErrFileDoesNotExist = errors.New("external_storage: file doesn't exist")
 
 // ErrListingUnsupported is a marker for indicating listing is unsupported.

diff --git a/pkg/cloud/gcp/gcs_storage_test.go b/pkg/cloud/gcp/gcs_storage_test.go
@@ -25,7 +25,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util/ioctx"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
-	"github.com/cockroachdb/errors"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/oauth2/google"
 )
@@ -327,8 +326,8 @@ func TestAntagonisticGCSRead(t *testing.T) {
 	cloudtestutils.CheckAntagonisticRead(t, conf, testSettings)
 }
 
-// TestFileDoesNotExist ensures that the ReadFile method of google cloud storage
-// returns a sentinel error when the `Bucket` or `Object` being read do not
+// TestFileDoesNotExist ensures that the ReadFile method of google cloud
+// storage returns a sentinel error when the `Object` being read does not
 // exist.
 func TestFileDoesNotExist(t *testing.T) {
 	defer leaktest.AfterTest(t)()
@@ -341,41 +340,20 @@ func TestFileDoesNotExist(t *testing.T) {
 
 	testSettings := cluster.MakeTestingClusterSettings()
 
-	{
-		// Invalid gsFile.
-		gsFile := "gs://cockroach-fixtures-us-east1/tpch-csv/sf-1/invalid_region.tbl?AUTH=implicit"
-		conf, err := cloud.ExternalStorageConfFromURI(gsFile, user)
-		require.NoError(t, err)
-
-		s, err := cloud.MakeExternalStorage(context.Background(), conf, base.ExternalIODirConfig{}, testSettings,
-			nil, /* blobClientFactory */
-			nil, /* db */
-			nil, /* limiters */
-			cloud.NilMetrics,
-		)
-		require.NoError(t, err)
-		_, _, err = s.ReadFile(context.Background(), "", cloud.ReadOptions{NoFileSize: true})
-		require.Error(t, err, "")
-		require.True(t, errors.Is(err, cloud.ErrFileDoesNotExist))
-	}
-
-	{
-		// Invalid gsBucket.
-		gsFile := "gs://cockroach-fixtures-us-east1-invalid/tpch-csv/sf-1/region.tbl?AUTH=implicit"
-		conf, err := cloud.ExternalStorageConfFromURI(gsFile, user)
-		require.NoError(t, err)
+	// Invalid gsFile.
+	gsFile := "gs://cockroach-fixtures-us-east1/tpch-csv/sf-1/invalid_region.tbl?AUTH=implicit"
+	conf, err := cloud.ExternalStorageConfFromURI(gsFile, user)
+	require.NoError(t, err)
 
-		s, err := cloud.MakeExternalStorage(context.Background(), conf, base.ExternalIODirConfig{}, testSettings,
-			nil, /* blobClientFactory */
-			nil, /* db */
-			nil, /* limiters */
-			cloud.NilMetrics,
-		)
-		require.NoError(t, err)
-		_, _, err = s.ReadFile(context.Background(), "", cloud.ReadOptions{NoFileSize: true})
-		require.Error(t, err, "")
-		require.True(t, errors.Is(err, cloud.ErrFileDoesNotExist))
-	}
+	s, err := cloud.MakeExternalStorage(context.Background(), conf, base.ExternalIODirConfig{}, testSettings,
+		nil, /* blobClientFactory */
+		nil, /* db */
+		nil, /* limiters */
+		cloud.NilMetrics,
+	)
+	require.NoError(t, err)
+	_, _, err = s.ReadFile(context.Background(), "", cloud.ReadOptions{NoFileSize: true})
+	require.ErrorIs(t, err, cloud.ErrFileDoesNotExist)
 }
 
 func TestCompressedGCS(t *testing.T) {