Azure / bicep

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

Validation for Options property not consistent across microsoft.documentdb #7710

Closed markjbrown closed 2 years ago

markjbrown commented 2 years ago

Describe the bug The Cosmos DB resource provider (microsoft.documentdb) has an options property that sits within the resource definition for child resources such as SqlDatabase and Containers for Cosmos DB (see full list below).

When submitting updates to our ARM/Bicep samples to the Quickstarts repo some of these samples fail validation with a warning because the Options property. You can also see this in VS Code when authoring a Bicep (it does not appear when authoring ARM templates). This is the error I see below.

Warning BCP037: The property "options" is not allowed on objects of type "CassandraTableResourceOrCassandraTableGetPropertiesResource".

You can also see more details on the warning generated in the Pipeline here https://dev.azure.com/azurequickstarts/azure-quickstart-templates/_build/results?buildId=107141&view=logs&j=275f1d19-1bd8-5591-b06b-07d489ea915a&t=14b22f05-782e-55fa-3e59-b66c82f514a9&l=12

It appears this warning only happens when authoring Bicep files that have standard throughput and not when using autoscale throughput. Below is the difference between the two.

Standard Throughput: options: { throughput: throughput }

Autoscale Throughput: options: { autoscaleSettings: { maxThroughput: autoscaleMaxThroughput } }

These are the list of child resources that have an options property

Microsoft.DocumentDB/databaseAccounts/cassandraKeyspaces Microsoft.DocumentDb/databaseAccounts/cassandraKeyspaces/tables Microsoft.DocumentDB/databaseAccounts/gremlinDatabases Microsoft.DocumentDB/databaseAccounts/gremlinDatabases/graphs Microsoft.DocumentDB/databaseAccounts/mongodbDatabases Microsoft.DocumentDB/databaseAccounts/mongodbDatabases/collections Microsoft.DocumentDB/databaseAccounts/sqlDatabases Microsoft.DocumentDB/databaseAccounts/sqlDatabases/containers Microsoft.DocumentDB/databaseAccounts/tables

Thanks!

To Reproduce Steps to reproduce the behavior: Open up VS Code, copy/paste an existing Cosmos DB Bicep sample using standard throughput from Quickstart Gallery, hover over Options property.

Additional context Add any other context about the problem here.

markjbrown commented 2 years ago

Not sure why but this run resulted in an error rather than a warning. Really strange why these are not consistent.

https://dev.azure.com/azurequickstarts/azure-quickstart-templates/_build/results?buildId=107203&view=results

anthony-c-martin commented 2 years ago

Not sure why but this run resulted in an error rather than a warning. Really strange why these are not consistent.

https://dev.azure.com/azurequickstarts/azure-quickstart-templates/_build/results?buildId=107203&view=results

Bicep did raise a warning, but it looks like the PowerShell script for quickstarts validation converted it into an error:

##[error]D:\a\1\s\quickstarts\microsoft.documentdb\cosmosdb-mongodb\main.bicep(162,7) : Warning BCP037: The property "options" is not allowed on objects of type "MongoDBCollectionResourceOrMongoDBCollectionGetPropertiesResource". Permissible properties include "analyticalStorageTtl". If this is an inaccuracy in the documentation, please report it to the Bicep Team. [https://aka.ms/bicep-type-issues]

We'll probably need @bmoore-msft's help to troubleshoot further.

alex-frankel commented 2 years ago

I'm including a link to swagger which shows the options property present: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/cosmos-db/resource-manager/Microsoft.DocumentDB/stable/2022-05-15/cosmos-db.json#L6748

jeskew commented 2 years ago

In the quickstarts, the options object is nested under the resource property, but the swagger describes those two properties as siblings instead. I can compile the following template with no warnings:

resource acct 'Microsoft.DocumentDB/databaseAccounts@2022-05-15' existing = {
  name: 'myacct'

  resource mdb 'mongodbDatabases' = {
    name: 'webscale'
    properties: {
      resource: {
        id: 'foo'
      }
      options: {
        throughput: 100

      }
    }

    resource coll 'collections' = {
      name: 'mycollection'
      properties: {
        resource: {
          id: 'foo'
        }
        options: {
          throughput: 100
        }
      }
    }
  }
}

but loading the quickstart template into VS Code will display the following diagnostic because options is a child of properties.resource instead of a direct child of properties:

image

From what I can tell from the API and ARM docs, this is correct (though both of those sources are derived from the Swagger document, so if the Swagger is wrong, they would be, too).

It appears this warning only happens when authoring Bicep files that have standard throughput and not when using autoscale throughput.

This looks to be due to a difference between the Gremlin CosmosDB quickstarts. The autoscaled version of the quickstart has options as a direct child of properties property on the 'Microsoft.DocumentDb/databaseAccounts/gremlinDatabases/graphs' resource, whereas the standard throughput version has options as a child of properties.resource.

bmoore-msft commented 2 years ago

Not sure why but this run resulted in an error rather than a warning. Really strange why these are not consistent. https://dev.azure.com/azurequickstarts/azure-quickstart-templates/_build/results?buildId=107203&view=results

Bicep did raise a warning, but it looks like the PowerShell script for quickstarts validation converted it into an error:

##[error]D:\a\1\s\quickstarts\microsoft.documentdb\cosmosdb-mongodb\main.bicep(162,7) : Warning BCP037: The property "options" is not allowed on objects of type "MongoDBCollectionResourceOrMongoDBCollectionGetPropertiesResource". Permissible properties include "analyticalStorageTtl". If this is an inaccuracy in the documentation, please report it to the Bicep Team. [https://aka.ms/bicep-type-issues]

We'll probably need @bmoore-msft's help to troubleshoot further.

I'm pretty sure it's because that task is configured to "Fail on Std Error" in AzDO. The deployment task, which is not configured that way (and also does bicep build), throws a warning before the deployment fails.

markjbrown commented 2 years ago

Ok, I think I may understand the problem. It looks like your validation routine does a GET on the MongoDBCollectionResource resource where it is looking for these values passed in the options property. The problem is the values passed in are sent via the MongoDBCreateUpdateParameters in the PUT, that includes the CreateUpdateOptions definition. However, the values passed are not defined as part of the MongoDBCollectionResource in the subsequent GET operation so the validation fails. This also explains why it does not appear in VS Code either.

I'm not quite sure how to deal with this from your validation script. Is it possible to white-list this property when doing a GET on the resource? What's strange here is we've always used options in the CreateUpdate resource definitions to pass throughput values. They've never triggered warnings or exceptions before.

jeskew commented 2 years ago

The Bicep types are able to account for properties that exist only on the GET or the PUT side of a resource (the property will be flagged as ReadOnly or WriteOnly, respectively), but the CosmosDB swagger shows options as a direct child of properties on the GET request, too. I did encounter warnings using the quickstart templates with an earlier version of the Bicep CLI, though if the build pipeline's ADO configuration was recently changed, it's possible the warning didn't previously result in a failed task:

image

The Bicep type generation process doesn't invoke any live APIs -- it just looks at the Swagger definition for a given resource provider -- so it's entirely possible that the Swagger is wrong and that the types consequently do not reflect the actual API behavior. We should verify with the CosmosDB team what the correct input would be, then update either the quickstarts or the RP swagger documents.

markjbrown commented 2 years ago

Ah yes, sorry, they are in the Get as well with the throughput values.

So, I'm now confused. If Options is in the Get, why is this validation failing?

btw, I'm also confirming that this Options property that sits within Resource is not used. My current PR's do not include it.

jeskew commented 2 years ago

Sorry, confusing wording on my part. On the GET side, properties.options exists, but properties.resource.options does not (same as on the PUT side, at least according to the Swagger).

alex-frankel commented 2 years ago

@markjbrown - if you are 100% sure that properties.resource.options is the right property path, then it means the swagger is incorrect and needs to be updated by your team.

markjbrown commented 2 years ago

It should be properties.resource which is what I'm using in these PR's now and why I'm confused validation is failing.

alex-frankel commented 2 years ago

Validation is failing because the swagger spec (which we use to generate type information for bicep) is wrong, so bicep thinks you are describing a property that doesn't exist.

The next step is to get the swagger fixed in the azure-rest-api-specs repo, at which point we can generate an updated type for bicep to resolve the error. That will be something your team needs to take care of (swagger specs for each resource type are owned by the RP team). Does that help clarify?

For getting the quickstart validation to pass in the short term, @bmoore-msft may have a workaround.

markjbrown commented 2 years ago

I am looking at my bicep in VSCode and I'm still confused why this is failing. According to the bubble help in VSCode, Bicep is looking at the CreateUpdateOptions from our swagger spec. This is the swagger for

Shouldn't this work then without throwing an exception given the throughput properties are defined within that? I'm just not seeing where our swagger is incorrect. It is consistent for all of the child resources which can have throughput provisioned.

image

jeskew commented 2 years ago

@markjbrown Can you post a full snippet for that screenshot? From the closing braces, it looks like that resource roughly matches the following:

resource mdb 'Microsoft.DocumentDB/databaseAccounts/mongoDbDatabases/collections@2022-05-15' = {
  ...
  properties: {
    resources: {...}
    options: {
      throughput: dedicatedThroughput
    }
  }
}

I.e., with properties.options defined rather than properties.resources.options.

markjbrown commented 2 years ago

Here's a screen cap of VS Code. Hope this helps.

image

jeskew commented 2 years ago

Is the issue that VS code is erroneously saying this is correct (since the template is specifying properties.options instead of properties.resource.options), or are you seeing validation errors when you build the template shown above via the CLI?

markjbrown commented 2 years ago

Yes, the issue is VS Code says this is correct, yet it fails the validation occurring during the PR.

PS, this is correct, properties.options. This is not, properties.resource.options

Also, Mongo resources is the only one it fails on. I just submitted 8 PR's for the other 4 API's and none of them throw this exception.

markjbrown commented 2 years ago

Am now able to get the mongo templates to pass validation.

There were multiple elements at work here. Errors on my part which until recently had passed validation were no longer working. There must have been changes or improvements to the validation routines that had left issues gone unnoticed until now.

Closing. Apologies for the ongoing churn over this. Definitely appreciate your help.