diff --git a/google/cloud/storage/internal/async/connection_tracing.cc b/google/cloud/storage/internal/async/connection_tracing.cc index 4b8b9e1b22859..dd03daaeb6781 100644 --- a/google/cloud/storage/internal/async/connection_tracing.cc +++ b/google/cloud/storage/internal/async/connection_tracing.cc @@ -267,7 +267,6 @@ class AsyncConnectionTracing : public storage::AsyncConnection { private: static constexpr char kProjectBucketPrefix[] = "projects/_/buckets/"; - static constexpr char kGlobalLocation[] = "global"; BucketMetadataCache& cache() const { return *cache_; } @@ -291,17 +290,14 @@ class AsyncConnectionTracing : public storage::AsyncConnection { StatusOr metadata = f.get(); if (metadata.ok()) { BucketCacheEntry entry = BucketCacheEntry::FromLocation( - metadata->project() + "/buckets/" + - BucketMetadataCache::NormalizeBucketName(bucket_name), + BucketCacheEntry::ResourceName(metadata->project(), + bucket_name), metadata->location(), metadata->location_type()); cache->Put(bucket_name, std::move(entry)); } else if (metadata.status().code() == StatusCode::kPermissionDenied) { - BucketCacheEntry entry{ - std::string(kProjectBucketPrefix) + - BucketMetadataCache::NormalizeBucketName(bucket_name), - kGlobalLocation}; - cache->Put(bucket_name, std::move(entry)); + cache->Put(bucket_name, + BucketCacheEntry::FromUnknownProject(bucket_name)); } }); } @@ -321,8 +317,7 @@ class AsyncConnectionTracing : public storage::AsyncConnection { google::cloud::storage_experimental::OTelSpanEnrichmentOption>(); if (!enabled) return; auto entry = BucketCacheEntry::FromLocation( - bucket.project() + "/buckets/" + - BucketMetadataCache::NormalizeBucketName(bucket_name), + BucketCacheEntry::ResourceName(bucket.project(), bucket_name), bucket.location(), bucket.location_type()); EnrichSpan(span, entry); cache.Put(bucket_name, std::move(entry)); diff --git a/google/cloud/storage/internal/async/connection_tracing_test.cc b/google/cloud/storage/internal/async/connection_tracing_test.cc index 588b486e1458a..a7d38829bd1c8 100644 --- a/google/cloud/storage/internal/async/connection_tracing_test.cc +++ b/google/cloud/storage/internal/async/connection_tracing_test.cc @@ -590,7 +590,8 @@ TEST(ConnectionTracing, RewriteObjectSpanEnrichment) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -890,7 +891,8 @@ TEST(ConnectionTracing, GetBucketSpanEnrichment) { SpanHasInstrumentationScope(), SpanKindIsClient(), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -953,7 +955,8 @@ TEST(ConnectionTracing, BucketMetadataCacheSuccess) { SpanHasInstrumentationScope(), SpanKindIsClient(), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -1054,7 +1057,8 @@ TEST(ConnectionTracing, DeleteObjectNoEvictOnError) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } diff --git a/google/cloud/storage/internal/bucket_metadata_cache.cc b/google/cloud/storage/internal/bucket_metadata_cache.cc index aa9f75b9ecf12..7d7aa312864d8 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache.cc @@ -24,6 +24,13 @@ namespace cloud { namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +std::string BucketCacheEntry::ResourceName(std::string const& project, + std::string const& bucket) { + return std::string(kStorageResourceNamePrefix) + + (project.empty() ? std::string("projects/_") : project) + "/buckets/" + + BucketMetadataCache::NormalizeBucketName(bucket); +} + BucketCacheEntry BucketCacheEntry::FromLocation( std::string id, std::string location, std::string const& location_type) { if (location_type == "multi-region" || location_type == "dual-region") { @@ -34,9 +41,16 @@ BucketCacheEntry BucketCacheEntry::FromLocation( BucketCacheEntry BucketCacheEntry::FromMetadata( storage::BucketMetadata const& m) { - return FromLocation( - "projects/" + std::to_string(m.project_number()) + "/buckets/" + m.name(), - m.location(), m.location_type()); + auto project = m.project_number() == 0 + ? std::string{} + : "projects/" + std::to_string(m.project_number()); + return FromLocation(ResourceName(project, m.name()), m.location(), + m.location_type()); +} + +BucketCacheEntry BucketCacheEntry::FromUnknownProject( + std::string const& bucket) { + return {ResourceName(std::string{}, bucket), "global"}; } std::string BucketMetadataCache::NormalizeBucketName( diff --git a/google/cloud/storage/internal/bucket_metadata_cache.h b/google/cloud/storage/internal/bucket_metadata_cache.h index d60a821a55df9..36a8e81c1a26d 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache.h +++ b/google/cloud/storage/internal/bucket_metadata_cache.h @@ -38,13 +38,33 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END namespace storage_internal { GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN +// The App Hub / Cloud Asset Inventory full resource name prefix for Cloud +// Storage resources. +inline constexpr char kStorageResourceNamePrefix[] = + "//storage.googleapis.com/"; + struct BucketCacheEntry { std::string id; std::string location; + /** + * Returns the full resource name for a bucket, i.e. + * `//storage.googleapis.com/{project}/buckets/{bucket}`. + * + * @param project the project in `projects/{project-id-or-number}` format. If + * empty, `projects/_` is used. + * @param bucket the bucket name, optionally in `projects/_/buckets/{bucket}` + * format. + */ + static std::string ResourceName(std::string const& project, + std::string const& bucket); + static BucketCacheEntry FromLocation(std::string id, std::string location, std::string const& location_type); static BucketCacheEntry FromMetadata(storage::BucketMetadata const& m); + // Creates an entry for a bucket whose project and location are unknown, + // e.g. when fetching its metadata fails with `kPermissionDenied`. + static BucketCacheEntry FromUnknownProject(std::string const& bucket); }; class BucketMetadataCache { diff --git a/google/cloud/storage/internal/bucket_metadata_cache_test.cc b/google/cloud/storage/internal/bucket_metadata_cache_test.cc index b5e6eb2ebe799..7325f6bdcec90 100644 --- a/google/cloud/storage/internal/bucket_metadata_cache_test.cc +++ b/google/cloud/storage/internal/bucket_metadata_cache_test.cc @@ -13,6 +13,7 @@ // limitations under the License. #include "google/cloud/storage/internal/bucket_metadata_cache.h" +#include "google/cloud/storage/bucket_metadata.h" #include namespace google { @@ -39,36 +40,71 @@ TEST(BucketMetadataCacheTest, NormalizeBucketName) { Eq("test-bucket")); } +TEST(BucketCacheEntryTest, ResourceName) { + EXPECT_THAT(BucketCacheEntry::ResourceName("projects/123", "test-bucket"), + Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); + EXPECT_THAT(BucketCacheEntry::ResourceName("projects/123", + "projects/_/buckets/test-bucket"), + Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); + EXPECT_THAT(BucketCacheEntry::ResourceName("", "test-bucket"), + Eq("//storage.googleapis.com/projects/_/buckets/test-bucket")); +} + +TEST(BucketCacheEntryTest, FromMetadata) { + auto entry = BucketCacheEntry::FromMetadata(storage::BucketMetadata() + .set_name("test-bucket") + .set_location("US-EAST1")); + // project_number() defaults to 0, which means "unknown". + EXPECT_THAT(entry.id, + Eq("//storage.googleapis.com/projects/_/buckets/test-bucket")); +} + +TEST(BucketCacheEntryTest, FromUnknownProject) { + auto entry = BucketCacheEntry::FromUnknownProject("test-bucket"); + EXPECT_THAT(entry.id, + Eq("//storage.googleapis.com/projects/_/buckets/test-bucket")); + EXPECT_THAT(entry.location, Eq("global")); +} + TEST(BucketMetadataCacheTest, HitAndMiss) { BucketMetadataCache cache(10); EXPECT_FALSE(cache.Get("test-bucket").has_value()); - BucketCacheEntry entry{"projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry{ + "//storage.googleapis.com/projects/123/buckets/test-bucket", + "us-central1"}; cache.Put("test-bucket", entry); auto res = cache.Get("test-bucket"); ASSERT_TRUE(res.has_value()); - EXPECT_THAT(res->id, Eq("projects/123/buckets/test-bucket")); + EXPECT_THAT(res->id, + Eq("//storage.googleapis.com/projects/123/buckets/test-bucket")); EXPECT_THAT(res->location, Eq("us-central1")); } TEST(BucketMetadataCacheTest, PutUpdatesExisting) { BucketMetadataCache cache(10); - BucketCacheEntry entry1{"projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry1{ + "//storage.googleapis.com/projects/123/buckets/test-bucket", + "us-central1"}; cache.Put("test-bucket", entry1); - BucketCacheEntry entry2{"projects/456/buckets/test-bucket", "global"}; + BucketCacheEntry entry2{ + "//storage.googleapis.com/projects/456/buckets/test-bucket", "global"}; cache.Put("test-bucket", entry2); auto res = cache.Get("test-bucket"); ASSERT_TRUE(res.has_value()); - EXPECT_THAT(res->id, Eq("projects/456/buckets/test-bucket")); + EXPECT_THAT(res->id, + Eq("//storage.googleapis.com/projects/456/buckets/test-bucket")); EXPECT_THAT(res->location, Eq("global")); } TEST(BucketMetadataCacheTest, InvalidateAndClear) { BucketMetadataCache cache(10); - BucketCacheEntry entry{"projects/123/buckets/test-bucket", "us-central1"}; + BucketCacheEntry entry{ + "//storage.googleapis.com/projects/123/buckets/test-bucket", + "us-central1"}; cache.Put("test-bucket", entry); EXPECT_TRUE(cache.Get("test-bucket").has_value()); diff --git a/google/cloud/storage/internal/tracing_connection.cc b/google/cloud/storage/internal/tracing_connection.cc index ca37f072453a7..68391cea6028a 100644 --- a/google/cloud/storage/internal/tracing_connection.cc +++ b/google/cloud/storage/internal/tracing_connection.cc @@ -69,18 +69,19 @@ void TracingConnection::MaybeTriggerBackgroundFetch( auto guard = ScopedFetch(cache_, bucket_name); auto current_options = google::cloud::internal::SaveCurrentOptions(); - runner()([impl = impl_, cache = cache_, bucket_name, current_options, - guard]() { - google::cloud::internal::OptionsSpan span(current_options); - storage::internal::GetBucketMetadataRequest request(bucket_name); - auto result = impl->GetBucketMetadata(request); - - if (result.ok()) { - cache->Put(bucket_name, BucketCacheEntry::FromMetadata(*result)); - } else if (result.status().code() == StatusCode::kPermissionDenied) { - cache->Put(bucket_name, {"projects/_/buckets/" + bucket_name, "global"}); - } - }); + runner()( + [impl = impl_, cache = cache_, bucket_name, current_options, guard]() { + google::cloud::internal::OptionsSpan span(current_options); + storage::internal::GetBucketMetadataRequest request(bucket_name); + auto result = impl->GetBucketMetadata(request); + + if (result.ok()) { + cache->Put(bucket_name, BucketCacheEntry::FromMetadata(*result)); + } else if (result.status().code() == StatusCode::kPermissionDenied) { + cache->Put(bucket_name, + BucketCacheEntry::FromUnknownProject(bucket_name)); + } + }); } void TracingConnection::EnrichSpan(opentelemetry::trace::Span& span, diff --git a/google/cloud/storage/internal/tracing_connection_test.cc b/google/cloud/storage/internal/tracing_connection_test.cc index 3cc75811ff639..a8f8eb7355848 100644 --- a/google/cloud/storage/internal/tracing_connection_test.cc +++ b/google/cloud/storage/internal/tracing_connection_test.cc @@ -158,7 +158,8 @@ TEST(TracingClientTest, CreateBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -210,7 +211,8 @@ TEST(TracingClientTest, GetBucketMetadataSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -263,7 +265,8 @@ TEST(TracingClientTest, BucketMetadataCacheSuccess) { SpanNamed("storage::Client::DeleteObject"), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -338,7 +341,8 @@ TEST(TracingClientTest, UpdateBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -388,7 +392,8 @@ TEST(TracingClientTest, PatchBucketSuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); } @@ -511,7 +516,8 @@ TEST(TracingClientTest, LockBucketRetentionPolicySuccess) { SpanWithStatus(opentelemetry::trace::StatusCode::kOk), SpanHasAttributes( OTelAttribute("gcp.resource.destination.id", - "projects/123456/buckets/test-bucket"), + "//storage.googleapis.com/projects/" + "123456/buckets/test-bucket"), OTelAttribute("gcp.resource.destination.location", "us-east1"))))); }