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

Fix: updating solution to work with Service Accounts #13

Merged
merged 19 commits into from
Jan 23, 2023
Merged

Conversation

tpryan
Copy link
Collaborator

@tpryan tpryan commented Nov 3, 2022

No description provided.

@tpryan tpryan requested a review from bharathkkb November 3, 2022 17:54
@@ -25,3 +25,8 @@ output "sqlservername" {
value = module.three_tier_app.sqlservername
description = "The name of the database that we randomly generated."
}

output "networkname" {
Copy link
Member

Choose a reason for hiding this comment

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

nit: let's camel case these network_name

main.tf Outdated
Comment on lines 193 to 196
redis_host = var.labels,
db_host = var.labels,
db_user = var.labels,
db_conn = var.labels,
Copy link
Member

Choose a reason for hiding this comment

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

Since these are non secrets, should we remove secret manager all together and just use env vars?

main.tf Outdated
display_name = "Service Account for Cloud Run"
}

# Handle Permissions
variable "run_roles_list" {
Copy link
Member

Choose a reason for hiding this comment

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

nit: lets move this to variables.tf

"roles/cloudsql.client",
]
}

resource "google_project_iam_member" "allrun" {
Copy link
Member

Choose a reason for hiding this comment

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

Rather than granting roles/secretmanager.secretAccessor could we grant granular secret level IAM so the SA does not have access to all secrets within the project? I have proposed we remove secret manager all together so please ignore if that is feasible?

main.tf Outdated
"run.googleapis.com",
"redis.googleapis.com",
]
}

resource "google_service_account" "runsa" {
project = var.project_id
account_id = "${var.deployment_name}-run-sa"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for removing the -run-sa prefix? I think it helps to understand it's the SA used by CR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wasn't intentional. Will update.

main.tf Outdated
module "network-safer-mysql-simple" {
source = "terraform-google-modules/network/google"
version = "~> 4.0"
resource "random_id" "name" {
Copy link
Member

Choose a reason for hiding this comment

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

Do we use this any where?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Removing.

}

module "private-service-access" {
source = "GoogleCloudPlatform/sql-db/google//modules/private_service_access"
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for switching the modules to resources?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. Was using a module for setting up networks that was tailored for mysql, not postgres, so I should replace that anyway.
  2. I wasn't saving myself a lot of effort by using them.
  3. I was troubleshooting the issue with destroy not working, and needed granular control over all of the parts to troubleshoot. (selectively commenting out bits of the infrastructure to see if they were leading to this not working)

@tpryan tpryan requested a review from a team as a code owner December 2, 2022 05:07
tpryan and others added 2 commits December 2, 2022 07:53
…16)

* chore: Add a test to verify multiple deployments in a single project

* bump timeout
@bharathkkb bharathkkb mentioned this pull request Jan 4, 2023
@tpryan tpryan merged commit ebd64c4 into main Jan 23, 2023
@tpryan tpryan deleted the sql-iam-auth branch January 23, 2023 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants