Generate type definitions for readonly resources #780
Conversation
The initial set of flags only includes two markers, one of which indicates that a resource can be read via an RP API, the other of which indicates that the resource can be written. This change was made in order to support read-only resources (which are currently excluded from Bicep type files) as "existing" resources in bicep files. Additional flags may be added in the future to support special treatment by the bicep compiler (e.g., for singleton or immutable resources).
Codecov Report
@@ Coverage Diff @@
## main #780 +/- ##
==========================================
+ Coverage 95.57% 95.81% +0.24%
==========================================
Files 18 18
Lines 294 311 +17
==========================================
+ Hits 281 298 +17
Misses 13 13
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| * **Valid Scope(s)**: Subscription, ResourceGroup | ||
| ### Properties | ||
| * **apiVersion**: '2020-01-01' (ReadOnly, DeployTimeConstant): The resource api version | ||
| * **id**: string (ReadOnly, DeployTimeConstant): The resource id | ||
| * **name**: 'default' (Required, DeployTimeConstant): The resource name | ||
| * **properties**: [ConfigDataProperties](#configdataproperties): Configuration data properties | ||
| * **properties**: [ConfigDataProperties](#configdataproperties) (WriteOnly): Configuration data properties |
There was a problem hiding this comment.
This resource is being flagged correctly as WriteOnly (the singular configurations endpoint only defines a PUT operation and no GET operation), but I don't think that WriteOnly status should be propagated down to individual properties. This particular property is part of the response the RP returns on resource creation, so it should be something that can be dereferenced off the resource elsewhere in a template. The test that currently determines whether a property is WriteOnly is a check to see whether the property defined on the resource's PUT request is also defined on the resource's GET request, but there are properties that can be specified on a PUT that the RP will not echo back in the response, e.g., the certificates provided as part of the creation of an Microsoft.Attestation/attestationProviders resource. I believe the latter is what is actually meant by WriteOnly, but updating the generator to reflect that meaning is potentially a breaking change (and in any case should happen in a separate PR).
/cc @shenglol
There was a problem hiding this comment.
My understanding is that if a property is write-only if it can set in PUT requests but is not returned in GET responses. When a resource's property is referenced in a Bicep file, once compiled, it will be transformed into an ARM template expression containing a reference function call. To resolve the reference function, the ARM deployment engine will not read the resource PUT response, but it will send a GET request to get the resource body.
There was a problem hiding this comment.
I think there's a bit more complexity to untangle here, but I don't think this PR is the place to do it. JSON ARM templates will permit reading from a Microsoft.Advisor/configurations resource that was created in the same template iff the resource is referenced by name rather than resource ID:
When the same template is altered to use a resource ID rather than a resource name, the deployment fails:
To account for this in the Bicep compiler, properties would need to carry more metadata, and the compiler would need to distinguish between existing vs owned resources when evaluating property access. The set of circumstances in which that level of distinction is necessary seems fairly uncommon, since there are only a handful of write-only resources, and I believe most RP APIs return the same structure for both GET and PUT operations.
There was a problem hiding this comment.
Good find! Totally agree that this is a complex problem, and it should be out of the scope of this PR. I think I was not entirely correct about how reference functions are evaluated. After checking the deployment engine code again, it seems that the deployment engine will always send a GET request if the API version parameter is provided for a reference function. Otherwise, it reads the job metadata which contains a resource definition once a resource deployment job is complete. However, whether the resource definition is from a PUT or GET response depends on if the resource type supports asynchronous PUT operations...which we cannot tell from the resource type's Swagger definition.
@jorgecotillo as FYI (and please correct me if I'm wrong).
There was a problem hiding this comment.
However, whether the resource definition is from a PUT or GET response depends on if the resource type supports asynchronous PUT operations...which we cannot tell from the resource type's Swagger definition.
Can this be inferred from the presence of a x-ms-long-running-operation extension? Or is the GET flow only used for resources that use the Azure-AsyncOperation header (which wouldn't be encoded in the Swagger definition)?
There was a problem hiding this comment.
Ahh yes the x-ms-long-running-operation extension can be used (thanks for reminding me!). It needs to be set as long as a resource operation is asynchronous regardless of the use of the Azure-AsyncOperation header. The only minor issue might be that there exists RPs who forgot to set the extension for their resource types (e.g., Blueprint...which is our team 😅), and they won't be fixed since that will cause breaking changes in the generated SDKs, but I think we just can treat them as provider errors.
|
Closing in favor of #805 |
As a first step towards resolving Azure/bicep#1398, this PR adds a
Flagsproperty to the serialized bicep resource type to inform the compiler whether resources are read-only, write-only, or support both reading and writing. This will allow the compiler to handleexistingresources for read-only types while disallowing their use as regular resource types.Because the aforementioned change surfaced a few write-only resources, additional flags were added to object properties to allow the compiler to know what could be read off of a PUT response vs off of a GET response. Per this comment, PUT responses are not used for resources that are provisioned asynchronously, so an
Asyncflag was added to resources whose PUT operation has thex-ms-long-running-operationswagger extension set to a truthy value.For backwards compatibility, flags only indicate deviations from the norm, so
ResourceFlags.Nonerepresents a resource that is both readable and writable. TheBicep.Typeslibrary, even with this change, will treat existing serialized resources as having a flag ofResourceFlags.None, so there's no need to coordinate a release of that library with one of the k8s types.