feat(azure): sync Key Vault to AKS via External Secrets Operator

[amdove]

Description

Adds External Secrets Operator (ESO) support for AKS so Azure Key Vault secrets sync into native Kubernetes Secrets instead of being applied by hand — the AKS counterpart to the AWS Secrets Store CSI driver. team-operator needs no changes: it keeps reading native Secrets by name (SecretType: kubernetes) and is unaware of ESO.

Opt-in per cluster via external_secrets_enabled. AKS only; AWS still uses the CSI driver.

Code Flow

• ESO install (clusters_azure.go) — per release: managed identity + federated identity credential (workload identity) with Key Vault Secrets User scoped to the workload vault; ESO Helm release + controller ServiceAccount in posit-team-system (reusing the namespace team-operator's release creates, via DependsOn); a ClusterSecretStore using authType: WorkloadIdentity.
• Per-site ExternalSecret — dataFrom.find on ^<compound>-<site>- with a rewrite that strips the prefix, so keys land exactly as team-operator reads them. dev-admin-token / dev-user-token are always empty and can't be stored in Key Vault, so they're emitted as empty literals via target.template + mergePolicy: Merge.
• creationPolicy: Orphan instead of ESO's default Owner. Owner sets an ownerReference, so deleting the ExternalSecret (e.g. flipping the flag off) would garbage-collect a live product Secret. Orphan adopts, updates and prunes identically but never destroys.
• CRDs are retained — the chart renders its CRDs as templates, so an Atomic rollback of a failed upgrade would delete them and cascade-delete every ExternalSecret and ClusterSecretStore in the cluster (observed while testing). crds.annotations."helm.sh/resource-policy": keep prevents that, matching the team-operator release's crd.keep.
• Key Vault naming — bootstrap names Azure site secrets <compound>-<site>-<field>, gated on the flag; without it the historical <site>-<field> name is kept.
• Bug fix (azure/secretstore.go) — CreateSecretIfNotExists ran json.Marshal on strings, storing them wrapped in literal quotes. Harmless while values were only stored statically, corrupting once ESO syncs them verbatim (a quoted DB password fails auth). Strings now store verbatim; non-strings still JSON-encode.
• Docs — docs/guides/external-secrets-aks.md: naming, which Key Vault secrets PTD creates vs. hand-created, Secrets to keep out of ESO, lifecycle policies, migration procedure, cleanup checklist.

Why the site-secret name helpers are inert

buildAzureSiteSpec computed <compound>-<site>.posit.team (dotted) while every live Site CR uses hyphens — alarming, but it never reaches the cluster. applySiteOverrides merges each site_<name>/site.yaml over the computed spec with mergo.WithOverride, and all 10 Azure site configs explicitly set secret, workloadSecret and mainDatabaseCredentialSecret (type: kubernetes + hyphenated vaultName). Confirmed empirically: sites --dry-run on an Azure workload reports no changes with either an origin/main or a branch binary.

So azureSiteSecretName / azureWorkloadSecretName change nothing today. They fix the default for a future workload shipping no vaultName override (previously pointing team-operator at a nonexistent Secret) and give the Site CR and ExternalSecret target one shared source of truth.

Merge safety

Every change is gated or inert:

• ESO resources — gated (bool, defaults false).
• Key Vault naming — gated; unmigrated vaults untouched.
• Site/workload secret names — masked by site.yaml overrides on all existing Azure workloads.
• AWS workloads — unaffected; all changes are Azure-only code paths.

Validation

Verified on an internal Azure staging cluster: store reports Valid; find + rewrite work (the Key Vault Secrets User role already grants the readMetadata action listing needs); empty template values persist under Merge; Orphan adopts a hand-applied Secret, updates on Key Vault change, prunes sourceless keys, and survives ExternalSecret deletion (also verified against a full ESO teardown); add → update → delete Key Vault lifecycle propagates; migration reproduced the live Secret with every value hash-identical.

Category of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Documentation: documentation changes

Checklist

• I have reviewed my own diff and added inline comments on lines I want reviewers to focus on or that I am uncertain about

[amdove]

@claude review please

[claude]

Claude finished @amdove's task in 1m 29s —— View job

---

PR Review

• Gather context and understand the request
• Read changed files
• Analyze code for bugs, security issues, correctness
• Post review feedback

---

This is a well-structured PR with a clear description and good test coverage for the new encodeSecretValue function. The overall design is sound. I have a few findings across correctness, observability, and documentation gaps.

---

Findings

1. ExternalSecretsVersion default may be too old

lib/types/workload.go:634 — the default ESO version is "2.8.0". ESO 2.x is now well past its end-of-life; the current stable release is the 0.x series (v0.10.x / v0.11.x). If the chart repo at https://charts.external-secrets.io is Helm-versioned with the 0.x numbering, "2.8.0" will fail a Helm --version lookup immediately. Confirm this is the correct chart version string for external-secrets.io/external-secrets before merging. Fix this →

2. EnsureWorkloadSecret skips the encodeSecretValue fix

lib/azure/secretstore.go:63-73 — EnsureWorkloadSecret still uses fmt.Sprintf("%v", fieldValue) to format each field value. For string fields this produces the correct output, but for numeric or other non-string types it differs from json.Marshal. More importantly, it bypasses encodeSecretValue. If someone adds a string field to AzureWorkloadSecret and its value happens to be a struct later, or if a new code path passes a non-string any, it will encode differently than CreateSecretIfNotExists. Ideally, call encodeSecretValue(fieldValue) here to be consistent — but at minimum add a note that this is intentional. Fix this →

3. Bootstrap skips empty values but doesn't create empty-keyed KV entries

lib/steps/bootstrap.go:289-292 — the bootstrap loop skips fields where fieldValueStr == "". That's correct (Key Vault can't store an empty string). However, the ExternalSecret target.template in clusters_azure.go hard-codes dev-admin-token and dev-user-token as the two empty-value exceptions. If secrets.NewSiteSecret ever gains a third empty field (or if these two are populated by bootstrap on some site variants), the target.template data map will be wrong — either over-injecting or silently missing keys.

The relationship between the two-place hard-coding (target.template data map in clusters_azure.go:857-861 and the bootstrap skip logic) should be documented or made data-driven. This is a fragility, not a bug today.

4. refreshInterval in ExternalSecret vs docs discrepancy

clusters_azure.go:844 — the generated ExternalSecret has "refreshInterval": "1h", but the guide's illustrative YAML at docs/guides/external-secrets-aks.md:51 shows refreshInterval: 1m. These should agree; the guide example will confuse operators who use it as a template. Fix this →

5. Missing alias option on ESO Pulumi resources

clusters_azure.go:688-820 — all new ESO resources (identity, role assignment, service account, federated credential, Helm release, ClusterSecretStore, ExternalSecrets) are created without withAlias() or withSubComponentAlias(). Every other resource in this function carries an alias so Pulumi can adopt existing state from the old Python deployment. The ESO resources are new (no Python predecessor), so no alias is technically needed right now. However, adding them at creation time (with a sub-component alias like ptd:ExternalSecrets) would be consistent and cheaper than a state migration later. This is a style nit, not a bug.

6. ExternalSecretsVersion field in AzureWorkloadClusterComponentConfig lacks the json struct tag

lib/types/workload.go:603 — ExternalSecretsVersion *string only has a yaml tag. All other pointer fields on this struct (AlloyVersion, ExternalDnsVersion, etc.) are consistent in having only yaml tags — this is intentional for the Azure config path. No change needed; just confirming this is consistent with the surrounding fields.

7. Documentation: workload reserved site name is only mentioned in prose

docs/guides/external-secrets-aks.md:128-131 describes that workload is a reserved site name (its Key Vault entries use <compound>-workload-<field> to avoid colliding with the ^<compound>-main- site selector). This constraint has no enforcement anywhere in code. Consider adding a validation in the types or bootstrap that rejects a site named workload. Fix this →

---

Observations (no action required)

