Skip to content

attempt 1#84

Open
gmechali wants to merge 1 commit into
datacommonsorg:mainfrom
clincoln8:depends
Open

attempt 1#84
gmechali wants to merge 1 commit into
datacommonsorg:mainfrom
clincoln8:depends

Conversation

@gmechali
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a time_sleep resource to delay infrastructure provisioning until Google Cloud APIs are fully enabled, passing this dependency down to submodules. It also centralizes several IAM member resources from individual modules into the main stack module. The review feedback highlights critical improvements: first, restricting the roles/bigquery.dataEditor role to the dataset level rather than the project level to respect the principle of least privilege; second, adding a dependency on the foundation delay to the BigQuery service account data source to prevent plan-time evaluation failures; and third, using the one() function with splat syntax instead of direct index [0] access for conditional modules to avoid out-of-bounds evaluation errors when those modules are disabled.

Comment on lines +396 to +404
resource "google_project_iam_member" "helper_bq_roles" {
for_each = toset(var.ingestion_config.enable_ingestion && var.ingestion_config.workflow_enable_bigquery_postprocessing ? [
"roles/bigquery.dataEditor",
"roles/bigquery.jobUser"
] : [])
project = var.global.project_id
role = each.value
member = "serviceAccount:${module.ingestion_helper_service.service_account_email}"
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The roles/bigquery.dataEditor role is granted at the project level for the ingestion helper service account. To adhere to the principle of least privilege, BigQuery data editor roles should be granted at the dataset level rather than the project level. Project-level access should be restricted to roles like roles/bigquery.jobUser which are required to run jobs in the project.\n\nPlease remove roles/bigquery.dataEditor from the project-level IAM binding and instead grant it at the dataset level using google_bigquery_dataset_iam_member.

resource "google_project_iam_member" "helper_bq_roles" {
  for_each = toset(var.ingestion_config.enable_ingestion && var.ingestion_config.workflow_enable_bigquery_postprocessing ? [
    "roles/bigquery.jobUser"
  ] : [])
  project = var.global.project_id
  role    = each.value
  member  = "serviceAccount:${module.ingestion_helper_service.service_account_email}"
}
References
  1. Grant BigQuery roles, such as 'roles/bigquery.dataEditor', at the dataset level rather than the project level to adhere to the principle of least privilege.

Comment on lines +361 to +363
data "google_bigquery_default_service_account" "bq_sa" {
project = var.global.project_id
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The google_bigquery_default_service_account data source is evaluated during the Terraform plan phase. If the BigQuery API (bigquery.googleapis.com) is not already enabled in the target GCP project (which is common during initial bootstrapping), this data source will fail to evaluate, blocking the plan.\n\nTo prevent this, add an explicit dependency on var.foundation_dependency (which waits for the APIs to be enabled) to defer the data source evaluation until the apply phase.

data "google_bigquery_default_service_account" "bq_sa" {
  project = var.global.project_id

  depends_on = [var.foundation_dependency]
}

]), var.spanner_config.enable ? [] : ["roles/spanner.databaseUser"]) : []

project = var.global.project_id
member = "serviceAccount:${module.datacommons_services[0].service_account_email}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Referencing module.datacommons_services[0] directly can cause an evaluation error (index out of bounds / empty tuple) during plan or apply if var.datacommons_services_config.enable is false (meaning the module's count is 0).\n\nUsing the one() function with the splat operator [*] is a safer and more idiomatic way to handle conditional modules in Terraform, as it safely returns null if the list is empty.

  member  = "serviceAccount:${one(module.datacommons_services[*].service_account_email)}"

count = var.ingestion_config.enable_ingestion ? 1 : 0
project = var.global.project_id
role = "roles/workflows.invoker"
member = "serviceAccount:${module.ingestion_preprocessing_job[0].service_account_email}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Referencing module.ingestion_preprocessing_job[0] directly can cause an evaluation error (index out of bounds / empty tuple) during plan or apply if var.ingestion_config.enable_ingestion is false (meaning the module's count is 0).\n\nUsing the one() function with the splat operator [*] is a safer and more idiomatic way to handle conditional modules in Terraform, as it safely returns null if the list is empty.

  member  = "serviceAccount:${one(module.ingestion_preprocessing_job[*].service_account_email)}"

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