Skip to content

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

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.

Sign up for GitHub

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

Open
clincoln8 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion infra/dcp/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,6 @@ locals {
instructions_path = var.datacommons_services_mcp_instructions_path != null ? trimsuffix(var.datacommons_services_mcp_instructions_path, "/") : null
allow_unauthenticated_access = var.datacommons_services_allow_unauthenticated_access
website_disable_google_maps_api = var.datacommons_services_website_disable_google_maps_api
resolve_with_spanner_embeddings = var.datacommons_services_resolve_with_spanner_embeddings
website_search_scope = var.datacommons_services_website_search_scope
}

Expand Down
14 changes: 1 addition & 13 deletions infra/dcp/modules/datacommons_services/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ locals {
"roles/workflows.invoker"
],
var.use_spanner ? ["roles/spanner.databaseUser"] : [],
var.use_spanner && var.resolve_with_spanner_embeddings ? ["roles/aiplatform.user"] : []
var.use_spanner ? ["roles/aiplatform.user"] : []

@gemini-code-assist gemini-code-assist Bot Jul 28, 2026

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"] : []

))
}

Expand Down Expand Up @@ -101,18 +101,6 @@ resource "google_cloud_run_v2_service" "dc_web_service" {
name = "DC_INSTRUCTIONS_DIR"
value = var.mcp_instructions_path != null ? "gs://${var.artifacts_bucket_name}/${var.mcp_instructions_path}" : ""
}
env {
name = "RESOLVE_WITH_SPANNER_EMBEDDINGS"
value = var.resolve_with_spanner_embeddings ? "true" : "false"
}
env {
name = "ENABLE_UNIQUE_HISTORY_RECORDS"
value = "true"
}
env {
name = "USE_SPANNER_KEY_VALUE_STORE"
value = var.use_spanner ? "true" : "false"
}
env {
name = "V2_RESOLVE_INDICATORS_TARGET"
value = var.website_search_scope
Expand Down
3 changes: 0 additions & 3 deletions infra/dcp/modules/datacommons_services/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,9 +69,6 @@ variable "mcp_instructions_path" {
default = null
}

variable "resolve_with_spanner_embeddings" {
type = bool
}

@gemini-code-assist gemini-code-assist Bot Jul 28, 2026

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"
}

variable "website_search_scope" {
type = string
Expand Down
5 changes: 0 additions & 5 deletions infra/dcp/modules/ingestion/helper_service/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,6 @@ resource "google_cloud_run_v2_service" "ingestion_helper" {
name = "REDIS_PORT"
value = var.redis_port
}
env {
name = "ENABLE_UNIQUE_INGESTION_RUNS"

@clincoln8 clincoln8 Jul 28, 2026

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?

value = "true"
# Temporary variable to control changes to the ingestion history table. To be deleted after migration complete.
}
}

dynamic "vpc_access" {
Expand Down
3 changes: 1 addition & 2 deletions infra/dcp/modules/stack/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ module "ingestion_preprocessing_job" {
ingestion_artifacts_path = var.ingestion_config.ingestion_artifacts_path
run_database_init = false
use_spanner = true
enable_spanner_embeddings = var.datacommons_services_config.resolve_with_spanner_embeddings
enable_spanner_embeddings = var.spanner_config.enable_embeddings_generation
env_vars = local.cloud_run_shared_env_variables
env_secrets = {
DC_API_KEY = {
Expand Down Expand Up @@ -286,7 +286,6 @@ module "datacommons_services" {
}
])
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

@gemini-code-assist gemini-code-assist Bot Jul 28, 2026

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.


depends_on = [module.ingestion_preprocessing_job]
Expand Down
1 change: 0 additions & 1 deletion infra/dcp/modules/stack/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@ variable "datacommons_services_config" {
instructions_path = string
allow_unauthenticated_access = bool
website_disable_google_maps_api = bool
resolve_with_spanner_embeddings = bool
website_search_scope = optional(string, "")
})
}
Expand Down
6 changes: 0 additions & 6 deletions infra/dcp/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,12 +315,6 @@ variable "datacommons_services_mcp_instructions_path" {
default = null
}

# TODO(shixiao): Remove this variable to only resolve on spanner embeddings
variable "datacommons_services_resolve_with_spanner_embeddings" {
description = "Enable resolving search queries with Spanner embeddings. Requires Spanner to be enabled (enable_spanner = true)."
type = bool
default = true
}

variable "datacommons_services_website_search_scope" {
description = "Controls the scope for indicator resolution on the website Explore page (e.g., restricting queries to custom variables). Valid values are 'base_only', 'custom_only', 'base_and_custom'."
Expand Down
2 changes: 1 addition & 1 deletion tests/datacommons-integration-tests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ While running with `--keep-containers`, you can access:

Local integration tests run with the Spanner multi-entity schema enabled by default in the Mixer backend. This is controlled by mounting [custom_feature_flags.yaml](./custom_feature_flags.yaml) inside the `website` container at `/workspace/deploy/featureflags/custom.yaml`.

The container entrypoint detects the environment variable `RESOLVE_WITH_SPANNER_EMBEDDINGS=true` (defined in `docker-compose.test.yml`) and starts Mixer using these flags.
The container entrypoint starts Mixer using canonical feature flags along with these test overrides.

You can modify the flags inside `custom_feature_flags.yaml` if you need to tweak the Mixer server configuration for local debugging.

Expand Down
2 changes: 1 addition & 1 deletion tests/datacommons-integration-tests/custom_feature_flags.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# This file overrides Mixer feature flags for local integration tests.
# It is mounted as '/workspace/deploy/featureflags/custom.yaml' inside the website container,
# which gets activated when RESOLVE_WITH_SPANNER_EMBEDDINGS=true is set in the environment.
# which overrides canonical feature flags when running local docker-compose tests.
#
# Doing this enables testing the new schema layout without changing production website code.
flags:
Expand Down
1 change: 0 additions & 1 deletion tests/datacommons-integration-tests/docker-compose.test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ services:
- SPANNER_EMULATOR_HOST=spanner:${SPANNER_INTERNAL_GRPC_PORT:-15000}
- STORAGE_EMULATOR_HOST=http://gcs:9099
- OUTPUT_DIR=gs://test-bucket/output
- RESOLVE_WITH_SPANNER_EMBEDDINGS=true
- PROJECT_ID=default
- GCP_PROJECT_ID=default
- GCP_SPANNER_INSTANCE_ID=default
Expand Down
Loading