Pull Request Review: External Secrets Operator Integration

Summary

This is a comprehensive and well-executed PR that successfully integrates External Secrets Operator (ESO) across all three mop-core environments. The implementation demonstrates strong architectural decisions, excellent documentation, and follows infrastructure-as-code best practices.

✅ Strengths

1. Architecture & Design

• Multi-backend support: Clean abstraction for Kubernetes, AWS Secrets Manager, GCP Secret Manager, and HashiCorp Vault
• Reusable library patterns: The tanka/lib/eso.libsonnet library provides excellent composable helpers
• Environment consistency: ESO is deployed uniformly across all three environments (mop-central, mop-cloud, mop-edge)
• Separation of concerns: ESO configuration is properly isolated in dedicated eso.jsonnet files per environment

2. Code Quality

• Clean Jsonnet code: Consistent formatting, proper use of Jsonnet composition patterns (inheritance with +, helper functions)
• DRY principle: No duplication between environments - each uses the same library with environment-specific configuration
• Type safety: All ExternalSecret helpers properly structure the API resources according to ESO CRD specs
• Naming conventions: Consistent naming across files and resources

3. Documentation

• Comprehensive CLAUDE.md updates: Added detailed ESO commands, architecture overview, and migration patterns
• Example-driven learning: Four excellent example files with inline comments and usage instructions
• README with troubleshooting: The tanka/examples/README.md provides quick start guide and common issues
• Real-world migration example: The Backstage migration example is invaluable for understanding practical usage

4. Observability Integration

• Prometheus ServiceMonitor: ESO metrics are properly exposed for monitoring (line 33-36 in eso.jsonnet)
• Aligns with platform goals: Fits well with the observability platform mission

🔍 Areas for Improvement

1. Security Considerations (Medium Priority)

Issue: The awsSecretsManager helper in eso.libsonnet:80-86 has incomplete authentication configuration.

auth: if role != '' then {
  jwt: {
    serviceAccountRef: {
      name: 'external-secrets',
    },
  },
} else {},
role: role,

Problems:

• The role field is placed outside the auth block, which is incorrect according to ESO AWS provider spec
• Missing namespace in serviceAccountRef when used with IRSA
• The else {} results in an empty auth block, which may cause authentication failures

Recommendation:

awsSecretsManager(name, region='us-east-1', role='', namespace='mop'):: {
  apiVersion: 'external-secrets.io/v1beta1',
  kind: 'ClusterSecretStore',
  metadata: {
    name: name,
  },
  spec: {
    provider: {
      aws: {
        service: 'SecretsManager',
        region: region,
      } + (if role != '' then {
        auth: {
          jwt: {
            serviceAccountRef: {
              name: 'external-secrets',
              namespace: namespace,
            },
          },
        },
        role: role,
      } else {}),
    },
  },
},

File: tanka/lib/eso.libsonnet:69-91

2. GCP Configuration Issues (Medium Priority)

Issue: Hardcoded cluster configuration in gcpSecretManager helper (lines 105-111)

auth: {
  workloadIdentity: {
    clusterLocation: 'us-central1',  // ❌ Hardcoded
    clusterName: 'cluster-name',     // ❌ Placeholder value
    serviceAccountRef: {
      name: 'external-secrets',
    },
  },
},

Problems:

• clusterLocation and clusterName are hardcoded, won't work across different GCP environments
• Missing namespace in serviceAccountRef
• No validation that these values are provided

Recommendation:

gcpSecretManager(name, projectID='', clusterLocation='', clusterName='', namespace='mop'):: {
  assert projectID != '' : 'projectID is required for GCP Secret Manager',
  assert clusterLocation != '' : 'clusterLocation is required for Workload Identity',
  assert clusterName != '' : 'clusterName is required for Workload Identity',
  
  apiVersion: 'external-secrets.io/v1beta1',
  kind: 'ClusterSecretStore',
  metadata: {
    name: name,
  },
  spec: {
    provider: {
      gcpsm: {
        projectID: projectID,
        auth: {
          workloadIdentity: {
            clusterLocation: clusterLocation,
            clusterName: clusterName,
            serviceAccountRef: {
              name: 'external-secrets',
              namespace: namespace,
            },
          },
        },
      },
    },
  },
},

File: tanka/lib/eso.libsonnet:94-116

3. Missing Error Handling (Low Priority)

Issue: No validation for required parameters in helper functions

Examples:

• secretStore.new() doesn't validate that name is non-empty
• externalSecret.new() doesn't validate required parameters
• Pattern helpers don't validate secret store existence

Recommendation: Add Jsonnet assertions

new(name, namespace='mop'):: {
  assert name != '' : 'SecretStore name cannot be empty',
  assert namespace != '' : 'Namespace cannot be empty',
  // ... rest of implementation
}

File: tanka/lib/eso.libsonnet (multiple locations)

4. Linkerd Integration (Note)

Observation: This PR also includes Linkerd service mesh integration, which seems outside the scope of the PR title "Add External Secrets Operator for centralized secret management"

Files:

