Skip to content
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.

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

Add Azure Container Apps as a host option #1952

Merged
merged 35 commits into from
Sep 19, 2024

Conversation

1yefuwang1
Copy link
Contributor

@1yefuwang1 1yefuwang1 commented Sep 3, 2024

Purpose

Hi maintainers, thank you for making this great sample. I'm with Azure Container Apps team and trying to add Azure Container Apps as a host option.

Due to a limitation of azd, only one azure.yaml file can exist in the project root folder and there's no way to define both ACA and AppService in it at the same time.

This PR initially suggested using a separate folder for the ACA azure.yaml, but that had the drawback of affecting relative paths for scripts and affecting the ability to fetch the azd environment variables.

After discussion with Pamela, we decided to use the approach of a commented out line in the root azure.yaml. That minimizes the amount of redudant files needed, and makes sure that the azd environment stays in the root .azure/ folder.

Does this introduce a breaking change?

The changes are isolated in new files and folders. So I believe there will not be any breaking changes.

[ ] Yes
[X] No

Does this require changes to learn.microsoft.com docs?

This repository is referenced by this tutorial
which includes deployment, settings and usage instructions. If text or screenshot need to change in the tutorial,
check the box below and notify the tutorial author. A Microsoft employee can do this for you if you're an external contributor.

[X ] Yes
[ ] No

Type of change

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Documentation content changes
[ X] Other... Please describe: Add ACA as a new host option

Code quality checklist

See CONTRIBUTING.md for more details.

No python code changes.

  • The current tests all pass (python -m pytest).
  • I added tests that prove my fix is effective or that my feature works
  • I ran python -m pytest --cov to verify 100% coverage of added lines
  • I ran python -m mypy to check for type errors
  • I either used the pre-commit hooks or ran ruff and black manually on my code.

Copy link

github-actions bot commented Sep 3, 2024

Check Country Locale in URLs

We have automatically detected added country locale to URLs in your files.
Review and remove country-specific locale from URLs to resolve this issue.

Check the file paths and associated URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
docs/azure_container_apps.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development10
aca-host/README.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development10

Copy link

github-actions bot commented Sep 3, 2024

Check Country Locale in URLs

We have automatically detected added country locale to URLs in your files.
Review and remove country-specific locale from URLs to resolve this issue.

Check the file paths and associated URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
docs/azure_container_apps.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development15
aca-host/README.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development14

Copy link

github-actions bot commented Sep 3, 2024

Check Country Locale in URLs

We have automatically detected added country locale to URLs in your files.
Review and remove country-specific locale from URLs to resolve this issue.

Check the file paths and associated URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
docs/azure_container_apps.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development16
aca-host/README.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development15

Copy link

github-actions bot commented Sep 3, 2024

Check Country Locale in URLs

We have automatically detected added country locale to URLs in your files.
Review and remove country-specific locale from URLs to resolve this issue.

Check the file paths and associated URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
docs/azure_container_apps.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development16
aca-host/README.md
#LinkLine Number
1https://learn.microsoft.com/en-us/windows/apps/get-started/enable-your-device-for-development16

@pamelafox
Copy link
Collaborator

Thanks for the PR! We do want to optionally support ACA deployment, and I had a branch that did that las year, but I want to make sure it's maintainable. A few questions:

  1. We are trying to move to AVM. Have you tried using the AVM modules for ACA? It may not be possible yet with the way azd deploys to ACA, I'm not sure.
  2. Does your ACA deployment also include private endpoint support, and have you tested that it works?
  3. Did you consider having a parameter like appHost sent into main.bicep, and just reusing the same main.bicep, to minimize the amount of redundant code?

cc @tonybaloney who's been working on the AVM effort,
cc @vhvb1989 from azd team who can comment on whether azd offers any better ways to switch hosts

@1yefuwang1
Copy link
Contributor Author

1yefuwang1 commented Sep 4, 2024

  1. I have already used AVM modules for managed environment, container registry. But I'm not using AVM for user assignment managed identity and role assignments. AVM cannot be used for all aca resources because, azd has its own convention on deploying ACA. Specifically container-app-upsert.bicep defines exists, which I believe is used by azd to deploy the container image after infra has been provisioned. My implementation is based on this project https://github.com/Azure-Samples/azure-django-postgres-flexible-aca
  2. Private endpoint is still in private preview for ACA, so it is not tested.
  3. I wanted to keep my changes isolated so it's in its own bicep file. Sure, I'll try to reuse main.bicep

@1yefuwang1
Copy link
Contributor Author

Hi @pamelafox, my current problem is that after putting aca's azure.yaml in a subfolder, the scripts referenced in the azure.yaml report error because they are written with paths relative to cwd that assumes they are called from project root like this one.

image

To workaround this, I create some symlinks to scripts , data and app in the aca-host folder. It works fine on Unix based systems, but not on windows. Because symlinks are not enabled by default on windows, which is a big problem.

