Repository navigation
Conversation
…IAM mode When AZURE_CLIENT_SECRET is set, both the Go object storage client (pkg/objectstorage) and the C++ chunk manager (AzureBlobChunkManager) now select DefaultAzureCredential before trying Workload Identity or Managed Identity. This enables SPN/service-principal auth via standard Azure env vars (AZURE_CLIENT_ID, AZURE_CLIENT_SECRET, AZURE_TENANT_ID) without requiring IMDS, a pod identity, or a VM-attached identity. Credential priority matches DefaultAzureCredential's own chain order: 1. AZURE_CLIENT_SECRET set -> DefaultAzureCredential (SPN via EnvironmentCredential) 2. AZURE_FEDERATED_TOKEN_FILE set -> WorkloadIdentityCredential 3. otherwise -> ManagedIdentityCredential Go: unit test added in util_test.go. C++: no behaviour change to existing Workload Identity / Managed Identity paths. /kind improvement Signed-off-by: Sarvesh Srivastava <sarveshsrivastava72@gmail.com>
sre-ci-robot
added
the
kind/improvement
|
Welcome @sarveshls! It looks like this is your first PR to milvus-io/milvus 🎉 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
mergify
Bot
added
dco-passed
|
[ci-v2-notice] To rerun ci-v2 checks, comment with:
If you have any questions or requests, please contact @zhikunyao. |
|
Related PRs applying the same credential selection pattern across the ecosystem:
|
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 14.3min | - |
| ❌ Code-Check | FAILURE | 2.4min | - |
| ✅ UT-Integration | SUCCESS | 64.5min | - |
| ✅ UT-GO | SUCCESS | 29.6min | - |
| ✅ UT-CPP-Cov | SUCCESS | 60.7min | 22338 total, 22338 passed, 0 failed |
Total: 120min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 10 miss, 10 measurable lines, 3 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 3 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/10 measurable lines, 6 unmeasured)
Failed Test Logs:
- Code-Check: view log
…name test Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
sarveshls
changed the title
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 13.6min | - |
| ❌ Code-Check | FAILURE | 3.5min | - |
| ✅ UT-GO | SUCCESS | 31.0min | - |
| ❌ UT-CPP-Cov | FAILURE | 56.3min | 22338 total, 22337 passed, 1 failed |
| ✅ UT-Integration | SUCCESS | 48.4min | 658 total, 650 passed, 0 failed, 8 skipped |
Total: 102min | Pipeline | Artifacts
Overall Coverage: 77.4%
Diff Coverage: CPP 0.0% (0 hit, 14 miss, 14 measurable lines, 7 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/14 measurable lines, 21 unmeasured)
Failed Test Logs:
This PR adds a Service Principal (client secret) credential path for Azure Blob in both the Go and C++ object storage clients; review found one lint failure introduced by the new Go code, a coverage gap in the new test, a C++ validation gap, and two pre-existing dependency hazards that leave other Azure storage paths on Managed Identity. Medium
Low
Pre-existing hazards — not introduced by this PR (follow-up only)The following hazards are real but pre-existing and were not introduced by this PR. They should be tracked as separate follow-up work and do not block this PR.
|
Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
sre-ci-robot
added
size/L
|
@sarveshls Please associate the related issue to the body of your Pull Request. (eg. "issue: #") |
Thanks for reviewing @tinswzy. I've made the changes you've addressed in the code. |
|
@bigsheeper (not able to mention zwd1208) |
|
@foxspy - your approval is required on one of the CIs please |
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 5.7min | - |
| ❌ Code-Check | FAILURE | 2.4min | - |
| ✅ UT-Integration | SUCCESS | 54.8min | - |
| ❌ UT-GO | FAILURE | 30.9min | - |
| ✅ UT-CPP-Cov | SUCCESS | 41.3min | 22338 total, 22338 passed, 0 failed |
Total: 104min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 16 miss, 16 measurable lines, 7 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/16 measurable lines, 21 unmeasured)
Failed Test Logs:
TestNewAzureClientWITakesPriorityOverSPN previously set CLIENT_ID and
TENANT_ID to non-empty UUIDs. With those values set, NewWorkloadIdentity-
Credential construction succeeds (token file is not read until GetToken,
not at construction time), so NewAzureObjectStorageClient returns nil error
and require.Error failed.
Fix: clear CLIENT_ID and TENANT_ID. The WI branch is still entered first
(FEDERATED_TOKEN_FILE is set), but NewWorkloadIdentityCredential now fails
at construction with a missing-tenant/client error — distinct from the SPN
validation error ('SPN authentication requires...'), which is what the
assert.NotContains assertion verifies.
Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
Fix two formatting issues detected by Code-Check CI: - Reformat has_client_secret bool assignment to fit ColumnLimit: 80 - Split long string literal in throw std::invalid_argument() across two lines Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 4.7min | - |
| ❌ Code-Check | FAILURE | 2.1min | - |
| ✅ UT-Integration | SUCCESS | 56.0min | - |
| ✅ UT-GO | SUCCESS | 9.0min | - |
| ✅ UT-CPP-Cov | SUCCESS | 41.2min | 22338 total, 22338 passed, 0 failed |
Total: 80min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 16 miss, 16 measurable lines, 7 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/16 measurable lines, 21 unmeasured)
Failed Test Logs:
- Code-Check: view log
Match the &&-continuation style used for has_federated_token above it, which is what clang-format-15 (ColumnLimit:80, AlignAfterOpenBracket: Align) produces — break at && and align the second operand, not break-after-= as Apple clang-format v21 did. Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 5.3min | - |
| ❌ Code-Check | FAILURE | 6.7min | - |
| ✅ UT-Integration | SUCCESS | 55.4min | - |
| ✅ UT-GO | SUCCESS | 30.2min | - |
| ✅ UT-CPP-Cov | SUCCESS | 59.5min | 22338 total, 22338 passed, 0 failed |
Total: 106min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 16 miss, 16 measurable lines, 8 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/16 measurable lines, 22 unmeasured)
Failed Test Logs:
- Code-Check: view log
Use the exact output of clang-format-15.0.0 (with project .clang-format): break after = and use IndentWidth continuation for the boolean, matching the formatter's preference over &&-alignment for this expression length. Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
…SPN validation The segcore error boundary check requires every throw in internal/core/src to use a milvus-typed exception (SegcoreError via ThrowInfo) so that it reaches the cgo boundary with a proper ErrorCode rather than UnexpectedError. Replace throw std::invalid_argument(...) with ThrowInfo(ConfigInvalid, ...) and add #include "common/EasyAssert.h" to bring in ThrowInfo/ConfigInvalid. Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
…Manager ThrowInfo(ConfigInvalid, ...) failed to compile because ConfigInvalid lives in the milvus namespace and AzureBlobChunkManager.cpp is in the azure namespace with no using-directive. Use milvus::ErrorCode::ConfigInvalid to match the pattern used in loon_ffi/util.cpp (the only other file outside the milvus namespace that throws this code). issue: milvus-io#54702 Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 4.6min | - |
| ❌ Code-Check | FAILURE | 2.3min | - |
| ✅ UT-Integration | SUCCESS | 55.9min | - |
| ✅ UT-GO | SUCCESS | 3.6min | - |
| ✅ UT-CPP-Cov | SUCCESS | 39.3min | 22338 total, 22338 passed, 0 failed |
Total: 77min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 16 miss, 16 measurable lines, 8 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/16 measurable lines, 22 unmeasured)
Failed Test Logs:
- Code-Check: view log
Advances the milvus-storage pin from 3bc763b to 1067385 so that the storage-v2 Azure filesystem path (azure_fs_producer.cpp use_iam) gains WI→SPN→MI credential priority in the same release as the C++ chunk manager. Key commits pulled in: - b420145 feat: support Azure SPN credentials via EnvironmentCredential in IAM mode (milvus-io#742) - 5523f37 fix: retry initial Azure SAS broker credential requests (milvus-io#743) - f943ea5 fix: invalidate manifest indexes by field ID (milvus-io#744) - dd87daa enhance: [StorageV2] Replace the footer LRU with file contexts (milvus-io#738) Note: woodpecker's Azure SPN fix (zilliztech/woodpecker#449) is merged to upstream/master but not yet tagged (post-v0.1.47). The go.mod pin will be bumped in a follow-up once v0.1.48 is released. issue: milvus-io#54702 Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
❌ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 3.7min | - |
| ❌ Code-Check | FAILURE | 4.6min | - |
| ✅ UT-Integration | SUCCESS | 54.2min | - |
| ✅ UT-GO | SUCCESS | 9.2min | - |
| ✅ UT-CPP-Cov | SUCCESS | 40.4min | 22338 total, 22338 passed, 0 failed |
Total: 80min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 16 miss, 16 measurable lines, 8 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/16 measurable lines, 22 unmeasured)
Failed Test Logs:
- Code-Check: view log
Paimon commit 1067385 pulls in paimon@0.4.0 which requires rustc >=1.94.0; milvus CI has 1.92.0 and fails. b420145 includes the Azure SPN fix (milvus-io#742) without the Paimon feature, so CI can build again. Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
✅ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 4.9min | - |
| ✅ Code-Check | SUCCESS | 6.8min | - |
| ✅ UT-Integration | SUCCESS | 55.7min | - |
| ✅ UT-GO | SUCCESS | 30.8min | - |
| ✅ UT-CPP-Cov | SUCCESS | 67.4min | 22338 total, 22338 passed, 0 failed |
Total: 106min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 15 miss, 15 measurable lines, 10 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/15 measurable lines, 24 unmeasured)
…mode (milvus-io#742) ## What When `AZURE_CLIENT_SECRET` is set, `AzureFileSystemProducer` now selects `ConfigureEnvironmentCredential()` (which reads `AZURE_CLIENT_ID`, `AZURE_CLIENT_SECRET`, and `AZURE_TENANT_ID`) instead of falling through to Managed Identity. This enables SPN/service-principal auth without requiring IMDS access. ## Credential selection order The selection now uses **WI → SPN → MI** priority: 1. `AZURE_FEDERATED_TOKEN_FILE` set → `ConfigureWorkloadIdentityCredential()` 2. `AZURE_CLIENT_SECRET` set → `ConfigureEnvironmentCredential()` (SPN) 3. otherwise → `ConfigureManagedIdentityCredential()` WI is checked before SPN so that existing deployments with valid Workload Identity config are not silently switched to a different identity if `AZURE_CLIENT_SECRET` happens to be present in the environment. When `AZURE_CLIENT_SECRET` is set, `AZURE_CLIENT_ID` and `AZURE_TENANT_ID` are validated upfront — returning a clear error at startup rather than silently falling through to another credential type. A startup log line is emitted per branch so the selected credential type is visible. ## Related PRs Same credential selection pattern applied across the ecosystem: | Repo | PR | |------|----| | zilliztech/woodpecker | [milvus-io#449](zilliztech/woodpecker#449) | | zilliztech/milvus-operator | [milvus-io#558](zilliztech/milvus-operator#558) | | milvus-io/milvus | [#54582](milvus-io/milvus#54582) | --------- Signed-off-by: Sarvesh Srivastava <sarveshsrivastava72@gmail.com> Signed-off-by: sarveshls <sarveshsrivastava72@gmail.com>
✅ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 5.8min | - |
| ✅ Code-Check | SUCCESS | 7.1min | - |
| ✅ UT-Integration | SUCCESS | 55.9min | - |
| ✅ UT-GO | SUCCESS | 3.5min | - |
| ✅ UT-CPP-Cov | SUCCESS | 42.4min | 22338 total, 22338 passed, 0 failed |
Total: 81min | Pipeline | Artifacts
Overall Coverage: 77.5%
Diff Coverage: CPP 0.0% (0 hit, 15 miss, 15 measurable lines, 10 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/15 measurable lines, 24 unmeasured)
|
@bigsheeper @tinswzy - CI Loop has passed. I've updated the milvus-storage hash in 'internal/core/thirdparty/milvus-storage/CMakeLists.txt', but the Woodpecker tag still needs to updated in the go.mod, what should we do about that? |
|
@bigsheeper @tinswzy - All checks have passed, awaiting your review/approval on the PR |
|
Hey @bigsheeper @tinswzy , requesting your review and approval on this PR |
FYI - operator PR is now merged. |
✅ CI Loop Results
|
| Stage | Result | Duration | Tests |
|---|---|---|---|
| ✅ Build | SUCCESS | 5.5min | - |
| ✅ Code-Check | SUCCESS | 6.6min | - |
| ✅ UT-Integration | SUCCESS | 54.5min | - |
| ✅ UT-GO | SUCCESS | 35.0min | - |
| ✅ UT-CPP-Cov | SUCCESS | 40.7min | 22633 total, 22633 passed, 0 failed |
Total: 112min | Pipeline | Artifacts
Overall Coverage: 78.0%
Diff Coverage: CPP 0.0% (0 hit, 15 miss, 15 measurable lines, 10 unmeasured) | Go N/A (0 hit, 0 miss, 0 measurable lines, 14 unmeasured)
Diff Coverage HTML: view changed lines
Go Patch Warning: WARNING: Go patch coverage unavailable; changed Go lines did not match the coverage profile.
Total Patch Coverage: N/A (untrusted: changed source lines have no coverage data)
Measured Patch Coverage: 0.0% (0/15 measurable lines, 24 unmeasured)
|
Merge conflicts resolved. @bigsheeper @tinswzy pls review the PR |
What
When
AZURE_CLIENT_SECRETis set, both the Go object storage client(
pkg/objectstorage/util.go) and the C++ chunk manager(
AzureBlobChunkManager.cpp) now build an explicit credential for SPN authinstead of
DefaultAzureCredential.Why explicit credential instead of DefaultAzureCredential
DefaultAzureCredentialbuilds a chain internally. IfAZURE_CLIENT_IDor
AZURE_TENANT_IDare missing,EnvironmentCredentialfails silentlyinside the chain and DAC falls through to the next source (Workload Identity,
Managed Identity, Azure CLI). The result is that storage is accessed as
a different identity with no error at startup. Using
ClientSecretCredential(Go) /
ClientSecretCredential(C++ Azure SDK) fails fast at constructionif any required variable is absent.
Credential selection order
Both Go and C++ now use WI → SPN → MI priority:
AZURE_FEDERATED_TOKEN_FILEset → Workload IdentityAZURE_CLIENT_SECRETset → explicit SPN credential (returns error ifAZURE_TENANT_IDorAZURE_CLIENT_IDare missing)WI is checked before SPN so that pods with valid Workload Identity config
are not silently switched to SPN if a leftover or expired
AZURE_CLIENT_SECRETis present in the environment.Dependency pin bumps‼️
internal/core/thirdparty/milvus-storage/CMakeLists.txt):pin advanced from
3bc763b→b420145, which is the merged commit for#742.
The storage-v2
use_iampath (azure_fs_producer.cpp) now has the sameWI→SPN→MI priority as the C++ chunk manager.
The subsequent upstream commit (
1067385, Paimon feature) was excludedbecause it requires rustc ≥ 1.94.0 and milvus CI currently has 1.92.0.
pkg/go.mod): #449is merged to
upstream/masterbut not yet in a tagged release (post-v0.1.47).The
go.modpin will be bumped in a follow-up PR once v0.1.48 is released.Until then, the woodpecker WAL path in an SPN-only environment falls back
to Managed Identity.
issue: #54702