Azure / bicep

Bicep is a declarative language for describing and deploying Azure resources
MIT License
3.23k stars 748 forks source link

Set or skip an object property based on a condition #387

Open majastrz opened 4 years ago

majastrz commented 4 years ago

Is your feature request related to a problem? Please describe. When conditionally enabling ipv6 in a template, there are cases where some properties need to be either set or omitted from a resource declaration. Currently in Bicep, this requires creating two separate object literals and choosing between them via the ternary operator.

Describe the solution you'd like Can we create a simpler way of doing this that doesn't involve declaring two separate objects? Should we consider introducing a concept of undefined that is separate from null?

alex-frankel commented 4 years ago

@marcre - we're pretty sure null with the ternary operator would fulfill this, so wondering if we are missing something.

marcre commented 4 years ago

Yes and no - depends on how we handle NULL.. Does null mean we omit the value when we send to the RP, or do we send the null on. If we don't send the null, then we don't need undefined. However, with PUTCH RPs like compute you may need to be able to explicitly set something to null to make sure it is cleared if previously set.

majastrz commented 4 years ago

Agree with that. We would like the distinction between null and undefined/omitted properties not to exist, but the reality disagrees.

slavizh commented 4 years ago

agree as well. Not all RPs react on null equally.

pacodelacruz commented 3 years ago

I have had to rely on the createObject function nested in a compex comparison function inside a copy for an array property and it is very painful. Particularly when error messages from the RPs do not help.

Happy to share a sample of this!

I'd say that having an easy way of defining if you want a property to be omitted, set to null, or set to an object would be very useful.

khowling commented 3 years ago

+1, this would be super useful in so many use-cases. for example, like in Javascript

param autoscaller bool
param agentCount int = 3

// define the regular node profile
var nodepool1Profile = {
  name: 'nodepool1'
  count: agentCount
  enableAutoScaling: autoscaller
}

// add the 'maxCount' property to agentPoolProfiles  if autoscaller is true
let agentPoolProfiles =  autoscaller? {...nodepool1Profile, maxCount: agentCount+10} : nodepool1Profile 
yuanyi2121 commented 3 years ago

+1 really like this to be supported. My scenarios is - I have 3 different set of nsg rules, which contains 90% same rules. I'd like to be able to define all rules in one array and use bool parameters to control which rules to be included when instantiated. I think if makes more sense than ternary in this case: myarray: [ if (includeA) { itemA } { ItemB } ]

onionhammer commented 3 years ago

Would love if this existed. I'm trying to define the 'analytical TTL' on a Cosmos container and it only accepts an 'int' but in the .NET SDK it's a nullable int, so 'undefined' would be the only way I could conditionally set an analytical storage ttl.

