Skip to content

clean(infra/dcp): remove obsolete dcp environment variables and redundant embeddings flags - #194

Open
clincoln8 wants to merge 1 commit into
datacommonsorg:mainfrom
clincoln8:cleanup-dcp-env-vars
Open

clean(infra/dcp): remove obsolete dcp environment variables and redundant embeddings flags#194
clincoln8 wants to merge 1 commit into
datacommonsorg:mainfrom
clincoln8:cleanup-dcp-env-vars

Conversation

@clincoln8

Copy link
Copy Markdown
Contributor

Overview

Removes obsolete environment variables and redundant Terraform variables across DCP infrastructure (infra/dcp) and integration test configurations, aligning with recent cdc_services cleanup in datcom-website.

Changes

  • Datacommons Services Module: Removed obsolete environment variables RESOLVE_WITH_SPANNER_EMBEDDINGS, ENABLE_UNIQUE_HISTORY_RECORDS, and USE_SPANNER_KEY_VALUE_STORE from the Cloud Run container definition. Simplified IAM role condition for roles/aiplatform.user to var.use_spanner.
  • Ingestion Helper Module: Removed temporary migration environment variable ENABLE_UNIQUE_INGESTION_RUNS.
  • Stack & Top-Level Modules: Removed redundant resolve_with_spanner_embeddings / datacommons_services_resolve_with_spanner_embeddings variable definitions and pass-throughs. Aligned ingestion_preprocessing_job to pass enable_spanner_embeddings = var.spanner_config.enable_embeddings_generation.
  • Integration Tests: Removed - RESOLVE_WITH_SPANNER_EMBEDDINGS=true override from docker-compose.test.yml and updated documentation/comments in custom_feature_flags.yaml and README.md.

Verification

  • Validated template generation and CLI tests via uv run pytest packages/datacommons-admin/tests/test_admin_cli.py.
  • Verified Terraform syntax and module integrity via terraform validate.

value = var.redis_port
}
env {
name = "ENABLE_UNIQUE_INGESTION_RUNS"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

is this ready?

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

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.

Code Review

This pull request cleans up configuration variables by removing resolve_with_spanner_embeddings and other temporary environment variables across the Terraform infrastructure and integration tests. The review feedback suggests adhering to the principle of least privilege by conditionally granting the roles/aiplatform.user IAM role only when both Spanner and embeddings generation are enabled. To achieve this, the reviewer recommends adding and passing the enable_embeddings_generation variable through the nested Terraform modules.

],
var.use_spanner ? ["roles/spanner.databaseUser"] : [],
var.use_spanner && var.resolve_with_spanner_embeddings ? ["roles/aiplatform.user"] : []
var.use_spanner ? ["roles/aiplatform.user"] : []

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.

security-medium medium

To adhere to the principle of least privilege, the roles/aiplatform.user role should only be granted if both Spanner and embeddings generation are enabled. Granting it unconditionally when use_spanner is true could lead to excessive permissions if embeddings generation is disabled.

var.use_spanner && var.enable_embeddings_generation ? ["roles/aiplatform.user"] : []

variable "resolve_with_spanner_embeddings" {
type = bool
}

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

Add the enable_embeddings_generation variable to allow conditionally granting the roles/aiplatform.user role based on whether embeddings generation is enabled.

variable "enable_embeddings_generation" {
  type        = bool
  default     = true
  description = "Enable embedding generation in Spanner/Vertex AI"
}

])
secret_env_vars = local.datacommons_services_secrets
resolve_with_spanner_embeddings = var.datacommons_services_config.resolve_with_spanner_embeddings
website_search_scope = var.datacommons_services_config.website_search_scope

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

Pass the enable_embeddings_generation variable from the stack configuration to the datacommons_services module. To ensure the default value defined in the innermost module is respected, make sure this attribute is declared as optional in the intermediate configuration object (spanner_config) and defaults to null.

  enable_embeddings_generation    = var.spanner_config.enable_embeddings_generation
  website_search_scope            = var.datacommons_services_config.website_search_scope
References
  1. When designing nested Terraform modules, set the top-level variable default to null and declare the attribute as optional in intermediate module configuration objects. This ensures that the default value defined in the innermost module is respected and not overridden by parent defaults.

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.

1 participant