Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 4 additions & 0 deletions apps/web/.env.example
Original file line number Diff line number Diff line change
Expand Up @@ -69,3 +69,7 @@ MEDIA_START_PAGE_LINK=

# Service URL for generating links in notifications
CATH_SERVICE_URL=https://www.court-tribunal-hearings.service.gov.uk

# Azure Blob Storage (local development uses Azurite emulator - start with: docker-compose up azurite)
AZURE_STORAGE_CONNECTION_STRING=DefaultEndpointsProtocol=http;AccountName=devstoreaccount1;AccountKey=Eby8vdM02xNOcqFlqUwJPLlmEtlCDXJ1OGLHnZMd6iY7h1teSXIRAsTr2kPiSSrJg==;BlobEndpoint=http://127.0.0.1:10000/devstoreaccount1;
AZURE_STORAGE_ACCOUNT_NAME=devstoreaccount1
12 changes: 11 additions & 1 deletion docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,16 @@ services:
start_period: 10s
command: redis-server --appendonly yes

azurite:
image: mcr.microsoft.com/azure-storage/azurite:latest
container_name: cath-azurite
ports:
- "10000:10000"
volumes:
- azurite_data:/data
command: azurite-blob --blobHost 0.0.0.0 --blobPort 10000

volumes:
postgres_data:
redis_data:
redis_data:
azurite_data:
137 changes: 137 additions & 0 deletions docs/tickets/569/plan.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
# Technical Plan: #569 – Add Azure Blob Storage to cath-service infrastructure Terraform

## 1. Technical Approach

Provision Azure Blob Storage for cath-service by:
1. Creating `infrastructure/storage.tf` using `cnp-module-storage-account@4.x` β€” the same pattern used by `pip-shared-infrastructures`
2. Adding storage-related variables to `infrastructure/variables.tf`
3. Writing Key Vault secrets (connection string + account name) into the existing application KV
4. Updating the API and web Helm `values.yaml` to mount those secrets as environment variables
5. Adding `MANAGED_IDENTITY_CLIENT_ID` as a direct env var in the stg/prod Helm template

The existing `data "azurerm_user_assigned_identity" "app_mi"` in `keyvault.tf` already looks up `cath-${var.env}-mi` from `managed-identities-${var.env}-rg`. This can be referenced directly β€” no new data source or managed identity is needed.

## 2. Implementation Details

### Files to create

**`infrastructure/storage.tf`**

```hcl
module "sa" {
source = "git::https://github.com/hmcts/cnp-module-storage-account?ref=4.x"

env = var.env
storage_account_name = "cathsa${var.env}"
resource_group_name = azurerm_resource_group.rg.name
location = var.location
account_kind = var.sa_account_kind
account_tier = var.sa_account_tier
account_replication_type = var.sa_account_replication_type
access_tier = var.sa_access_tier
common_tags = var.common_tags

# 3 private blob containers
containers = [
{ name = "artefact", access_type = "private" },
{ name = "files", access_type = "private" },
{ name = "publications", access_type = "private" },
]

# Grant existing app managed identity Storage Blob Data Contributor
role_assignments = [
{
role_definition_name = "Storage Blob Data Contributor"
principal_id = data.azurerm_user_assigned_identity.app_mi.principal_id
}
]
}

resource "azurerm_key_vault_secret" "storageaccount_connection_string" {
name = "storageaccount-connection-string"
value = module.sa.storageaccount_primary_connection_string
key_vault_id = module.application_key_vault.key_vault_id
}

resource "azurerm_key_vault_secret" "storageaccount_name" {
name = "storageaccount-name"
value = module.sa.storageaccount_name
key_vault_id = module.application_key_vault.key_vault_id
}
```

### Files to modify

**`infrastructure/variables.tf`** β€” append 4 SA variables:

```hcl
variable "sa_account_tier" {
description = "Storage account tier"
type = string
default = "Standard"
}

variable "sa_account_kind" {
description = "Storage account kind"
type = string
default = "StorageV2"
}

variable "sa_account_replication_type" {
description = "Storage account replication type"
type = string
default = "RAGRS"
}

variable "sa_access_tier" {
description = "Storage account access tier"
type = string
default = "Cool"
}
```

**`apps/api/helm/values.yaml`** β€” add storage KV secrets under `keyVaults.cath-ss-kv.secrets`:

```yaml
- name: storageaccount-connection-string
alias: AZURE_STORAGE_CONNECTION_STRING
- name: storageaccount-name
alias: AZURE_STORAGE_ACCOUNT_NAME
```

**`apps/web/helm/values.yaml`** β€” same additions (if the web app reads blobs directly; omit if only the API does β€” the ticket says "application files" broadly, so add to API at minimum; add to web if needed).

**`helm/cath-service/values.template.yaml`** (stg/prod template) β€” add `MANAGED_IDENTITY_CLIENT_ID` as a direct environment variable under the `cath-api` section (and `cath-web` if applicable):

```yaml
cath-api:
nodejs:
environment:
MANAGED_IDENTITY_CLIENT_ID: "${MANAGED_IDENTITY_CLIENT_ID}"
```

## 3. Key Technical Decisions

- **Storage account name** `cathsa${env}` satisfies Azure's 3–24 char, lowercase alphanumeric constraint for all envs (`cathsastg`, `cathsaprod`, `cathsaithc`, `cathsademo`, `cathsatest`).
- **`data "azurerm_user_assigned_identity" "app_mi"`** is already declared in `keyvault.tf` with `depends_on = [module.application_key_vault]`. The `storage.tf` module call uses `data.azurerm_user_assigned_identity.app_mi.principal_id` directly β€” no circular dependency since keyvault is a prerequisite.
- **KV secrets** go into `module.application_key_vault` (the `cath-ss-kv-${env}` vault) which is already mounted by the app pods.
- **`cnp-module-storage-account` version pin**: use `?ref=4.x` matching pip-shared-infrastructures.

## 4. Acceptance Criteria Mapping

| Criterion | How satisfied |
|-----------|--------------|
| `infrastructure/storage.tf` created using `cnp-module-storage-account@4.x` | New file with `module "sa"` |
| Storage account named `cathsa${env}` | `storage_account_name = "cathsa${var.env}"` |
| 3 private containers: `artefact`, `files`, `publications` | `containers` list in module |
| `cath-${env}-mi` granted `Storage Blob Data Contributor` | `role_assignments` block using existing `app_mi` data source |
| KV secrets `storageaccount-connection-string` and `storageaccount-name` | Two `azurerm_key_vault_secret` resources |
| Helm values updated to inject storage env vars from KV | Added to `apps/api/helm/values.yaml` (and web if applicable) |
| `MANAGED_IDENTITY_CLIENT_ID` injected in stg/prod template | Added to `helm/cath-service/values.template.yaml` |
| `terraform plan` produces no errors | Verified by correct module refs and existing data sources |

## 5. Open Questions / Clarifications Needed

- **Does the web app need `AZURE_STORAGE_*` env vars?** The ticket mentions "generated PDFs, media application ID proof images" which could be read by either app. If web only serves pre-generated content from the API, only the API needs the vars. Added to both apps conservatively.
- **`cnp-module-storage-account` exact container API**: the module's `containers` variable name and `role_assignments` schema should be verified against the `4.x` branch README before applying β€” the exact variable names may differ slightly from the pattern shown above.
- **`MANAGED_IDENTITY_CLIENT_ID` value**: this is the client ID of `cath-${env}-mi`. The ticket implies it should be the environment-specific value injected at deploy time via the template substitution variable `${MANAGED_IDENTITY_CLIENT_ID}`.
105 changes: 105 additions & 0 deletions docs/tickets/569/review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
# Code Review: Issue #569 – Add Azure Blob Storage to cath-service infrastructure Terraform

## Summary

The implementation covers all five files required by the ticket: `infrastructure/storage.tf` (new), `infrastructure/variables.tf` (modified), `apps/api/helm/values.yaml` (modified), `apps/web/helm/values.yaml` (modified), and `helm/cath-service/values.template.yaml` (modified). The core Terraform structure is correct and follows existing patterns in the codebase. There are no security or data-loss issues. However, there are several verifiability gaps around unconfirmed module output names, one meaningful functional concern about the `cath-crons` app being excluded, and a questionable decision to add storage secrets to the web app without evidence it needs them.

---

## CRITICAL Issues

None.

---

## HIGH PRIORITY Issues

### 1. `cnp-module-storage-account@4.x` output names are unverified β€” plan cannot succeed if they are wrong

**File:** `infrastructure/storage.tf`, lines 30 and 36

The KV secret values reference `module.sa.storageaccount_primary_connection_string` and `module.sa.storageaccount_name`. These names are taken directly from the plan document but have not been verified against the `4.x` branch of `cnp-module-storage-account`. If the actual output names differ (e.g., `primary_connection_string`, `storage_account_name`, or similar), `terraform plan` will fail with an "output not found" error and the deployment pipeline will break.

Required action before merge: check the `4.x` branch README or `outputs.tf` at `https://github.com/hmcts/cnp-module-storage-account/tree/4.x` and confirm both output names exactly.

### 2. `cnp-module-storage-account@4.x` input variable names `containers` and `role_assignments` are unverified

**File:** `infrastructure/storage.tf`, lines 14–25

The module is called with `containers` and `role_assignments` as input variable names. These names are assumed from the `pip-shared-infrastructures` reference pattern but have not been verified against the `4.x` module's `variables.tf`. If the module uses different names (e.g., `blob_containers`, `rbac_role_assignments`) the plan will fail. Both names must be confirmed from the module source before merge.

### 3. `cath-crons` app excluded from storage env vars without justification

**File:** `apps/crons/helm/values.yaml` β€” not modified

The crons app processes the same file-related workloads referenced in the ticket ("generated PDFs, media application ID proof images"). Its `values.yaml` already pulls secrets from `cath-ss-kv` via the existing pattern. If any cron job reads or writes to blob storage, it will fail at runtime with a missing env var rather than a configuration error. Either the crons app should receive `AZURE_STORAGE_CONNECTION_STRING` and `AZURE_STORAGE_ACCOUNT_NAME`, or there should be an explicit record of why it does not need them. The ticket and plan both leave this unresolved.

### 4. Storage secrets added to `cath-web` without confirmed requirement

**File:** `apps/web/helm/values.yaml`, lines 67–70

The web app receives `AZURE_STORAGE_CONNECTION_STRING` and `AZURE_STORAGE_ACCOUNT_NAME`. The plan document (section 2, note on `apps/web/helm/values.yaml`) explicitly flags this as uncertain: "add to web if needed." Adding KV secret mounts that the app does not consume is not harmless β€” any secret fetch failure (e.g., secret does not exist yet in a new environment) will prevent the pod from starting. This should be removed unless there is confirmed application code in the web app that reads from blob storage.

---

## SUGGESTIONS

### 1. `depends_on` on the KV secret resources may be needed

**File:** `infrastructure/storage.tf`, lines 28–38

The two `azurerm_key_vault_secret` resources reference `module.sa` (implicitly dependent) and `module.application_key_vault.key_vault_id` (implicitly dependent via the KV module). Terraform's implicit dependency graph should handle this correctly because both module outputs are referenced directly. However, the existing pattern in `keyvault-third-party.tf` (line 27) adds an explicit `depends_on = [module.third_party_key_vault]` as a defensive measure. Adding the same to both KV secret resources (`depends_on = [module.sa, module.application_key_vault]`) would make the dependency chain explicit and avoid any edge-case ordering issue during first-time apply.

### 2. `sa_access_tier = "Cool"` default may be wrong for artefacts written frequently

**File:** `infrastructure/variables.tf`, line 103

Cool tier has a minimum storage duration of 30 days and higher per-operation costs. If the `artefact` or `files` containers are written to and deleted frequently (e.g., temporary upload staging), Hot tier would be more appropriate and cheaper. This is a cost/correctness consideration to verify with the application team before the first production deployment.

### 3. Storage account name uses interpolation without validation

**File:** `infrastructure/storage.tf`, line 5

`storage_account_name = "cathsa${var.env}"` is correct for all known env values (stg=9, prod=10, ithc=10, demo=10, test=10 chars β€” all within the 3–24 char Azure limit). However, there is no Terraform `validation` block on `var.env` to enforce the constraint. If a new environment with a long name is introduced (e.g., a 15-character env string), the storage account name would silently exceed 24 chars. A validation block on the computed name, or a comment noting the constraint, would prevent future breakage.

### 4. `MANAGED_IDENTITY_CLIENT_ID` is injected into `cath-web` in `values.template.yaml` but the web app uses `aadIdentityName: cath` for workload identity

**File:** `helm/cath-service/values.template.yaml`, line 24

The web app's base `values.yaml` sets `aadIdentityName: cath` which is how the HMCTS Helm chart wires up the pod-managed-identity binding. `MANAGED_IDENTITY_CLIENT_ID` as a plain env var is a separate concept β€” it allows code using the Azure SDK to explicitly pick the client ID when `DefaultAzureCredential` is used. If the web app does not use the Azure SDK directly for blob access (i.e., it proxies through the API), then `MANAGED_IDENTITY_CLIENT_ID` in the web section is unnecessary. This should be removed if the web app does not interact with Azure services directly.

---

## Positive Feedback

- The `data "azurerm_user_assigned_identity" "app_mi"` data source is correctly reused from `keyvault.tf` β€” no duplicate data source, no circular dependency.
- All three container `access_type` values are correctly set to `"private"`. No public container access is present.
- The KV secret names (`storageaccount-connection-string`, `storageaccount-name`) exactly match the Helm values `name:` entries, so there is no key mismatch between Terraform and Helm.
- The `module.application_key_vault.key_vault_id` reference for the KV secrets is consistent with all other KV secrets in the codebase (`appinsights.tf`, `redis.tf`, `postgres.tf`).
- Storage account name `cathsa${env}` is well-formed β€” all five known env values produce valid Azure names (lowercase alphanumeric, 9–10 chars).
- The four new variables in `variables.tf` have sensible defaults that match the pip reference pattern and include descriptions.
- Template substitution syntax `"${MANAGED_IDENTITY_CLIENT_ID}"` is consistent with the existing `${TEAM_NAME}`, `${WEB_IMAGE}`, etc. in the same file.
- The `ref=4.x` branch pin matches the pip-shared-infrastructures reference pattern as specified in the ticket.

---

## Acceptance Criteria Verification

| Criterion | Status | Notes |
|-----------|--------|-------|
| `infrastructure/storage.tf` created using `cnp-module-storage-account@4.x` | PASS | File exists, correct source ref |
| Storage account named `cathsa${env}` | PASS | Line 5 of storage.tf |
| 3 private containers: `artefact`, `files`, `publications` | PASS | Lines 14–18 of storage.tf, all `access_type = "private"` |
| `cath-${env}-mi` granted `Storage Blob Data Contributor` | PASS β€” UNVERIFIED | `role_assignments` block present with correct role name; depends on unconfirmed module variable name |
| KV secrets `storageaccount-connection-string` and `storageaccount-name` created | PASS β€” UNVERIFIED | Resources present; depends on unconfirmed module output names |
| Helm values updated to inject storage env vars from KV | PASS (API), QUESTIONABLE (web) | API: confirmed; web: added without confirmed requirement; crons: not added |
| `MANAGED_IDENTITY_CLIENT_ID` injected in stg/prod template values files | PASS | Added to both `cath-web` and `cath-api` sections in `values.template.yaml` |
| `terraform plan` produces no errors | UNVERIFIABLE | Cannot verify without `terraform init` and confirmed module outputs |

---

## Overall Assessment

NEEDS CHANGES

The two HIGH PRIORITY items regarding unverified module output and input variable names are blockers β€” if either name is wrong, the apply will fail in the deployment pipeline. These must be confirmed from the `cnp-module-storage-account@4.x` source before merge. The crons exclusion and the speculative web app secrets should also be resolved one way or the other with a clear rationale recorded in the PR. Everything else is solid and follows existing codebase patterns correctly.
9 changes: 9 additions & 0 deletions docs/tickets/569/tasks.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Implementation Tasks – #569: Add Azure Blob Storage to cath-service infrastructure Terraform

## Implementation Tasks

- [x] Create `infrastructure/storage.tf` with `cnp-module-storage-account@4.x` module call, 3 private containers, role assignment for app MI, and 2 KV secret resources
- [x] Add storage account variables to `infrastructure/variables.tf` (`sa_account_tier`, `sa_account_kind`, `sa_account_replication_type`, `sa_access_tier`)
- [x] Add `storageaccount-connection-string` and `storageaccount-name` KV secret entries to `apps/api/helm/values.yaml`
- [x] Add `storageaccount-connection-string` and `storageaccount-name` KV secret entries to `apps/web/helm/values.yaml`
- [x] Add `MANAGED_IDENTITY_CLIENT_ID` direct env var to `helm/cath-service/values.template.yaml` under `cath-api` (and `cath-web` if applicable)
Loading
Loading