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.

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

Jump to bottom

Allow persisting CORS for ACA deployments (applies to .NET Aspire) #4325

Closed
wants to merge 2 commits into from
Closed

Allow persisting CORS for ACA deployments (applies to .NET Aspire) #4325

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f0a4e0
allow persisting cors for Aspire
vhvb1989 Sep 16, 2024
6333f01
Merge branch 'main' of github.com:Azure/azure-dev into aspire-persist…
vhvb1989 Sep 18, 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
5 changes: 0 additions & 5 deletions cli/azd/pkg/alpha/alpha_features.go
View file
Open in desktop

This file was deleted.

14 changes: 13 additions & 1 deletion cli/azd/pkg/containerapps/container_app.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ const (
pathConfigurationIngressFqdn = "properties.configuration.ingress.fqdn"
pathConfigurationIngressCustomDomains = "properties.configuration.ingress.customDomains"
pathConfigurationIngressStickySessions = "properties.configuration.ingress.stickySessions"
pathConfigurationCors = "properties.configuration.ingress.corsPolicy"
)

// ContainerAppService exposes operations for managing Azure Container Apps
Expand Down Expand Up @@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

We're going to end up with like 50 of these 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

Hopefully we will have the AppHost producing the ACA definition soon and get rid of all of these.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

getContainerApp is not part of deployment. We call it only if we want to persist something:

image

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@weikanglim weikanglim Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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")

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@weikanglim weikanglim Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


func (cas *containerAppService) persistSettings(
ctx context.Context,
Expand All @@ -138,8 +140,9 @@ func (cas *containerAppService) persistSettings(
) (map[string]any, error) {
shouldPersistDomains := cas.alphaFeatureManager.IsEnabled(persistCustomDomainsFeature)
shouldPersistIngressSessionAffinity := cas.alphaFeatureManager.IsEnabled(persistIngressSessionAffinity)
shouldPersistCors := cas.alphaFeatureManager.IsEnabled(persistCors)

if !shouldPersistDomains && !shouldPersistIngressSessionAffinity {
if !shouldPersistDomains && !shouldPersistIngressSessionAffinity && !shouldPersistCors {
return obj, nil
}

Expand Down Expand Up @@ -168,6 +171,15 @@ func (cas *containerAppService) persistSettings(
}
}

if shouldPersistCors {
cors, has := aca.GetMap(pathConfigurationCors)
if has {
if err := objConfig.Set(pathConfigurationCors, cors); err != nil {
return nil, fmt.Errorf("setting cors: %w", err)
}
}
}

return objConfig.Raw(), nil
}

Expand Down
2 changes: 2 additions & 0 deletions cli/azd/resources/alpha_features.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,5 @@
description: "Do not change Ingress Session Affinity when deploying Azure Container Apps."
- id: deployment.stacks
description: "Enables Azure deployment stacks for ARM/Bicep based deployments."
- id: aca.persistCors
description: "Do not change CORS when deploying Azure Container Apps."
Loading