• The creationPolicy: Owner + mergePolicy: Merge combination is the right design for the empty-token case. The note in the code is clear.
• The dataFrom.find approach is correct for selecting per-site KV entries, and the rewrite regexp is well-formed.
• The DependsOn([]pulumi.Resource{esoStore, positTeamNs}) on ExternalSecrets is correct — the CRD must exist (via the store's dep on the Helm release) and the namespace must exist before creating the ExternalSecret.
• encodeSecretValue is cleanly extracted and the unit tests cover the important cases (verbatim string, string with special chars, struct/map JSON encoding).
• The migration guide is thorough and the shasum verification approach is a good operational practice.
• The siteNames population via helpers.SortedKeys(cfg.Sites) ensures deterministic ordering, which matters for Pulumi state.

---

Blocking: Item 1 (ESO version string) should be verified before merging — a wrong chart version breaks ptd ensure immediately on a new cluster.
Recommended: Items 2 and 4 are low-risk, straightforward fixes.
Nice-to-have: Items 3 and 7 are future-proofing.

[claude]

Claude finished @amdove's task in 6m 51s —— View job

---

PR Review

• Gather context and understand the request
• Read review guidelines and changed files
• Analyze code for bugs, security issues, correctness
• Post inline comments and summary feedback

---

Summary

Design is sound and the description is accurate. Five inline comments posted; two are blocking.

Blocking:

1. ExternalSecretsVersion default "2.8.0" is almost certainly wrong — ESO's Helm chart uses 0.x versioning. A wrong version string breaks ptd ensure immediately on any cluster with external_secrets_enabled: true. See inline on workload.go:632.
2. bootstrap.go:305 still uses fmt.Sprintf("%v", …) encoding — EnsureWorkloadSecret was correctly updated to encodeSecretValue, but the site-secret loop in bootstrap.go was not. Today this is harmless since SiteSecret is all strings, but it's inconsistent and leaves a latent bug. See inline.

Important:
3. Federated credential silently skipped when OIDC issuer is unavailable — ESO installs but Key Vault auth fails at runtime, leaving ExternalSecrets silently stale. Add a comment explaining the failure mode. See inline on clusters_azure.go:743.
4. azRoleDefID not used for the Key Vault role — inconsistent with every other role assignment in the file. See inline on clusters_azure.go:712.

Nice-to-have:
5. clustersAzureEmptySiteSecretKeys hardcoded list requires manual sync with SiteSecret — suggest deriving it from the zero-value struct. See inline on clusters.go:49.

Confirmed correct:

• creationPolicy: Orphan + mergePolicy: Merge is the right combination.
• DependsOn(esoStore, positTeamNs) ordering is correct.
• dataFrom.find + rewrite regexp logic is well-formed.
• encodeSecretValue fix in secretstore.go and its unit tests are clean.
• AnyClusterExternalSecretsEnabled correctly gates the naming change so unmigrated vaults are untouched.
• siteNames sorted via helpers.SortedKeys ensures deterministic Pulumi state.

[amdove]

Both blocking items are resolved.

1. ExternalSecretsVersion default "2.8.0" — verified correct, not a bug. The chart moved from 0.x to 2.x versioning, so 0.10.x/0.11.x are historical releases rather than current:

$ helm search repo external-secrets/external-secrets --versions | head -4
NAME                               CHART VERSION  APP VERSION  DESCRIPTION
external-secrets/external-secrets  2.8.0          v2.8.0       External secrets management for Kubernetes
external-secrets/external-secrets  2.7.0          v2.7.0       External secrets management for Kubernetes
external-secrets/external-secrets  2.6.0          v2.6.0       External secrets management for Kubernetes

It is also already deployed and running from this branch on an internal Azure staging cluster — the release reports helm.sh/chart: external-secrets-2.8.0 with image ghcr.io/external-secrets/external-secrets:v2.8.0, and Key Vault secrets sync successfully. No change made.

2. bootstrap.go fmt.Sprintf("%v", …) — fixed in bcaffbe, though not with the suggested snippet: encodeSecretValue is unexported in lib/azure and that file is package steps, so it would not compile. The value also already passes through encodeSecretValue inside CreateSecretIfNotExists, so strings were encoded correctly. The field value is now type-asserted to string, so a future non-string field on SiteSecret fails loudly instead of being silently Go-formatted.

The remaining items are answered inline: the azRoleDefID consistency fix and the OIDC-issuer comment were applied; the clustersAzureEmptySiteSecretKeys derivation was declined because marshalling the zero-value SiteSecret returns every field as empty (~18 keys, not 2), which would add spurious keys and risk masking licence and client-secret values that legitimately come from find.

Separately, one real robustness gap this review prompted me to look at: the chart renders CRDs as templates, so an Atomic rollback of a failed upgrade would delete them and cascade-delete every ExternalSecret/ClusterSecretStore in the cluster (observed during testing). Fixed in ff11f22 by setting crds.annotations."helm.sh/resource-policy": keep, matching the team-operator release's crd.keep.