Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deployment should fail for existing resources that do not exist, even when the reference() call is not required for transpile #10097

Open
mtessier84 opened this issue Mar 13, 2023 · 22 comments
Assignees
Labels
Community Call intermediate language Related to the intermediate language Needs: Upvote This issue requires more votes to be considered

Comments

@mtessier84
Copy link

Bicep version
v0.14.85

Describe the bug
The documentation says:
If you attempt to reference a resource that doesn't exist, you get the NotFound error and your deployment fails. Check the name and scope of the resource you're trying to reference. Link

This is not true. The deployment will not fail if only referring to the name or id of the referenced existing resource. In this case, Bicep does not "resolve" the resource from Azure Resource Manager because Bicep can construct the resource id and get the name from the resource existing block without having to fetch the properties from Azure.

To Reproduce
Steps to reproduce the behavior:

Additional context
The resource existing block should allow enforcing validation or documentation should be updated to explain better the behavior.

@ghost ghost added the Needs: Triage 🔍 label Mar 13, 2023
@mtessier84
Copy link
Author

I will close it because I found the documentation under "troubleshooting", but good to keep for visibility.

@arsenmk
Copy link

arsenmk commented Mar 17, 2023

@mtessier84 , what documentation did you find ? Can you post the link? I also need to fail the deployment if the resource doesn't exist, but didn't find how yet.

@mtessier84
Copy link
Author

Sorry I was not clear. I could not find any documentation that says that the intended behavior is that it should fail the deployment. The documentation that I found and shared in the original post is under "Troubleshooting".

I dont have a solution for this. I could be reported as a feature request instead of a bug

@alex-frankel
Copy link
Collaborator

I think the deployment will fail when at least when that resource is referenced somewhere (beyond the initial declaration). Otherwise, maybe we do not make the underlying reference() call?

@mtessier84
Copy link
Author

mtessier84 commented Apr 3, 2023

The following code does not fail if the storage account does not exist.

param Location string
param StorageAccountName string

resource StorageAccount 'Microsoft.Storage/storageAccounts@2022-09-01' existing = {
  name: StorageAccountName
}

resource ManagedIdentity 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' = {
  name: 'MID${StorageAccount.name}'
  location: Location
}

There are cases when I wish the deployment would fail in such scenarios.

@alex-frankel
Copy link
Collaborator

I see now -- thank you for sharing this example. This is in fact a case where we won't even attempt to reference the storage account because the storage account name can be calculated without it. Here is what the compiled Template looks like for that sample:

{
  "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
  "contentVersion": "1.0.0.0",
  "metadata": {
    "_generator": {
      "name": "bicep",
      "version": "0.15.164.21910",
      "templateHash": "8213433503989717287"
    }
  },
  "parameters": {
    "Location": {
      "type": "string"
    },
    "StorageAccountName": {
      "type": "string"
    }
  },
  "resources": [
    {
      "type": "Microsoft.ManagedIdentity/userAssignedIdentities",
      "apiVersion": "2023-01-31",
      "name": "[format('MID{0}', parameters('StorageAccountName'))]",
      "location": "[parameters('Location')]"
    }
  ]
}

I will re-open the issue.

@alex-frankel alex-frankel reopened this Apr 3, 2023
@alex-frankel alex-frankel changed the title Referencing a resource that doesn't exist does not fail the deployment Deployment should fail for existing resources that do not exist, even when the reference() call is not required for transpile Apr 3, 2023
@stephaniezyen stephaniezyen added the Needs: Upvote This issue requires more votes to be considered label Apr 5, 2023
@brwilkinson
Copy link
Collaborator

@mtessier84 if you have an existing resource and you want to ensure it gets added to the template, you can use an output for this reference. That will ensure it is included in the compiled template and the resourceid reference expression will be evaluated in the deployment.

param plan string = 'StorageAccounts'
targetScope = 'subscription'
resource PRICING 'Microsoft.Security/pricings@2022-03-01' existing =  {
    name: plan
}
output storage string = PRICING.id // <-- output referencing existing resource
  "outputs": {
    "storage": {
      "type": "string",
      "value": "[subscriptionResourceId('Microsoft.Security/pricings', parameters('plan'))]"
    }
  }

