Skip to content

feat(infra): add validation for instance_name and namespace variables - #210

Open
saurabh-google wants to merge 1 commit into
datacommonsorg:mainfrom
saurabh-google:feature/validate-instance-name
Open

feat(infra): add validation for instance_name and namespace variables#210
saurabh-google wants to merge 1 commit into
datacommonsorg:mainfrom
saurabh-google:feature/validate-instance-name

Conversation

@saurabh-google

Copy link
Copy Markdown

Summary

Adds input validation rules for instance_name and namespace variables in infra/dcp/variables.tf.

Context & Problem

Google Cloud Service Account account_id values have a strict limit of 6 to 30 characters and must match ^[a-z]([-a-z0-9]*[a-z0-9])?$.

Several DCP modules append fixed suffixes up to 14 characters long (e.g. -dc-ing-hlp-sa, -dc-ing-pre-sa, -dc-ing-pst-sa) to the instance_name when generating Service Account names. Consequently, setting an instance_name longer than 16 characters causes Terraform planning / API execution to fail with downstream errors like:

Error: "account_id" ("dcp-custom-var-test-dc-ing-df-sa") must be between 6 and 30 characters long

@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 introduces validation blocks for the 'instance_name' and 'namespace' variables in 'infra/dcp/variables.tf' to enforce length and character constraints. The review feedback suggests improving maintainability by documenting the rationale behind the 16-character limit, as it is derived from specific service account ID constraints, and provides code suggestions to incorporate these comments into the validation blocks.

Comment thread infra/dcp/variables.tf
Comment on lines +21 to +24
validation {
condition = var.instance_name == "" || (length(var.instance_name) <= 16 && can(regex("^[a-z]([-a-z0-9]*[a-z0-9])?$", var.instance_name)))
error_message = "The instance_name must be at most 16 characters long, start with a lowercase letter, end with a lowercase letter or number, and contain only lowercase letters, numbers, and hyphens."
}

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

To improve long-term maintainability, it's good practice to document 'magic numbers' like 16. Adding a brief comment explaining that this number is derived from the 30-character limit for service account IDs minus a 14-character suffix will be very helpful for future developers.

This validation logic is also duplicated for the namespace variable. While this is hard to avoid with Terraform variable validations, it's something to be aware of for future maintenance.

  validation {
    # Service account IDs have a 30-character limit. Suffixes of up to 14 characters
    # are appended to the instance_name, so it must be at most 16 characters.
    condition     = var.instance_name == "" || (length(var.instance_name) <= 16 && can(regex("^[a-z]([-a-z0-9]*[a-z0-9])?$", var.instance_name)))
    error_message = "The instance_name must be at most 16 characters long, start with a lowercase letter, end with a lowercase letter or number, and contain only lowercase letters, numbers, and hyphens."
  }
References
  1. Prefer applying validation constraints directly on input variables (using Terraform validation blocks) to prevent invalid or null values, rather than handling these cases with complex conditional logic inside locals blocks.

Comment thread infra/dcp/variables.tf
Comment on lines +32 to +35
validation {
condition = var.namespace == "" || (length(var.namespace) <= 16 && can(regex("^[a-z]([-a-z0-9]*[a-z0-9])?$", var.namespace)))
error_message = "The namespace must be at most 16 characters long, start with a lowercase letter, end with a lowercase letter or number, and contain only lowercase letters, numbers, and hyphens."
}

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

Similar to the instance_name variable, it would be beneficial to add a comment here explaining the origin of the 16 character limit for better maintainability. This makes the code easier to understand without external context.

  validation {
    # Service account IDs have a 30-character limit. Suffixes of up to 14 characters
    # are appended to the namespace, so it must be at most 16 characters.
    condition     = var.namespace == "" || (length(var.namespace) <= 16 && can(regex("^[a-z]([-a-z0-9]*[a-z0-9])?$", var.namespace)))
    error_message = "The namespace must be at most 16 characters long, start with a lowercase letter, end with a lowercase letter or number, and contain only lowercase letters, numbers, and hyphens."
  }
References
  1. Prefer applying validation constraints directly on input variables (using Terraform validation blocks) to prevent invalid or null values, rather than handling these cases with complex conditional logic inside locals blocks.

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