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

Support evaluating resource IDs from symbolic names within the same resource #1852

Open
johndowns opened this issue Mar 12, 2021 · 36 comments
Open
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered

Comments

@johndowns
Copy link
Contributor

Some resources, such as load balancers, require referencing resource IDs as part of the properties of the resource definition, but those resource IDs are child resources of the resource itself. This means that Bicep detects this as a circular reference. For example, if I have a load balancer (simplified definition) like this:

resource loadBalancer 'Microsoft.Network/loadBalancers@2020-06-01' = {
  name: loadBalancerName
  location: location
  sku: {
    name: 'Standard'
  }
  properties: {
    frontendIPConfigurations: [
      {
        name: frontendIPConfigurationName
        properties: {
          privateIPAllocationMethod: 'Dynamic'
          subnet: {
            id: subnetResourceId
          }
        }
      }
    ]
    loadBalancingRules: [
      {
        name: 'HttpRule'
        properties: {
          frontendIPConfiguration: {
            id: loadBalancer.properties.frontendIPConfigurations[0].id
        }
      }
    ]
  }
}

then this line causes a circular reference:

            id: loadBalancer.properties.frontendIPConfigurations[0].id

The workaround is to use the resourceId() function directly. However, it would be nicer if Bicep could allow this scenario using the symbolic reference instead.

@anthony-c-martin
Copy link
Member

Related issues: #724, #1470 & #1787

@miqm
Copy link
Collaborator

miqm commented Oct 29, 2022

I've found a potential workaround - however it's not currently possible in bicep due to - I think - a bug:

resource vmsslb 'Microsoft.Network/loadBalancers@2021-05-01' = {
  name: '${vmssName}-lb'
  location: location
  properties: {
    frontendIPConfigurations: [
      {
        name: frontendIpConfig.name //ERROR: The expression is involved in a cycle ("frontendIpConfig" -> "fwlb"). bicep(BCP080)
        properties: {
          privateIPAddress: lbPrivateIp
        }
      }
    ]
  }

  resource frontendIpConfig 'frontendIPConfigurations' existing = {
    name: 'frontend'
  }
}

This in fact should be possible as frontendIpConfig is an existing resource and therefore should be (IMHO) excluded from the cycle detection.

@miqm
Copy link
Collaborator

miqm commented Oct 31, 2022

I found a working workaround but quite ugly:

resource _vmsslb 'Microsoft.Network/loadBalancers@2021-05-01' existing = {
  name: '${vmssName}-lb'

   resource frontendIpConfig 'frontendIPConfigurations' existing = {
    name: 'frontend'
  }
}

resource vmsslb 'Microsoft.Network/loadBalancers@2021-05-01' = {
  name: _vmsslb.name
  location: location
  properties: {
    frontendIPConfigurations: [
      {
        name: _vmss.frontendIpConfig.name //this works
        properties: {
          privateIPAddress: lbPrivateIp
        }
      }
    ]
  }
}

@miqm miqm added Needs: Upvote This issue requires more votes to be considered and removed Needs: Triage 🔍 labels Oct 31, 2022
@majastrz
Copy link
Member

If we proceed with excluding the existing resource dependency, we should also ensure that it works correctly with symbolic name codegen because we emit existing resources to JSON in that case.

@miqm miqm self-assigned this Oct 31, 2022
@miqm
Copy link
Collaborator

miqm commented Oct 31, 2022

Assigning this to myself, however if someone is willing to pick this up - please write down a comment

@4c74356b41
Copy link

this is mental

@miqm
Copy link
Collaborator

miqm commented Nov 8, 2022

this is mental

@4c74356b41 can you elaborate?

@4c74356b41
Copy link

4c74356b41 commented Nov 9, 2022

this issue is open for over a year, whereas this problem exists since the inception of bicep and its not yet fixed*.

@anthony-c-martin
Copy link
Member

@4c74356b41 one of the reasons this hasn't really been prioritized is that we haven't seen any great need for this feature outside of the Microsoft.Network/loadBalancers resource; it is hard to justify building a new feature to address quirks in a single resource API.

Do you have any example use-cases outside of the loadbalancer resource? If you're able to share, that would help us triage & prioritize this.

@4c74356b41
Copy link

appgw - the only one that pops up right now

@PolyphonyRequiem
Copy link

Working with appgw is a nightmare already in arm/bicep. This would help. Fixing the model to not work this way might help more though...

@4c74356b41
Copy link

honestly i dont understand why do they need to refer to each other by id? its not like you can reference a listener from another appgw, right?

@sc-atompower
Copy link

I came here because of app gateway but I have circular id calls all over the place converting our existing infrastructure to code. This is indeed mental.

@Ro3A
Copy link

Ro3A commented Apr 20, 2023

Also stuck on app gateway config.. Mine is particularly tricky as I cannot use resourceId due to cycle detection issue because I'm creating the resource via the use of Azure/ResourceModules app gateway module.

So I'm not even sure if there's a work around for this...

@brwilkinson
Copy link
Collaborator

brwilkinson commented Apr 20, 2023

@Ro3A

I have some examples here:

@Ro3A
Copy link

Ro3A commented Apr 21, 2023

@brwilkinson Thanks for the reply.

If I'm understanding correctly, in that example, what you're demonstrating is the creation of the resource id ahead of time. If so, that's where I ended up yesterday as well.

That said, even if this was resolved from a resource perspective, I suspect further work would need to be done to support this same objective when using a module, such as the app gateway module in the CARML project. The App Gateway module in the Azure/ResourceModules (CARML) project provides inputs for many of the nested resources required for app gateway. Backend pools, ssl, userAssignIdentity, etc. The networking aspects, especially, require the resourceId of the parent app gateway.

However, using resourceId() at all, with a module, is a code smell due to cycle detection.

@brwilkinson
Copy link
Collaborator

Since #10268 was a duplicate, I will just bring that over here

Consider allowing a resource to reference itself for the values that are required at build time e.g. Name, Id, api, scope

Many network resources use the fully qualified resourceId for properties that reference itself.

  • Since there are some properties that are required at build, consider allowing self-reference for these properties.

What we do now (workaround)

param AppGWName string = 'mywaf123'

// Note I put this is as a variable - example 1
var AppGWID = resourceId('Microsoft.Network/applicationGateways', AppGWName )

//  it's more common for people to use resourceid()  - example 2
// in many places throughout the template, which does not represent a clean 'Bicep worthy' solution

resource applicationGateway 'Microsoft.Network/applicationGateways@2022-09-01' = {
  name: AppGWName 
  location: resourceGroup().location
  properties: {
    frontendIPConfigurations: [
      {
        name: 'public' // we are cross referencing this property ____________________________________________\/
        properties: {
          publicIPAddress: { 
            id: 'id'
          }
        }
      }
    ]
    httpListeners: [
      {
        name: 'name'
        properties: {
          frontendIPConfiguration: {
            // id requires the resourceid use AppGWID as substitute
            id: '${AppGWID}/frontendIPConfigurations/public' // cross reference to frontendIPConfiguration ___/\
          }
          frontendPort: {
            // a second example of cross reference, which is the more common pattern
            id: resourceId('Microsoft.Network/applicationGateways/frontendPorts', AppGWName, 'fe-80')
          }
          protocol: 'Http'
          sslCertificate: null
        }
      }
    ]

The proposal - unblock the following self-reference, only allow access to: name, id, apiversion?

This expression is referencing its own declaration, which is not allowed.
[{
"resource": "Untitled-2",
"owner": "generated_diagnostic_collection_name#0",
"code": "BCP079",
"severity": 8,
"message": "This expression is referencing its own declaration, which is not allowed.",
"source": "bicep",
"startLineNumber": 20,
"startColumn": 21,
"endLineNumber": 20,
"endColumn": 39
}]

param AppGWName string = 'mywaf123'

resource applicationGateway 'Microsoft.Network/applicationGateways@2022-09-01' = {
  name: AppGWName 
  location: resourceGroup().location
  properties: {
    frontendIPConfigurations: [
      {
        name: 'public' // we are cross referencing this property
        properties: {
          publicIPAddress: {
            id: 'id'
          }
        }
      }
    ]
    httpListeners: [
      {
        name: 'name'
        properties: {
          frontendIPConfiguration: {
            id: '${applicationGateway.id}/frontendIPConfigurations/public' // use-self for id - example 1
          }
          frontendPort: {
            id: '${applicationGateway.id}/frontendPorts/fe-80' // use-self for id - example 2
          }
          protocol: 'Http'
          sslCertificate: null
        }
      }
    ]

There is a snippet for the Application Gateway to try this yourself res-app-gateway

  • There are other resources similar to this e.g. Virtual Network Gateway.

@miqm miqm removed their assignment Apr 24, 2023
@ckittel
Copy link
Member

ckittel commented May 26, 2023

Any additional movement on this story? The workarounds made by @brwilkinson and @miqm still the leading solutions? Anyone else find a creative way to resolve this while waiting for a feature change in bicep?

@brwilkinson
Copy link
Collaborator

@ckittel no update at this time.

Team has reviewed and did not schedule this work, so far.

So still sitting in the needs up vote stage... So the more comments and feedback will be useful to prioritize this for the future.

@acarter24
Copy link

acarter24 commented Jun 15, 2023

I tried to follow the example here:

https://learn.microsoft.com/en-us/azure/application-gateway/quick-create-bicep?tabs=CLI

    requestRoutingRules: [
      {
        name: 'myRoutingRule'
        properties: {
          ruleType: 'Basic'
          httpListener: {
            id: resourceId('Microsoft.Network/applicationGateways/httpListeners', applicationGateWayName, 'myListener')
          }
          backendAddressPool: {
            id: resourceId('Microsoft.Network/applicationGateways/backendAddressPools', applicationGateWayName, 'myBackendPool')
          }
          backendHttpSettings: {
            id: resourceId('Microsoft.Network/applicationGateways/backendHttpSettingsCollection', applicationGateWayName, 'myHTTPSetting')
          }
        }

And came across the same issue for the self referential bit, thanks for the workaround and a +1 on the suggested fix to allow self-referencing

@ckittel
Copy link
Member

ckittel commented Jun 23, 2023

Tangent: Maybe we can get the AppGW team to add a new property called name to each of those in a future API version, and we only need to provide myListener. Or id is built to support either the full ID or just the name part. :) It's not like we need to be referencing the listeners from a different app gateway. :)

@akhilthomas011
Copy link

+1. Not sure why this isn't prioritized. A new property called name or nameOrId should suffice.

@RVAHaver01
Copy link

RVAHaver01 commented Jul 12, 2023

I am using the work around for the frontendIPConfiguration on my app gateway httpListeners however I am getting the error messaage:

Resource /subscriptions/a2bee89e-23fc-4c75-9a78-79d0aed1c958/resourceGroups/pxise-dmx-dbh-trial-rg/providers/Microsoft.Network/applicationGateways/pxise-dmx-dbh-trial-appgw/frontendIPConfigurations/appGwPublicFrontendIp referenced by resource /subscriptions/a2bee89e-23fc-4c75-9a78-79d0aed1c958/resourceGroups/pxise-dmx-dbh-trial-rg/providers/Microsoft.Network/applicationGateways/pxise-dmx-dbh-trial-appgw/httpListeners/pxise-dmx-dbh-trial-appgw-lstner-20305 was not found. Please make sure that the referenced resource exists, and that both resources are in the same region. (Code: InvalidResourceReference)

Is there another workaround that I can try? Guess I could just do a separate module to run after the app gateway gets created to address these child references.

@tasdflkjweio
Copy link

I'm again back to setting up a nat gateway + nat gateway ip + vnet w/ subnets, and this would be useful functionality.

@thefaftek-git
Copy link

I don't understand why this is in a state of needs: upvote. Without this functionality, we have to use half-functional workarounds to even do things such as deploy app gateway via bicep. I'd think core functionality would be scheduled regarless of number of users.

@alex-frankel
Copy link
Collaborator

I disagree that this is core functionality. The feature ask here would only be to accommodate a strange API design that only AppGW (and maybe other networking resources?) implemented. That's why we need a lot of customer demand to justify implementing a fix for a specific case like this.

Ideally, the AppGW team implements somethign like what @ckittel is describing here: #1852 (comment)

@Ro3A
Copy link

Ro3A commented Oct 18, 2023

I disagree that this is core functionality. The feature ask here would only be to accommodate a strange API design that only AppGW (and maybe other networking resources?) implemented. That's why we need a lot of customer demand to justify implementing a fix for a specific case like this.

Ideally, the AppGW team implements somethign like what @ckittel is describing here: #1852 (comment)

I challenge anyone to implement the well-architected framework via IaC and not run into this issue. It's not just AppGW and a few networking components few people use. It's almost all networking related components which are foundational to well architected framework.

@alex-frankel
Copy link
Collaborator

@Ro3A - is there an inventory of the different networking resources that have this API design? I probably don't have a full understanding of the scope of the problem, but my point still stands if it is only the Networking RP that has APIs designed in this way.

It is not to say that there is not justification for implementing the fix on our side, but from a layering perspective it is not the right place to fix this problem.

@Ro3A
Copy link

Ro3A commented Oct 18, 2023

@alex-frankel I don't have a full inventory, but as we've begun the process to script our entire environment and adhere to Well architected framework, so far every networking related component has caused pain.

Top of mind: Virtual Networks, Subnets, API Management, Traffic Manager, App Gateway, regular Gateways.. I think it's generally anything that requires child resources, but child resources cannot be deployed independently. And it affects resources that depend on these resources as well, so again, scope is indeed fairly large.

Ideally, the fix is in those resources themselves. But I don't see that type of work getting done faster than the Bicep team might be able to implement a function to assist with this.

@thefaftek-git
Copy link

I disagree that this is core functionality. The feature ask here would only be to accommodate a strange API design that only AppGW (and maybe other networking resources?) implemented. That's why we need a lot of customer demand to justify implementing a fix for a specific case like this.

Ideally, the AppGW team implements somethign like what @ckittel is describing here: #1852 (comment)

I'm willing to agree the API/impelmentation is strange, though I'd argue from a customer perspective we see functionality as usability in the end-result, not who owns layers in the middle.

Is there a way to flag/bring this to the NRP team? I'm more than happy to have them fix the backend instead, though that would only work for API versions going forward and not fix the issue that already exists.

Our company has a call with PMs for Bicep and a few other teams, I'll bring this up there as well, but any guidance on how to push for this would be really useful.

@alex-frankel
Copy link
Collaborator

I agree with the assessment. Ultimately, an end user shouldn't have to care why something is not working as expected, but I am trying to give context on how best to route the issue. If you have a call with a Microsoft Product Group, definitely bring this up and they can help route this to the NRP team. I will also put this on their radar.

@ramuvr
Copy link

ramuvr commented Oct 25, 2023

Facing same issues with AppGateway module, with trustedClientCertificates needing to be referenced inside sslProfiles.

trustedClientCertificates itself is a array (doesn't have keyVaultSecretId like trustedRootCertificates), so difficult to reference inside sslProfiles>trustedClientCertificates>id field.

Currently using workaround

resourceId('Microsoft.Network/applicationGateways/trustedClientCertificates', appGatewayName, ClientCertName)

where ClientCertName is a parameter/variable with the name of the certificate.

@Xyloto91
Copy link

@johndowns any info about this issue?

@johndowns
Copy link
Contributor Author

@alex-frankel Are you able to provide any info for @Xyloto91 please?

@miqm
Copy link
Collaborator

miqm commented Feb 19, 2024

@alex-frankel now that I think why we request this and checkIfExists features it's not that RPs are badly designed. Obviously there are cases when we need to create certain child resources when creating resource for the first time (route table with at least one route, vnet with a subnet, k8s cluster with at least one agent pool). Perhaps we could find a way how to address this need?

@alex-frankel
Copy link
Collaborator

@miqm -- I think I hear you on the checkIfExists thread, but I am not following the ask on this thread. As far as I know, App Gateway is the only resource that requires this self-referencing, which is why we haven't been able to prioritize this one.

The parent property/child resource issue is separate and the networking team is actually working on fixing these on a case-by-case basis. You can follow the latest here: Azure/azure-quickstart-templates#2786 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Needs: Upvote This issue requires more votes to be considered
Projects
Status: Todo
Development

No branches or pull requests