eriqua
commented
1 year ago Additional comments from the related discussion
Open eriqua opened 1 year ago
eriqua
commented
1 year ago Additional comments from the related discussion
jikuja
commented
1 year ago Here is my first untested idea for controlling which app config values are being retained:
diff --git a/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep b/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep
index ceb99ff..205aa0c 100644
--- a/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep
+++ b/FunctionApp/template/Microsoft.Web/sites/config-appsettings/deploy.bicep
@@ -32,6 +32,12 @@ param enableDefaultTelemetry bool = true
@description('Optional. Existing function app configuration')
param existingConfig object = {}
+param retainAppSetting string
+
+param retainAppSettingsArray array
+
+param appSettingsKeyValuePairsOverridesExistingConfig bool
+
// =========== //
// Variables //
// =========== //
@@ -46,13 +52,13 @@ var appInsightsValues = !empty(appInsightId) ? {
APPLICATIONINSIGHTS_CONNECTION_STRING: appInsight.properties.ConnectionString
} : {}
-var filteredExistingConfig_ = [for item in items(existingConfig): item.key != 'APPINSIGHTS_INSTRUMENTATIONKEY' && item.key != 'APPLICATIONINSIGHTS_CONNECTION_STRING' && item.key != 'AzureWebJobsStorage' && item.key != 'AzureWebJobsDashboard' ? {
+var filteredExistingConfig_ = [for item in items(existingConfig): (retainAppSetting == 'retainAppSetting' && contains(retainAppSettingsArray, item.key) ) && item.key != 'APPINSIGHTS_INSTRUMENTATIONKEY' && item.key != 'APPLICATIONINSIGHTS_CONNECTION_STRING' && item.key != 'AzureWebJobsStorage' && item.key != 'AzureWebJobsDashboard' ? {
'${item.key}': item.value
} : {}]
var filteredExistingConfig = reduce(filteredExistingConfig_, {}, (cur, next) => union(cur, next))
-// TODO: add parameter to control appSettingsKeyValuePairs or filteredExistingConfig has higher preference
-var expandedAppSettings = union(appSettingsKeyValuePairs, filteredExistingConfig, azureWebJobsValues, appInsightsValues)
+// order of the filteredExistingConfig and appSettingsKeyValuePairs is controlled by appSettingsKeyValuePairsOverridesExistingConfig
+var expandedAppSettings = appSettingsKeyValuePairsOverridesExistingConfig ? union(filteredExistingConfig, appSettingsKeyValuePairs, azureWebJobsValues, appInsightsValues) : union(appSettingsKeyValuePairs, filteredExistingConfig, azureWebJobsValues, appInsightsValues)
// =========== //
// Existing resources //
diff --git a/FunctionApp/template/Microsoft.Web/sites/deploy.bicep b/FunctionApp/template/Microsoft.Web/sites/deploy.bicep
index 5c9a617..b182874 100644
--- a/FunctionApp/template/Microsoft.Web/sites/deploy.bicep
+++ b/FunctionApp/template/Microsoft.Web/sites/deploy.bicep
@@ -64,6 +64,16 @@ param appSettingsKeyValuePairs object = {}
@description('Optional. The auth settings V2 configuration.')
param authSettingV2Configuration object = {}
+@description('Optional. Retain existing app config')
+@allowed(['None', 'Selected', 'All'])
+param retainAppSetting string = contains(kind, 'functionapp') ? 'Selected' : 'None'
+
+@description('Optional. List of retained app config keys. Used when retainAppSetting = \'Selected\'')
+param retainAppSettingsArray array = contains(kind, 'functionapp') ? ['WEBSITE_RUN_FROM_PACKAGE'] : []
+
+@description('a')
+param appSettingsKeyValuePairsOverridesExistingConfig bool = false
+
// Lock
@allowed([
''
@@ -221,7 +231,7 @@ resource app 'Microsoft.Web/sites@2021-03-01' = {
}
}
-module existingConfig 'config-appsettings/existing-config.bicep' = {
+module existingConfig 'config-appsettings/existing-config.bicep' = if (retainAppSetting != 'None') {
name: '${uniqueString(deployment().name, location)}-Existing-Site-Config'
params: {
parent: app.name
@@ -238,7 +248,10 @@ module app_appsettings 'config-appsettings/deploy.bicep' = if (!empty(appSetting
setAzureWebJobsDashboard: setAzureWebJobsDashboard
appSettingsKeyValuePairs: appSettingsKeyValuePairs
enableDefaultTelemetry: enableReferencedModulesTelemetry
- existingConfig: existingConfig.outputs.existingConfig.properties
+ existingConfig: retainAppSetting != 'None' ? existingConfig.outputs.existingConfig.properties : { }
+ retainAppSetting: retainAppSetting
+ retainAppSettingsArray: retainAppSettingsArray
+ appSettingsKeyValuePairsOverridesExistingConfig: appSettingsKeyValuePairsOverridesExistingConfig
}
}This should add some basic control for app config retain:
Microsoft.Web/sites/deploy.bicep three more parameters:
None nothing is retainedAll everything except values that are created on Microsoft.Web/sites/config-appsettings/deploy.bicep are retainedSelected only selected values are retainedSupport for slots is still missing. Is there something else important missing or does anyone see problems with the current PoC.
Now default setting will retain WEBSITE_RUN_FROM_PACKAGE app config for functions apps. I'm not sure if that should be turned on by default or not.
Robbertbbb
commented
1 year ago my current solution is: create an user managed identity create an storage account to be reused for deployment scripts (speeds up the deployment script to reuse storage account) give the identity read access on the subscription via bicep deploy a deployment script with powershell or az cli command which checks if the resources exist output with an true/false (using the user managed identity)
if the resource exist i'm running a module which gets the existing function with appconfig and merge that app config with the infra infra netconfig
if it is false ill output an empty object which merges with the app config
AlexanderSehr
commented
1 year ago Hey @jikuja I wonder how complicated this may become given the vast amount of possible app settings (which are already split across multiple child items).
Reading the description and reasoning for the change I wonder if an alternative pattern wouldn't be to split the function app deployment (as part of a solution/workload) into its own pipeline to allow an independent update of the function app settings (& code). As you'd still need reference to resources you could then leverage 'exiting' resources instead.
This is not to say there is no merit in adding this 'only' update mode, but I have to wonder how 'advanced' is too advanced.
This would become a lot easier once #4023 is implemented. Just adding it here for reference.
jikuja
commented
10 months ago @tsc-buddy Has there been any discussions if/when problems mentinoed here could be fixed by RP instead of extra templating logic layers?
Highes priority should for WEBSITE_RUN_FROM_PACKAGE setting, all other are nice to have.
nad-au
commented
7 months ago Just enquiring if there's an update on this? Like others, my Linux consumption function app gets deleted when bicep is redeployed. I'm in the process of adding functions and as part of that I need to create additional app settings. Once I deploy all the functions are deleted.
I've tried setting and removing WEBSITE_RUN_FROM_PACKAGE = 1 from the bicep sites > properties > siteConfig > appSettings but makes no difference.
We're about to go live with a bunch of webhook handlers which run 24/7. I'm now afraid to put this into production for fear of accidental deletion.
FWIW we deploy our node apps with GHA Azure/functions-action@v1
jikuja
commented
7 months ago It might be a good idea to
You can also raise a ticket with Microsoft CSS (Microsoft Customer Services & Support) and your ticket will be triaged by them for any platform issues and if deemed not the platform but a module issue, it will be redirected to the AVM team and the module owner(s). because issue in on service, not really on the module.The fix I would like to see is in the first message under the title The proper solution
Robbertbbb
commented
7 months ago you can also look at: https://learn.microsoft.com/en-us/azure/azure-app-configuration/ this pulls the config apart
AlexanderSehr
commented
7 months ago Hola together, thanks @jikuja for referencing the AVM repo, it's correct that we've been migrating modules (leaving CARML eventually 'only' the CI for inner-sourcing) since September last year (as tracked in the 'Per Module Edits' section here). We'll also migrate all module issues over once we're done with that, which is why only very few issues got resolved in CARML since (and none for modules that already got migrated).
That being said, I agree that the RP doesn't exactly make it easy to implement. However, implementing that layer should be technically possible - and given that each module in AVM has voluntary owners that are assigned the issues and handle them, we should see some traction there too. Whether that's an implementation or inquiry to the PG(s) is up to the corresponding owner.
Last but not least, as the Web/sites module is already migrated, I'll go ahead and move this issue now. Will cc you to make sure you can find it after.
cc: @Robbertbbb @jikuja @nad-au
AlexanderSehr
commented
7 months ago @tsc-buddy, please note that I just transfered this issue from the original CARML repository. Would be great if you could take a look once you have a moment :)
tsc-buddy
commented
7 months ago Hey @AlexanderSehr - sure thing. I shall schedule some time aside to take a look this in the coming week.
AlexanderSehr
commented
7 months ago Hey @AlexanderSehr - sure thing. I shall schedule some time aside to take a look this in the coming week.
Much appreciated, thanks. If you need any support, let me/us know.
tsc-buddy
commented
6 months ago This will be scheduled for review in the Month or March 2024.
AlexanderSehr
commented
6 months ago This will be scheduled for review in the Month or March 2024.
Thanks @tsc-buddy, sorry the bot is very very persisent. @matebarabas would it be a possiblilty to reduce it's schedule? Some items have so many of these bot comments...
krbar
commented
5 months ago We just hit this issue in a customer project, so I'm also very interested in a fix.
EverybodyKurts
commented
5 months ago Hi @krbar, this article might be able to help you: https://blog.dotnetstudio.nl/posts/2021/04/merge-appsettings-with-bicep/
I think I've ran into the same issues that you've experienced and following the previous article helped me keep the old configuration settings.
krbar
commented
5 months ago Hi @krbar, this article might be able to help you: https://blog.dotnetstudio.nl/posts/2021/04/merge-appsettings-with-bicep/
Looks promising, thank you @EverybodyKurts!
@AlexanderSehr, @tsc-buddy this looks like a good inspiration on how we could implement it in AVM, what do you think?
jikuja
commented
5 months ago Looks really similar with mine.
Using currentAppSettings: list('{myAwesomeFunction.id}/config/appsettings', '2020-12-01').properties is really neat trick and simplifies code.
how we could implement it in AVM
First thing:
Then to plan how settings are merged?
Robbertbbb
commented
5 months ago But does it work if the app service is deployed for the very first time? (So no list possible)
AlexanderSehr
commented
5 months ago But does it work if the app service is deployed for the very first time? (So no list possible)
Fair point 😄 . @jikuja, any experience with that? It it works in any way like the existing resource declaration (which of course is also just a function), I have a wild guess ;)
jikuja
commented
5 months ago But does it work if the app service is deployed for the very first time? (So no list possible)
Fair point 😄 . @jikuja, any experience with that? It it works in any way like the
existingresource declaration (which of course is also just a function), I have a wild guess ;)
Good points here. I tested Schutten's code and works even on first deployment
currentAppSettings: list('{myAwesomeFunction.id}/config/appsettings', '2020-12-01').properties compiles to
"currentAppSettings": {
"value": "[list(format('{0}/config/appsettings', resourceId('Microsoft.Web/sites', 'myAwesomeFunctionfoo1')), '2020-12-01').properties]"
}Existing keyword documentation clearly states that deployment will fail when referenced resource is not found: If you attempt to reference a resource that doesn't exist, you get the NotFound error and your deployment fails. Check the name and scope of the resource you're trying to reference.
If non-accessor typed list*() invocations are called when resource is being deployed then function app and its config/appsettings should exist when list('${myAwesomeFunction.id}/config/appsettings', '2020-12-01').properties "is being executed"
@alex-frankel Do you want to comment if we are trying to build something that is working by luck or is it working by design? Main points starts from the comment https://github.com/Azure/bicep-registry-modules/issues/949#issuecomment-2037850258
Understanding exact details of IL is kind of black magic.
scrocquesel-ml150
commented
4 months ago Regarding WEBSITE_RUN_FROM_PACKAGE, why not just pass WEBSITE_RUN_FROM_PACKAGE=1 when necessary ? IMHO, how the app should be deployed and run is an infrastructure decision. Running the app from a package or a remote url location shouldn't be the concern of the binary deployment task itself (despite it is what AzureRmWebAppDeployment is doing by modifying the app settings instead of reading them).
Discussed in https://github.com/Azure/ResourceModules/discussions/2572
cc: @jikuja, @mikkark