Skip to content

Conversation

@pamelafox
Copy link
Contributor

This PR adds azd support with Bicep modules that setup container apps, openai, CosmosDB, and private endpoints. Also adds a deployed server that uses CosmosDB instead of a CSV,

I have not yet tested the private endpoints, that code came from BRK234 directly, so I believe that it largely works but I also expect to change it once I add a second container for the agent.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds comprehensive Azure infrastructure support using Azure Developer CLI (azd) and Bicep templates to deploy the MCP (Model Context Protocol) expense tracking application to Azure Container Apps. The changes include a production-ready CosmosDB-backed MCP server, containerization with Docker, and extensive networking infrastructure with optional private endpoints and VNet integration.

Key Changes

  • New deployed MCP server (servers/deployed_mcp.py) that replaces CSV file storage with Azure CosmosDB for scalable, cloud-native data persistence
  • Infrastructure as Code with comprehensive Bicep modules defining Container Apps, CosmosDB, OpenAI services, Virtual Networks, Private Endpoints, and RBAC role assignments
  • Containerization with multi-stage Dockerfile using UV package manager for efficient Python dependency management and secure non-root container execution

Reviewed changes

Copilot reviewed 22 out of 23 changed files in this pull request and generated 32 comments.

Show a summary per file
File Description
servers/deployed_mcp.py New production MCP server with CosmosDB async client integration for expense tracking
servers/Dockerfile Multi-stage Alpine-based Docker build with UV package manager and non-root user
servers/basic_mcp_http.py Whitespace cleanup (removed blank line)
infra/main.bicep Main infrastructure orchestration with Azure resources, networking, security, and role assignments
infra/aca.bicep Container App definition with managed identity and environment configuration
infra/core/host/*.bicep Modular Bicep templates for Container Apps environment, registry, and app lifecycle management
infra/core/security/*.bicep RBAC role assignments for OpenAI, ACR, and CosmosDB access control
infra/core/monitor/loganalytics.bicep Log Analytics workspace configuration for monitoring
infra/core/ai/cognitiveservices.bicep OpenAI/Cognitive Services deployment module
infra/main.parameters.json Deployment parameters with VNet and monitoring feature flags
infra/write_env.sh Bash script to generate .env file from azd environment variables
infra/write_env.ps1 PowerShell script to generate .env file from azd environment variables
azure.yaml Azure Developer CLI configuration defining services and post-provision hooks
pyproject.toml Added azure-cosmos>=4.9.0 dependency
uv.lock Updated lock file with azure-cosmos package
agents/langchainv1_http.py Made MCP_SERVER_URL configurable via environment variable
agents/agentframework_http.py Made MCP_SERVER_URL configurable via environment variable

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

]
}
}

Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing CosmosDB private DNS zone. A private DNS zone for privatelink.documents.azure.com should be created (similar to OpenAI's private DNS zone) to enable private endpoint DNS resolution for CosmosDB when useVnet is true.

Suggested change
// CosmosDB Private DNS Zone
module cosmosDbPrivateDnsZone 'br/public:avm/res/network/private-dns-zone:0.7.1' = if (useVnet) {
name: 'cosmosdb-dns-zone'
scope: resourceGroup
params: {
name: 'privatelink.documents.azure.com'
tags: tags
virtualNetworkLinks: [
{
registrationEnabled: false
virtualNetworkResourceId: virtualNetwork.outputs.resourceId
}
]
}
}

Copilot uses AI. Check for mistakes.
infra/main.bicep Outdated
Comment on lines 27 to 33
param useVnet bool = true

@description('Flag to enable or disable monitoring resources')
param useMonitoring bool = true

@description('Flag to enable or disable public ingress')
param usePrivateIngress bool = true
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The default values for VNet and private ingress are true (lines 27, 30, 33), but the parameters file defaults to false for these (lines 15, 18). This inconsistency can cause confusion. Either align the defaults between main.bicep and main.parameters.json, or document why they differ.

Suggested change
param useVnet bool = true
@description('Flag to enable or disable monitoring resources')
param useMonitoring bool = true
@description('Flag to enable or disable public ingress')
param usePrivateIngress bool = true
param useVnet bool = false
@description('Flag to enable or disable monitoring resources')
param useMonitoring bool = false
@description('Flag to enable or disable public ingress')
param usePrivateIngress bool = false

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made them both false here, monitoring true in both

@madebygps
Copy link
Collaborator

Just tried azd up with this and it works. Will you merge now or waiting for more changes? I have keycloak stuff on another branch and could use this code to test deployment.

@madebygps
Copy link
Collaborator

oh might want to add azd and docker in docker to devcontainer.json

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 8 comments.

Comments suppressed due to low confidence (1)

infra/core/host/container-registry.bicep:5

  • Admin user is enabled for the container registry (adminUserEnabled: true), which poses a security risk in production environments. Since the configuration already uses managed identity for authentication (as seen in container-app.bicep using user-assigned identity), admin user access should be disabled. Change to:
param adminUserEnabled bool = false
param adminUserEnabled bool = true

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

writer.writerow(
[date_iso, amount, category.value, description, payment_method.name]
)
writer.writerow([date_iso, amount, category.value, description, payment_method.name])
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistency detected: basic_mcp_http.py uses payment_method.value while basic_mcp_stdio.py uses payment_method.name at line 60. Both files should use the same approach for consistency. Since deployed_mcp.py uses .value for both category and payment_method, and the HTTP version now uses .value, consider updating basic_mcp_stdio.py to also use payment_method.value for consistency across all three files.

Suggested change
writer.writerow([date_iso, amount, category.value, description, payment_method.name])
writer.writerow([date_iso, amount, category.value, description, payment_method.value])

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 28 out of 29 changed files in this pull request and generated 6 comments.


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

@@ -1,9 +1,11 @@
{
"name": "Python MCP Demos",
"image": "mcr.microsoft.com/devcontainers/python:1-3.12-bullseye",
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The devcontainer is using Python 3.12 (python:1-3.12-bullseye on line 3), but the project requires Python 3.13 (see pyproject.toml line 6: requires-python = "==3.13.*"). This mismatch will cause issues when developers use the devcontainer.

Update the devcontainer image to:

"image": "mcr.microsoft.com/devcontainers/python:1-3.13-bullseye"
Suggested change
"image": "mcr.microsoft.com/devcontainers/python:1-3.12-bullseye",
"image": "mcr.microsoft.com/devcontainers/python:1-3.13-bullseye",

Copilot uses AI. Check for mistakes.
Comment on lines +25 to +37
# Configure Cosmos DB client and container
if RUNNING_IN_PRODUCTION and AZURE_CLIENT_ID:
credential = ManagedIdentityCredential(client_id=AZURE_CLIENT_ID)
else:
credential = DefaultAzureCredential()

cosmos_client = CosmosClient(
url=f"https://{AZURE_COSMOSDB_ACCOUNT}.documents.azure.com:443/",
credential=credential,
)
cosmos_db = cosmos_client.get_database_client(AZURE_COSMOSDB_DATABASE)
cosmos_container = cosmos_db.get_container_client(AZURE_COSMOSDB_CONTAINER)
logger.info(f"Connected to Cosmos DB: {AZURE_COSMOSDB_ACCOUNT}")
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Cosmos DB client is initialized at module level (lines 31-36), which means it's created when the module is imported rather than when it's actually used. This can cause issues:

  1. If the environment variables are not set, the module will fail to import
  2. The async client is created in a synchronous context, which may not properly initialize the event loop
  3. Resource cleanup may not happen properly

Consider initializing the Cosmos DB client lazily in an async context manager or startup function, similar to how FastMCP handles resources. For example:

cosmos_client = None
cosmos_container = None

async def get_cosmos_container():
    global cosmos_client, cosmos_container
    if cosmos_container is None:
        cosmos_client = CosmosClient(...)
        cosmos_db = cosmos_client.get_database_client(...)
        cosmos_container = cosmos_db.get_container_client(...)
    return cosmos_container
Suggested change
# Configure Cosmos DB client and container
if RUNNING_IN_PRODUCTION and AZURE_CLIENT_ID:
credential = ManagedIdentityCredential(client_id=AZURE_CLIENT_ID)
else:
credential = DefaultAzureCredential()
cosmos_client = CosmosClient(
url=f"https://{AZURE_COSMOSDB_ACCOUNT}.documents.azure.com:443/",
credential=credential,
)
cosmos_db = cosmos_client.get_database_client(AZURE_COSMOSDB_DATABASE)
cosmos_container = cosmos_db.get_container_client(AZURE_COSMOSDB_CONTAINER)
logger.info(f"Connected to Cosmos DB: {AZURE_COSMOSDB_ACCOUNT}")
# Configure Cosmos DB client and container (lazily, in async context)
cosmos_client = None
cosmos_container = None
async def get_cosmos_container():
global cosmos_client, cosmos_container
if cosmos_container is None:
if RUNNING_IN_PRODUCTION and AZURE_CLIENT_ID:
credential = ManagedIdentityCredential(client_id=AZURE_CLIENT_ID)
else:
credential = DefaultAzureCredential()
cosmos_client_local = CosmosClient(
url=f"https://{AZURE_COSMOSDB_ACCOUNT}.documents.azure.com:443/",
credential=credential,
)
cosmos_db = cosmos_client_local.get_database_client(AZURE_COSMOSDB_DATABASE)
cosmos_container_local = cosmos_db.get_container_client(AZURE_COSMOSDB_CONTAINER)
# Assign to globals for caching
globals()["cosmos_client"] = cosmos_client_local
globals()["cosmos_container"] = cosmos_container_local
logger.info(f"Connected to Cosmos DB: {AZURE_COSMOSDB_ACCOUNT}")
return cosmos_container

Copilot uses AI. Check for mistakes.
Comment on lines +11 to +17
echo "AZURE_OPENAI_CHAT_DEPLOYMENT=$(azd env get-value AZURE_OPENAI_CHAT_DEPLOYMENT)" >> "$ENV_FILE_PATH"
echo "AZURE_OPENAI_CHAT_MODEL=$(azd env get-value AZURE_OPENAI_CHAT_MODEL)" >> "$ENV_FILE_PATH"
echo "AZURE_OPENAI_ENDPOINT=$(azd env get-value AZURE_OPENAI_ENDPOINT)" >> "$ENV_FILE_PATH"
echo "AZURE_TENANT_ID=$(azd env get-value AZURE_TENANT_ID)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_ACCOUNT=$(azd env get-value AZURE_COSMOSDB_ACCOUNT)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_DATABASE=$(azd env get-value AZURE_COSMOSDB_DATABASE)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_CONTAINER=$(azd env get-value AZURE_COSMOSDB_CONTAINER)" >> "$ENV_FILE_PATH"
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The script doesn't check if azd env get-value commands succeed. If any of these commands fail (e.g., a value doesn't exist), the script will continue due to command substitution happening in a subshell, potentially creating an incomplete .env file with lines like VARIABLE_NAME= (empty values).

Consider adding error checking:

for var in AZURE_OPENAI_CHAT_DEPLOYMENT AZURE_OPENAI_CHAT_MODEL ...; do
    value=$(azd env get-value "$var") || { echo "Failed to get $var"; exit 1; }
    echo "$var=$value" >> "$ENV_FILE_PATH"
done

Or at minimum, verify the values are not empty before writing them.

Suggested change
echo "AZURE_OPENAI_CHAT_DEPLOYMENT=$(azd env get-value AZURE_OPENAI_CHAT_DEPLOYMENT)" >> "$ENV_FILE_PATH"
echo "AZURE_OPENAI_CHAT_MODEL=$(azd env get-value AZURE_OPENAI_CHAT_MODEL)" >> "$ENV_FILE_PATH"
echo "AZURE_OPENAI_ENDPOINT=$(azd env get-value AZURE_OPENAI_ENDPOINT)" >> "$ENV_FILE_PATH"
echo "AZURE_TENANT_ID=$(azd env get-value AZURE_TENANT_ID)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_ACCOUNT=$(azd env get-value AZURE_COSMOSDB_ACCOUNT)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_DATABASE=$(azd env get-value AZURE_COSMOSDB_DATABASE)" >> "$ENV_FILE_PATH"
echo "AZURE_COSMOSDB_CONTAINER=$(azd env get-value AZURE_COSMOSDB_CONTAINER)" >> "$ENV_FILE_PATH"
for var in \
AZURE_OPENAI_CHAT_DEPLOYMENT \
AZURE_OPENAI_CHAT_MODEL \
AZURE_OPENAI_ENDPOINT \
AZURE_TENANT_ID \
AZURE_COSMOSDB_ACCOUNT \
AZURE_COSMOSDB_DATABASE \
AZURE_COSMOSDB_CONTAINER
do
value=$(azd env get-value "$var") || { echo "Failed to get $var"; exit 1; }
if [ -z "$value" ]; then
echo "Value for $var is empty"; exit 1
fi
echo "$var=$value" >> "$ENV_FILE_PATH"
done

Copilot uses AI. Check for mistakes.
output defaultDomain string = containerAppsEnvironment.properties.defaultDomain
output imageName string = imageName
output name string = app.name
output hostName string = app.properties.configuration.ingress.fqdn
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The output hostName on line 127 will fail if ingressEnabled is false, because it unconditionally accesses app.properties.configuration.ingress.fqdn even when ingress may be null (see line 78).

This should be conditional:

output hostName string = ingressEnabled ? app.properties.configuration.ingress.fqdn : ''
Suggested change
output hostName string = app.properties.configuration.ingress.fqdn
output hostName string = ingressEnabled ? app.properties.configuration.ingress.fqdn : ''

Copilot uses AI. Check for mistakes.
Comment on lines +676 to +705
module openAiRoleUser 'core/security/role.bicep' = {
scope: resourceGroup
name: 'openai-role-user'
params: {
principalId: principalId
roleDefinitionId: '5e0bd9bd-7b93-4f28-af87-19fc36ad61bd' // Cognitive Services OpenAI User
principalType: 'User'
}
}

module openAiRoleBackend 'core/security/role.bicep' = {
scope: resourceGroup
name: 'openai-role-backend'
params: {
principalId: aca.outputs.identityPrincipalId
roleDefinitionId: '5e0bd9bd-7b93-4f28-af87-19fc36ad61bd' // Cognitive Services OpenAI User
principalType: 'ServicePrincipal'
}
}

// Cosmos DB Data Contributor role for user
module cosmosDbRoleUser 'core/security/documentdb-sql-role.bicep' = {
scope: resourceGroup
name: 'cosmosdb-role-user'
params: {
databaseAccountName: cosmosDb.outputs.name
principalId: principalId
roleDefinitionId: '/${subscription().id}/resourceGroups/${resourceGroup.name}/providers/Microsoft.DocumentDB/databaseAccounts/${cosmosDb.outputs.name}/sqlRoleDefinitions/00000000-0000-0000-0000-000000000002'
}
}
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The role assignments for the user (lines 676-684 and 697-705) will fail if principalId is empty (default value is ''). Since principalId has a default empty string value (line 13), these modules should be conditionally deployed only when principalId is provided.

Add a condition:

module openAiRoleUser 'core/security/role.bicep' = if (!empty(principalId)) {
  // ...
}

module cosmosDbRoleUser 'core/security/documentdb-sql-role.bicep' = if (!empty(principalId)) {
  // ...
}

Copilot uses AI. Check for mistakes.
param tags object = {}

// It would be better security-wise if this was false, but that is not yet working with the VNet+azd integration
param adminUserEnabled bool = true
Copy link

Copilot AI Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adminUserEnabled is set to true for the Azure Container Registry, which enables the global admin username/password on the registry. Attackers who obtain these credentials can pull/push images and compromise your supply chain. Disable the admin user by setting adminUserEnabled to false and rely on managed identities with AcrPull/AcrPush RBAC:

param adminUserEnabled bool = false

Also ensure registries in your Container Apps use a user-assigned identity for ACR access, which you already configure via registry-access.bicep.

Suggested change
param adminUserEnabled bool = true
param adminUserEnabled bool = false

Copilot uses AI. Check for mistakes.
@pamelafox
Copy link
Contributor Author

I have reviewed Copilot's latest comments and none of them seem like blockers at this point, so I am going to merge the PR.

@pamelafox pamelafox merged commit 617952c into Azure-Samples:main Dec 3, 2025
7 checks passed
@pamelafox pamelafox deleted the infra branch December 3, 2025 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants