fix: Restructure infrastructure and enhance WAF implementation - #1046
Merged
Conversation
…race wait - Add manage-network-access.ps1 to temporarily enable public network access on ACR/SQL/Storage/API/frontend during postprovision, and revert to private afterward - Wrap postprovision hook in azure.yaml with Enable/Disable calls around build/data-setup steps - Add Wait-ForIngestionCompletion to setup-data.ps1 to fix a restart-race where ingestion was interrupted before completion Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…d /api proxy, use .venv in post-provision - ACR adminUserEnabled and App Service admin-cred image pull now gated on enablePrivateNetworking (managed identity kept for public mode) - Add ENABLE_PRIVATE_NETWORKING output so post-provision scripts detect mode - Frontend nginx reverse-proxies /api to the backend over the VNet in private mode (fixes 405) - Keep vnetRouteAllEnabled=true when reverting network access - setup-data/setup-agent use the project .venv interpreter - Regenerated infra/main.json and infra/avm/main.json
Enable azureADAuthenticationAsArmPolicyStatus on ACR (both AVM and vanilla flavors) so App Service system-assigned MI can fetch ACR tokens; disable admin user; grant AcrPull to app MIs and AcrPush to deployer; wire imagePullTraffic/acrUseManagedIdentityCreds; build script repoints images via MI (no creds). Validated on WAF env over public and private-endpoint pull paths. Recompiled ARM JSON for all three entrypoints.
…afe) Wait-ForIngestionCompletion now verifies each uploaded file is indexed in Azure AI Search (source_file chunk count) instead of the API's in-memory /files list, which is stale on multi-instance WAF deployments. Adds Invoke-UploadBatchWithRetry (per-batch upload retry) and timed re-submit rounds to recover Content Understanding timeouts, with a SQL /refresh fallback when Search is unreachable. Ensures files are fully processed before the app is recycled/network re-locked.
On multi-instance App Service deploys, workers race to lazily create the shared CU analyzer and enrichment agent. Losers got a 409 Conflict and proceeded immediately, then failed: the analyzer path hit 404 ModelNotFound ('not ready for analysis') and files were marked failed with 0 chunks; the enrichment path disabled enrichment for that file.
service.py: add _wait_for_analyzer_ready() and poll the analyzer resource status until ready/succeeded on both the 409-conflict and GET-200 (not-yet-ready) branches before analyzing. enrichment_agent.py: treat a concurrent-create conflict as 'already exists' and reuse the agent instead of failing.
Drive manage-network-access.ps1 Enable/Disable off the ENABLE_PRIVATE_NETWORKING azd output instead of a temp state file, so a WAF deployment is reliably re-locked even if Enable ran in another session. Non-private deployments now skip network toggling entirely (build/SQL/data steps still run). Leave the user-facing frontend app public on Disable, and professionalize the terminal messages.
…ing and SQL firewall rule for non-azd/AVM testing Enable running post-provision scripts (setup-data, setup-sql-roles, setup-agent, connect-data, manage-network-access) and image build/push against an explicit resource group without azd env. Auto-generate a globally-unique ACR name (acrkm<suffix>) and rename the SQL firewall rule off the reserved word 'Windows' so fresh deployments succeed.
…restore If the caller isn't the SQL server's Entra ID admin, temporarily set the current principal (user or SP) as admin, run the role grant with a propagation retry, then restore the original admin in a finally block. Add -SkipAdminElevation to opt out. Fixes login 18456 when the admin is a managed identity.
Resolve config from the target resource group's deployed backend app settings when -ResourceGroupName is passed, instead of the local azd env, so the search index is created on and verified against the correct search service. Replace per-poll Write-Host spam with a Write-Progress bar, route diagnostics to Write-Verbose/Write-Warning, and return an ingestion status object so incomplete runs are reported accurately.
…-Architected Framework (WAF) configuration and deployment flavors in README, CustomizingAzdParameters, and DeploymentGuide.
fix: Select Azure credentials by environment
Pavan-Microsoft
marked this pull request as ready for review
August 14, 2026 04:13
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 94 out of 97 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/api/modules/ingestion/azure_storage.py:37
- Credential selection falls back to ManagedIdentityCredential for any non-"dev" APP_ENV value (including the default empty string). Local dev helpers (e.g. infra/scripts/utilities/start-local-backend.ps1) don't set APP_ENV, so this will attempt MSI locally and fail. Consider treating empty/non-prod envs as AzureCliCredential, and only using ManagedIdentityCredential when APP_ENV is prod/production (see src/api/config.py:173).
src/api/modules/ingestion/queue_service.py:35 - Same issue as azure_storage.py: when APP_ENV is unset/empty (the Settings default), this code will attempt ManagedIdentityCredential locally and fail. Only use ManagedIdentityCredential for prod/production; otherwise prefer AzureCliCredential.
src/api/modules/data_sources/azure_search.py:28 - Credential selection uses ManagedIdentityCredential for any non-"dev" APP_ENV value (including empty). That will break local BYOI usage when APP_ENV isn't set. Only use ManagedIdentityCredential for prod/production; otherwise prefer AzureCliCredential.
infra/scripts/build/build_and_push_images.ps1:151 - Enabling acrUseManagedIdentityCreds suppresses stderr and doesn't validate $webappId / exit code. If this update fails (e.g. insufficient permissions), the script will continue and image pulls may still fail with managed identity disabled, but the root cause is hidden.
- rag/service: drop data-source selector (source/source_type/source_name) from the agent scope preamble so it no longer forces a refuse on external indexes lacking the field, unblocking the SQL fallback (matches analytics_engine/retrieval_engine). - enrich_byod_data: write a seeded-shaped metadata column with scalar topic/category aliases (JSON_VALUE can't read JSON arrays) so the agent SQL tool and facets can group/count BYOD rows. - generate_agent_prompt: add a mandatory SQL fallback branch for azure_search BYOD while keeping the softer additive fallback for seeded scenarios. - azure_search: use ChainedTokenCredential (managed identity then Azure CLI) so local post-provision enrichment can authenticate.
…e_search BYOD only
…rompt field guidance
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 96 out of 99 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/api/modules/ingestion/queue_service.py:34
- The new credential selection treats only APP_ENV=="dev" as local development. In this codebase, local/dev is also represented by an empty APP_ENV (default) and values like "development"/"local" (see src/api/main.py:181). With the current logic, local runs where APP_ENV is "" or "development" will incorrectly use ManagedIdentityCredential and fail to authenticate.
Consider aligning the environment check with the existing convention ("", "dev", "development", "local") before choosing AzureCliCredential vs ManagedIdentityCredential.
src/api/modules/ingestion/azure_storage.py:36
- The new credential selection treats only APP_ENV=="dev" as local development. In this codebase, local/dev is also represented by an empty APP_ENV (default) and values like "development"/"local" (see src/api/main.py:181). With the current logic, local runs where APP_ENV is "" or "development" will incorrectly use ManagedIdentityCredential and fail to authenticate.
Align the environment check with the existing convention before choosing AzureCliCredential vs ManagedIdentityCredential.
infra/scripts/build/build_and_push_images.ps1:199
- The backend App Service can have public network access disabled when private networking is enabled (infra/avm/main.bicep:846). In that case, the direct health check to https://{apiApp}.azurewebsites.net/api/health may never succeed (or may return a non-health-related response), causing unnecessary waits and/or a misleading readiness signal.
Consider detecting whether the backend has public network access disabled and, if so, checking backend readiness through the frontend’s /api proxy (which is explicitly configured for private-networking mode).
src/api/modules/document_intelligence/service.py:245
- _wait_for_analyzer_ready() assumes client.get() always returns a response. If the poll request raises an httpx.HTTPError (transient network issue, DNS, timeout), analyzer provisioning will fail immediately rather than retrying until max_wait.
Since this method is specifically meant to tolerate concurrent/provisioning delays, it should also tolerate transient polling errors by logging and retrying.
chore: Pin github actions to commit SHAs
fix: correct dotnet-version key in CodeQL workflow configuration
chore: Pin GitHub Actions to commit SHAs
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 119 out of 122 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/api/modules/ingestion/queue_service.py:34
Settings.app_envdefaults to "" and the API treats "", "development", and "local" as non-prod in other places, but this credential selection only treats "dev" as local. That means local runs (app_env empty/development/local) will useManagedIdentityCredentialand likely fail outside Azure. Consider treating the other non-prod values as local here (or switching back toDefaultAzureCredential).
src/api/modules/ingestion/azure_storage.py:36- Same issue as in queue_service:
app_envdefaults to "" (and other code treats "development"/"local" as non-prod), but this check only special-cases "dev". Local runs with empty/development/local app_env will end up usingManagedIdentityCredentialand fail unless running in Azure.
infra/scripts/build/build_and_push_images.ps1:176 Wait-ForAppReadytreats any HTTP <500 (including 404) as "ready". For the backend call to/api/health, a 404 likely indicates the route/container is misconfigured but this would still report success. Consider only treating 2xx/3xx as ready (or explicitly allow expected auth codes if applicable).
fix: updated the nanoid package version
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 120 out of 124 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/api/modules/ingestion/queue_service.py:34
app_envis treated elsewhere as "", "development", or "local" for non-prod behavior (e.g., src/api/main.py:181), but this credential selection only treats exactly "dev" as local. Withapp_envunset or set to "development"/"local", local runs will tryManagedIdentityCredentialand fail.
src/api/modules/ingestion/azure_storage.py:36- Same issue as queue_service.py: only treating
app_env == "dev"as local is inconsistent with the rest of the codebase ("", "development", "local"). This makes local development or script runs attemptManagedIdentityCredentialunexpectedly.
infra/scripts/build/build_and_push_images.ps1:186 Wait-ForAppReadypolls the public*.azurewebsites.netendpoint. In WAF/private-networking deployments where App Service public network access is disabled, these calls will always time out, adding ~10 minutes to post-provisioning for no benefit. Consider skipping the wait whenpublicNetworkAccessis Disabled.
src/app/package.json:16nanoidis added as a dependency but there are no imports/usages under src/app/src. If it’s not needed yet, it should be removed to avoid unnecessary dependency surface area.
Avijit-Microsoft
approved these changes
Aug 21, 2026
Prajwal-Microsoft
approved these changes
Aug 21, 2026
|
Copilot Fix the code for all comments in this review thread. When a review comment includes a suggested change, apply the suggestion exactly. Do not make changes beyond what is described in the linked review thread. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
This pull request introduces comprehensive support and documentation for a new production-ready, Well-Architected Framework (WAF) aligned deployment flavor (
avm-waf). It adds robust private networking, security, and operational improvements for production deployments, along with corresponding documentation updates and enhancements to deployment scripts. The most important changes are grouped below:Production (WAF) Deployment Support:
avm-waf), which enables private networking, VNet integration, private endpoints, a Bastion-accessed jumpbox, and optional zone redundancy. This includes instructions for selecting deployment flavor and setting VM credentials inREADME.md,docs/DeploymentGuide.md, anddocs/CustomizingAzdParameters.md. [1] [2] [3] [4] [5] [6] [7] [8] [9]Deployment Automation and Security:
azure.yamldeployment hooks to temporarily enable public network access on data-plane resources (ACR, SQL, Storage, Cosmos) when private networking is active, ensuring build and data setup steps succeed, and automatically revert to private-only access on completion or failure. [1] [2]Parameterization and Configuration:
docs/CustomizingAzdParameters.md, includingdeploymentFlavor, region, SKUs, container image tags, and options for existing resources, to facilitate advanced and production deployments. [1] [2]B2toB3in the deployment guide to reflect production readiness.Operational Guidance:
Other:
infra/abbreviations.jsonfile.Does this introduce a breaking change?
Golden Path Validation
Deployment Validation
What to Check
Verify the all deployment types