fix: set ClientOption for quota project when possible

[ericnorris]

Hey @rileykarson, I wanted to get your opinion on this proposed fix for our particular use-case. I think that a fix that addresses user_project_override without an associated billing_project is achievable, but would require more work, and we've ran into an additional two issues that makes it more pressing for us to have this solved for at least when user_project_override and billing_project is set.

If this particular implementation isn't tenable, that's fine, but I'd really like to see something along these lines so that we can move forward without needing to enable 50+ APIs in the "host project" for our terraform service accounts. I'm also happy to make any changes requested to this PR.

Thanks!

[rileykarson]

My concern with this approach is that the older / presumably more-in-use method of associating a quota account (direct project) can only be determined at apply time, and won't be supported by this method.

However, since billing_project wins against direct projects it probably won't matter in those cases. There are a few resources (google_cloud_asset_*) which have resource-level billing_project fields, and that should have precedence over the provider level one. Can you confirm what headers get sent for one of them when you've configured a billing project in both places?

[ericnorris]

My concern with this approach is that the older / presumably more-in-use method of associating a quota account (direct project) can only be determined at apply time, and won't be supported by this method.

To confirm I'm on the same page: you're referring to user_project_override without billing_project, correct? If so, I understand, but I don't think that this solution precludes solving for that at a later point.

However, since billing_project wins against direct projects it probably won't matter in those cases.

I agree, which is why I think that this solution doesn't prevent a later solution that solves for when there is only the user_project_override attribute. I think that would necessarily be a more complex solution (e.g. some resources take an ID and not a project, which means we'd need to parse the project out of the ID, if I understand correctly), and I'd be hesitant to wait for it considering that we've run into this issue a couple more times already.

I do think that any future solution would need to be at least aware of this solution, as to not override the header, but that seems easy enough to me.

There are a few resources (google_cloud_asset_*) which have resource-level billing_project fields, and that should have precedence over the provider level one. Can you confirm what headers get sent for one of them when you've configured a billing project in both places?

I'll try! It might be hard considering I am not familiar with those resources, but at a minimum I could probably dump the generated request to confirm the correct header is being sent.

[ericnorris]

Okay @rileykarson, this was quite a journey but I'm glad I looked into this more. Unfortunately because the logging transport is set up after the Google API client transport [1], the DEBUG logs won't show the headers, even though they're being set.

There doesn't appear to be a way to wrap the logging transport with the Google API client transport, so I opted to use the existing headerTransport instead and set the header manually so that it's visible in the DEBUG logs.

With the following terraform configuration:

provider "google" {
  user_project_override = true
  billing_project       = "billing-project-set-by-provider"
}

resource "google_logging_project_sink" "my-sink" {
  name = "my-pubsub-instance-sink"

  # Can export to pubsub, cloud storage, or bigquery
  destination = "pubsub.googleapis.com/projects/my-project/topics/instance-activity"

  # Log all WARN or higher severity messages relating to instances
  filter = "resource.type = gce_instance AND severity >= WARNING"

  # Use a unique writer (creates a unique service account used for writing)
  unique_writer_identity = true

  project = "project-set-by-resource"
}

resource "google_cloud_asset_organization_feed" "test" {
  billing_project = "billing-project-set-by-resource"
  org_id          = "123456789"
  feed_id         = "network-updates"
  content_type    = "RESOURCE"

  asset_types = [
    "compute.googleapis.com/Subnetwork",
    "compute.googleapis.com/Network",
  ]

  feed_output_config {
    pubsub_destination {
      topic = "some-topic"
    }
  }
}

...you will see the correct headers being set in the DEBUG logs:

---[ REQUEST ]---------------------------------------
POST /v2/projects/project-set-by-resource/sinks?alt=json&prettyPrint=false&uniqueWriterIdentity=true HTTP/1.1
Host: logging.googleapis.com
User-Agent: google-api-go-client/0.5 Terraform/1.1.0-alpha20210728 (+https://www.terraform.io) Terraform-Plugin-SDK/2.5.0 terraform-provider-google/dev
Content-Length: 187
Content-Type: application/json
X-Goog-Api-Client: gl-go/1.15.3 gdcl/20210606
X-Goog-User-Project: billing-project-set-by-provider
Accept-Encoding: gzip

{
 "destination": "pubsub.googleapis.com/projects/my-project/topics/instance-activity",
 "filter": "resource.type = gce_instance AND severity \u003e= WARNING",
 "name": "my-pubsub-instance-sink"
}

-----------------------------------------------------: timestamp=2021-07-29T18:53:38.890-0400
2021-07-29T18:53:38.890-0400 [INFO]  provider.terraform-provider-google_v5.0.0: 2021/07/29 18:53:38 [DEBUG] Google API Request Details:
---[ REQUEST ]---------------------------------------
POST /v1/organizations/123456789/feeds?alt=json&feedId=network-updates HTTP/1.1
Host: cloudasset.googleapis.com
User-Agent: Terraform/1.1.0-alpha20210728 (+https://www.terraform.io) Terraform-Plugin-SDK/2.5.0 terraform-provider-google/dev
Content-Length: 184
Content-Type: application/json
X-Goog-User-Project: billing-project-set-by-resource
Accept-Encoding: gzip

{
 "feed": {
  "assetTypes": [
   "compute.googleapis.com/Subnetwork",
   "compute.googleapis.com/Network"
  ],
  "contentType": "RESOURCE",
  "feedOutputConfig": {
   "pubsubDestination": {
    "topic": "some-topic"
   }
  }
 }
}
-----------------------------------------------------: timestamp=2021-07-29T18:53:38.890-0400

I noted it in the commit message, but if googleapis/google-cloud-go#1962 is ever merged, switching the ordering of the transports and then going back to using a ClientOption would be preferable.

[1]

terraform-provider-google/google/config.go

Lines 362 to 374 in 0e315b0

	// 1. MTLS TRANSPORT/CLIENT - sets up proper auth headers
	client, _, err := transport.NewHTTPClient(cleanCtx, option.WithTokenSource(tokenSource))
	if err != nil {
	return err
	}
	// Userinfo is fetched before request logging is enabled to reduce additional noise.
	err = c.logGoogleIdentities()
	if err != nil {
	return err
	}
	// 2. Logging Transport - ensure we log HTTP requests to GCP APIs.
	loggingTransport := logging.NewTransport("Google", client.Transport)

[rileykarson]

Awesome, thanks for exercising that path and making those changes! I'm sufficiently convinced this is backwards compatible enough to release in a minor version, LGTM.

I'm going to upstream the change to https://github.com/GoogleCloudPlatform/magic-modules so this applies to google-beta as well, and retouch some related documentation while I'm at it. I can't do that immediately- my Mondays are loaded- but should be able to get to it later today / tomorrow. Ping here if there's been no activity by EOD Wednesday.

There's no other action needed from you, I'll merge this PR simultaneously w/ the upstream PR or supersede it, just depending how the change shakes out.

[ericnorris]

Sounds good to me! Appreciate you looking into this, and feel free to do whatever you think is necessary to merge the change.

[rileykarson]

Superseded w/ GoogleCloudPlatform/magic-modules#5086, given there's a merge conflict from other changes in this branch.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.