• tanka/environments/*/linkerd.jsonnet (new files, 136-93 lines each)
• Linkerd charts added to all chartfile.yaml files

Question: Was Linkerd intentionally included in this PR, or should it be in a separate PR? This makes the changeset significantly larger and combines two major infrastructure components.

Recommendation: Consider splitting Linkerd into a separate PR for clearer review and atomic changes. If they're related (e.g., using ESO for Linkerd certificate management), please clarify in the PR description.

5. Backstage Configuration Changes (Medium Priority)

Issue: The Backstage configuration in tanka/environments/mop-central/backstage.jsonnet contains hardcoded values that should use ESO

Examples (lines 7-182):

• PostgreSQL password references are updated to use secrets (good! ✅)
• BUT the chart still has postgresPassword: '${POSTGRES_ADMIN_PASSWORD}' (line 57) which expects environment variable substitution
• No corresponding ExternalSecret is created for this in the PR

Current state:

postgresql+: {
  auth+: {
    postgresPassword: '${POSTGRES_ADMIN_PASSWORD}',  // ❌ Where does this come from?
  },
}

Recommendation:

• Create an ExternalSecret for PostgreSQL admin password
• Reference it properly in the Backstage configuration
• OR document that this migration is incomplete and will be addressed in a follow-up

File: tanka/environments/mop-central/backstage.jsonnet:57

6. Missing Namespace Creation (Low Priority)

Issue: The ESO deployment references namespace mop via common.namespace, but there's no guarantee this namespace exists in all environments

Recommendation:

• Add a Namespace resource to ensure it's created
• OR document as a prerequisite in the README/CLAUDE.md

7. Version Pinning (Best Practice)

Good: Helm chart versions are pinned (✅ external-secrets@0.11.0)

Consider: Document the ESO version upgrade strategy

• How to test new ESO versions before upgrading
• Compatibility considerations with different backend providers
• Breaking changes between ESO versions

8. Testing Gaps (Medium Priority)

From PR description test plan:

• ✅ Vendored ESO charts
• ✅ Validated Jsonnet syntax
• ✅ Created documentation
• ❌ Deploy to development cluster (not completed)
• ❌ Test SecretStore creation (not completed)
• ❌ Test ExternalSecret syncing (not completed)
• ❌ Verify Prometheus ServiceMonitor (not completed)

Recommendation: Complete the test plan before merging, especially:

1. Actual deployment test to validate the generated manifests work
2. Secret sync verification to ensure the library functions produce working configurations
3. Validation that Prometheus can scrape ESO metrics

🐛 Potential Bugs

1. File Naming Inconsistency

Location: tanka/environments/mop-central/main.jsonnet:1,4

local alloy_operator = import 'alloy-operator.jsonnet';  // ❌ Dash
local config = import 'config.libsonnet';               // ❌ .libsonnet extension

Changed to:

local alloy_operator = import 'alloy_operator.jsonnet'; // ✅ Underscore
local config = import 'config.jsonnet';                 // ✅ .jsonnet extension

Risk: If alloy-operator.jsonnet or config.libsonnet don't exist, this will cause import errors. Verify these files exist or that the renames are intentional.

2. Incomplete Template Data

Location: tanka/lib/eso.libsonnet:340

TLS secret template uses:

data: {
  'tls.crt': '{{ .cert }}',
  'tls.key': '{{ .key }}',
},

But the secret keys are cert and key, which must exactly match the template variables. This is correct, but fragile - consider adding a comment or validation.

📊 Metrics & Complexity

• Files changed: 19 files
• Lines added: 1,709
• Lines deleted: 10
• New library: 360 lines (high quality, well-structured)
• Example code: 428 lines (excellent documentation)
• Documentation: 188 lines added to CLAUDE.md

Assessment: This is a large PR, but the size is justified given it's adding a complete new subsystem. The code quality is high enough to warrant the size.

🚀 Recommendations Summary

Must Fix (Before Merge)

1. ✅ Fix AWS authentication structure in eso.libsonnet:69-91
2. ✅ Fix GCP hardcoded values in eso.libsonnet:94-116
3. ✅ Complete test plan (deploy and verify)
4. ✅ Resolve Backstage PostgreSQL password management

Should Fix

1. Add parameter validation with assertions
2. Decide on Linkerd inclusion (separate PR or document relationship)
3. Add namespace resource or document prerequisite

Nice to Have

1. Add version upgrade documentation
2. Add integration tests for ESO library functions
3. Consider adding generator expressions to reduce boilerplate in data mappings

🎯 Verdict

Recommendation: Approve with changes required

This PR represents high-quality infrastructure code with excellent documentation and reusable patterns. The ESO library is well-designed and will serve as a strong foundation for secret management across the platform.

The issues identified are fixable and mostly concentrated in the cloud provider authentication configurations. Once the AWS/GCP configuration issues are resolved and the test plan is completed, this PR will be ready to merge.

Estimated effort to address feedback: 2-3 hours

Great work on this implementation! The patterns and documentation will be valuable for the team. 🎉