Add resource types in outputs and parameters #4971

rynowak · 2021-10-23T02:32:54Z

This change implements functionality for declaring strongly type
parameters and outputs using resource types.

Example:

param storage resource 'Microsoft.Storage/storageAccounts@2020-01-01'

This declares a parameter that can be interacted with as-if it were an
'existing' resource type declaration of the provided type.

In addition you can do the same with outputs:

output out resource 'Microsoft.Storage/storageAccounts@2020-01-01' = foo

or using type inference (outputs):

output out resource = foo

These features together allow you to pass resources across module
boundaries, and as command line parameters (using the resource ID).

This PR implements #2246 with a few caveats that I discussed offline
with one of the maintainers.

It does not include the 'any resource type' that's outlined in the
proposal. It's not clear how that part of the proposal will work
in the future with extensibility.
It does not support extensibility resources currently. To do this we
need to define the semantics of how an extensibility resource can
be serialized (what's the equivalent of .id). This is explicitly
blocked in the first cut and can be added as extensibility is
designed.
Parameter resources cannot be used with the .parent or .scope
properties because it would allow you to bypass scope validation
easily. The set of cases that we could actually provide validation
for these use cases are really limited.

Contributing a feature

I have opened a new issue for the proposal, or commented on an existing one, and ensured that the Bicep maintainers are good with the design of the feature being implemented
I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged
I have appropriate test coverage of my new feature

src/Bicep.Core.IntegrationTests/NestedResourceTests.cs

src/Bicep.Core.Samples/DataSets.cs

src/Bicep.Core.Samples/Files/AKS_LF/main.syntax.bicep

src/Bicep.Core.Samples/Files/InvalidParameters_LF/main.diagnostics.bicep

rynowak · 2021-10-23T02:43:37Z

src/Bicep.Core.Samples/Files/Parameters_CRLF/main.json

+      "type": "string",
+      "metadata": {
+        "resourceType": "Microsoft.Storage/storageAccounts@2019-06-01"
+      }


Worth discussion on the best way to encode this. We need the type to round-trip through modules. The way I encoded it we always include the API Version when we write, but when we read we ignore it for type validation purposes.

I put in a comment elsewhere in the PR about this, but I'm hoping we can add this as a top-level property in a param that acts like a constraint for string types. Similar how the other constraints work:

We should probably continue the discussions on this part after this PR goes in.

src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs

src/Bicep.Core/Emit/TemplateWriter.cs

src/Bicep.Core/Semantics/ArmTemplateSemanticModel.cs

src/Bicep.Core/Semantics/ResourceAncestorVisitor.cs

src/Bicep.Core/TypeSystem/DeclaredTypeManager.cs

src/Bicep.Core/TypeSystem/TypeValidator.cs

alex-frankel · 2021-10-25T01:42:44Z

First off, this is very cool.

Some things I noticed when testing:

I know at one point when discussing with Anders, he felt it was unnecessary to specify the type and assign a value for the output. So instead of output appPlan resource = appPlan, you would simply do output appPlan. I know we went back and forth on this, but can't remember what the final decisions was. @anthony-c-martin, do you remember?
I would have expected the type resource to be green instead of purple here:
for some reason, I am not getting completions in the following scenario:

I have a module site.bicep which outputs a microsoft.web/sites resource. When I reference that output, I don't get completions on the properties property:

I wonder if it is related to this being a module collection resulting from a loop expression?

miqm · 2021-10-25T11:18:11Z

Something that we could consider as a follow-up - a function to get the scope of the resource. This way we could say to module - deploy with a scope of this particular resource.
It might not be a part of this PR, but should be done ASAP as this will be a crucial part.
~~Also - would be good if we could just take the resource type and cast it to a string to get the ID so we can do split (i.e. for compatibility with existing code based on resourceId string).~~ nevermind, we can use .id

alex-frankel · 2021-10-25T12:53:00Z

I would have expected the type resource to be green instead of purple here:

Just realized this is probably the same issue as #4343

alex-frankel · 2021-10-25T12:57:23Z

Something that we could consider as a follow-up - a function to get the scope of the resource. This way we could say to module - deploy with a scope of this particular resource.

We shouldn't necessarily need a function to get the scope if I am understanding the scenario correctly. We need to support a generic resource param without a specific type so that the following scenario of creating an extension resource for several different types can work:

param genericResource resource

resource diagnostic 'Microsoft.Insights/diagnosticSettings@2021-05-01-preview' = {
  scope: genericResource
  ...
}

EDIT:

Looks this is currently blocked, but I think we need this to work:

alex-frankel · 2021-10-25T13:00:40Z

Completions behavior for resource types for a parameter is slightly different than a resource definition. In a resource definition, completions show up both with and without ', but with a param foo resource it only shows up with a ':

vs

rynowak · 2021-10-25T17:01:08Z

Looks this is currently blocked, but I think we need this to work (using a parameter as a resource parent or scope):

We should catch up on this today. When @anthony-c-martin was explaining to me the set of validations that take place for scoping, my reaction was that we can't really enforce those with any amount of certainty. Maybe I'm wrong about this, or maybe the feature is still valuable without the validation 😆

rynowak · 2021-10-25T17:03:41Z

I know at one point when discussing with Anders, he felt it was unnecessary to specify the type and assign a value for the output. So instead of output appPlan resource = appPlan, you would simply do output appPlan. I know we went back and forth on this, but can't remember what the final decisions was. @anthony-c-martin, do you remember?

Currently the type is required for all outputs. So if it's an int then it's output foo int. The new resource ... without type string is a special form but still specifies a type. I feel like this would be simpler for everyone if the type was just optional and there were no special forms.

rynowak · 2021-10-25T17:08:00Z

I would have expected the type resource to be green instead of purple here:

Just realized this is probably the same issue as #4343

Maybe not. I told the editor to colorize this as a keyword, which sounds like it's not what we want. https://github.com/Azure/bicep/pull/4971/files#diff-ee8a096413276bc466bb160724239726958a72cb0c16dd566bb268dacfbdafe0R264

rynowak · 2021-10-25T17:10:41Z

We shouldn't necessarily need a function to get the scope if I am understanding the scenario correctly. We need to support a generic resource param without a specific type so that the following scenario of creating an extension resource for several different types can work:

Let's discuss this today. After we planned to block this use case the generic 'any resource' type didn't really make sense so we didn't implement it. If we need to his this use case then we need to bring these back into the design.

miqm · 2021-10-25T20:56:51Z

We shouldn't necessarily need a function to get the scope if I am understanding the scenario correctly. We need to support a generic resource param without a specific type so that the following scenario of creating an extension resource for several different types can work:

Let's discuss this today. After we planned to block this use case the generic 'any resource' type didn't really make sense so we didn't implement it. If we need to his this use case then we need to bring these back into the design.

Use case scenario for this function would be to have a module that adds secrets to given key vault or sets up key vault secrets user role. Key vault might be in a different RG (scope) than module that deploys i.e. WebApp. We need to set module scope so the deployment goes where it should, hence the need for a function to extract scope from resourceId.

miqm · 2021-10-25T21:05:22Z

I know at one point when discussing with Anders, he felt it was unnecessary to specify the type and assign a value for the output. So instead of output appPlan resource = appPlan, you would simply do output appPlan. I know we went back and forth on this, but can't remember what the final decisions was. @anthony-c-martin, do you remember?

Currently the type is required for all outputs. So if it's an int then it's output foo int. The new resource ... without type string is a special form but still specifies a type. I feel like this would be simpler for everyone if the type was just optional and there were no special forms.

There was also idea to just mark resource as 'public' and use the symbol name but that could give extra complication with nested ones, so perhaps let's stick with output.

anthony-c-martin · 2021-10-26T01:09:06Z

I know at one point when discussing with Anders, he felt it was unnecessary to specify the type and assign a value for the output. So instead of output appPlan resource = appPlan, you would simply do output appPlan. I know we went back and forth on this, but can't remember what the final decisions was. @anthony-c-martin, do you remember?

There are a few scenarios where specifying the type is required, without a way around it - so we cannot infer it completely:

When using the any type (either via the any function, or via a case where type information is not propagated fully by Bicep
When RP types are unavailable or inaccurate.

resource statements don't have this problem as the type is declared, and is unambiguous. We could definitely consider making the type optional on an output statement here, and in other unambiguous scenarios.

src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs

src/Bicep.Core.IntegrationTests/ParametersTests.cs

src/Bicep.Core.IntegrationTests/OutputsTests.cs

src/Bicep.Core/Emit/ExpressionConverter.cs

rynowak · 2021-10-31T21:29:02Z

Completions behavior for resource types for a parameter is slightly different than a resource definition. In a resource definition, completions show up both with and without ', but with a param foo resource it only shows up with a ':

Fixing this.

brwilkinson · 2021-11-10T22:29:00Z

subscribing, to do some more testing once these artifacts are generated.

miqm · 2022-01-23T20:53:00Z

Is this being worked on? Can't wait to get this functionality...

rynowak · 2022-02-05T01:26:56Z

I pushed an update here. This doesn't yet address all of the scenarios that we've discussed (PR description is up to date). The plan I discussed with the maintainers is to merge this as an experimental feature first and then address the gaps in a series of smaller changes.

src/Bicep.LangServer/Completions/BicepCompletionProvider.cs

src/Bicep.Decompiler/Rewriters/ParentChildResourceNameRewriter.cs

majastrz · 2022-02-07T07:17:45Z

src/Bicep.Core/TypeSystem/TypeAssignmentVisitor.cs

+                // We need to validate all of the parameters and outputs to make sure they are valid types.
+                // This is where we surface errors for 'unknown' resource types.
+                if (singleDeclaredType is ModuleType moduleType &&
+                    moduleType.Body is ObjectType objectType)


Body

You may need to handle loops and/or conditionals here. There's a method that can get the body for all those cases (TryGetBody or something like that).

Are you sure? This is looking at types not syntax. L281 ^^^ handles the array case.

src/Bicep.Core/TypeSystem/TypeAssignmentVisitor.cs

src/Bicep.Core/Syntax/ModuleDeclarationSyntax.cs

src/Bicep.Core/Syntax/SimpleTypeSyntax.cs

majastrz

majastrz · 2022-02-09T02:20:48Z

I know there's still work left but this is awesome! Since this is behind a feature flag, all the comments I added can be addressed in future PRs.

Fixes: Azure#2246 This change implements functionality for declaring strongly type parameters and outputs using resource types. Example: ``` param storage resource 'Microsoft.Storage/storageAccounts@2020-01-01' ``` This declares a parameter that can be interacted with as-if it were an 'existing' resource type declaration of the provided type. In addition you can do the same with outputs: ``` output out resource 'Microsoft.Storage/storageAccounts@2020-01-01' = foo ``` or using type inference (outputs): ``` output out resource = foo ``` These features together allow you to pass resources across module boundaries, and as command line parameters (using the resource ID). --- This PR implements Azure#2246 with a few caveats that I discussed offline with one of the maintainers. 1. It does not include the 'any resource type' that's outlined in the proposal. It's not clear how that part of the proposal will work in the future with extensibility. 2. It does not support extensibility resources currently. To do this we need to define the semantics of how an extensibility resource can be serialized (what's the equivalent of `.id`). This is explicitly blocked in the first cut and can be added as extensibility is designed. 3. Parameter resources cannot be used with the `.parent` or `.scope` properties because it would allow you to bypass scope validation easily. The set of cases that we could actually provide validation for these use cases are really limited.

Fixes: #2246 This change implements functionality for declaring strongly type parameters and outputs using resource types. Example: ``` param storage resource 'Microsoft.Storage/storageAccounts@2020-01-01' ``` This declares a parameter that can be interacted with as-if it were an 'existing' resource type declaration of the provided type. In addition you can do the same with outputs: ``` output out resource 'Microsoft.Storage/storageAccounts@2020-01-01' = foo ``` or using type inference (outputs): ``` output out resource = foo ``` These features together allow you to pass resources across module boundaries, and as command line parameters (using the resource ID). --- This PR implements #2246 with a few caveats that I discussed offline with one of the maintainers. 1. It does not include the 'any resource type' that's outlined in the proposal. It's not clear how that part of the proposal will work in the future with extensibility. 2. It does not support extensibility resources currently. To do this we need to define the semantics of how an extensibility resource can be serialized (what's the equivalent of `.id`). This is explicitly blocked in the first cut and can be added as extensibility is designed. 3. Parameter resources cannot be used with the `.parent` or `.scope` properties because it would allow you to bypass scope validation easily. The set of cases that we could actually provide validation for these use cases are really limited.