There is a current linter rule, that should pickup unused resources (when you don't reference it).

        "no-unused-existing-resources": {
          "level": "warning"
        }

image

@mtessier84
Copy link
Author

Thank you Ben, I think that would be a good workaround for my case!

@brwilkinson
Copy link
Collaborator

Thank you @mtessier84

Given we have the linter rule (to catch this) and an appropriate way to handle the requirement I will "close as - by design".

@alex-frankel
Copy link
Collaborator

I'm not sure I follow the resolution. My understanding there's a desire to fail the deployment if the existing resource does not exist. @brwilkinson -- I think even for the case of emitting the id of the resource, there won't ever be a reference() call so the deployment will still succeed even if the resource exists. Is that right?

If so, then a workaround could be to reference a non-compile-time property like so:

targetScope = 'subscription'

param plan string = 'StorageAccounts'

resource PRICING 'Microsoft.Security/pricings@2022-03-01' existing =  {
    name: plan
}
output storage object = PRICING.properties // <-- output referencing existing resource

If that workaround is good enough, we can keep the issue closed, but I wonder if we should somehow emit a dummy reference() for all existing resources so that this would not be required. That would be a breaking change, so we would need to really think through it if it's something we wanted to do.

@brwilkinson
Copy link
Collaborator

brwilkinson commented Apr 6, 2023

@alex-frankel got it yes, without the reference there is no validation, I missed this one.

would still need an explicit reference for this to work in bicep.

resource storage 'Microsoft.Storage/storageAccounts@2022-09-01' existing = {
  name: 'doesnotexist'
}
output storage object = reference(storage.id)

Error: Code=InvalidTemplate; Message=Deployment template validation failed: 'The resource 'Microsoft.Storage/storageAccounts/doesnotexist' referenced in output is not defined in the template.

<edit, let me test this out some more>

@brwilkinson
Copy link
Collaborator

re-opening, since above does not cover the initial ask. Thanks @alex-frankel

@brwilkinson brwilkinson reopened this Apr 6, 2023
@brwilkinson
Copy link
Collaborator

seems like this maybe related to below, in functionality.

@brwilkinson
Copy link
Collaborator

Looping back on the workaround scenario.

  • Need to add api version to the reference since resource is not defined in current template
resource storage 'Microsoft.Storage/storageAccounts@2022-09-01' existing = {
  name: 'doesnotexist'
}
output storage object = reference(storage.id, storage.apiVersion)

correctly fails if resource does not exist

The Resource 'Microsoft.Storage/storageAccounts/doesnotexist' under resource group 'AEU1-PE-CTL-RG-D1' was not found

resource storage 'Microsoft.Storage/storageAccounts@2022-09-01' existing = {
  name: 'storageaccountexists'
}
output storage object = reference(storage.id, storage.apiVersion)

correctly deploys, if resources exists

image

@brwilkinson
Copy link
Collaborator

brwilkinson commented Apr 8, 2023

Okay I found the inconsistency on the behavior ... worth adding ... relating to extensibility or symbolicNameCodegen

param vNETName string = 'doesnotexist'

resource VNET 'Microsoft.Network/virtualNetworks@2022-09-01' existing = {
  name: vNETName
}

output vnetid string = VNET.id
{
  "experimentalFeaturesEnabled": {
      // "extensibility": false
       "symbolicNameCodegen": false
  }

with extensibility|symbolicNameCodegen turned off, this does no evaluate the reference deployment has no error 🟩

Outputs :
Name Type Value
=============== ========================= ==========
vnetid String "/subscriptions/4185fa9b-f470-466a-b3ae-8e6c3314a543/resourceGroups/AEU1-PE-CTL-RG-D1/providers/Microsoft.Network/virtualNetworks/doesnotexist"

image


param vNETName string = 'doesnotexist'

resource VNET 'Microsoft.Network/virtualNetworks@2022-09-01' existing = {
  name: vNETName
}
// even without any output

with extensibility|symbolicNameCodegen turned on, this always evaluates the reference so deployment has an error 🟥

{
  "experimentalFeaturesEnabled": {
    //"extensibility": true
    "symbolicNameCodegen": true
  }

The deployment 'temp' failed with error(s). Showing 1 out of 1 error(s). Status Message: The Resource 'Microsoft.Network/virtualNetworks/doesnotexist' under resource group 'AEU1-PE-CTL-RG-D1' was not found.

image

{
  "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
  "languageVersion": "1.10-experimental",
  "contentVersion": "1.0.0.0",
  "metadata": {
    "_EXPERIMENTAL_WARNING": "Symbolic name support in ARM is experimental, and should be enabled for testing purposes only. Do not enable this setting for any production usage, or you may be unexpectedly broken at any time!",
    "_generator": {
      "name": "bicep",
      "version": "0.16.1.55165",
      "templateHash": "9658580584476014833"
    }
  },
  "parameters": {
    "vNETName": {
      "type": "string",
      "defaultValue": "doesnotexist"
    }
  },
  "resources": {
    "VNET": {
      "existing": true,
      "type": "Microsoft.Network/virtualNetworks",
      "apiVersion": "2022-09-01",
      "name": "[parameters('vNETName')]"
    }
  }
}

@brwilkinson
Copy link
Collaborator

@jeskew I was meaning to ask if above was expected behavior?

@jeskew
Copy link
Contributor

jeskew commented Apr 13, 2023

@brwilkinson The symbolic name template behavior is expected, though I'll have to tag in @anthony-c-martin as to whether this is an intentional behavioral difference between symbolic name and "classic" templates.

FWIW, though, that the symbolic name template behavior is I would expect from the Bicep syntax. The "classic" template behavior makes sense, but only if you know how existing resources get compiled to JSON template expressions (and are also familiar with how those expressions get processed at deploy time). From that perspective, I would say the issue is fixed in symbolic name templates.

@brwilkinson
Copy link
Collaborator

Thank you @jeskew

I am okay either way, although I will mention 4 things:

  1. I opened below where using an existing reference may be conditional
  2. It appears that people like conditional use case for existing resources
  3. It would be a breaking change.
  4. Obviously this issue was opened in favor of the behavior

@stephaniezyen stephaniezyen added the discussion This is a discussion issue and not a change proposal. label Apr 19, 2023
@miqm
Copy link
Collaborator

miqm commented Apr 19, 2023

I think changing this behaviour if we introduce symbolic templates as a "default" can be a big breaking change. sometimes we use existing resources to construct an id (no reference call, just resoruceId). We might also not have permission to read the resource but we know it exists (i.e. creating private endpoints).

@brwilkinson
Copy link
Collaborator

@jeskew you mentioned below, I was wondering that also relates to this or was it a separate issue?

#9457 (comment)

The fix for this is in the w16 ARM release.

@jeskew
Copy link
Contributor

jeskew commented Apr 23, 2023

@brwilkinson The fix in w16 only addresses what ARM does when there is more than one declared resource targeting the same resource ID but only one is active due to conditions. That manifested as a behavioral difference for templates that used a pattern like the following (which would not cause any issues in a non-symbolic name template):

param redeploy bool

resource alreadyThere 'provider/kind@version' existing = if (!redeploy) {
  ...
}

resource new 'provider/kind@version' = if (redeploy) {
  ...
}

This issue is specifically for unreachable/nonexistent existing resources that aren't deactivated via an if clause.

@jeskew jeskew added intermediate language Related to the intermediate language and removed discussion This is a discussion issue and not a change proposal. labels May 15, 2023
@jeskew
Copy link
Contributor

jeskew commented May 15, 2023

The consensus of a team discussion was that there shouldn't be any divergence between the behavior of a symbolic name template and a template not using symbolic name syntax. The ARM runtime should be updated to allow existing resources that don't exist so long as no reference function invocation refers to those resources.

If we do add a mechanism to make sure an existing resource actually does exist, it should work regardless of whether the template was compiled with symbolic name codegen enabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Community Call intermediate language Related to the intermediate language Needs: Upvote This issue requires more votes to be considered
Projects
Status: Todo
Development

No branches or pull requests

7 participants