refactor: update credential retrieval logic based on environment - #685
Conversation
Coverage Report •
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR refactors Azure authentication helpers to adjust how async credentials are selected and to align bearer token provider setup with the (synchronous) get_async_azure_credential() implementation.
Changes:
- Updated async bearer token provider setup to call
get_async_azure_credential()withoutawait. - Modified
get_async_azure_credential()(async path) to select a fallback credential based onAPP_ENV(dev vs non-dev), with additional logging/console output.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ContentProcessor/src/libs/utils/credential_util.py | Removes await when retrieving the async credential for the async token provider. |
| src/ContentProcessor/src/libs/utils/azure_credential_utils.py | Removes await in async token provider and adds APP_ENV-based fallback credential selection in async credential helper. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/ContentProcessor/src/libs/utils/azure_credential_utils.py:198
- The APP_ENV gating here only treats exactly "prod" as production; any other value (e.g., "staging", "qa") falls back to AsyncDefaultAzureCredential, which contradicts the PR description (managed identity for production and other non-dev environments) and makes the CodeQL note inaccurate (DefaultAzureCredential would be used outside development). Consider inverting the condition: use DefaultAzureCredential only when APP_ENV=dev; otherwise use Managed Identity.
# All async CLI credentials failed. Select the final credential based on the
# environment: production uses Managed Identity, while development uses
# DefaultAzureCredential. Defaults to production when APP_ENV is not set.
app_env = os.getenv("APP_ENV", "prod").lower()
if app_env == "prod":
src/ContentProcessor/src/libs/utils/azure_credential_utils.py:199
- The new APP_ENV-based fallback behavior introduces a distinct path where managed identity is selected even when no Azure environment indicators are present (e.g., APP_ENV=prod/qa/staging and CLI creds fail). There isn't a corresponding unit test covering this branch, so regressions here may go unnoticed.
app_env = os.getenv("APP_ENV", "prod").lower()
if app_env == "prod":
client_id = os.getenv("AZURE_CLIENT_ID")
infra/main.bicep:564
- Adding the resource group tag
SecurityControl: 'Ignore'can be interpreted by governance/policy tooling as an explicit opt-out and may weaken compliance posture. If this tag is not required by an approved policy exception workflow, it should be removed.
DeploymentName: deployment().name
SecurityControl: 'Ignore'
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/ContentProcessor/src/libs/utils/azure_credential_utils.py:198
APP_ENVdefaults to "prod" here and, when no Azure environment indicators are present, the function will still returnAsyncManagedIdentityCredential(). In non-Azure runtimes (e.g., local dev withoutAPP_ENVset) this can cause long timeouts while probing IMDS/managed identity endpoints. Since you already gate managed identity onazure_env_indicatorsabove, consider failing fast forAPP_ENV=prodwhen no Azure MI indicators are detected, and only usingAsyncDefaultAzureCredentialfor non-prod/local.
Also, the trailing comment says DefaultAzureCredential is "only used in development", but this branch triggers for any non-"prod" value (staging/test/etc.), so the comment is inaccurate.
app_env = os.getenv("APP_ENV", "prod").lower()
if app_env == "prod":
client_id = os.getenv("AZURE_CLIENT_ID")
if client_id:
logging.info(
src/tests/ContentProcessor/utils/test_azure_credential_utils_extended.py:113
- This test clears Azure environment indicators before asserting the CLI->Default fallback, but it doesn't clear
CONTAINER_REGISTRY_LOGIN, which is also treated as an Azure-hosted indicator inget_async_azure_credential(). If the CI environment happens to set this variable, the function will return managed identity and this test will become flaky.
for key in ["WEBSITE_SITE_NAME", "AZURE_CLIENT_ID", "MSI_ENDPOINT",
"IDENTITY_ENDPOINT", "KUBERNETES_SERVICE_HOST"]:
monkeypatch.delenv(key, raising=False)
monkeypatch.setenv("APP_ENV", "dev")
src/ContentProcessor/src/libs/utils/credential_util.py:50
azure_credential_utils.get_async_azure_credential()now contains APP_ENV-based selection logic, butcredential_util.get_async_azure_credential()still uses the older behavior. Since both modules exposeget_async_bearer_token_provider(), this creates inconsistent auth behavior depending on which helper a caller uses.
To avoid drift, consider delegating to libs.utils.azure_credential_utils.get_async_azure_credential() from this function (or otherwise consolidating these two nearly-identical modules).
Returns:
A callable suitable for SDK clients that accept a token provider.
"""
credential = get_async_azure_credential()
return identity_get_async_bearer_token_provider(
credential, "https://cognitiveservices.azure.com/.default"
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/ContentProcessor/src/libs/utils/azure_credential_utils.py:198
APP_ENVbranching only treats exactlyprodas managed identity; any other non-devenvironment (e.g.,staging,test) will fall back toAsyncDefaultAzureCredential, which contradicts the PR description (“production and other environments”) and also makes the CodeQL comment (“only used in development”) inaccurate. Consider makingdevthe only case that usesAsyncDefaultAzureCredentialand using managed identity for all otherAPP_ENVvalues (consistent withContentProcessorAPI/app/utils/azure_credential_utils.py).
app_env = os.getenv("APP_ENV", "prod").lower()
if app_env == "prod":
client_id = os.getenv("AZURE_CLIENT_ID")
if client_id:
logging.info(
Purpose
This pull request updates the Azure credential selection logic to improve environment-specific authentication and fixes the usage of the
get_async_azure_credentialfunction in token provider setup. The most important changes are:Azure Credential Selection Logic:
get_async_azure_credentialinazure_credential_utils.pyto select credentials based on theAPP_ENVenvironment variable: usesAsyncDefaultAzureCredentialfor development (dev), andAsyncManagedIdentityCredentialwith a client ID for production and other environments. Added logging and print statements to clarify which credential is being used.Token Provider Setup:
get_async_bearer_token_providerinazure_credential_utils.pyandcredential_util.pyto callget_async_azure_credential()synchronously (removedawait), aligning with the updated credential function's implementation. [1] [2]Does this introduce a breaking change?
Golden Path Validation
Deployment Validation
What to Check
Verify that the following are valid
Other Information