Skip to content

Better support for deploying to non-home tenant #1964

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Sep 13, 2024
Merged

Better support for deploying to non-home tenant #1964

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3b19a5b
Better support for multitenant
pamelafox Sep 11, 2024
5ad3bd0
Update app/backend/app.py
pamelafox Sep 11, 2024
bf0dcf7
Better types
pamelafox Sep 11, 2024
6f78a19
Better types
pamelafox Sep 11, 2024
e4bb655
Update the mock
pamelafox Sep 11, 2024
f34b9ae
Fix Bicep workflow
pamelafox Sep 11, 2024
aab78e9
Add process timeout in both places
pamelafox Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions app/backend/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@
SpeechSynthesizer,
)
from azure.core.exceptions import ResourceNotFoundError
from azure.identity.aio import DefaultAzureCredential, get_bearer_token_provider
from azure.identity.aio import (
AzureDeveloperCliCredential,
ManagedIdentityCredential,
get_bearer_token_provider,
)
from azure.monitor.opentelemetry import configure_azure_monitor
from azure.search.documents.aio import SearchClient
from azure.search.documents.indexes.aio import SearchIndexClient
Expand Down Expand Up @@ -436,11 +440,13 @@ async def setup_clients():
USE_SPEECH_OUTPUT_BROWSER = os.getenv("USE_SPEECH_OUTPUT_BROWSER", "").lower() == "true"
USE_SPEECH_OUTPUT_AZURE = os.getenv("USE_SPEECH_OUTPUT_AZURE", "").lower() == "true"

# Use the current user identity to authenticate with Azure OpenAI, AI Search and Blob Storage (no secrets needed,
# just use 'az login' locally, and managed identity when deployed on Azure). If you need to use keys, use separate AzureKeyCredential instances with the
# keys for each service
# If you encounter a blocking error during a DefaultAzureCredential resolution, you can exclude the problematic credential by using a parameter (ex. exclude_shared_token_cache_credential=True)
azure_credential = DefaultAzureCredential(exclude_shared_token_cache_credential=True)
# Use the current user identity for keyless authentication to Azure services.
# This assumes you use 'azd auth login' locally, and managed identity when deployed on Azure.
# The managed identity is setup in the infra/ folder.
if os.getenv("WEBSITE_HOSTNAME"):
azure_credential = ManagedIdentityCredential()
else:
azure_credential = AzureDeveloperCliCredential(tenant_id=AZURE_TENANT_ID)

# Set up clients for AI Search and Storage
search_client = SearchClient(
Expand Down
11 changes: 6 additions & 5 deletions app/backend/prepdocs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,11 +381,12 @@ async def main(strategy: Strategy, setup_index: bool = True):
use_int_vectorization = args.useintvectorization and args.useintvectorization.lower() == "true"

# Use the current user identity to connect to Azure services unless a key is explicitly set for any of them
azd_credential = (
AzureDeveloperCliCredential()
if args.tenantid is None
else AzureDeveloperCliCredential(tenant_id=args.tenantid, process_timeout=60)
)
if args.tenantid:
logger.info("Connecting to Azure services using the azd credential for tenant %s", args.tenantid)
azd_credential = AzureDeveloperCliCredential(tenant_id=args.tenantid, process_timeout=60)
else:
logger.info("Connecting to Azure services using the azd credential for home tenant")
azd_credential = AzureDeveloperCliCredential()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this one not have the same timeout?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know! I thought about removing it for consistency but then thought I shouldnt change things I dont know the origin for. Maybe @mattgotteiner recalls a reason?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we should put the timeout in both, just to give the credential more time to come back locally.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think adding the timeout will have much issue. Sure, we can add it to both


if args.removeall:
document_action = DocumentAction.RemoveAll
Expand Down
3 changes: 0 additions & 3 deletions infra/main.parameters.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,6 @@
"enableUnauthenticatedAccess": {
"value": "${AZURE_ENABLE_UNAUTHENTICATED_ACCESS=false}"
},
"tenantId": {
"value": "${AZURE_TENANT_ID}"
},
"authTenantId": {
"value": "${AZURE_AUTH_TENANT_ID}"
},
Expand Down
Loading