Azure / bicep

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

BCP073 thrown for properties in resourceDerivedTypes #15183

Open schaijkw opened 2 months ago

schaijkw commented 2 months ago

Bicep version last worked in 0.29.47 broken since 0.30.3

Describe the bug We use the experimental resourceDerivedTypes feature and the new bicep versions are breaking our setup. We have this parameter defined in a module:

param siteProperties resource<'Microsoft.Web/sites@2022-09-01'>.properties = {
  clientAffinityEnabled: false
  clientCertEnabled: true
  clientCertMode: 'OptionalInteractiveUser'
  httpsOnly: true
}

In the module there are no warnings or exceptions. But in the file that is using this module we see exceptions, like this: The property "inProgressOperationId" is read-only. Expressions cannot be assigned to read-only properties. bicep (BCP073)

image

To Reproduce Create 2 modules;

Enable Experimental Feature in bicepconfig.json

    "experimentalFeaturesEnabled": {
        "resourceDerivedTypes": true
    }

Module 1, parameter with a set of default options:

param siteProperties resource<'Microsoft.Web/sites@2022-09-01'>.properties = {
  clientAffinityEnabled: false
  clientCertEnabled: true
  clientCertMode: 'OptionalInteractiveUser'
  httpsOnly: true
}

resource appService 'Microsoft.Web/sites@2022-09-01' = {
  name: appServiceName
  location: location
  tags: tags
  kind: kind
  identity: {
    type: 'SystemAssigned'
  }
  properties: union({
    serverFarmId: servicePlanId}, 
    siteProperties
  )

Module 2

@description('Site resource specific properties.')
param siteProperties resource<'Microsoft.Web/sites@2022-09-01'>.properties?

module appService 'module1.bicep' = {
  name: 'appService'
  params: {
    siteProperties: siteProperties
  }
}

First use bicep version 0.29.47, then update to 0.30.3 and you'll see the exceptions.

jeskew commented 1 month ago

Bicep 0.30 included an update that performs deep type checking on values passed in as parameters, which has the unexpected side effect documented here. Resource-derived types (unlike standard user-defined types) can have read-only properties, which makes an assignment of type resource<'Microsoft.Web/sites@2022-09-01'>.properties to a target of type resource<'Microsoft.Web/sites@2022-09-01'>.properties raise some warning diagnostics.

One thing we could do here is skip the check that raises BCP073 on read-only properties of the assigned value. This make sense for the case of a resource-derived-typed parameter, since a parent module wouldn't be able to populate those read-only properties without raising its own BCP073 diagnostic.

But it would miss cases like this:

main.bicep

resource appService 'Microsoft.Web/sites@2022-09-01' = {...}

module mod 'mod.bicep' = {
  name: 'mod'
  params: {
    siteProperties: appService.properties
  }
}

mod.bicep

param siteProperties resource<'Microsoft.Web/sites@2022-09-01'>.properties?

resource appService 'Microsoft.Web/sites@2022-09-01' = {
  ...
  properties: siteProperties 
}

since appService.properties is likely to have read-only properties populated since it's a return value from the RP. I expect RPs already have to handle this gracefully, though, since GET-modify-PUT is a pretty common pattern in imperative deployments.

schaijkw commented 1 month ago

If the RPs don't handle this gracefully, is it possible to detect if the property is read-only? Because in that case you can ignore or remove those properties. It will behave as expected because if you try to set a read-only property manually the BCP073 will be raised, and if you were referencing the resource as you do in your example, you don't intent to change those read-only properties.

jeskew commented 1 month ago

Writeup for discussion: https://gist.github.com/jeskew/150611ca5280493e0f10d22823f86e67