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

using module outputs in reference results into errors #2526

Open
Stefan-Franke-M10 opened this issue May 5, 2021 · 9 comments
Open

using module outputs in reference results into errors #2526

Stefan-Franke-M10 opened this issue May 5, 2021 · 9 comments

Comments

@Stefan-Franke-M10
Copy link

Stefan-Franke-M10 commented May 5, 2021

Bicep version
Bicep CLI version 0.3.255 (589f037)

Describe the bug
When using modules, using the module output results in errors while referencing them in a other Bicep file.
In my case a Function App with App settings. Here I want to reference the Storage Account connection string.

error: The template function 'reference' is not expected at this location.
This bug doesn't occur when using a resource deployment of a storage Account instead of using a module.

Desired solution: the possibility to use module outputs in combination with reference functions

To Reproduce
See provided bicep templates: Bicep issue sample.zip

Additional context
The error occurs when I want to link the outputs in a Azure Function appSetting : AzureWebJobsStorage.

Outputs used:

  • name
  • id
  • apiVersion

sample:

'DefaultEndpointsProtocol=https;AccountName=${stgModule.outputs.storageAccount.name};AccountKey=${listKeys(stgModule.outputs.storageAccount.id, stgModule.outputs.storageAccount.name.apiVersion).keys[0].value}'

@ghost ghost added the Needs: Triage 🔍 label May 5, 2021
@alex-frankel
Copy link
Collaborator

alex-frankel commented May 5, 2021

Looking at your output from the storage-account.bicep file:

output storageAccount object  = {
  id: stg.id
  name: stg.name
  apiVersion: stg.apiVersion
}

Shouldn't this reference:

stgModule.outputs.storageAccount.name.apiVersion

instead be:

stgModule.outputs.storageAccount.apiVersion

As a side note, this would have benefited from the planned improvements to give you the ability to output a resource and then we could have validated this.

@Stefan-Franke-M10
Copy link
Author

Stefan-Franke-M10 commented May 5, 2021

Sorry, my bad when extracting the bicep templates from my original project I've made a typo.
The error still exists when using :

stgModule.outputs.storageAccount.apiVersion

I've removed the error and included the fixed template:
Bicep issue sample fixed typo.zip

@alex-frankel
Copy link
Collaborator

@bmoore-msft - do we not allow reference() in appSettings? Seems like this should be working..

For this line:

'DefaultEndpointsProtocol=https;AccountName=${stgModule.outputs.storageAccount.name};AccountKey=${listKeys(stgModule.outputs.storageAccount.id, stgModule.outputs.storageAccount.apiVersion).keys[0].value}'

stgModule.outputs.storageAccount.apiVersion will translate to a reference call:

