Azure / bicep-types-az

Bicep type definitions for ARM resources
MIT License
84 stars 27 forks source link

Microsoft.LoadTestService loadTests: It's not possible to use 'existing' resource references for encryption properties #1987

Open sebassem opened 9 months ago

sebassem commented 9 months ago

Bicep version Bicep CLI version 0.23.1

Describe the bug My attempt to add encryption to a load test template failed as its encryption property does not allow me to use 'existing' resource references.

As soon as any of the properties are not a direct string reference, but for example an output of an 'existing' key vault resource reference, the template starts failing with an error such as:

image

To Reproduce Try to deploy using this template

Additional context Similar to Azure/bicep#7066

alex-frankel commented 9 months ago

Any time you see InternalServerError it is a bug in the RP error handling logic. You should file a support case with the LoadTestService RP team. In this particular case, it looks like a bad preflight implementation where they are trying to evaluate a template language expression that they should be ignoring. cc @stephaniezyen as FYI.

venkatr21 commented 8 months ago

@sebassem Looks like we got the value

[if(not(empty(parameters('customerManagedKey'))), createObject('identity', createObject('type', if(coalesce(tryGet(parameters('customerManagedKey'), 'systemAssigned'), false()), 'SystemAssigned', if(not(empty(tryGet(parameters('customerManagedKey'), 'userAssignedIdentityResourceId'))), 'UserAssigned', null())), 'resourceId', if(not(empty(tryGet(parameters('customerManagedKey'), 'userAssignedIdentityResourceId'))), extensionResourceId(format('/subscriptions/{0}/resourceGroups/{1}', split(coalesce(tryGet(parameters('customerManagedKey'), 'userAssignedIdentityResourceId'), '//'), '/')[2], split(coalesce(tryGet(parameters('customerManagedKey'), 'userAssignedIdentityResourceId'), '////'), '/')[4]), 'Microsoft.ManagedIdentity/userAssignedIdentities', last(split(coalesce(tryGet(parameters('customerManagedKey'), 'userAssignedIdentityResourceId'), 'dummyMsi'), '/'))), null())), 'keyUrl', if(not(empty(coalesce(tryGet(parameters('customerManagedKey'), 'keyVersion'), ''))), format('{0}/{1}', reference('cMKKeyVault::cMKKey').keyUri, parameters('customerManagedKey').keyVersion), reference('cMKKeyVault::cMKKey').keyUriWithVersion)), null())]

but the model expects the format mentioned here https://github.com/Azure/azure-rest-api-specs/blob/d1eb3c20113d1018f25a8d97fdfa5f8bb5c659ea/specification/loadtestservice/resource-manager/Microsoft.LoadTestService/stable/2022-12-01/loadtestservice.json#L693

Can you please help verify if the actual payload sent over the http response is a valid encryption payload as mentioned in the spec?

sebassem commented 8 months ago

@venkatr21 Yes it is, if I replace the values with manually entered values it works. It only shows the error when I reference existing resources.

venkatr21 commented 8 months ago

@sebassem can you mention what do you mean my reference existing resources? Do you mean some kind of parameterization in the actual payload? Can you help us with the actual deployment payload?

sebassem commented 8 months ago

@venkatr21 sure, here is the method we are using for referencing existing resources "key vault"

venkatr21 commented 8 months ago

@sebassem @alex-frankel It looks like when we reference existing resources "key vault", the payload which we get in the http request for resource creation is a bicep template (which makes our RP to error out). I am not sure if this should be handled by our RP.

As per the REST API guidelines, our RP defines a request body format which should be adhered by the clients. I believe the templatization should be either handled by the bicep engine or the ARM deployment engine before sending out the request to RP. I think it should be fixed by either the ARM deployment engine or the bicep engine before fanning out the request to RP.

sebassem commented 8 months ago

@venkatr21 thanks for your comment, I will leave the reply for @alex-frankel regarding the ARM/Bicep engine handling but I've seen multiple cases like this, and they were always handled by the PR team, sample one here

venkatr21 commented 8 months ago

@sebassem For the issue you have shared, the nested link https://github.com/Azure/ResourceModules/issues/1590 this says the issue has not been resolved in the latest API version of cognitive services.

I am not sure if this is a provider issue, provider expects payload https://learn.microsoft.com/en-us/rest/api/loadtesting/resourcemanager/load-test-resource/create-or-update?view=rest-loadtesting-resourcemanager-2022-12-01&tabs=HTTP but looks like the payload in the request is some kind of a template, and it is not a responsibility of resource provider to handle templatization, it should be handled by the bicep engine, before sending the request to RP. needs triage at the bicep engine / arm templatizing level. cc:// @alex-frankel

AlexanderSehr commented 8 months ago

Hey @venkatr21, I hope you're doing well. I noticed you're referring to the issue I opened back in the day. The issue has actually been resolved as the issue states and after the PGs fix, the logic went in and works like a charm since then.

The issue @sebassem describes sounds suspiciously similar and I recall the same conversations. At the end it came down to an issue on the Provider's end where code is interprested which should be supressed. As @alex-frankel mentioned it's about the 'preflight implementation'.

I guess there is a chance this is not the same issue afterall - but all the clues point to it, so it would be great if you could make sure the validation is implemented in the way it's supposed to to rule that possibility out. Please refer to link.

Some more context: In ARM templates/bicep, prior to the deployment, there's a preflight validation step where the relevant RP(s) are notified that a resource is about to be created (including it's configuration) so they have an opportunity to validate. Sometimes a "template language expression" is send along with it in the validation request which RPs are supposed to ignore. If they are not ignored, you will get failures like this.

venkatr21 commented 8 months ago

@AlexanderSehr thanks for the inputs, and the links. I stand corrected by my above statement, this then looks like a bug in the provider. Will work on this and provide an update.

sebassem commented 7 months ago

Hello @venkatr21 , just checking with you if this bug has been already resolved or not yet?

matebarabas commented 7 months ago

@venkatr21, do you have any updates on this issue?

Please note that this is currently blocking the release of this AVM module which is also needed by the AZD team.

Thank you!

CC: @sebassem, @sydkar