Skip to content

Conversation

@coryan
Copy link
Contributor

@coryan coryan commented Feb 26, 2022

If present, use the AuthorityOption to configure the Host: header
or the .set_authority() attribute in grpc::ClientContext. By
default, configure AuthorityOption to be storage.googleapis.com,
applications can override this when initializing the storage::Client.

There is already an issue open to add per-call Options that would
provide more fine-grained control of this field.

Fixes #3317. I am adding a node to #8164 because we will need to undo some of this work to support per-call options.


This change is Reviewable

If present, use the `AuthorityOption` to configure the `Host: ` header
or the `.set_authority()` attribute in `grpc::ClientContext`. By
default, configure `AuthorityOption` to be `storage.googleapis.com`,
applications can override this when initializing the `storage::Client`.

There is already an issue open to add per-call `Options` that would
provide more fine-grained control of this field.
@product-auto-label product-auto-label bot added the api: storage Issues related to the Cloud Storage API. label Feb 26, 2022
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 6b4bda895702aa08181522dc7fa50c91dacad6e2

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@codecov
Copy link

codecov bot commented Feb 26, 2022

Codecov Report

Merging #8462 (82ae158) into main (44e7fea) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #8462   +/-   ##
=======================================
  Coverage   93.88%   93.88%           
=======================================
  Files        1451     1451           
  Lines      125175   125231   +56     
=======================================
+ Hits       117515   117569   +54     
- Misses       7660     7662    +2     
Impacted Files Coverage Δ
google/cloud/storage/internal/curl_client.cc 98.22% <100.00%> (+<0.01%) ⬆️
google/cloud/storage/internal/curl_client_test.cc 99.44% <100.00%> (+<0.01%) ⬆️
google/cloud/storage/internal/grpc_client.cc 88.33% <100.00%> (+0.83%) ⬆️
google/cloud/storage/internal/grpc_client_test.cc 99.81% <100.00%> (+<0.01%) ⬆️
google/cloud/internal/async_rpc_details.h 94.44% <0.00%> (-5.56%) ⬇️
google/cloud/pubsub/subscriber_connection_test.cc 97.22% <0.00%> (-0.70%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 44e7fea...82ae158. Read the comment docs.

@coryan coryan marked this pull request as ready for review February 26, 2022 18:16
@coryan coryan requested a review from a team as a code owner February 26, 2022 18:16
@coryan coryan enabled auto-merge (squash) February 26, 2022 18:16
Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @coryan)


google/cloud/storage/internal/curl_client.cc, line 117 at r1 (raw file):

  // or their own proxy, and need to provide the target's service host.
  if (!options.get<AuthorityOption>().empty()) {
    return absl::StrCat("Host: ", options.get<AuthorityOption>());

Consider:

if (options.has<AuthorityOption>()) {
  auto const auth = options.get<AuthorityOption>();
  if (!auth.empty()) return absl::StrCat("Host: ", auth);
}

google/cloud/storage/internal/curl_client.cc, line 119 at r1 (raw file):

    return absl::StrCat("Host: ", options.get<AuthorityOption>());
  }
  auto const& endpoint = options.get<RestEndpointOption>();

Hmm... do you think we should add a note here saying that AuthorityOption takes precedence over storage::RestEndpointOption ?

/// Configure the REST endpoint for the GCS client library.


google/cloud/storage/internal/grpc_client_test.cc, line 354 at r1 (raw file):

  EXPECT_CALL(*mock, WriteObject)
      .WillOnce([this](std::unique_ptr<grpc::ClientContext> context) {
        auto metadata = GetMetadata(*context);

Check AuthorityOption here?

EXPECT_EQ(CurrentOptions().get<AuthorityOption>(), kAuthority);

Copy link
Contributor Author

@coryan coryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 2 of 5 files reviewed, all discussions resolved (waiting on @dbolduc)


google/cloud/storage/internal/curl_client.cc, line 117 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

Consider:

if (options.has<AuthorityOption>) {
  auto const auth = options.get<AuthorityOption>();
  if (!auth.empty()) return absl::StrCat("Host: ", auth);
}

Done. Though a slightly different implementation, note that not options.has<AuthorityOption>() implies options.get<AuthorityOption>().empty().


google/cloud/storage/internal/curl_client.cc, line 119 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

Hmm... do you think we should add a note here saying that AuthorityOption takes precedence over storage::RestEndpointOption ?

/// Configure the REST endpoint for the GCS client library.

Done.


google/cloud/storage/internal/grpc_client_test.cc, line 354 at r1 (raw file):

Previously, dbolduc (Darren Bolduc) wrote…

Check AuthorityOption here?

EXPECT_EQ(CurrentOptions().get<AuthorityOption>(), kAuthority);

Done.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: b47b749d7f6e806232e6b1718d337c5b2a22353c

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@dbolduc dbolduc disabled auto-merge February 28, 2022 16:56
@dbolduc
Copy link
Member

dbolduc commented Feb 28, 2022

Done. Though a slightly different implementation, note that not options.has<AuthorityOption>() implies options.get<AuthorityOption>().empty().

Part of the motivation for checking .has<>() was to avoid constructing the empty string once when the option is not present. But maybe that is not worth the trouble.

@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 82ae158248a86b60d1d5a29418290b9c999f2508

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

@coryan coryan merged commit d4f3ebf into googleapis:main Feb 28, 2022
@coryan coryan deleted the feat-storage-authority-option branch February 28, 2022 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Set the ClientContext authority to support PSC and VPC-SC

3 participants