Or it could be done w/ spread ( #1560 )

Or per the ugly way:

image

khowling commented 3 years ago

Looks like AppGateway uses a number of resource specific top-level properties (zones, identity etc). I cannot see any way to conditionally include these (for my use-case, the identity object). Looks like you cannot use a variable for the entire resource body resource appgw 'Microsoft....@2020' = varname , so cannot use the union method. Any suggestions?

image

alex-frankel commented 3 years ago

@khowling does using the ternary operator work?

var shouldUseIdentity = true

resource appGw 'Microsoft.Network/applicationGateways@2021-02-01' = {
  name: 'foo'
  identity: shouldUseIdentity ? {
    type: 'UserAssigned'
    userAssignedIdentities: {
      '${userAssignedMI}': {}
    }
  } : null
}
khowling commented 3 years ago

@alex-frankel Unfortunately not, that produces:

Deployment template parse failed: 'Required property 'type' not found in JSON. Path

if I replace {} with null, I get

Error BCP036: The property "identity" expected a value of type "ManagedServiceIdentity" but the provided value is of type "null | object".

daniellittledev commented 3 years ago

I ran into this issue trying to create a bicep file that creates an Azure function with a consumption plan in the test environment and a standard plan (for always on) in production.

Cannot update the site 'test-func' because it uses AlwaysOn feature which is not allowed in the target compute mode.

It fails the value for AlwaysOn is null or false, so the property needs to be omitted entirely.

ehrnst commented 3 years ago

I encountered this today when trying to conditionally add vnet rules to a storage account depending on a boolean input. ARM will not accept an empty virtualNetworkRules property.

onionhammer commented 3 years ago

Maybe undefined or omit should be a literal value thats assignable to properties.

reidcurry commented 3 years ago

@khowling does using the ternary operator work?

var shouldUseIdentity = true

resource appGw 'Microsoft.Network/applicationGateways@2021-02-01' = {
  name: 'foo'
  identity: shouldUseIdentity ? {
    type: 'UserAssigned'
    userAssignedIdentities: {
      '${userAssignedMI}': {}
    }
  } : null
}

Thanks Alex. I'm new to Bicep and your little snip helped me out. Only thing I changed was the null for {}

Here is what I ended up using when one probe used statusCodes, but the second probe did not need "Use probe matching conditions" checked:

probes: [for probe in appGateway.probes : {
      name: probe.name
      properties: {
        protocol: probe.protocol
        path: probe.path
        interval: probe.interval
        timeout: probe.timeout
        unhealthyThreshold: probe.unhealthyThreshold
        pickHostNameFromBackendHttpSettings: probe.pickHostNameFromBackendHttpSettings
        minServers: probe.minServers
        match: probe.shouldShowMatchStatusCodes ? {
          statusCodes: [
            probe.matchStatusCodes
          ]
        } : {}
      }
    }]
jrobins-tfa commented 3 years ago

Another use case where this is causing trouble: I'm making a template for an App Gateway. In the backendHttpSettingsCollection, there are two attributes, hostname and pickHostNameFromBackendAddress. If pickHostNameFromBackendAddress is true, you don't need hostname, otherwise you do. I tried to do this conditionally based on my parameters:

backendHttpSettingsCollection: [for backendHttpSetting in backendHttpSettingsCollection : {
   name: backendHttpSetting.name
   properties: {
     port: (contains(backendHttpSetting.properties, 'port')) ? backendHttpSetting.properties.port : null
     protocol: (contains(backendHttpSetting.properties, 'protocol')) ? backendHttpSetting.properties.protocol : null
     hostName: (contains(backendHttpSetting.properties, 'hostname')) ? backendHttpSetting.properties.hostname : null
     pickHostNameFromBackendAddress: (contains(backendHttpSetting.properties, 'pickHostNameFromBackendAddress')) ? backendHttpSetting.properties.pickHostNameFromBackendAddress : false

The problem is that even having "hostname: null" in your deployment causes an error if pickHostNameFromBackendAddress is true:

ApplicationGatewayBackendHttpSettingsHostNameFieldConflict - HostName field may only be specified if pickHostNameFromBackendAddress is set to false in context '/subscriptions/......'

I have two backendHttpSettingsCollections, one with pickHostNameFromBackendAddress false, the other with it true. So I need to be able to conditionally include or exclude the hostname attribute, not just conditionally assign a value or a null to it.

onionhammer commented 3 years ago

We need an 'undefined' value that can be put in.. it's familiar to people who know any JavaScript

erlandsen-tech commented 3 years ago

What we do now as a workaround (awaiting a proper alternative) is using modules and conditional deployment. We have several VM images that needs to deploy, but only some of the have plans. We now need one module for VmWithPlan, and one VmWithoutPlan. It works, but It means we need to repeat our code in two files which we don't want. Please keep me posted regarding a fix.

AlexanderSehr commented 2 years ago

We have a similar issue. We're trying to structure our templates with dedicated modules for child-resources. However, the user does not need to provide all values in the template's parameter file. The problem is that the child-resource template (e.g. container for storage account) comes with its own set of parameters, default values & validation - and not all parameters are mandatory. The problem is, the invoking template (e.g. storage account) will need an option to pass all parameters to this child-template the user specified, but not more. However, there is no way of achieving this and e.g. hand over null so that the child-template would fall back tot he default:

For example: image will result into: Error: The value of deployment parameter 'cors' is null. Please specify the value or use the parameter reference

The only workaround here is to not pass null but instead duplicate the default value we have already specified in the child-template parameter: image

With some sort of undefined type value we could solve this issue and have the child-template just fall back to the default.

nubgamerz commented 2 years ago

Also have another use case for this.

When deploying a subnet, in some cases a route table is not required (say for example, appgw, or bastion), but other subnets that contain IaaS resources or others may require a route table.

I tried applying some of the workarounds listed here, but was still getting errors, but found usingjson('null') instead of {} fixed these issues. Issues such as:

Error: The value of deployment parameter 'routeTable' is null. Please specify the value or use the parameter reference

image

image

stan-sz commented 2 years ago

The problem of conditionally passing params to a resource reminds me of #3351 where the coherent expectation of null/empty/missing property value would be required by the RPs.

paulhasell commented 2 years ago

Could we use a version of the conditional for resource itself to only include the setting in the compiled template if the condition is true?

resource storage 'Microsoft.Storage/storageAccounts@2021-06-01' = { name: storageName location: resourceGroup().location kind: 'StorageV2' sku:{ name:'Standard_LRS' } properties: { accessTier if(accessRequired):'Hot' } }

slavizh commented 2 years ago

Will not be possible. This needs to be implemented in Azure Resource Manager rather in Bicep as Bicep needs to know the value for the condition at compilation and if the value comes from parameter Bicep would not know the value thus not knowing to turn the defined resource in json with the property or without it.

onionhammer commented 2 years ago

@slavizh wouldn't it just compile to a union?

I prefer the 'undefined' approach persnally;

resource storage 'Microsoft.Storage/storageAccounts@2021-06-01' = {
name: storageName
location: resourceGroup().location
kind: 'StorageV2'
sku:{
name:'Standard_LRS'
}
properties: {
accessTier:  accessRequired ? 'Hot' : undefined
}
}
slavizh commented 2 years ago

@onionhammer accessRequired can be a parameter and parameter is passed during deployment which means that when bicep converts to ARM it will not know what is the value of accessRequired thus not knowing to remove accessTier property from properties or leave it .

paulhasell commented 2 years ago

These limitations leave ARM and Bicep unsuitable for a large range of deployment scenarios, especially 'upgrades' of existing environments with new features which need to conditionally add/update properties and collections. Powershell will have to remain our primary tool for scripting deployments although that seems to be the lowest priority in getting updated by MS.

onionhammer commented 2 years ago

@slavizh I'm not saying it is known at compile time, I'm saying it compiles to ARM's 'union' the same exact way this is done today with bicep, albeit with a TON more code..

union({ 
 ..other properties
}, accessRequired ? {
  accessTier: 'Hot'
} : {})
alex-frankel commented 2 years ago

This will most likely require both a Bicep and ARM template change. On the bicep side, we need some new keyword/gesture for saying "don't include this" and on the ARM Template/Deployments service side we (most likely) need an equivalent gesture. I don't think the template language has a way of expressing this today.

onionhammer commented 2 years ago

@alex-frankel slavizh's concern was that if 'accessRequired' is a paameter, this wouldnt work - I use parameters like this today

param isPremium bool = env == 'prod'

resource app 'Microsoft.Web/sites@2020-06-01' = { 

   resource settings 'config' = { 
      properties: union({
          // non-conditional settings...
      }, isPremium ? {
          // conditional on 'premium' parameter
          WEBSITE_CONTENTAZUREFILECONNECTIONSTRING: storageAccountString
          WEBSITE_CONTENTSHARE: '${env}-worker'
       } : { })
    }
}

What am I not understanding where this fails?

slavizh commented 2 years ago

Some examples that should work today. Code is a mock so when you use actual resources you will need to check which properties are required.

param isPremium bool
param env string = 'prod'

resource app 'Microsoft.Web/sites@2020-06-01' = {
   resource settings 'config' = {
      properties: isPremium ? {} : {
          WEBSITE_CONTENTAZUREFILECONNECTIONSTRING: storageAccountString
          WEBSITE_CONTENTSHARE: '${env}-worker'
      }
    }
}
param isPremium bool
param env string = 'prod'

resource app 'Microsoft.Web/sites@2020-06-01' = {
   resource settings 'config' = {
     properties: union( {
       someProperty: 'value'
     }, isPremium ? {
          WEBSITE_CONTENTAZUREFILECONNECTIONSTRING: storageAccountString
          WEBSITE_CONTENTSHARE: '${env}-worker'
      }: {})
    }
}
paulhasell commented 2 years ago

@slavizh That looks interesting, can you use 'if' to make the nested resource conditional? How does union() resolve duplicates?

slavizh commented 2 years ago

@paulhasell https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-functions-object#union

I do not use nested resources as it looks ugly to me so try it on your own if it is possible. I gave the above example only to look like the code posted from the other user.

paulhasell commented 2 years ago

@slavizh Nice, so it looks like union() could be used to conditionally upgrade/append settings with 'existing' being used to get current values, is there a way to make 'existing' cope gracefully with non-existence? eg If you're scripting for v3 of the product which could be applied against v1 and v2 where the resource was only added at v2

slavizh commented 2 years ago

@paulhasell Not sure what you mean. Overall bicep/ARM does not have a way to check if a resource exists or not. Existing only works when you actually know for sure that the resource exists. If you are talking about brownfield scenarios where the resource is already created trough some other way - PS, CLI, Portal, etc. and you want to bring it under management yes your bicep code could work with existing but that code will work only in brownfield scenarios and not on greenfield. But if the code works only on brownfield scenarios why not get rid of existing at all, make it work with both greenfield and brownfield, gather what are the values for the properties of the resource and implement your configuration/code to deploy with those. If your code works in a way that current properties are always taken from existing resource you are not really managing that resource via Bicep as you cannot change those properties via Bicep and when someone modifies them outside of Bicep you will just reapply those changed properties. Somehow this workflow does not seems to me like proper IaC/CI CD

paulhasell commented 2 years ago

@slavizh We are a partner creating solutions for use by customers whose own IT departments may tweak/add additional settings to the Azure elements that we script. This means we do not have the luxury of forcing all settings when upgrading, we have to take into account any changes made by the customers IT.

slavizh commented 2 years ago

@paulhasell it is dangerous road managing the same resources from two different places. It is like having two sources of truth. Overall it is your decision how you manage things I am just stating what is possible and what is my opinion.

onionhammer commented 2 years ago

Some examples that should work today. Code is a mock so when you use actual resources you will need to check which properties are required.

The code is not mock, I use it today. I just skipped several properties that aren't relevant for discussion. My point is that 'undefined' based on a condition could 'compile' to a union without changing how ARM works.

slavizh commented 2 years ago

@onionhammer my code is a mockup :) . It could but it gets a little bit more complex when you have more advanced code like for and multiple nested conditions. Only the Bicep engineers can say if they can make it work in every scenario. For me if it does not work for the majority of scenarios it will be confusing as you will have to figure out when you can use and when not. My 2 cents. No pun intended, just discussing.

onionhammer commented 2 years ago

@slavizh it's really more about NOT having the properties due to a quirk in that resources' API. There are some inconsistencies throughout the various resource types where having ANY value in a property (even null) will mess things up, this would only be for those scenarios where you cannot substitute a value for null and the property simply doesnt have any acceptable other value other than not being present.

slavizh commented 2 years ago

@onionhammer I was referring to more of the different scenarios by how you build your code in bicep language rather to a specific RP.

miqm commented 2 years ago

I was recently going through some documentation on ARM and there's quite important info about null: https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-expressions#null-values

To set a property to null, you can use null or [json('null')]. The json function returns an empty object when you provide null as the parameter. In both cases, Resource Manager templates treat it as if the property isn't present.

However bicep's null it's compiled to ARM function [null()] - which I assume does not omit property, but set's it's value to null.

I think we could leverage that behavior to make property to appear conditionally, but we'd need to distinguish null value from the omit property.

I would suggest using bang operator after null null! (seems to me that Microsoft loves bang operator, especially in C# 😉 ), so for following bicep code:

resource res 'type' = {
name: 'xxx'
properties: {
  prop1: condition ? 'value' : null!
}

the expression would compile to [if(condition, 'value', json('null'))] in contrast to when used null without bang (as it's currently: [if(condition, 'value', null())] so no longer we would need to use the workaround with json('null') as described https://github.com/Azure/bicep/issues/387#issuecomment-973871485. This would be built in bicep lang - effectively json('null') == null!.

I also noticed that compiler sometimes compiles bicep's null, to json null, and sometimes to null() - I think it depends if we use it explicitly on property or within expression. As it has different effect, we should be consistent in to which (json('null') or null()) we compile bicep's null.

WDYT @majastrz, @alex-frankel , @anthony-c-martin, @shenglol?

anthony-c-martin commented 2 years ago

I was recently going through some documentation on ARM and there's quite important info about null: https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-expressions#null-values

To set a property to null, you can use null or [json('null')]. The json function returns an empty object when you provide null as the parameter. In both cases, Resource Manager templates treat it as if the property isn't present.

However bicep's null it's compiled to ARM function [null()] - which I assume does not omit property, but set's it's value to null.

I'm actually wondering if we have an issue in our documentation - this seems to be demonstrably false.

Using the following output in a JSON template:

"foo": {
  "type": "Object",
  "value": {
    "foo": "[json('null')]",
    "bar": "[null()]",
    "baz": null
  }
}

I get the following output for foo:

{"foo":null,"bar":null,"baz":null}

@mumian any ideas?

anthony-c-martin commented 2 years ago

Personally I'd like to see #1560 implemented to solve this problem. It definitely takes a bit of getting used to but IMO is very intuitive when you are used to it.

E.g. conditionally setting a property:

var additionalProps = cond ? {
  setIfCondTrue: 'abc'
} : {}

var foo = {
  ...additionalProps
  alwaysSet: 'def'
}

Or on one line:

var foo = {
  ...(cond ? {
    setIfCondTrue: 'abc'
  } : {})
  alwaysSet: 'def'
}

This would be equivalent to writing:

var foo = cond ? {
  setIfCondTrue: 'abc'
  alwaysSet: 'def'
} : {
  alwaysSet: 'def'
}

IMO introducing the concept of undefined as well as null is not something we should take lightly - it's responsible for a LOT of confusion in the JavaScript world.

slavizh commented 2 years ago

I think the null thing is issue in documentation. From my experience null() and json('null') were always equal and always resulted in passing null. How a property interpretates null depends on the property itself and the RP.

The examples given can now be achieved with union. It is not pretty and it could be something that bicep can improve to make it pretty so you do not have to use union. Example:

var foo = union(cond ? {
  setIfCondTrue: 'abc'
} : {}, {
  alwaysSet: 'def'
})
majastrz commented 2 years ago

I'm actually wondering if we have an issue in our documentation - this seems to be demonstrably false.

That myth was going around the team a while ago, but there are also reports about resources behaving differently when some property is omitted vs set to null, which also disproves that.

I think the null thing is issue in documentation. From my experience null() and json('null') were always equal and always resulted in passing null. How a property interpretates null depends on the property itself and the RP.

Yes, exactly. The null() function evaluates to JSON null.

majastrz commented 2 years ago

IMO introducing the concept of undefined as well as null is not something we should take lightly - it's responsible for a LOT of confusion in the JavaScript world.

Agree. Plus, it would be very tricky to represent it internally as Newtonsoft doesn't have a token type that represents undefined.

majastrz commented 2 years ago

I think the null thing is issue in documentation. From my experience null() and json('null') were always equal and always resulted in passing null. How a property interpretates null depends on the property itself and the RP.

The examples given can now be achieved with union. It is not pretty and it could be something that bicep can improve to make it pretty so you do not have to use union. Example:

var foo = union(cond ? {
  setIfCondTrue: 'abc'
} : {}, {
  alwaysSet: 'def'
})

I agree. Some syntactic sugar like @anthony-c-martin's spread operator idea should work pretty well.

slavizh commented 2 years ago

With union() you only have to be careful when you are not union just simple objects but when they are complex ones. For example having array within object. If you try to merge two objects that has the same property that is array and the values for both arrays are different the one that is last in the union statement wins. If you do union only on those arrays you get the mixture of the values for both.

miqm commented 2 years ago

I did some tests and I think the RP people are most looking for this feature is Microsoft.Resources... I mean the modules. It seems like since we can define what default value for a param we want to set if the param is not provided - but when we provide it with null - it's being set to null, not to the default we set up inside module.

Ppkd2021 commented 2 years ago

@alex-frankel Adding one similar challenge that is being faced by us when we are trying to create application gateway. Scenario: We have 6 app services from which 2 are ui which we do not want to be configured inside the application gateway. Therefore we want to to skip creation of backend address pool, backend http settings, probes while deploying but when we try to put empty object or null or json("null") then it throws an error cannot parse request.

Sample snippet has been attached for probes.

image

Any solution to resolve above challenge will be really great.