feat(azure-ac): add Azure App Configuration provider - #659
Conversation
Reads key-values from an Azure App Configuration store, the Azure home for non-secret configuration. RemoteRead only; optional label and prefix. Closes jdx#658
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Azure App Configuration as a read-only remote provider with authenticated key retrieval, CLI support, schema updates, provider-add tests, and documentation. ChangesAzure App Configuration provider
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Fnox
participant DeveloperToolsCredential
participant AzureAppConfiguration
Fnox->>DeveloperToolsCredential: Request Entra ID token
DeveloperToolsCredential-->>Fnox: Return access token
Fnox->>AzureAppConfiguration: GET labeled /kv/{key}
AzureAppConfiguration-->>Fnox: Return key/value payload or HTTP error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Greptile SummaryAdds the read-only Azure App Configuration provider and addresses the previously reported endpoint, label, and connection-validation issues.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (5): Last reviewed commit: "fix(azure-ac): require a key-value list ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // An empty label is a distinct label in App Configuration, not the absence of one. | ||
| Ok(Self { | ||
| endpoint: endpoint.trim_end_matches('/').to_string(), | ||
| label: label.filter(|l| !l.is_empty()), |
There was a problem hiding this comment.
When fnox.toml explicitly sets label = "", configuration resolution preserves that value but this filter converts it to None, causing the request to omit the label parameter and retrieve the unlabeled key-value instead of the distinct empty-label value.
| label: label.filter(|l| !l.is_empty()), | |
| label, |
Knowledge Base Used: Provider system
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@crates/fnox-core/src/providers/azure_ac.rs`:
- Around line 162-171: The test_connection method currently ignores the 404
status that get intentionally passes through. Update test_connection to inspect
the response status and return an error for any non-success response, while
preserving successful connection behavior and the existing filtered /kv request.
- Around line 28-34: Validate the endpoint in AzureAppConfiguration provider
constructor new before storing or using it, requiring HTTPS and an approved
Azure App Configuration host suffix, including supported sovereign-cloud
domains. Reject invalid or untrusted endpoints with an error, while preserving
the existing trailing-slash normalization and label/prefix handling for valid
endpoints.
- Around line 7-10: Update the SCOPE constant in the Azure App Configuration
provider to use the data-plane resource scope https://azconfig.io/.default
instead of the current management-plane scope, while leaving PROVIDER_NAME,
PROVIDER_URL, and API_VERSION unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e109e1fa-179f-4651-b769-b4f9e8cf8b45
📒 Files selected for processing (15)
README.mdcrates/fnox-core/providers/azure-ac.tomlcrates/fnox-core/src/providers/azure_ac.rscrates/fnox-core/src/providers/mod.rsdocs/.vitepress/config.mjsdocs/cli/commands.jsondocs/cli/provider/add.mddocs/index.mddocs/providers/azure-ac.mddocs/providers/overview.mddocs/public/schema.jsonfnox.usage.kdlsrc/commands/provider/add.rssrc/commands/provider/mod.rstest/provider_add.bats
| const PROVIDER_NAME: &str = "Azure App Configuration"; | ||
| const PROVIDER_URL: &str = "https://fnox.jdx.dev/providers/azure-ac"; | ||
| const SCOPE: &str = "https://appconfig.azure.com/.default"; | ||
| const API_VERSION: &str = "2023-11-01"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'const SCOPE' crates/fnox-core/src/providers/azure_ac.rs
curl -fsSL 'https://learn.microsoft.com/en-us/rest/api/data-plane/appconfiguration/get-key-values/get-key-values?view=rest-data-plane-appconfiguration-2023-11-01' |
rg -F 'https://azconfig.io/.default'Repository: jdx/fnox
Length of output: 1132
Use the App Configuration data-plane scope.
Azure App Configuration uses https://azconfig.io/.default; the current https://appconfig.azure.com/.default scope is for a different resource and won’t authorize data-plane requests.
Proposed fix
-const SCOPE: &str = "https://appconfig.azure.com/.default";
+const SCOPE: &str = "https://azconfig.io/.default";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const PROVIDER_NAME: &str = "Azure App Configuration"; | |
| const PROVIDER_URL: &str = "https://fnox.jdx.dev/providers/azure-ac"; | |
| const SCOPE: &str = "https://appconfig.azure.com/.default"; | |
| const API_VERSION: &str = "2023-11-01"; | |
| const PROVIDER_NAME: &str = "Azure App Configuration"; | |
| const PROVIDER_URL: &str = "https://fnox.jdx.dev/providers/azure-ac"; | |
| const SCOPE: &str = "https://azconfig.io/.default"; | |
| const API_VERSION: &str = "2023-11-01"; |
🤖 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 `@crates/fnox-core/src/providers/azure_ac.rs` around lines 7 - 10, Update the
SCOPE constant in the Azure App Configuration provider to use the data-plane
resource scope https://azconfig.io/.default instead of the current
management-plane scope, while leaving PROVIDER_NAME, PROVIDER_URL, and
API_VERSION unchanged.
The endpoint decides where fnox sends an Entra token, so an auto-loaded fnox.toml could point it at an arbitrary host. Parse it, require https, match the parsed host against the App Configuration domains, and derive the audience from that host.
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
Review rounds are addressed — On the perf gate: it's reporting a real regression, not noise. Measured on x86 runners, both binaries built first and then measured interleaved, 30 runs per side:
One existing provider costs ~0.95% and azure-ac costs roughly 3× that, but deleting 277 of its 310 lines moves it 0.06% — so the cost is the config variant, not the provider code. There's no new dependency (the diff touches no Cargo file), and the schema fragment is 6 fields with 1 required, which is unremarkable next to azure-kms or gcp-kms. On arm64 both azure-ac and doppler measure ~1%, so the 3× looks x86-specific. One thing that may interest you independently of this PR: with azure-ac present the bench goes bimodal — 6,850,655 min against a 6,978,267 median, same binary, same machine. That ~128k swing is why one commit on this branch passed the gate and the others failed. I don't have a way to make this cheaper from inside the provider. How would you like me to handle it? |
|
I investigated the performance gate in detail. The reported Cachegrind profiles show that the higher mode is dominated by glibc I opened #670 to stabilize the benchmark: pin Ubuntu 24.04 and Rust 1.91.1, use one Tokio worker, start a compatible new measurement series, and set the gate to 1.25% so intentional provider-schema growth fits without masking larger regressions. My recommendation is not to require an Azure-specific performance optimization here. Once #670 lands and establishes the new baseline, this PR should rerun/rebase against it. This is independent of Greptile's AI-assisted — Tool: Codex; model: unavailable/unavailable; version: unavailable. |
A 200 from a reverse proxy or captive portal was accepted as a healthy store. Deserialising the list payload is what proves the endpoint is App Configuration.
2 participants
Adds a read-only
azure-acprovider for Azure App Configuration. Discussed in #658.Azure splits its config surface in two: Key Vault for secrets, which
azure-smalready covers, and App Configuration for everything that is not a secret — endpoints, feature toggles, tuning values. Without the second half, those values get hardcoded infnox.tomlor exported from a shell script sitting next to it, andfnox.tomlstops being the one place to look.RemoteReadonly, so nothing writes back. Optionallabelandprefix. A key in App Configuration can carry several values distinguished by a label, which maps onto profiles:Omitting
labelreturns the key-value that has no label.Notes
No new dependencies —
azure_identityandazure_coreare already in the tree forazure-sm. Auth isDeveloperToolsCredential, same asazure-sm.The scope constant is
https://appconfig.azure.com/.default. Azure's own TypeSpec still saysazconfig.io, but the Go, Python, .NET, Java and JS SDKs all useappconfig.azure.com, and the live test below confirms the SDKs are right.fnox setagainst aRemoteReadprovider writes the plaintext intofnox.tomlrather than erroring (src/commands/set.rs:141-183). That is pre-existing behaviour shared withpasswordstate, so I documented it in the provider page rather than change it here.Verification
cargo test --workspace249 passed,cargo clippy --all-targets -D warningsclean,cargo fmtclean,bats test/provider_add.bats6/6,mise run renderidempotent. All at MSRV 1.91.1.Also ran against a real store, since a few things could not be confirmed from the docs. Flat keys, hierarchical keys (
myapp/api-url, confirming/percent-encodes correctly in the path), the same key reached viaprefix, and label vs no-label all resolved. The 404, 403 and connection-failure paths each produced the right error variant. The store has been deleted.Summary by CodeRabbit
azure-ac) as a read-only remote configuration provider.fnox provider addforazure-ac, including starter template generation.endpointwith optionallabel/prefix.azure-acand linked it in provider listings/navigation and CLI docs.azure-ac, plus added unit test coverage for connection validation and endpoint handling.