Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support service_identity #448

Closed
umairidris opened this issue Aug 26, 2020 · 18 comments · Fixed by #450
Closed

Support service_identity #448

umairidris opened this issue Aug 26, 2020 · 18 comments · Fixed by #450
Assignees
Labels
enhancement New feature or request

Comments

@umairidris
Copy link
Contributor

Googled managed API service accounts are being moved to a just-in-time activation rather than on API enablement. This breaks many Terraform based workflows which create projects, enable APIs and grants additional IAM permissions to them.

A new google_project_service_identity resource is being created which can force create the SA (see hashicorp/terraform-provider-google#7020).

I believe the best place to call this resource is project factory. Would you accept a contribution to add field activate_service_identitities alongisde activate_apis to this module? This will in turn create the identities for the listed APIs.

We could go a step further and automatically append activate_service_identities to activate_apis. I can't think of a case where you would want to create an API SA but not enable the API itself.

@umairidris umairidris added the enhancement New feature or request label Aug 26, 2020
@umairidris umairidris self-assigned this Aug 26, 2020
@bharathkkb
Copy link
Member

+1 I have seen this start to break some workflows. Automatically appending activate_service_identities to activate_apis makes sense to me.

@morgante
Copy link
Contributor

This makes sense to me too.

Question: is there a workflow where you would want to activate APIs but not activate service identities? I'm wondering if we should automatically activate all the identities for listed activate_apis.

@umairidris
Copy link
Contributor Author

In most cases you should not need to call the service identity API and the default behaviour is sufficient.

The only case I have seen is when you need to grant the SA additional permissions. One example is in order to use healthcare store export to BQ, we need to grant bigquery.jobUser on the SA (can't be granted on the BQ dataset as this is a project level permission). Otherwise for regular operations the JIT behaviour should be fine.

We can create the SA for all the APIs being activated, but I think it may create a lot of unncessary SAs. I am happy to implement either way.

@imrannayer
Copy link
Contributor

This is definitely breaking our own modules where we were granting permission to composer service account on shared vpc project. Composer service accounts are not created when enabling service anymore.

@imrannayer
Copy link
Contributor

TPU service accounts are also not part of enabling service.

@morgante
Copy link
Contributor

I'm in favor of creating the SA for all APIs activated, since it matches historical behavior and I don't necessarily see immediate downsides.

@imrannayer
Copy link
Contributor

You can also add an extra flag which user can set to false if they dont want to create service account. Default value can be true to keep historical behavior.

@morgante
Copy link
Contributor

You can also add an extra flag which user can set to false if they dont want to create service account. Default value can be true to keep historical behavior.

Good idea, let's include this.

@umairidris
Copy link
Contributor Author

I synced with the eng working on this and they have advised not to automatically call this for all APIs and make it explicit only when needed.

cc @xdqyzy

@morgante
Copy link
Contributor

What's the issue with calling it for all APIs? I do feel like it's rather verbose/unnecessary to list APIs twice.

@ghost
Copy link

ghost commented Aug 27, 2020

  • not all the APIs have service identities defined.
  • activate_service_identities should be used only for services doing cross project access in the following pattern. For operations within the same project, we should expect the service identifies provisioned automatically by API producers.
    1. create project A,
    2. enable API X in project A,
    3. provision service identity for the API X in project A,
    4. grant service identity for (API X, project A) to resource (e.g. bigquery, Cloud Storage) in project B,
    5. create service instance in project A with references to resources in project B.
  • activate_service_identities may cause the automatic service agent role not granted on the same project. Some producers may only perform the action based on the existence of the identity.
  • automatic provisioning may cause extra resources being created but never used, and clients may go over the default quota in-place.

@umairidris
Copy link
Contributor Author

umairidris commented Aug 27, 2020

I just discovered point 3 today while doing my testing.

In order to fully reproduce the initial behaviour we actually need two resources:

  1. the service identity to force create the SA
  2. project iam member to grant the healthcare.serviceAgent role in addition to any extra roles (e.g. bigquery.jobUser).

@morgante we could reduce duplication by appending activate_service_identities automatically to activate_apis.

@morgante
Copy link
Contributor

not all the APIs have service identities defined.

That seems like something we could handle with a list.

activate_service_identities should be used only for services doing cross project access in the following pattern. For operations within the same project, we should expect the service identifies provisioned automatically by API producers.

I'm confused why this would only apply for cross-project access. If I want to grant additional roles to the API (even in my same project) don't I need to create the service identity?

activate_service_identities may cause the automatic service agent role not granted on the same project. Some producers may only perform the action based on the existence of the identity.

This seems like a bug we'd have to deal with either way?

@morgante we could reduce duplication by appending activate_service_identities automatically to activate_apis.

Alright I guess that's a sensible approach though I do still wonder why we broke existing users / are forcing them to add this additional variable.

@umairidris
Copy link
Contributor Author

The response from the generateServiceIdentity API also contains the service account identifier, so we could output that through the provider and add a helper to grant roles in factory too

e.g.

activate_service_identities [{
  service = "healthcare.googleapis.com"
  roles   = [
    "roles/healthcare.serviceAgent",
    "roles/bigquery.jobUser",
  ]
}]

I will look into that to make things a bit less painful. I'll let Danny answer the rest of the questions.

@umairidris
Copy link
Contributor Author

Exposing email field in GoogleCloudPlatform/magic-modules#3924.

@billyfoss
Copy link

This is especially important in Shared VPC environments with managed services like data proc and composer being created in service projects.
The team managing the host project needs to grant host project permissions to the service account in the service project when the API is enabled. The team actually creating the dataproc or cloud composer cluster will not have permission to grant permissions on the host project, so the resource creation will fail.

@ghost
Copy link

ghost commented Aug 30, 2020

The change has the following benefits to service ecosystem.

  • reduce reliability risk: most service identities provisioned at activation time are unused. Activation time provisioning adds unnecessary load and reliability concern to identity global systems.
  • improve long-term user experience: we want to support folder or organization activation or require no activation for using services.

Service identities would be created when the service is used for the first time. The change should be transparent to most use cases where services and resources are used within the same project. For cross project access features such as pubsub CMEK or shared VPC case, we will make it transparent on Cloud Console UI. For users using deployment scripts for these use cases, we are sorry about the change and thanks umairidris@ to fill the Terraform feature gap. We are trying our best to notify potential affected users, but we might also miss you due to log retention or low deployment activity. Let Cloud Support know and we can offer exemption for a period of time.

activate_service_identities should only be used when necessary. For example, Service Networking service identities, also for shared VPC case [1], are provisioned when the API is used for the first time automatically. We don't expect users to grant service agent role to host project policy themselves. So for affected scripts, we can simply try to remove the additional granting logic and API is enabled on both host and service project.

For users that change the default service agent role to other permission set on the same project, we would like to hear the use case and requirements. But we can either alter the role after the deployment (automatic provisioning for the first usage) or use this activate_service_identities feature.

[1]
https://cloud.google.com/sql/docs/mysql/private-ip#api_and_iam_requirements

@ericnorris
Copy link

@xdqyzy apologies for commenting on this closed issue; feel free to redirect me if there is a better place that I could leave this feedback.

One concern I did not see outlined in this PR is how this interacts with the google_project_iam_policy resource (here). A pattern I have used is to codify the entirety of a project's permissions using that resource, and currently this depends on the service accounts for an API being created when the API is activated.

As a (rough) example:

resource "google_project_service" "containerregistry" {
  project = google_project.project.project_id
  service = "containerregistry.iam.gserviceaccount.com"
}

data "google_iam_policy" "project" {
  binding {
    role = "roles/editor"

    members = [
      "serviceAccount:service-${google_project.project.number}@containerregistry.iam.gserviceaccount.com"
    ]
  }

  # ...other bindings here
}

resource "google_project_iam_policy" "project" {
  project     = module.self.google_project.project_id
  policy_data = data.google_iam_policy.project.policy_data

  depends_on = ["google_project_service.containerregistry"]
}

This works by setting the IAM policy after the API is enabled. What's nice about this is that there is a centralized configuration for all roles within the project. If, for example, a service identity's role was removed by mistake, running terraform apply would re-grant it the role it was supposed to have.

How can this behavior be preserved going forward? If I understand correctly you'd need to maintain a list of all the APIs that have service identities defined and take the intersection of that and the APIs that you want enabled; this is perhaps a burden that would be better solved upstream than by someone writing terraform themselves. Perhaps there could be an API that lists APIs that support service identities?

To put it another way: is there an easy way for someone consuming this module to know what APIs they should pass in to activate_service_identitities?

Let me know if there is anything I can clarify, or if there is another place you'd like me to raise this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants