search

hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.33k stars 1.73k forks source link

Add `user_project_override`, `billing_project` to IAM resources, logging sinks #9437

Open ericnorris opened 3 years ago

ericnorris commented 3 years ago

Community Note

Terraform Version

N/A, but v1.0.0

Affected Resource(s)

Terraform Configuration Files

resource "google_logging_project_sink" "sink" {
  name                   = "gce_instance_deletion"
  filter                 = <<EOF
logName = projects/${var.project}/logs/cloudaudit.googleapis.com%2Factivity AND
 protoPayload.methodName = v1.compute.instances.delete
EOF
  project = var.project
  destination            = "pubsub.googleapis.com/projects/${var.destination_project}/topics/gce_instance_deletion"
  unique_writer_identity = true
}

resource "google_pubsub_topic_iam_member" "pubsub_sink_member" {
  project = var.destination_project
  topic   = "gce_instance_deletion"
  role    = "roles/pubsub.publisher"
  member  = google_logging_project_sink.sink.writer_identity
}

Debug Output

Not available to me; I could reproduce this issue if necessary but believe I have enough evidence without the debug output.

Panic Output

N/A

Expected Behavior

All API calls should use the billing_project (which is set to var.project in the case of the terraform code example above) for the API precondition check and for quota.

Actual Behavior

It used the project the service account lives in for the google_logging_project_sink resource, and the project of the pubsub topic for google_pubsub_topic_iam_member.

Due to this, we saw a 403 due to logging.googleapis.com not being enabled in our host project (see important factoids) and then another 403 due to not having serviceusage.services.use permission in the destination project (see snippet).

Steps to Reproduce

  1. Set user_project_override to true and billing_project to a project that is separate from the service account, and separate from the destination project in the above snippet
  2. terraform apply

Important Factoids

We are using a single service account per GCP project, scoped to only that project, for making terraform configuration changes in the GCP project. There is a single "host" project that contains all of the service accounts.

Doing some digging, the logging_sink issue appears to be due to https://github.com/hashicorp/terraform-provider-google/blob/3ccb36914a5221127c38ca07924ef7e40dff85f9/google/resource_logging_project_sink.go#L58, which is not setting an option for the user project at all. This appears to be a handwritten resource, which per #7660 may be why it is not supported.

The issue with the google_pubsub_topic_iam_member appears to be due to https://github.com/hashicorp/terraform-provider-google/blob/3ccb36914a5221127c38ca07924ef7e40dff85f9/google/iam_pubsub_topic.go#L134, which is passing in the project of the IAM binding, and not checking for the billing_project parameter, which things like https://github.com/hashicorp/terraform-provider-google/blob/e2e533f83c388fecbf51ae8ca541326b152cede0/google/resource_notebooks_location.go#L91-L98 does, for example.

References

Closing Comment

While I pointed out these two specific resources, how far away is the provider from ensuring that all resources respect the user_project_override and billing_project parameters? I appreciate that they've worked so far, as it prevents a class of issues that we've seen where users get confused about needing to enable an API in the project that their service account lives in, vs. the project that they're trying to use the API in. Having it work consistently for all APIs (or as many as conceivably possible) would improve the user experience of our developers writing terraform configuration.

Is it possible for: https://github.com/hashicorp/terraform-provider-google/blob/8446514f04154092aa5a262788163eabb8815f05/google/config.go#L441-L453 (and all other clients in this file) to add a parameter https://pkg.go.dev/google.golang.org/api/option#WithQuotaProject if user_project_override and billing_project is set, at least for things that use handwritten clients?

It also appears that autogenerated clients don't always use the user_project_override and billing_project parameters (e.g. the google_pubsub_topic_iam_member resource), is there a way to ensure this is the case going forward?

Thanks!

b/359705188

edwardmedia commented 3 years ago

@ericnorris help me to understand what the problem is. You provided project in both resources (below). Did you see they do not use the project you provided? If not, can you post the debug log along with values for these two variables?

resource "google_logging_project_sink" "sink" {
...
  project = var.project
...
}

resource "google_pubsub_topic_iam_member" "pubsub_sink_member" {
  project = var.destination_project
...
}
ericnorris commented 3 years ago

@edwardmedia I do not have the debug logs, but I don't think that I need them, hopefully I can explain the issue here.

We have three projects:

In the "Actual Behavior", I noted that:

I noted in the "Important Factoids" that, if you look at the code, it's obvious that it's not sending the billing_project for either resource. In the case of the google_pubsub_topic_iam_member resource it is following the user_project_override parameter, but it's still not using the billing_project parameter.

If either of the resources were using the billing_project parameter, the error would have been a 403 "API not enabled in ZZZ project", where ZZZ is project-b, but this was not the case.

While I would like for both of these resources to have the code incorporated for them to respect the billing_project parameter, ideally all, or nearly all, resources would support this. The linked issue #7660 is an example of other things that don't support this parameter, so it's clear that there are more than just these resources missing this functionality. What would it take to make all resources / data sources respect the billing_project parameter?

rileykarson commented 3 years ago

@edwardmedia: billing_project is a provider-level field that interacts with user_project_override: https://registry.terraform.io/providers/hashicorp/google/latest/docs/guides/provider_reference#billing_project

Our documentation for billing_project is a little over-optimistic, it's implemented for the majority of full resources (those generated by Magic Modules) but isn't for most handwritten resources (many of which don't support user project overrides at all), or for IAM resources.

It's definitely a bug that billing_project isn't supported for generated IAM resources, and that'll be fairly simple to fix. Adding support for overrides+billing project to all handwritten resources is an expensive backfill and many don't need it or it's of limited value.

@ericnorris: Do you have a specific list of resources you're interested in having support this? Sorry to have to ask you to produce one, but even if we're unable to schedule a general backfill right now we can spot fix a few if it'll help!

ericnorris commented 3 years ago

Ah, apologies @edwardmedia, yes, as @rileykarson said this is a provider-level configuration setting that we're using in order to override the project used for quota and API precondition checks.

@rileykarson hah, I definitely assumed going in to this that billing_project would solve all of our problems, so I was blind to the over-optimism :). Unfortunately I don't have a specific list of resources, and my concern is that Etsy somehow finds a way to use every possible feature in the google provider, so it'd essentially be all or most of them at some point. That said, for now IAM resources and google_logging_*_sink would be a good start, and maybe I could try to find a list of every resource that Etsy uses so far.

Finally, was my suggestion in my "Closing Comment" tenable? Would it be "easy" enough to ensure that in every client constructor it checks for the user_project_override and billing_project config option, and then passes that into the Google go library for the client?

rileykarson commented 3 years ago

It's a useful bit of configuration! Sorry that coverage isn't all the way there, and getting coverage on IAM + sinks should be doable :+1: (I'll also file an enhancement followup to get the backfill done generally, and note there that anyone needing specific resources can file issues for them)

Your closing comment would work for billing_project at the provider level, but wouldn't work for direct/indirect projects that are determined based on the resource in question or for the resource-level billing_project field a couple of resources have got. It's worth considering using, honestly I wasn't aware that client option existed!

ericnorris commented 3 years ago

Thanks @rileykarson! Not a problem, I appreciate that you all even have this feature in the provider to begin with.

Good point, I suppose the client option wouldn't work for people that are only using user_project_override and not setting billing_project at the provider level, and the other situations you described. I, of course, would be fine with that, since I am setting the billing_project at the provider level via an environment variable, so it's not up to the user. I'm a little biased towards at least trying this out, since it'd solve my problem specifically, but I'm open to hearing that you'd like to think about a more general fix.

In the meantime, I'm +1 to covering the resources mentioned in this issue and having a backfill ticket open.

karlkfi commented 3 years ago

This issue now affects Config Controller users trying to use the config-control namespace to manage resources with Config Connector, which delegates resource management to terraform-provider-google.

The config-control namespace is configured to use a Google-managed service account in another project. So it uses UserProjectOverride.

Looking through magic-modules, I see hundreds of client requests that don't pass through the UserProjectOverride header.

I suspect that instead of trying to patch each resource individually we should instead bake the header into the service clients for all calls.

We probably also need some end to end tests for UserProjectOverride, because even some of the resources that ostensibly support UserProjectOverride don't pass it through for all calls, especially when using async operations like in https://github.com/hashicorp/terraform-provider-google/issues/9368

rileykarson commented 3 years ago

Note: general discussion should go in https://github.com/hashicorp/terraform-provider-google/issues/9454, I've retitled this issue to cover what will actually close it

ScottSuarez commented 2 years ago

Relabeling this as enhancement to get triaged through normal process

wyardley commented 2 years ago

Seeing similar behavior; closed my duplicate bug report.

rileykarson commented 2 years ago

(just removing a stale assignment on this)

ggtisc commented 2 months ago

The user is proposing all API calls should use the billing_project for the API precondition check and for quota. So far this is optional and needs to be evaluated if it is optimal to implement it

roaks3 commented 1 month ago

Per Riley's comment:

It's definitely a bug that billing_project isn't supported for generated IAM resources, and that'll be fairly simple to fix

The IAM piece of this likely belongs to the Terraform team, not specific service teams.

(however, logging sinks should still be handled by the service team)