...
"siteConfig": {
  "appSettings": [
    {
      "name": "AzureWebJobsStorage",
      "value": "[format('DefaultEndpointsProtocol=https;AccountName={0};AccountKey={1}', reference(resourceId('Microsoft.Resources/deployments', parameters('functionApp').storageAccount.name), '2019-10-01').outputs.storageAccount.value.name, listKeys(reference(resourceId('Microsoft.Resources/deployments', parameters('functionApp').storageAccount.name), '2019-10-01').outputs.storageAccount.value.id, reference(resourceId('Microsoft.Resources/deployments', parameters('functionApp').storageAccount.name), '2019-10-01').outputs.storageAccount.value.name.apiVersion).keys[0].value)]"
    },
...

@bmoore-msft
Copy link
Contributor

You can use in reference() in appSettings but you cannot use reference() within another runtime function - which is what the module output is trying to do... This:

value: 'DefaultEndpointsProtocol=https;AccountName=${stgModule.outputs.storageAccount.name};AccountKey=${listKeys(stgModule.outputs.storageAccount.id, stgModule.outputs.storageAccount.name.apiVersion).keys[0].value}'

needs to be:

 value: 'DefaultEndpointsProtocol=https;AccountName=${functionApp.storage.name};AccountKey=${listKeys(resourceId('Microsoft.Storage/storageAccounts', functionApp.storageAccount.name), '2021-04-01').keys[0].value}'

Notes:

  • bicep makes it too easy use a pattern that won't work in a deployment - we should probably block that until we fix it in the engine (which I suspect will be a really long time)
  • the first one (accountName reference) doesn't have to be changed but it keeps the code/pattern consistent
  • the apiVersion should never be an expression, that will break your code eventually (it happened when the storage keys became an array for example (they used to be key1 and key2))

@Stefan-Franke-M10
Copy link
Author

Stefan-Franke-M10 commented May 6, 2021

@bmoore-msft
Could you explain me the difference between using the module deployment and a normal resource deployment.

Last night I've extracted the storage account resource from the module and added it to the function app deployment as a resource. When I use the resource outputs as inputs in the value, the deployment works fine. Not exactly what I want but it seems to me that the problem lies in using module outputs instead of regular outputs.

the apiVersion should never be an expression, that will break your code eventually (it happened when the storage keys became an array for example (they used to be key1 and key2))

I don't completely agree with you on the expression part of this one. In Bicep the resource Type includes the API Version used.
In your case the API Version, in the listkeys functions, needs to be manually adjusted every time a newer resource Type (API Version) is being used.
So the code will break when a newer version of the Storage Account resource has been deployed.
And yes, it won't work anymore when there will be new breaking changes introduced in the listKeys function for Storage Accounts, but that's less likely to happen than updating the API Version of the resource.

@alex-frankel
Copy link
Collaborator

Could you explain me the difference between using the module deployment and a normal resource deployment.

id and apiVersion are what we call "compile-time constants" which means we don't need a reference() call in the ARM layer to retrieve this information for a normal resource declaration. However, to get any module output (regardless of if it's a compile-time constant property), we need the reference call.

I didn't know that reference() cannot be used inside of listKeys(). @majastrz/@shenglol - we should validate this in the language service.

@bmoore-msft
Copy link
Contributor

Looks like @alex-frankel covered the first question...

re: apiVersion - the shape of the object returned on listKeys() is determined by that apiVersion - i.e. the properties you reference > keys[0].value. If that apiVersion is an expression that's used in a module elsewhere it requires you to also scrub indirect uses of it to ensure they aren't broken. So you're creating a dependency that doesn't provide any benefit (since it needs to be authored and maintained separately) but creates risk.

Another way to look at it - the apiVersion used to create the resource has no effect on accessing that resource via listKeys. They are unrelated/independent api calls.

That help?

@Stefan-Franke-M10
Copy link
Author

I think I understand it now.

So if I'm correct, there are two options left (and one nobody should use):

1. Don't use a module in this case, rather use a resource deployment that doesn't give any errors.
Pro: no dependency issues, and we can use the outputs of the resource deployment in te function App deployment.
Con: there woud be 2 separate templates for a Storage Account creation.

2. Use the suggested value by @bmoore-msft
Pro: We can still use the module to deploy the Storage Account.
Con: There would be dependency issues between the Storage Account and the creation of the Function App Settings. So we would need to add a dependsOn in the functionApp deployment.

A third Bad suggestion could be: Moving the creation of the connection string to the Storage Account Module.
Pro: The connection string is created with the Storage Account so every resource can get the connection string by the output value.

Con's:

  • This is against the best practice to use ListKeys to retrieve the key.
  • This would also be a security risk because outputs can't be secured, and there would be a connectionstring in the output blade in plaintext.

The last one could be the best option if there was a possibility to secure output variables.

What would be your suggested go to option?

@bmoore-msft
Copy link
Contributor

bmoore-msft commented May 6, 2021

$0.02

#1 - I'm not sure I understand why there would be 2 templates for the storageAccount, but seems like not allowing a module (which is needed for the fix) may be too restrictive...

#2 - good catch on the dependsOn, though I'm not sure I understand why that's a con... the dependency is there regardless, just a difference of whether it's visible in bicep. @alex-frankel - a bizzarre idea here would be to look for the use of an expression that uses the name property of another resource in the template - and use that as an indication that we should add an implicit dependency. Definitely needs more thinking, but may help here...

#3 - I don't know that we'll ever get to the point where everyone will be satisfied with the use of arbitrary secrets in the deployment engine - they should be in keyvault and reference by the resource (ideally)... TDLR I wouldn't use this one or wait for the ideal handling of it.

(oh sorry - and if I had to pick I'd pick #2)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants