Allow persisting CORS for ACA deployments (applies to .NET Aspire) #4325
Conversation
vhvb1989
requested review from
ellismg,
wbreza,
hemarina and
weikanglim
as code owners
| @@ -127,6 +128,7 @@ const apiVersionKey = "api-version" | |||
|
|
|||
| var persistCustomDomainsFeature = alpha.MustFeatureKey("aca.persistDomains") | |||
| var persistIngressSessionAffinity = alpha.MustFeatureKey("aca.persistIngressSessionAffinity") | |||
| var persistCors = alpha.MustFeatureKey("aca.persistCors") | |||
There was a problem hiding this comment.
We're going to end up with like 50 of these 😄
There was a problem hiding this comment.
Hopefully we will have the AppHost producing the ACA definition soon and get rid of all of these.
There was a problem hiding this comment.
Maybe the alpha feature should be something like: "give us the paths of keys that you want preserved" instead of adding a switch per property? That would be a little more generalized and would at least force folks to think about the underlying container app object we are building.
There was a problem hiding this comment.
I'm not as familiar with many of the Aspire use cases - but why do we need to explicitly call out parts of the configuration vs taking the snapshot of the current version, then layer the yaml manifest config to form the final container app spec to update?
There was a problem hiding this comment.
taking snapshot of the current version is not part of the flow, @wbreza
Currently, azd is only taking this snapshot when requested by the user by turning any of alpha features ON.
We could move into always get the current ACA and overlay with YAML, but we are already working on moving from YAML to BICEP. And long term, this is expected to be replaced 100% by the AppHost. I don't think we should invest into a more solid approach than what we have because it would be destined to be removed
There was a problem hiding this comment.
getContainerApp is not part of deployment. We call it only if we want to persist something:
There was a problem hiding this comment.
I guess I'm not fully understanding the use case. Why wouldn't we persist all of the configuration data that was set during the app provisioning?
There was a problem hiding this comment.
What we're doing here is ignoring IaC as the source-of-truth to allow users to overcome a current bottleneck in the current design (i.e., ACA resource definition not being full representable at the AppHost layer):
- Bicep/IaC is usually the source-of-truth of the Azure resource definition.
- But here, we are saying "let's allow the user to tweak this value in Azure Portal, and we will just ignore Bicep when we reprovision"
"give us the paths of keys that you want preserved" instead of adding a switch per property
I agree with @ellismg, I think this would be most flexible. Can we make a config.json property for a .JSONPath to ignore?
"aspire.containerapp.unmanaged.properties": [
"properties.configuration.ingress.corsPolicy",
"properties.configuration.ingress.stickySessions"
]We would just need better docs to show how to do all these things. Alternatively, we could still make the alpha feature trigger such a thing underneath, but it'd perhaps make the code churn easier?
The other thing we could do is move this all the way up to AppHost:
thing.AddUnmanagedProperty("properties.configuration.ingress.corsPolicy")There was a problem hiding this comment.
Thanks for providing more context here @weikanglim. I'm just not convinced that ignoring IaC as the source of truth is the right choice here.
I'm all for users experimenting with configuration in the Azure Portal, but if the user wants to keep those changes they should go back and make the appropriate tweaks in their IaC / or manifest, etc.
Is the root cause here that the current AppHost layer does not support all the knobs required to set/persist these types of settings?
There was a problem hiding this comment.
In case it wasn't clear from the previous message -- yes, this is a point-in-time thing. I would assume that Aspire will eventually surface these knobs, but it requires modeling it at the AppHost layer, which I could imagine takes time. We are going to solve a similar problem in upcoming azd composability work.
|
I think we should look at preserving all of the data from the remote but only in the case where infra synth isn't run. |
@davidfowl Have you thought about how this is going to interact with the work you've started on for compute customization? It feels like if we do this it is going to force you to fetch the current state of each container app during manifest publishing and then merge it with the desired state from the user. Is that something you want? Another thing to note, the shape of the object in azure is dependent on the API version you use when you send your My gut says you're walking into a quagmire trying to do a diff and merge here... |
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
|
Closing this as not needed anymore. The path to customizing ACA is set for Aspire9 by using the AppHost + Azure Provisioning libraries |
fix: dotnet/aspire#5029
Turn featue on with
azd config set alpha.aca.persistCors onfix #4395