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

Simplify properties that are resource references #285

Open
askew opened this issue Aug 21, 2020 · 14 comments
Open

Simplify properties that are resource references #285

askew opened this issue Aug 21, 2020 · 14 comments
Labels
enhancement New feature or request

Comments

@askew
Copy link

askew commented Aug 21, 2020

ARM makes extensive use of the format

"otherresource": {
  "id": "[resourceId(....)]"
}

Take for example the network interface in Bicep Playground 0.0.8, there is subnet, publicIpAddress, and networkSecurityGroup.

resource nic1 'Microsoft.Network/networkInterfaces@2017-06-01' = {
  name: nic1Name
  location: location
  properties: {
    ipConfigurations: [
      {
        name: 'ipconfig1'
        properties: {
          subnet: {
            id: '${vnet.id}/subnets/${subnet1Name}'
          }
          privateIPAllocationMethod: 'Dynamic'
          publicIpAddress: {
            id: pip.id
          }
        }
      }
    ]
    networkSecurityGroup: {
      id: nsg.id
    }
  }
}

Could this be simplified to:

resource nic1 'Microsoft.Network/networkInterfaces@2017-06-01' = {
  name: nic1Name
  location: location
  properties: {
    ipConfigurations: [
      {
        name: 'ipconfig1'
        properties: {
          subnet: vnet.subnets.subnet1Name
          privateIPAllocationMethod: 'Dynamic'
          publicIpAddress: pip
        }
      }
    ]
    networkSecurityGroup: nsg
  }
}

Bicep should be able to tell that the ARM schema for the resource needs an object of the format { "id": "" } so it can extract this directly from the reference.

Also, the reference to the sub-resource (subnet in this case) could be simple dot notation as every sub-resource will have a name.

@ghost ghost added the Needs: Triage 🔍 label Aug 21, 2020
@askew askew changed the title Simply properties that are resource references Simplify properties that are resource references Aug 21, 2020
@alex-frankel
Copy link
Collaborator

This is a great point that you are bringing up. This one is a bit more challenging because the subnet is declared as a property not a resource, so bicep doesn't understand how to get an ID from it.

Ideally the subnets would be declared as child resources, and then you could get the id as you would like:

resource subnet1 'microsoft.network/virtualNetworks/subnets@2017-06-01' = {
  ...
}

resource nic1 '...' = {
  ...
  subnet: {
    id: subnet1.id
  }
}

The problem is that the vnet resourceType does not handle subnets as child resources particularly well. In theory we could special case what you want to do, but I would prefer to fix the networking APIs to better accommodate this.

Does that make sense?

Curious what others think about this one

@bmoore-msft
Copy link
Contributor

bmoore-msft commented Aug 22, 2020

I kinda like the idea - it implies that the id property is the default property but that would have to then work for every scenario because we lose the option to reference the object in that case. I could see the object wanting to be used in outputs (rare but possible).

@bmoore-msft
Copy link
Contributor

Another option would be to assert that if the user did something like:

vnet.subnets[subnetName].id

We create a resourceId with the resourceId function as the user requested. It may not be a valid resourceId but that's no different than today when using the resourceId function.

I'm struggling to think of a resource, other than subnets where it would be useful though... To @alex-frankel 's point of, maybe we should just fix the RP

@alex-frankel alex-frankel added the enhancement New feature or request label Aug 24, 2020
@askew
Copy link
Author

askew commented Sep 22, 2020

Not getting side-tracked by the fact that subnets are properties, the main point of this issue is to simplify the referencing of resources. Use the NSG reference as an example. In ARM the NSG reference requires a property that is an object with an id property.

    "networkSecurityGroup": {
        "id": "[resourceId('Microsoft.Network/.... )]"
    }

rather than requiring the Bicep to be:

    networkSecurityGroup: {
      id: nsg.id
    }

It could be:

    networkSecurityGroup: nsg

There are lots of examples in ARM where resources are referenced like this.

@slavizh
Copy link
Contributor

slavizh commented Sep 23, 2020

@askew True, but problem is that no RP is the same. I have seen cases where the reference does not follow that logic and you only have the property in which you put the resource ID as string. AKS for example does this. I guess overall it is a matter if they can make this via some pattern without having to code per RP and would it be confusing for new users or not. Not sharing particular opinion just discussion the topic and my experience with RPs.

@beforan
Copy link

beforan commented Mar 26, 2021

sorry to bump this from months ago, but since it's still open, the current behaviour did kinda burn me today as a newcomer, because I thought the desired approach from this issue was actually what I was supposed to do!

using the vscode tooling, I assumed, due to intellisense suggesting an NSG was the required type, that the symbolic reference was enough.

So I went ahead and typed:

networkSecurityGroup: nsg

giving the following in the built ARM template:

"networkSecurityGroup": "[reference(resourceId('Microsoft.Network/networkSecurityGroups', variables('nsgName')), '2020-08-01', 'full')]",

and the tooling says it's fine, but deploying gives:

// Deployment failed. Correlation ID: .
{
  "error": {
    "code": "InvalidRequestFormat",
    "message": "Cannot parse the request.",
    "details": [
      {
        "code": "MissingJsonReferenceId",
        "message": "Value for reference id is missing. Path properties.networkSecurityGroup."
      }
    ]
  }
}

Only by finding this issue did I understand I needed to do:

networkSecurityGroup: {
  id: nsg.id
}

@alex-frankel
Copy link
Collaborator

@anthony-c-martin, it would be interesting if RPs could somehow declare in swagger that this networkSecurityGroup property could be an id or a resource of type networkSecurityGroup, then we could allow a direct resource reference in bicep. Or maybe if we have a strict rule that if an object's swagger only has one property called ID, we could assume it is a resource reference of a specific type.

@anthony-c-martin
Copy link
Member

Copying comment from #2906:

I think this is a duplicate of #285. IMO if we want to support this with a high degree of accuracy, we would need a swagger annotation from RPs to mark places where a direct resource might be permitted. That unfortunately would a pretty huge effort.

A more general idea I was exploring a while back is to allow a shorthand for declaring an object with a single property by allowing . chars in object keys to auto-insert objects, e.g. so all of the following are semantically equivalent:

Original:

resource childManagementGroup2 'Microsoft.Management/managementGroups@2020-05-01' = {
  name: 'example-child-2'
  properties: {
    displayName: 'Example child 2'
    details: {
      parent: {
        id: parentManagementGroup.id
      }
    }
  }
}

Also works:

resource childManagementGroup2 'Microsoft.Management/managementGroups@2020-05-01' = {
  name: 'example-child-2'
  properties: {
    displayName: 'Example child 2'
    details: {
      parent.id: parentManagementGroup.id
    }
  }
}

Further nested:

resource childManagementGroup2 'Microsoft.Management/managementGroups@2020-05-01' = {
  name: 'example-child-2'
  properties: {
    displayName: 'Example child 2'
    details.parent.id: parentManagementGroup.id
  }
}

If there's interest in the above, I'd be happy to put together a formal proposal.

Alternatively, if we get around to doing #146, we could at least save some whitespace:

resource childManagementGroup2 'Microsoft.Management/managementGroups@2020-05-01' = {
  name: 'example-child-2'
  properties: {
    displayName: 'Example child 2'
    details: {
      parent: { id: parentManagementGroup.id }
    }
  }
}

@bmoore-msft
Copy link
Contributor

bmoore-msft commented Jun 1, 2021

This syntax perplexes me a bit...

details.parent.id: foo

On one hand it seems handy on another it seems like it's just confusing to mix the syntaxes... also thinking about a scenario where the details object has other properties... would I:

details.parent.id: foo
details.parent.name: bar

or

details.parent.id: foo
details: {
    name: bar
}

Granted I assume we'd have restrictions (like disallowing that second example), but the benefit escapes me... esp over your example from #146...

@anthony-c-martin
Copy link
Member

Granted I assume we'd have restrictions (like disallowing that second example), but the benefit escapes me... esp over your example from #146...

Terseness is the only benefit; semantically it's equivalent. In my example, it cuts 5 lines down to 1 line, and reduces the amount of punctuation required. I'm just trying to gauge interest with the above write-up.

@johndowns
Copy link
Contributor

I think I prefer the solution in #146, @anthony-c-martin. The other suggestion is nice in that it's terse, but I feel like it's obscuring the way the type is defined. A single-line object declaration feels like it gives the same terseness without adding any conceptual overhead.

@rohancragg
Copy link

rohancragg commented Nov 4, 2021

I wasted hours today with the exact scenario that @beforan cites. I was assuming that the correct syntax would be:

    networkSecurityGroup: nsg

...eventually realising that I have to reference it like this

    networkSecurityGroup: {
      id: nsg.id
    }

The same happened with networkInterfaces

resource subnet 'Microsoft.Network/virtualNetworks/subnets@2020-11-01' existing = {
...
  properties: {
    ipConfigurations: [
      {
        name: 'ipconfig1'
        properties: {
            subnet: {
                id: subnet.id
            }

...which I'd assumed would be:

    subnet: subnet

@alex-frankel
Copy link
Collaborator

alex-frankel commented Nov 5, 2021

Not dismissing the frustration, but you should have seen a warning with the above code:

image

Did you see that?

EDIT:

Sorry, misread. With the code you have, there is no warning because the property expects an object.

@rohancragg
Copy link

Thank you, yes, I do see the warning with the code above but not with

subnet: existingSubnet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

8 participants