Reference deployment for a workload trust zone cluster in aws#18
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the AWS infrastructure stack and Kubernetes deployment configurations for provisioning a Cofide Connect trust zone workload cluster, including EKS, SPIRE server, and cert-manager setups. Key feedback includes using try() in Terragrunt to safely handle missing required variables, adding the --all flag to helm get values commands to ensure default chart values are retrieved, and enabling the OIDC discovery provider in values.yaml to align with documentation. Additionally, it is recommended to add fail-fast dependency checks for external CLI tools across several shell scripts.
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.
| merged_config = merge(local.default_config, local.user_config) | ||
|
|
||
| # trust_domain has no default — it must be set in common.local.hcl. | ||
| trust_domain = local.merged_config.trust_domain |
There was a problem hiding this comment.
If trust_domain is not defined in common.local.hcl, accessing local.merged_config.trust_domain directly will cause a fatal Terragrunt parsing error ("Unsupported attribute") instead of letting Terraform handle the missing required variable cleanly. Using try() allows it to safely fall back to null so that Terraform can raise a standard validation error.
trust_domain = try(local.merged_config.trust_domain, null)
| echo " distribution_domain: ${CP_DIST_DOMAIN}" | ||
|
|
||
| echo "Reading Connect trust domain from deployed spire Helm release (control-plane cluster)..." | ||
| CONNECT_TRUST_DOMAIN=$(helm get values spire --namespace spire-mgmt -o json | yq '.global.spire.trustDomain') |
There was a problem hiding this comment.
helm get values only retrieves user-supplied values by default. If global.spire.trustDomain was defined as a default in the chart and not explicitly overridden during the control-plane installation, this command will return null or empty. Adding the --all flag ensures that all values (including defaults) are retrieved.
| CONNECT_TRUST_DOMAIN=$(helm get values spire --namespace spire-mgmt -o json | yq '.global.spire.trustDomain') | |
| CONNECT_TRUST_DOMAIN=$(helm get values spire --all --namespace spire-mgmt -o json | yq '.global.spire.trustDomain') |
| echo " bundle_endpoint_url: ${CONNECT_BUNDLE_ENDPOINT_URL}" | ||
|
|
||
| echo "Reading PSAT audience from deployed connect-api Helm release (control-plane cluster)..." | ||
| PSAT_AUDIENCE=$(helm get values connect --namespace connect -o json | yq '.connect.connectPSATAudience') |
There was a problem hiding this comment.
Similar to the trust domain retrieval, helm get values only retrieves user-supplied values. If connectPSATAudience is defined as a default in the chart, it won't be retrieved unless the --all flag is specified.
| PSAT_AUDIENCE=$(helm get values connect --namespace connect -o json | yq '.connect.connectPSATAudience') | |
| PSAT_AUDIENCE=$(helm get values connect --all --namespace connect -o json | yq '.connect.connectPSATAudience') |
| strictMode: true | ||
|
|
||
| spiffe-oidc-discovery-provider: | ||
| enabled: false |
There was a problem hiding this comment.
The README states that "The OIDC discovery provider is deployed as a cluster-internal ClusterIP service." However, in values.yaml, spiffe-oidc-discovery-provider.enabled is explicitly set to false, which disables its deployment. Consider setting it to true to align with the documented behavior.
enabled: true| # The PSAT audience is read from the connect-api Helm release in the connect namespace. | ||
| # kubectl must be configured for the control-plane cluster when this script runs. | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
The script relies on several external CLI tools (terragrunt, helm, yq, kubectl) being installed. Adding a dependency check at the beginning of the script will fail fast with a clear error message if any are missing.
| set -euo pipefail | |
| set -euo pipefail | |
| # Check for required dependencies | |
| for cmd in terragrunt helm yq kubectl; do | |
| if ! command -v "$cmd" &>/dev/null; then | |
| echo "Error: $cmd is required but not installed." >&2 | |
| exit 1 | |
| fi | |
| done |
| @@ -0,0 +1,25 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -euo pipefail | |||
There was a problem hiding this comment.
The script relies on helm and yq being installed. Adding a dependency check at the beginning of the script will fail fast with a clear error message if any are missing.
| set -euo pipefail | |
| set -euo pipefail | |
| # Check for required dependencies | |
| for cmd in helm yq; do | |
| if ! command -v "$cmd" &>/dev/null; then | |
| echo "Error: $cmd is required but not installed." >&2 | |
| exit 1 | |
| fi | |
| done |
| @@ -0,0 +1,26 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -euo pipefail | |||
There was a problem hiding this comment.
The script relies on helm and yq being installed. Adding a dependency check at the beginning of the script will fail fast with a clear error message if any are missing.
| set -euo pipefail | |
| set -euo pipefail | |
| # Check for required dependencies | |
| for cmd in helm yq; do | |
| if ! command -v "$cmd" &>/dev/null; then | |
| echo "Error: $cmd is required but not installed." >&2 | |
| exit 1 | |
| fi | |
| done |
| @@ -0,0 +1,7 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -euo pipefail | |||
There was a problem hiding this comment.
The script relies on kubectl being installed. Adding a dependency check at the beginning of the script will fail fast with a clear error message if it is missing.
| set -euo pipefail | |
| set -euo pipefail | |
| # Check for required dependencies | |
| if ! command -v kubectl &>/dev/null; then | |
| echo "Error: kubectl is required but not installed." >&2 | |
| exit 1 | |
| fi |
45c1efd to
556739d
Compare
556739d to
979365f
Compare
No description provided.