Repository navigation
Add connection string support to AZURE_BLOB plugin - #1351
Conversation
Specifically allows to easily connect to Azurite for local testing instead of requiring authentication using Microsoft Entra Id and making acutal API requests to Azure Blob Storage. In addition, nixlbench and the end to end tests were updated to accept connection strings to be able to test directly against Azurite. Signed-off-by: Kyle Knapp <kyleknapp@microsoft.com>
|
👋 Hi kyleknap! Thank you for contributing to ai-dynamo/nixl. Your PR reviewers will review your contribution then trigger the CI to test your changes. 🚀 |
📝 WalkthroughWalkthroughThis change introduces Azure Blob connection string support as an alternative authentication method to account URL across the nixlbench benchmark and Azure Blob plugin. Documentation is expanded with Azurite configuration guidance, and logic updated to prefer connection string when available, with account URL as fallback. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmark/nixlbench/src/utils/utils.cpp (1)
629-636:⚠️ Potential issue | 🟠 MajorDo not print full Azure connection strings to stdout.
Line 634 prints the entire connection string, which embeds an account key and will leak secrets into logs and shells. Please redact or omit the value.🔒 Proposed redaction
- printOption("Azure Blob Storage connection string " - "(--azure_blob_connection_string=connection-string)", - azure_blob_connection_string); + if (!azure_blob_connection_string.empty()) { + printOption("Azure Blob Storage connection string " + "(--azure_blob_connection_string=connection-string)", + "<redacted>"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmark/nixlbench/src/utils/utils.cpp` around lines 629 - 636, The print of azure_blob_connection_string in utils.cpp (call site using printOption when backend == XFERBENCH_BACKEND_AZURE_BLOB) leaks secrets—change the behavior to avoid printing the full connection string: either omit the value entirely or replace it with a redacted placeholder (e.g., "<redacted>" or show only a non-sensitive suffix/prefix). Update the printOption invocation for azure_blob_connection_string so it passes a redacted string instead of the raw azure_blob_connection_string variable (or add a small helper that masks the secret) while keeping the descriptive label unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmark/nixlbench/README.md`:
- Around line 779-809: Replace the hard-coded Azurite account key string with a
placeholder or env var (e.g. use '<AZURITE_CONNECTION_STRING>' or an environment
variable) wherever it's used (the az storage container create
--connection-string and the --azure_blob_connection_string CLI flag), and add
blank lines before and after each fenced code block to address MD031 so all
fenced blocks in README.md are separated by blank lines.
In `@src/plugins/azure_blob/README.md`:
- Around line 114-149: The README contains literal Azurite connection strings
and missing blank lines around fenced blocks; remove the hard-coded AccountKey
and connection string values and replace them with a placeholder (e.g.,
<AZURITE_CONNECTION_STRING>) and environment variable references
(AZURE_STORAGE_CONNECTION_STRING / AZURE_STORAGE_CONTAINER_NAME) wherever the
examples show the connection string (the bash az storage container create
command, the nixl_b_params_t connection_string entry, and the export lines), and
ensure there is a blank line before and after each fenced code block to satisfy
MD031; keep references to agent.createBackend("AZURE_BLOB", ... ) and
nixl_b_params_t so the examples still demonstrate usage but without secrets.
---
Outside diff comments:
In `@benchmark/nixlbench/src/utils/utils.cpp`:
- Around line 629-636: The print of azure_blob_connection_string in utils.cpp
(call site using printOption when backend == XFERBENCH_BACKEND_AZURE_BLOB) leaks
secrets—change the behavior to avoid printing the full connection string: either
omit the value entirely or replace it with a redacted placeholder (e.g.,
"<redacted>" or show only a non-sensitive suffix/prefix). Update the printOption
invocation for azure_blob_connection_string so it passes a redacted string
instead of the raw azure_blob_connection_string variable (or add a small helper
that masks the secret) while keeping the descriptive label unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (7)
benchmark/nixlbench/README.mdbenchmark/nixlbench/src/utils/utils.cppbenchmark/nixlbench/src/utils/utils.hbenchmark/nixlbench/src/worker/nixl/nixl_worker.cppsrc/plugins/azure_blob/README.mdsrc/plugins/azure_blob/azure_blob_client.cpptest/gtest/plugins/azure_blob/azure_blob_test.cpp
|
/build |
|
/build |
|
/ok to test 56855be |
|
/build |
Specifically allows to easily connect to Azurite for local testing instead of requiring authentication using Microsoft Entra Id and making acutal API requests to Azure Blob Storage. In addition, nixlbench and the end to end tests were updated to accept connection strings to be able to test directly against Azurite. Signed-off-by: Kyle Knapp <kyleknapp@microsoft.com> Co-authored-by: ovidiusm <ovidium@nvidia.com>
What?
Adds support for Azure Storage connection strings for
AZURE_BLOBplugin. When set using theconnection_stringbackend parameter orAZURE_STORAGE_CONNECTION_STRINGenv var, it overrides the account URL and auth is sourced from the connection string instead of default Azure credentials.In addition, nixlbench and the end to end tests were updated to accept connection strings to be able to test directly against Azurite.
Why?
This allows to easily connect to Azurite for local testing instead of requiring authentication using Microsoft Entra Id and making actual API requests to Azure Blob Storage. While technically, you can use OAuth to communicate to Azurite, it requires additional scaffolding that is not as straightforward as just providing its well-known connection string.
Summary by CodeRabbit
New Features
Documentation