feat(operator-wandb): use deployment-wide Azure storage identity - #606
Conversation
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/operator-wandb/tests/azure_storage_auth_test.yaml (1)
36-58: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winMissing scenario: customer bucket with
azureAuthMethod: workloadIdentityand no bucket-scoped IDs.
_bucket.tplassignsglobal.azureStorageIdentity.tenantId/clientIdfor customer buckets whenazureAuthMethodisworkloadIdentity, butazure_storage_auth_test.yamlonly covers this with bucket-scoped legacy IDs or access-key BYOB. Add a test analogous to the JSON helper serialization case, usingglobal.bucket.provider: az, noazureTenantId/azureClientId, and literal orvalueFromcredentials underglobal.azureStorageIdentityto assertAZURE_STORAGE_TENANT_IDandAZURE_STORAGE_CLIENT_ID.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/operator-wandb/tests/azure_storage_auth_test.yaml` around lines 36 - 58, Add a workload-identity customer-bucket test in azure_storage_auth_test.yaml using global.bucket.provider az, azureAuthMethod workloadIdentity, no bucket-scoped tenant/client IDs, and credentials from global.azureStorageIdentity (literal or valueFrom). Render templates/bucket.yaml and assert AZURE_STORAGE_TENANT_ID and AZURE_STORAGE_CLIENT_ID are populated, matching the JSON helper serialization scenario.
🧹 Nitpick comments (1)
charts/wandb-base/templates/_helpers.tpl (1)
102-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSame
$usesDeploymentIdentitycondition duplicated across wandb-base and bufstream. Both helpers independently recompute "customer bucket +azureAuthMethod: workloadIdentity, or default bucket +provider: az" to decide whether a workload should use the deployment-wide Azure storage identity, with no shared source of truth.
charts/wandb-base/templates/_helpers.tpl#L102-L125: extract the$usesDeploymentIdentitycomputation (lines 108-111) into a single named template that both this helper and the bufstream one can call.charts/operator-wandb/charts/bufstream/templates/_helpers.tpl#L28-L39: call the shared template instead of recomputing$usesDeploymentIdentity(lines 34-37) inline.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/wandb-base/templates/_helpers.tpl` around lines 102 - 125, Extract the duplicated deployment-identity condition from wandb-base’s azureStorageServiceAccountEnabled helper into a shared named template, preserving the existing customer-bucket and default-bucket checks. Update charts/wandb-base/templates/_helpers.tpl lines 102-125 to call that template, and update charts/operator-wandb/charts/bufstream/templates/_helpers.tpl lines 28-39 to replace its inline $usesDeploymentIdentity calculation with the shared helper call.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@charts/operator-wandb/tests/azure_storage_auth_test.yaml`:
- Around line 36-58: Add a workload-identity customer-bucket test in
azure_storage_auth_test.yaml using global.bucket.provider az, azureAuthMethod
workloadIdentity, no bucket-scoped tenant/client IDs, and credentials from
global.azureStorageIdentity (literal or valueFrom). Render templates/bucket.yaml
and assert AZURE_STORAGE_TENANT_ID and AZURE_STORAGE_CLIENT_ID are populated,
matching the JSON helper serialization scenario.
---
Nitpick comments:
In `@charts/wandb-base/templates/_helpers.tpl`:
- Around line 102-125: Extract the duplicated deployment-identity condition from
wandb-base’s azureStorageServiceAccountEnabled helper into a shared named
template, preserving the existing customer-bucket and default-bucket checks.
Update charts/wandb-base/templates/_helpers.tpl lines 102-125 to call that
template, and update
charts/operator-wandb/charts/bufstream/templates/_helpers.tpl lines 28-39 to
replace its inline $usesDeploymentIdentity calculation with the shared helper
call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b294bb59-cb46-4b23-817c-94cd0a598f1f
⛔ Files ignored due to path filters (2)
test-configs/operator-wandb/__snapshots__/azure-workload-identity-refs.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/azure-workload-identity.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
charts/operator-wandb/charts/bufstream/templates/_helpers.tplcharts/operator-wandb/charts/bufstream/templates/cluster-role-bind.yamlcharts/operator-wandb/charts/bufstream/templates/deployment.yamlcharts/operator-wandb/charts/bufstream/templates/service-account.yamlcharts/operator-wandb/templates/_bucket.tplcharts/operator-wandb/templates/azure-storage-serviceaccount.yamlcharts/operator-wandb/tests/azure_storage_auth_test.yamlcharts/operator-wandb/values.yamlcharts/wandb-base/templates/_helpers.tplcharts/wandb-base/templates/serviceaccount.yamltest-configs/operator-wandb/azure-workload-identity-refs.yaml
|
@coderabbitai full review. |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 55 minutes. |
Surface azureTenantId / azureClientId from the bucket config and, when both are set, inject AZURE_STORAGE_TENANT_ID + AZURE_STORAGE_CLIENT_ID into the app deployments alongside the (now optional) AZURE_STORAGE_KEY. This lets gorilla authenticate to Azure Blob storage with an Entra ID workload identity (user-delegation SAS) instead of a storage account access key, which dedicated Azure customers (e.g. Novo Nordisk) must adopt before losing Account SAS. - _bucket.tpl: read azureTenantId/azureClientId from bucket / defaultBucket and add them to the wandb.bucket helper output. - _env.tpl (wandb.bucketEnvs): emit the two env vars only when present, so existing key-only deployments are unchanged. - values.yaml: document the fields and the workload-identity prerequisites. Pairs with core (reads these env vars) and console (writes the fields into the user spec). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
charts/operator-wandb/tests/azure_storage_auth_test.yaml (2)
320-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert shared-account and pod-label wiring for workload-identity BYOB.
This case verifies only the Azure environment variables. Also assert
spec.template.spec.serviceAccountNameandspec.template.metadata.labels["azure.workload.identity/use"]. The test can otherwise pass if the app keeps its component account or disables Azure Workload Identity injection.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/operator-wandb/tests/azure_storage_auth_test.yaml` around lines 320 - 361, Extend the workload-identity BYOB test identified by “uses deployment identity references for workload-identity BYOB” to assert that spec.template.spec.serviceAccountName uses the configured shared Azure identity service account and that spec.template.metadata.labels["azure.workload.identity/use"] is enabled. Keep the existing Azure environment-variable assertions unchanged.
363-491: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover Weave URI mapping and object-prefix rejection.
These cases verify ServiceAccount selection only. Add an assertion for
WF_FILE_STORAGE_URIthat uses the managed default bucket, notglobal.bucket. Add afailedTemplatecase foruseDefaultBucket: truewith a path such asmanaged-container/object-prefix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/operator-wandb/tests/azure_storage_auth_test.yaml` around lines 363 - 491, Extend the Weave deployment tests around the useDefaultBucket cases to assert WF_FILE_STORAGE_URI is built from global.defaultBucket’s name and path, not global.bucket. Add a failedTemplate test for useDefaultBucket: true with a defaultBucket path containing an object prefix such as managed-container/object-prefix, and verify rendering is rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@charts/operator-wandb/tests/azure_storage_auth_test.yaml`:
- Around line 320-361: Extend the workload-identity BYOB test identified by
“uses deployment identity references for workload-identity BYOB” to assert that
spec.template.spec.serviceAccountName uses the configured shared Azure identity
service account and that
spec.template.metadata.labels["azure.workload.identity/use"] is enabled. Keep
the existing Azure environment-variable assertions unchanged.
- Around line 363-491: Extend the Weave deployment tests around the
useDefaultBucket cases to assert WF_FILE_STORAGE_URI is built from
global.defaultBucket’s name and path, not global.bucket. Add a failedTemplate
test for useDefaultBucket: true with a defaultBucket path containing an object
prefix such as managed-container/object-prefix, and verify rendering is
rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2501072d-f61c-4f9c-9fa6-9d176ac0ccdc
⛔ Files ignored due to path filters (57)
charts/lumen/Chart.lockis excluded by!**/*.lockcharts/operator-wandb/Chart.lockis excluded by!**/*.lockcharts/orchestrator/Chart.lockis excluded by!**/*.locktest-configs/operator-wandb/__snapshots__/activity-store-disabled.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/activity-store-serve-backfill-off.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/azure-byob-access-key.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/azure-workload-identity-refs.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/azure-workload-identity.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/default.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/fmb.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/glue-leader-election.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/history-reader.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/historystore-parquet-grpc.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/historystore-parquet-only.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/keda-api-prometheus.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/keda-executor.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/keda-frfu.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/keda-parquet.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/local-bypass-no-app.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/local-bypass-with-app.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/mcp-server-empty-trace-url.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/mcp-server-external-trace.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/mcp-server-no-weave.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/mcp-server.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/no-local-bypass.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/oidc-secret-from-k8s.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/olap-features-enabled.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/olap-multi-feature.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/run-store-accelerator-run-updater.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/runs-v2-bufstream.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-api-rate-limits.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-ch-migration-job-no-olap.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-ch-migration-job.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-console-env-defaults.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-console-env.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-console-extraEnv.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-enable-backfill.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-image-digest-override.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-image-tag-override.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-lumen-aws.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-lumen-azure.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-lumen-gcp.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-lumen-onprem-custom-bucket.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-lumen-onprem-default-bucket.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-mysql-cacert-inline.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-no-oidc-settings-extra-cors.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-oidc-settings-default.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-oidc-settings-extra-cors.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-priority-classes.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-smtp-mail-from-secret.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-smtp-mail-from.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/snap-tolerations-and-selectors.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/url-encoded-password.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/user-defined-clickhouse-secret.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/user-defined-secrets.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/weave-trace-with-worker.snapis excluded by!**/*.snaptest-configs/operator-wandb/__snapshots__/weave-trace.snapis excluded by!**/*.snap
📒 Files selected for processing (21)
.github/workflows/test-operator-wandb.yamlcharts/lumen/Chart.yamlcharts/operator-wandb/Chart.yamlcharts/operator-wandb/charts/bufstream/templates/_helpers.tplcharts/operator-wandb/charts/bufstream/templates/cluster-role-bind.yamlcharts/operator-wandb/charts/bufstream/templates/deployment.yamlcharts/operator-wandb/charts/bufstream/templates/service-account.yamlcharts/operator-wandb/templates/_bucket.tplcharts/operator-wandb/templates/_env.tplcharts/operator-wandb/templates/azure-storage-serviceaccount.yamlcharts/operator-wandb/templates/bucket.yamlcharts/operator-wandb/templates/weave-trace.yamlcharts/operator-wandb/tests/azure_storage_auth_test.yamlcharts/operator-wandb/values.yamlcharts/orchestrator/Chart.yamlcharts/wandb-base/Chart.yamlcharts/wandb-base/templates/_helpers.tplcharts/wandb-base/templates/serviceaccount.yamltest-configs/operator-wandb/azure-byob-access-key.yamltest-configs/operator-wandb/azure-workload-identity-refs.yamltest-configs/operator-wandb/azure-workload-identity.yaml
🚧 Files skipped from review as they are similar to previous changes (15)
- charts/operator-wandb/charts/bufstream/templates/deployment.yaml
- .github/workflows/test-operator-wandb.yaml
- charts/wandb-base/templates/serviceaccount.yaml
- charts/operator-wandb/charts/bufstream/templates/_helpers.tpl
- test-configs/operator-wandb/azure-workload-identity.yaml
- charts/operator-wandb/charts/bufstream/templates/cluster-role-bind.yaml
- charts/operator-wandb/templates/weave-trace.yaml
- charts/operator-wandb/templates/bucket.yaml
- test-configs/operator-wandb/azure-byob-access-key.yaml
- test-configs/operator-wandb/azure-workload-identity-refs.yaml
- charts/operator-wandb/templates/_bucket.tpl
- charts/operator-wandb/templates/_env.tpl
- charts/operator-wandb/charts/bufstream/templates/service-account.yaml
- charts/operator-wandb/templates/azure-storage-serviceaccount.yaml
- charts/operator-wandb/values.yaml
Summary
This chart adds a deployment-wide Azure storage identity while preserving existing key-based and legacy BYOB behavior.
global.azureStorageIdentityholds the deployment tenant, client, principal, and shared ServiceAccount contractwandb-bucket-accessServiceAccount by default, or uses an externally created account withcreate: falsewandb-base, Weave, and Bufstream storage consumers select the shared account and keep their RoleBindings alignedglobal.bucket.azureAuthMethodlets an Azure customer bucket selectaccessKeyorworkloadIdentityWeave file storage remains off by default.
global.weave-trace.fileStorage.useDefaultBucketmaps only the managedglobal.defaultBuckettoWF_FILE_STORAGE_URI; it rejects bucket paths containing an object prefix. Mixed access-key BYOB plus Weave default-bucket identity is covered explicitly.Grant the deployment identity
ReaderandStorage Blob Data Contributoron the storage account. Existing Azure BYOB continues to use storage-account keys unless it explicitly selectsworkloadIdentity.The operator chart remains at feature release
0.44.0.Verification
ct lintAzure QA validation
Validated on 2026-07-30 with chart prerelease
0.44.0-PR606-13c71a2band Core prerelease0.84.0-azure-sas-blob-test.1.wandb verifycheck passed on the restored runtimeThe final follow-up commit adds regression coverage only; it does not change rendered chart output.
Rollout
Release this chart before enabling wandb/core#49026. Keep the Core migration disabled for existing installations until they are explicitly selected for rollout.
Jira: WB-38011
Companion PRs: wandb/core#49026, wandb/docs#2967
Summary by CodeRabbit
New Features
Bug Fixes
Tests