Feature/vpn deployment by vgfitzger-microsoft · Pull Request #57 · microsoft/healthcare-ai-model-evaluator

vgfitzger-microsoft · 2026-02-09T16:35:12Z

Refactor to cosmos without mongo api and deploying inside a vnet by default to satisfy security feedback.

…yStorage

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 67 out of 67 changed files in this pull request and generated 11 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-10T09:08:47Z

infra/modules/cosmos.bicep

+<<<<<<< HEAD
+
+@description('List of SQL API container names to provision')
+param containerNames array = [
+  'Users'


This file still contains unresolved Git merge conflict markers (e.g., <<<<<<< HEAD, =======, >>>>>>>). Bicep compilation will fail until the conflict is resolved and the markers are removed.

Copilot · 2026-02-10T09:08:47Z

infra/main.bicep

 @description('Enable IP filtering for the web application (when true, only allowedWebIp can access the Container App API)')
+<<<<<<< HEAD
+param enableWebIpFiltering bool = false
+=======
 param enableWebIpFiltering bool = true
+>>>>>>> origin/main


Unresolved merge conflict markers are present in this file (e.g., around enableWebIpFiltering and networking params). The template will not compile/apply until these conflicts are resolved and the duplicated/branch-only blocks are removed.

Copilot · 2026-02-10T09:08:48Z

backend/src/MedBench.API/Program.cs

+<<<<<<< HEAD
+using Microsoft.Azure.Cosmos;
+using System.Text.Json;
+=======
+>>>>>>> origin/main


This file contains unresolved merge conflict markers in the using/import section. The project will not build until the conflict is resolved and only the intended imports remain.

Suggested change

<<<<<<< HEAD

using Microsoft.Azure.Cosmos;

using System.Text.Json;

=======

>>>>>>> origin/main

using Microsoft.Azure.Cosmos;

using System.Text.Json;

Copilot · 2026-02-10T09:08:48Z

backend/src/MedBench.API/Program.cs

 builder.Services.AddSingleton(sp =>
 {
+<<<<<<< HEAD
+    var dbName = Environment.GetEnvironmentVariable("COSMOSDB_DATABASE")
+        ?? builder.Configuration["CosmosDb:DatabaseName"];
+
+    if (string.IsNullOrWhiteSpace(dbName))
+    {
+        throw new InvalidOperationException("Cosmos DB database name not found. Set COSMOSDB_DATABASE or CosmosDb:DatabaseName.");
+    }
+
+    return new MedBench.Core.Cosmos.CosmosContainerProvider(
+        sp.GetRequiredService<CosmosClient>(),
+        dbName);
+=======


There is an unresolved merge conflict inside the DI registration for Cosmos/Mongo (starting at <<<<<<< HEAD). Resolve the conflict and remove the MongoDB-based branch entirely if the codebase has migrated to Cosmos SQL API, otherwise the application will not compile.

Copilot · 2026-02-10T09:08:48Z

infra/modules/cosmos.bicep

+// Store Cosmos DB endpoint for managed identity authentication
+resource cosmosEndpointSecret 'Microsoft.KeyVault/vaults/secrets@2022-07-01' = {
  parent: keyVault
-  name: 'cosmos-connection-string'
+  name: 'cosmos-endpoint'
+  properties: {
+    value: cosmosAccount.properties.documentEndpoint
+  }
+}


cosmosEndpointSecret is defined here, but the file also contains another cosmosEndpointSecret resource later (duplicate declaration). Deployment will fail with a duplicate resource definition; keep only one secret resource for the Cosmos endpoint.

Copilot · 2026-02-10T09:08:49Z

infra/main.json

      "name": "bicep",
      "version": "0.38.33.27573",
-      "templateHash": "16783774531187217924"
+      "templateHash": "3963363190777727422",
    }
  },


This JSON is invalid due to a trailing comma after the templateHash property. Remove the trailing comma so infra/main.json parses correctly.

Copilot · 2026-02-10T09:08:49Z

DEPLOYMENT.md

+<<<<<<< HEAD
+This project supports two deployment networking modes:
+
+- `DEPLOYMENT_NETWORKING=open` (default): public ingress
+- `DEPLOYMENT_NETWORKING=private`: internal-only networking (VNet)
+


This documentation file still contains unresolved Git merge conflict markers (<<<<<<<, =======, >>>>>>>). Resolve the conflict and remove the markers so the rendered docs are correct.

@vgfitzger-microsoft this is a valid one

Copilot · 2026-02-10T09:08:49Z

tools/cosmos-migration/import_mongo_export.py

+    with open(path, "r", encoding="utf-8") as f:
+        content = f.read().strip()
+        if not content:
+            return []
+
+        if content.startswith("["):
+            data = json.loads(content)
+            if not isinstance(data, list):
+                raise ValueError("Expected a JSON array")
+            return data


_iter_documents reads the entire file into memory to detect JSON-array vs NDJSON, and later docs = list(_iter_documents(...)) materializes everything again. For large exports this can cause high memory usage; consider streaming (peek first non-whitespace char, then iterate items/lines) and upserting incrementally.

Copilot · 2026-02-10T09:08:50Z

tools/cosmos-migration/import_mongo_export.py

+        if key in ("$numberDouble", "$numberDecimal"):
+            return float(inner)


Mongo Extended JSON $numberDecimal is converted to float, which can lose precision. Consider using decimal.Decimal (or preserving as string) for $numberDecimal while keeping $numberDouble as float.

Copilot · 2026-02-10T09:08:50Z

backend/src/MedBench.Core/Repositories/DataObjectRepository.cs

+    public async Task<DataObject> GetByIdWithIndexAsync(string id)
+    {
+        var dataObject = await GetByIdAsync(id);
+
+        // Backwards compatibility: populate OriginalDataFile and OriginalIndex if needed


This GetByIdWithIndexAsync method was added, but the file still contains another GetByIdWithIndexAsync implementation further down (MongoDB-based). The duplicate method signature will prevent compilation; remove/rename the stale implementation so only one method remains.

sashimono-san

I tested the deployment and fixed a few things along the way, please check: #58

Main pending issue is the mismatch between DB and FE admin roles, and the merge conflicts in DEPLOYMENT.md. I left some other comments to consider

sashimono-san · 2026-02-17T08:49:36Z

backend/src/MedBench.API/Services/RootAdminSeeder.cs

+				Id = Guid.NewGuid().ToString(),
+				Name = name,
+				Email = email,
+				Roles = new List<string> { "Administrator" },


The DB does get populated when env variables are set, but the role itself is different from what the frontend expects, in the FE these pages checks for admin instead of Administrator:

Admin.tsx

Dashboard.tsx

Navigation.tsx

AuthContext.tsx

But then, RequireAdministratorRole checks for Administrator.

sashimono-san · 2026-02-17T08:50:29Z

azure.yaml

+    deploymentNetworking: "${DEPLOYMENT_NETWORKING=private}"
+    createVnet: "${CREATE_VNET=true}"
+    existingVnetResourceId: "${EXISTING_VNET_RESOURCE_ID=}"
+    existingAcaInfrastructureSubnetId: "${EXISTING_ACA_INFRASTRUCTURE_SUBNET_ID=}"
+    existingFunctionsIntegrationSubnetId: "${EXISTING_FUNCTIONS_INTEGRATION_SUBNET_ID=}"
+    vnetAddressSpace: "${VNET_ADDRESS_SPACE=10.30.0.0/16}"
+    acaInfrastructureSubnetPrefix: "${ACA_INFRASTRUCTURE_SUBNET_PREFIX=10.30.0.0/23}"
+    functionsIntegrationSubnetPrefix: "${FUNCTIONS_INTEGRATION_SUBNET_PREFIX=10.30.2.0/24}"
+    gatewaySubnetPrefix: "${GATEWAY_SUBNET_PREFIX=10.30.255.0/27}"
+    createVpnGateway: "${CREATE_VPN_GATEWAY=true}"
+    vpnClientAddressPool: "${VPN_CLIENT_ADDRESS_POOL=172.16.200.0/24}"
+    vpnGatewaySku: "${VPN_GATEWAY_SKU=VpnGw1}"
+    createPrivateEndpoint: "${CREATE_PRIVATE_ENDPOINT=true}"
+    privateEndpointSubnetPrefix: "${PRIVATE_ENDPOINT_SUBNET_PREFIX=10.30.4.0/27}"
+    createDnsResolver: "${CREATE_DNS_RESOLVER=true}"
+    dnsResolverSubnetPrefix: "${DNS_RESOLVER_SUBNET_PREFIX=10.30.3.0/27}"
+    dnsResolverInboundIp: "${DNS_RESOLVER_INBOUND_IP=}"
+    rootAdminEmail: "${ROOT_ADMIN_EMAIL=}"
+    rootAdminName: "${ROOT_ADMIN_NAME=}"
+    rootAdminPassword: "${ROOT_ADMIN_PASSWORD=}"
    enableWebIpFiltering: "${ENABLE_WEB_IP_FILTERING}"


We have all that under main.parameters.json, and I think having both is redundant.

sashimono-san · 2026-02-17T08:51:21Z

azure.yaml

-    hooks:
-      prepackage:
-        shell: sh
-        run: ../../package_addon.sh evaluator
-        continueOnError: false


We still need to run this script, so I added some instructions in the deployment guide (see #58)

sashimono-san · 2026-02-17T08:57:39Z

DEPLOYMENT.md

 #### IP Filtering & Security Configuration

+<<<<<<< HEAD
+This project supports two deployment networking modes:


Deployments take long, so if possible it's better to have these instructions should come before the Step 3: Deploy the Infrastructure section.

sashimono-san · 2026-02-17T08:58:47Z

DEPLOYMENT.md

 To start the deployment process, run:

   ```bash
   azd up
   ```

 During deployment you will be prompted for any required variable not yet set, such as subscription, resource group and location.

 This command will:
 - Provision all Azure infrastructure (Container Apps, Functions, Storage, Cosmos DB, etc.)
 - Deploy backend API (with integrated frontend)
 - Create or configure Azure OpenAI service
 - Set up shared blob storage for all components
 - Configure authentication via Entra ID App Registration


This section kind of duplicates and extends from section Step 3: Deploy the Infrastructure. When resolving merge conflicts we should merge these two parts of the guide

sashimono-san · 2026-02-17T09:03:18Z

README.md

+> Cosmos DB **local auth is disabled** in this deployment. For local development, run `az login` and ensure your current identity has Cosmos **data-plane** permissions on the target account.
+> The API also uses **managed identity / Entra ID** for Storage, so your signed-in identity needs Storage Blob **data-plane** permissions on the target account.


We could add this permission to the user running deployment directly with bicep. I think it would be a nice to have.

Also, the postprovision script is not assigning this data-plane permission. Instead it's assigning the Azure RBAC role for cosmos. See my new PR for this fix

vgfitzger and others added 19 commits November 19, 2025 17:09

add image url to experiment export

e09c6f3

add index tracking to dataobject for alignment later

257aabf

add index tracking to dataobject for alignment later

2245bef

readd env

205e4e9

make the webapp subdir configurable

4438f30

fix dynamic msal urls

2d040be

update powershell scripts for app registration and frontend env settings

ad9db40

fix post provision app registration command

dbc2300

allow an admin user to be created on azd up

e994de4

Threat model updates

a01183e

progress

154e5d6

Merge branch 'feature/ExportDataObjectMapping' into feature/secretsKe…

dcc91ea

…yStorage

fix the js routing fallback

9ffb4af

fix static file routing

cf134d5

fix connection identity config

9a14f65

disable key based auth to cosmo

4382e36

deploy server in VNET by default'

73dfd8c

refactor to non mongo db and user injection.

bf64eab

Merge remote-tracking branch 'origin/main' into feature/VPNDeployment

c270740

vgfitzger-microsoft assigned vgfitzger-microsoft and sashimono-san Feb 9, 2026

Copilot AI review requested due to automatic review settings February 9, 2026 16:35

Copilot AI reviewed Feb 9, 2026

View reviewed changes

sashimono-san requested a review from Copilot February 10, 2026 09:01

Copilot started reviewing on behalf of sashimono-san February 10, 2026 09:02 View session

Copilot AI reviewed Feb 10, 2026

View reviewed changes

vgfitzger-microsoft added 4 commits February 10, 2026 18:34

fix json merge

bc211cc

fix json merge

60189e3

fix merge

20d4ef0

fix merge

e897ef4

vgfitzger-microsoft added 2 commits February 11, 2026 17:21

fix merge

dc9791d

fix merge

4737a64

sashimono-san reviewed Feb 17, 2026

View reviewed changes

		if key in ("$numberDouble", "$numberDecimal"):
		return float(inner)

		> Cosmos DB local auth is disabled in this deployment. For local development, run `az login` and ensure your current identity has Cosmos data-plane permissions on the target account.
		> The API also uses managed identity / Entra ID for Storage, so your signed-in identity needs Storage Blob data-plane permissions on the target account.

Conversation

vgfitzger-microsoft commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

sashimono-san Feb 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 10, 2026

Choose a reason for hiding this comment

Uh oh!

sashimono-san left a comment

Choose a reason for hiding this comment

Uh oh!

sashimono-san Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

sashimono-san Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

sashimono-san Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sashimono-san Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

sashimono-san Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sashimono-san Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sashimono-san Feb 16, 2026 •

edited

Loading

sashimono-san Feb 17, 2026 •

edited

Loading

sashimono-san Feb 17, 2026 •

edited

Loading