My questions are:
1.do you mind if I change the scripts and make the paths relative to the script's directory(e.g. use $PSScriptRoot for powershell) instead of being relative to cwd? So that the scripts work no matter which cwd they get called from, and I can remove all the symlinks. An example is below
image

2.What do you think about the unified approach for supporting different deployment target in this repo azure-search-openai-demo-java/deploy at main · Azure-Samples/azure-search-openai-demo-java (github.com)

Copy link

github-actions bot commented Sep 6, 2024

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394256
samples/chat/README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394265
aca-host/README.md
#LinkLine Number
1https://stackoverflow.com/questions/5917249/git-symbolic-links-in-windows16
docs/azure_container_apps.md
#LinkLine Number
1https://stackoverflow.com/questions/5917249/git-symbolic-links-in-windows16

infra/main.bicep Outdated Show resolved Hide resolved
infra/main.bicep Outdated Show resolved Hide resolved
@mattgotteiner
Copy link
Collaborator

The description is referring to a bunch of symlinks and an "aca-host" folder that doesn't exist. Is this outdated?

@pamelafox
Copy link
Collaborator

@mattgotteiner Yes, sorry, we've had a lot of offthread discussion, both with Yefu and with the azd team. I really wanted to make sure that we kept the azd environment in ".azure" at the root, and to reduce our maintenance burden, so I voted for using the approach of the comment in azure.yaml.

@mattgotteiner
Copy link
Collaborator

@mattgotteiner Yes, sorry, we've had a lot of offthread discussion, both with Yefu and with the azd team. I really wanted to make sure that we kept the azd environment in ".azure" at the root, and to reduce our maintenance burden, so I voted for using the approach of the comment in azure.yaml.

Got it. Any way to update the description?

@pamelafox
Copy link
Collaborator

@mattgotteiner I've updated it to reflect current approach

@pamelafox
Copy link
Collaborator

I merged in main after committing the Bicep formatting changes there, so main.bicep only has ACA related changes now.

Copy link

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394262
samples/chat/README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394265

Copy link

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394262
samples/chat/README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394265

@pamelafox
Copy link
Collaborator

Update: I had to modify our logic in app.py for selecting which credential to use, as we recently changed that to specifically use ManagedIdentityCredential. My latest deploy isn't working with the change, as it says "azure.core.exceptions.ClientAuthenticationError: (None) Unable to load the proper Managed Identity."
Not sure why that's happening, as AZURE_CLIENT_ID is set as needed, so it should use that value for the Managed Identity.

If you have time to debug it, let me know if you see the issue.

@pamelafox
Copy link
Collaborator

Update: It's working for me now when I change ManagedIdentityCredential to

azure_credential = ManagedIdentityCredential(client_id=os.getenv("AZURE_CLIENT_ID"))

But it's strange that it's not correctly finding AZURE_CLIENT_ID. That works for me in other samples.

Anyway, I guess it's fine to add yet another if condition for the AZURE_CLIENT_ID existence.

@1yefuwang1
Copy link
Contributor Author

1yefuwang1 commented Sep 18, 2024

Sorry, I just come back from holiday. According to microsoft/azure-container-apps#442,

  1. Previously, DefaultAzureCredential is used and it will automatically use AZURE_CLIENT_ID environment variable. It is azure SDK's behavior.
  2. With ManagedIdentityCredential, you'll need to manually pass the value of AZURE_CLIENT_ID to its constructor if a user assigned managed identity (UAMI) is used, which is the case in my PR because a UAMI is needed to pull docker images from ACR and it is reused for authentication to other resources. If nothing is passed to ManagedIdentityCredential's constructor, container app's system assigned managed identity(SAMI) will be tried, which is not enabled by my bicep and causes your error.

My guess is that you are using SAMI in other samples, which explains why they work without passing client id to
ManagedIdentityCredential.

Copy link

Check Broken URLs

We have automatically detected the following broken URLs in your files. Review and fix the paths to resolve this issue.

Check the file paths and associated broken URLs inside them. For more details, check our Contributing Guide.

File Full Path Issues
README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394262
samples/chat/README.md
#LinkLine Number
1https://stackoverflow.com/questions/35569042/ssl-certificate-verify-failed-with-python3/43855394#43855394265

app/backend/app.py Outdated Show resolved Hide resolved
@pamelafox
Copy link
Collaborator

Looking good! I did a test deploy to ACA and app service. I'll merge tomorrow.

@pamelafox pamelafox merged commit 0225f75 into Azure-Samples:main Sep 19, 2024
16 checks passed
pavsingh7 added a commit to pavsingh7/azure-search-openai-demo that referenced this pull request Sep 23, 2024
Add Azure Container Apps as a host option (Azure-Samples#1952)
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.

3 participants