-
Notifications
You must be signed in to change notification settings - Fork 545
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
Creating shared_vpc_access submodule #434
Creating shared_vpc_access submodule #434
Conversation
/****************************************** | ||
Setting API service accounts for shared VPC | ||
*****************************************/ | ||
module "shared_vpc_access" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we should also modify the output for this module to
output "project_id" {
description = "If provided, the project uses the given project ID. Mutually exclusive with random_project_id being true."
value = module.shared_vpc_access.project_id
}
this way when this module is used, the project-id is returned only after module.shared_vpc_access
is completed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@marko7460 overall this lgtm. @morgante I believe this is a breaking change and we should have an upgrade guide as well? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one nit.
This will be a breaking change and we'll need a short migration guide (no script though, since recreating IAM resources is safe).
/****************************************** | ||
Setting API service accounts for shared VPC | ||
*****************************************/ | ||
module "shared_vpc_access" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
I'll add a migration guide to |
This submodule is used to control access of the API service account on the host project. Service Accounts are given appropriate permissions based on the list of active apis.
compute.networkUser role granted to dataproc service account for dataproc on shared VPC subnets | ||
See: https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/network#creating_a_cluster_that_uses_a_vpc_network_in_another_project | ||
*****************************************/ | ||
resource "google_project_iam_member" "dataproc_shared_vpc_network_user" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marko7460 Shouldn't this be google_compute_subnetwork_iam_member
to avoid granting access to all subnets in the host?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'd need to support both paths, so adding another option like https://github.com/terraform-google-modules/terraform-google-project-factory/pull/434/files#diff-25530a290a058747ad4e0e2f230fc886R39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The feature was coded based on https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/network#creating_a_cluster_that_uses_a_vpc_network_in_another_project. I did try giving permissions only on the subnet level and I was able to create a cluster only by specifying permissions on the subnet level. I can only confirm that those permissions are enough to create a cluster but i can't confirm if running and operating the cluster will not be an issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@morgante What do you think about a future enhancement that breaks this one module into one module per API service, for the following reasons:
Consider two teams, red and blue, each wanting GKE and Dataproc. There are 4 subnets, {red,blue}-{dataproc,gke}. The purpose of the subnets are to prevent someone spinning up a cluster from impacting running clusters by exhausting IP space.
To support this use case a per-service module would take as input a list of subnets. For each subnet it would manage a google_compute_subnetwork_iam_member
resource with the service agent as a member and network user as a role.
Thoughts?
This submodule is used to control access of the API service accounts
on the host project. Service Accounts are given appropriate permissions
based on the list of active apis. The module is meant to be extended in
the future with more support for API service accounts.
Note: I have set
skip_gcloud_download = "true"
inexamples/shared_vpc/main.tf
to speed up integration testing.