Skip to content

feat(gcp): enable k8s deployment with federated workload identity#224

Merged
dmitsh merged 1 commit into
mainfrom
ds-gcp-helm
Mar 4, 2026
Merged

feat(gcp): enable k8s deployment with federated workload identity#224
dmitsh merged 1 commit into
mainfrom
ds-gcp-helm

Conversation

@dmitsh
Copy link
Copy Markdown
Collaborator

@dmitsh dmitsh commented Mar 3, 2026

No description provided.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 3, 2026

Greptile Summary

This PR extends the GCP provider Helm chart to support two distinct Kubernetes authentication paths — mounting a service account key Secret (serviceAccountKeysSecret) and Workload Identity Federation via a projected OIDC token (workloadIdentityFederation) — in addition to renames of the existing credentialsSecretNamecredentialsSecret key and a guard to skip RBAC resource creation when serviceAccount.create is false.

Key observations:

  • New _validation.tpl: Correctly enforces that the two GCP auth methods are mutually exclusive and that WIF requires both credentialsConfigmap and audience. Logic is sound.
  • rbac.yaml guard: Correctly wraps ClusterRole/ClusterRoleBinding in {{- if .Values.serviceAccount.create -}}.
  • deployment.yaml volume mounts: The GCP credential mounting has been refactored to support both service-account-key and Workload Identity Federation paths. Volume setup appears correct with proper subPath handling for file mounts.
  • docs/providers/gcp.md: Missing documentation section for the newly-added Workload Identity Federation feature. Users cannot learn how to create the WIF pool/provider, generate the credentials config JSON, or configure the required IAM bindings without external knowledge.
  • values.yaml: The new global.provider.params sub-keys (serviceAccountKeysSecret, workloadIdentityFederation.*) are absent from the default values file, making them invisible to users running helm show values.

Confidence Score: 3/5

  • Safe to merge from a runtime perspective, but documentation and discoverability gaps limit user ability to leverage new features.
  • The core implementation (deployment.yaml, _validation.tpl, rbac.yaml) is sound. Volume mounts use proper subPath configuration, and auth method validation is correct. However, the new WIF feature and parameter documentation gaps are quality issues that will frustrate users trying to adopt the feature. These are not breaking bugs but represent incomplete feature delivery — users cannot discover or configure WIF without external knowledge.
  • docs/providers/gcp.md (missing WIF documentation) and charts/topograph/values.yaml (missing param stubs for discoverability).

Important Files Changed

Filename Overview
charts/topograph/templates/_validation.tpl New validation helper that enforces GCP auth methods are mutually exclusive and that WIF requires both credentialsConfigmap and audience fields. Logic is correct, though it only runs when provider is "gcp".
charts/topograph/templates/deployment.yaml Refactored GCP credential mounting to support both service-account-key and Workload Identity Federation paths. Volume mounts and environment variables are correctly structured with proper subPath configuration for file mounts and appropriate conditional logic for auth method selection.
charts/topograph/values.k8s-gcp-federated-workload-identity-example.yaml New example values file for the WIF path; correctly omits serviceAccountKeysSecret to avoid conflicting with WIF, but does not document what the referenced credentialsConfigmap must contain or the expected token mount path.
charts/topograph/values.yaml Renames the commented-out credentialsSecretName hint to credentialsSecret; global.provider.params (the new GCP-specific params block) is absent from the defaults file, leaving its available sub-keys completely undocumented for chart users.
docs/providers/gcp.md Updated to reflect credential secret rename, but entirely lacks documentation for the newly-added Workload Identity Federation feature. No section covers WIF pool/provider setup, credentials config generation, or IAM bindings.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Helm Install / Upgrade] --> B{global.provider.name == gcp?}
    B -- No --> C[No GCP env vars or volumes]
    B -- Yes --> D{_validation.tpl checks}
    D -- both set --> E[FAIL: mutually exclusive]
    D -- WIF missing field --> F[FAIL: requires both fields]
    D -- Valid --> G{Which auth method?}

    G -- serviceAccountKeysSecret --> H[Set GOOGLE_APPLICATION_CREDENTIALS\n/etc/topograph/gcp/service-account-keys.json]
    H --> I[Mount Secret as volume\nat /etc/topograph/gcp/service-account-keys.json\nwith subPath for file mount]

    G -- workloadIdentityFederation --> J[Set GOOGLE_APPLICATION_CREDENTIALS\n/etc/topograph/gcp/credentials-config.json]
    J --> K[Mount credentialsConfigmap\nat /etc/topograph/gcp/]
    K --> L[Mount projected ServiceAccount token\nat /var/run/service-account/token]

    G -- neither set --> M[No GOOGLE_APPLICATION_CREDENTIALS set\nRelies on metadata server / GKE WI]

    N[config.credentialsSecret set?] -- Yes --> O[Mount secret at /etc/topograph/credentials\nAdd credentialsPath to ConfigMap]
    N -- No --> P[No generic credentials mount]
