Enable svidstore#19
Conversation
45c1efd to
556739d
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a new AWS reference architecture stack for deploying a Cofide Connect trust zone workload, including EKS cluster provisioning, SPIRE server and agent IAM roles, Connect registration, and Kubernetes manifests for cert-manager and SPIRE. The review feedback focuses on improving the robustness of the deployment scripts and Terragrunt configurations. Specifically, it recommends using jq -r with helm get values ... --all in generate-local-values.sh to safely extract unquoted configuration values, and utilizing try() in Terragrunt locals to prevent evaluation errors when optional configuration keys like trust_domain or subnet_ids are omitted.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
I am having trouble creating individual review comments. Click here to see my feedback.
workload/deployment/aws/k8s/spire-server/spire/generate-local-values.sh (61)
Using yq to parse the JSON output of helm get values can result in quoted strings (e.g., "example.com" instead of example.com), which will produce invalid URLs in the generated configuration. Additionally, if the trust domain is defined as a default value in the chart rather than a user-supplied value, helm get values without --all will return empty.
Switch to using helm get values ... --all -o json and pipe it to jq -r to ensure the raw, unquoted string is correctly extracted from the computed values, matching the pattern used in the control-plane scripts.
CONNECT_TRUST_DOMAIN=$(helm get values spire --namespace spire-mgmt --all -o json | jq -r '.global.spire.trustDomain')
workload/deployment/aws/k8s/spire-server/spire/generate-local-values.sh (69)
Similar to the trust domain extraction, use helm get values ... --all -o json and pipe to jq -r to safely extract the PSAT audience as a raw, unquoted string from the computed values.
PSAT_AUDIENCE=$(helm get values connect --namespace connect --all -o json | jq -r '.connect.connectPSATAudience')
workload/deployment/aws/infra/stack/connect/trust-zone/terragrunt.hcl (36)
If trust_domain is not defined in common.local.hcl, accessing local.merged_config.trust_domain directly will cause a Terragrunt parsing error.
Using try(local.merged_config.trust_domain, null) allows Terragrunt to parse successfully and defers the validation to Terraform, which will fail with a standard missing variable error (since trust_domain has no default in variables.tf). This matches the robust pattern used for other optional/required configuration parameters in this stack.
trust_domain = try(local.merged_config.trust_domain, null)
workload/deployment/aws/infra/stack/eks-cluster/cluster/terragrunt.hcl (135)
If a user defines a custom node group in common.local.hcl but does not explicitly specify subnet_ids, accessing ng.subnet_ids directly will cause a Terragrunt evaluation error.
Using try(ng.subnet_ids, []) provides a safe fallback to an empty list, allowing the fallback logic to correctly inject the private subnets.
subnet_ids = length(try(ng.subnet_ids, [])) > 0 ? ng.subnet_ids : (
556739d to
979365f
Compare
No description provided.