Loading

Last reviewed commit: 5c496c8

Comment thread charts/topograph/templates/deployment.yaml Outdated
Comment thread charts/topograph/templates/deployment.yaml Outdated
Comment thread charts/topograph/values.k8s-gcp-federated-workload-identity-example.yaml Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 66.35%. Comparing base (ebbb330) to head (5c496c8).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #224   +/-   ##
=======================================
  Coverage   66.35%   66.35%           
=======================================
  Files          82       82           
  Lines        4559     4559           
=======================================
  Hits         3025     3025           
  Misses       1415     1415           
  Partials      119      119           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread charts/topograph/templates/deployment.yaml
Comment thread charts/topograph/templates/deployment.yaml Outdated
Comment thread charts/topograph/values.yaml
@dmitsh dmitsh force-pushed the ds-gcp-helm branch 3 times, most recently from 369165b to b127be4 Compare March 3, 2026 23:40
Comment thread charts/topograph/templates/deployment.yaml
Comment thread charts/topograph/values.yaml
Comment thread charts/topograph/templates/deployment.yaml Outdated
Comment thread docs/providers/gcp.md
Comment thread charts/topograph/values.k8s-gcp-federated-workload-identity-example.yaml Outdated
Comment thread charts/topograph/templates/deployment.yaml
Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 4, 2026

Additional Comments (2)

docs/providers/gcp.md, line 71
No documentation for the new Workload Identity Federation feature

This PR adds full Helm support for GCP Workload Identity Federation (projected OIDC tokens + credentials config), but gcp.md has no section describing how to use it. A user who discovers values.k8s-gcp-federated-workload-identity-example.yaml has no documentation to explain:

  • How to create a GCP Workload Identity Pool and Provider that trusts Kubernetes service account tokens from the cluster.
  • What the credentialsConfigmap (referenced by global.provider.params.workloadIdentityFederation.credentialsConfigmap) must contain — specifically, the credential configuration JSON file must be stored under the key credentials-config.json and must reference the projected token path /var/run/service-account/token that the chart mounts.
  • What IAM principal bindings are needed (e.g., granting roles/compute.viewer to the identity pool principal).
  • How to generate the credential config file using gcloud iam workload-identity-pools create-cred-config.

Consider adding a new ## Authentication Using Workload Identity Federation section analogous to the existing service-account section, covering these steps so operators can self-serve the setup end-to-end.


charts/topograph/values.yaml, line 11
global.provider.params sub-keys are undocumented in default values

The new GCP-specific parameters (serviceAccountKeysSecret, workloadIdentityFederation.credentialsConfigmap, workloadIdentityFederation.audience) exist only in example files. Users inspecting the default values.yaml (which is the primary reference for helm show values) have no visibility into these options.

Consider adding commented-out stubs, similar to how other optional keys are documented:

global:
  provider:
    name: test
    # params:
    #   # GCP: mutually exclusive auth options
    #   # serviceAccountKeysSecret: ""
    #   # workloadIdentityFederation:
    #   #   credentialsConfigmap: ""
    #   #   audience: ""

This makes the available configuration surface discoverable without requiring users to locate the example files.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

@dmitsh dmitsh merged commit 57c5ca1 into main Mar 4, 2026
4 checks passed
@dmitsh dmitsh deleted the ds-gcp-helm branch March 4, 2026 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants