Skip to content

Commit

Permalink
Block nested deployment resources with inner scoped evaluation and sy…
Browse files Browse the repository at this point in the history
…mbolic references to outer scope (#11884)

Resolves #11846 

This PR updates EmitLimitationCalculator to emit an error when a
resource of type `Microsoft.Resources/deployments` uses inner scoped
expression evaluation but refers to symbols defined in the outer scope
from within the `template` property.

This PR may be a **breaking change** for some templates, as the added
check may catch scenarios that would not result in a runtime deployment
failure, e.g., when a template has a symbol with the same name and type
defined in both contexts:

```bicep
var fizz = 'buzz'

resource nested 'Microsoft.Resources/deployments@2020-10-01' = {
  name: 'name'
  properties: {
    mode: 'Incremental'
    expressionEvaluationOptions: {
      scope: 'inner'
    }
    template: {
      '$schema': 'https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#'
      contentVersion: '1.0.0.0'
      variables: {
        fizz: 'pop'
      }
      resources: [
        {
          apiVersion: '2022-09-01'
          type: 'Microsoft.Resources/tags'
          name: 'default'
          properties: {
            tags: {
              tag1: fizz // <-- Not an error, but surprise! Its value is 'pop', not 'buzz'
            }
          }
        }
      ]
    }
  }
}
```

In that case, the user *may* be using a Bicep symbolic reference to save
a few characters, but what the template will do (dereference the
inner-scoped symbol) does not line up with the communicated intent of
the syntax (i.e., to dereference the outer-scoped symbol), so I chose to
flag that as an error, too, as this usage would almost certainly
represent a *logical* error even if the compiled nested deployment would
not raise a runtime error.
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/11884)
  • Loading branch information
jeskew committed Sep 22, 2023
1 parent e964970 commit 5f83b12
Show file tree
Hide file tree
Showing 13 changed files with 297 additions and 86 deletions.
125 changes: 119 additions & 6 deletions src/Bicep.Core.IntegrationTests/ScenarioTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5069,12 +5069,125 @@ public void Test_Issue11742()
""");

result.Template.Should().NotBeNull();
// uncomment after merging https://github.com/Azure/bicep/pull/11740
// result.ExcludingLinterDiagnostics().Should().HaveDiagnostics(new[]
// {
// ("BCP333", DiagnosticLevel.Warning, "The provided value (whose length will always be less than or equal to 8) is too short to assign to a target for which the minimum allowable length is 36."),
// ("BCP333", DiagnosticLevel.Warning, "The provided value (whose length will always be less than or equal to 8) is too short to assign to a target for which the minimum allowable length is 36."),
// });
result.ExcludingLinterDiagnostics().Should().HaveDiagnostics(new[]
{
("BCP333", DiagnosticLevel.Warning, "The provided value (whose length will always be less than or equal to 8) is too short to assign to a target for which the minimum allowable length is 36."),
("BCP333", DiagnosticLevel.Warning, "The provided value (whose length will always be less than or equal to 8) is too short to assign to a target for which the minimum allowable length is 36."),
});
}

// https://github.com/Azure/bicep/issues/11846
[TestMethod]
public void Test_Issue11846()
{
var withOuterScopeEvaluation = """
param tags object
param tag1 string
var tag2 = 'tag2'
var deploymentName = 'name'
var deploymentMode = 'Incremental'

resource nestedDeployment 'Microsoft.Resources/deployments@2020-10-01' = {
name: deploymentName
properties: {
mode: deploymentMode
template: {
'$schema': 'https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#'
contentVersion: '1.0.0.0'
resources: [
{
apiVersion: '2022-09-01'
type: 'Microsoft.Resources/tags'
name: 'default'
properties: {
tags: union(tags, {tag1: tag1, tag2: tag2})
}
}
]
}
}
}
""";
var withExplicitInnerScopeEvaluation = """
param tags object
param tag1 string
var tag2 = 'tag2'
var deploymentName = 'name'
var deploymentMode = 'Incremental'

resource nestedDeployment 'Microsoft.Resources/deployments@2020-10-01' = {
name: deploymentName
properties: {
expressionEvaluationOptions: {
scope: 'inner'
}
mode: deploymentMode
template: {
'$schema': 'https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#'
contentVersion: '1.0.0.0'
resources: [
{
apiVersion: '2022-09-01'
type: 'Microsoft.Resources/tags'
name: 'default'
properties: {
tags: union(tags, {tag1: tag1, tag2: tag2, tag3: join(map(['a'], x => x), ',')})
}
}
]
}
}
}
""";
var withImplicitInnerScopeEvaluation = """
param tags {*: string}
param tag1 string
var tag2 = 'tag2'
var deploymentName = 'name'
var deploymentMode = 'Incremental'

resource nestedDeployment 'Microsoft.Resources/deployments@2020-10-01' = {
name: deploymentName
properties: {
mode: deploymentMode
template: {
'$schema': 'https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#'
contentVersion: '1.0.0.0'
resources: [
{
apiVersion: '2022-09-01'
type: 'Microsoft.Resources/tags'
name: 'default'
properties: {
tags: union(tags, {tag1: tag1, tag2: tag2, tag3: join(map(['a'], x => x), ',')})
}
}
]
}
}
}
""";

using (new AssertionScope())
{
var result = CompilationHelper.Compile(withOuterScopeEvaluation);
result.Should().HaveDiagnostics(new[]
{
("no-deployments-resources", DiagnosticLevel.Warning, "Resource 'nestedDeployment' of type 'Microsoft.Resources/deployments@2020-10-01' should instead be declared as a Bicep module."),
});

foreach (var innerScoped in new[] { withExplicitInnerScopeEvaluation, withImplicitInnerScopeEvaluation })
{
result = CompilationHelper.Compile(innerScoped);
result.Should().HaveDiagnostics(new[]
{
("no-deployments-resources", DiagnosticLevel.Warning, "Resource 'nestedDeployment' of type 'Microsoft.Resources/deployments@2020-10-01' should instead be declared as a Bicep module."),
("nested-deployment-template-scoping", DiagnosticLevel.Error, "The symbol \"tags\" is declared in the context of the outer deployment and cannot be accessed by expressions within a nested deployment template that uses inner scoping for expression evaluation."),
("nested-deployment-template-scoping", DiagnosticLevel.Error, "The symbol \"tag1\" is declared in the context of the outer deployment and cannot be accessed by expressions within a nested deployment template that uses inner scoping for expression evaluation."),
("nested-deployment-template-scoping", DiagnosticLevel.Error, "The symbol \"tag2\" is declared in the context of the outer deployment and cannot be accessed by expressions within a nested deployment template that uses inner scoping for expression evaluation."),
});
}
}
}

// https://github.com/Azure/bicep/issues/11883
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -577,7 +577,7 @@ public void ListKeys_Fail()
]
}
}
}
}
",
new string[]
{
Expand Down Expand Up @@ -621,7 +621,7 @@ public void AzListKeys_Fail()
]
}
}
}
}
",
new string[]
{
Expand Down Expand Up @@ -661,7 +661,7 @@ public void FooListKeys_Ignore()
]
}
}
}
}
",
new string[]
{
Expand Down Expand Up @@ -704,7 +704,7 @@ public void IsDeployment_CaseInsensitive_Fail()
}",
new[]
{
"[6] 'nested' is an outer scoped nested deployment that accesses secure string parameters ('stgAccountName'), which could expose their values in deployment history. Either set the deployment's properties.expressionEvaluationOptions.scope to 'inner' or use a Bicep module instead.",
"[6] 'nested' is an outer scoped nested deployment that accesses secure string parameters ('stgAccountName'), which could expose their values in deployment history. Either set the deployment's properties.expressionEvaluationOptions.scope to 'inner' or use a Bicep module instead.",
});
}

Expand Down Expand Up @@ -740,7 +740,7 @@ public void ListKeys_InLoop_Fail()
]
}
}
}]
}]
",
new string[]
{
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using Bicep.Core.CodeAction;
using Bicep.Core.Diagnostics;
using Bicep.Core.Navigation;
using Bicep.Core.Parsing;
using Bicep.Core.Semantics;
using Bicep.Core.Syntax;
using Bicep.Core.TypeSystem;
using Bicep.Core.TypeSystem.Az;
using Bicep.Core.Workspaces;
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;

namespace Bicep.Core.Analyzers.Linter.Rules
{
public sealed class NoSymbolicReferencesInInnerScopedDeploymentResources : LinterRuleBase
{
public new const string Code = "nested-deployment-template-scoping";

public NoSymbolicReferencesInInnerScopedDeploymentResources() : base(
code: Code,
description: CoreResources.NoSymbolicReferencesInInnerScopedDeploymentResourcesDescription,
docUri: new Uri($"https://aka.ms/bicep/linter/{Code}"),
diagnosticLevel: DiagnosticLevel.Error)
{
}

public override string FormatMessage(params object[] values)
=> string.Format(CoreResources.NoSymbolicReferencesInInnerScopedDeploymentResourcesMessageFormat, values);

public override IEnumerable<IDiagnostic> AnalyzeInternal(SemanticModel model, DiagnosticLevel diagnosticLevel)
{
var innerScopedEvaluationIsDefault = model.EmitterSettings.EnableSymbolicNames;
var topLevelSymbols = model.Root.Declarations.ToHashSet();

foreach (var resource in model.DeclaredResources.Where(r => r.IsAzResource))
{
if (!LanguageConstants.ResourceTypeComparer.Equals(resource.TypeReference.FormatType(), AzResourceTypeProvider.ResourceTypeDeployments) ||
resource.Symbol.DeclaringResource.TryGetBody()?.TryGetPropertyByName("properties", StringComparison.OrdinalIgnoreCase)?.Value is not ObjectSyntax propertiesObject ||
propertiesObject.TryGetPropertyByName("template", StringComparison.OrdinalIgnoreCase)?.Value is not SyntaxBase nestedTemplate)
{
continue;
}

bool explicitInnerScope = (propertiesObject.TryGetPropertyByName("expressionEvaluationOptions", StringComparison.OrdinalIgnoreCase)?.Value as ObjectSyntax)
?.TryGetPropertyByName("scope", StringComparison.OrdinalIgnoreCase)?.Value is SyntaxBase explicitScope &&
model.GetTypeInfo(explicitScope) is StringLiteralType folded &&
folded.RawStringValue.Equals("inner", StringComparison.OrdinalIgnoreCase);

if (explicitInnerScope || innerScopedEvaluationIsDefault)
{
foreach (var (symbolReferenced, references) in SymbolicReferenceCollector.CollectSymbolsReferenced(model.Binder, nestedTemplate))
{
if (!topLevelSymbols.Contains(symbolReferenced))
{
continue;
}

foreach (var reference in references)
{
yield return CreateDiagnosticForSpan(diagnosticLevel, reference.Span, symbolReferenced.Name);
}
}
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using Bicep.Core.Syntax;
using Bicep.Core.Syntax.Visitors;
using Bicep.Core.TypeSystem;
using Bicep.Core.TypeSystem.Az;
using System;
using System.Collections.Generic;
using System.Linq;
Expand Down Expand Up @@ -39,6 +40,12 @@ public override string FormatMessage(params object[] values)

override public IEnumerable<IDiagnostic> AnalyzeInternal(SemanticModel model, DiagnosticLevel diagnosticLevel)
{
// outer scoped expression evaluation is blocked in symbolic name templates
if (model.EmitterSettings.EnableSymbolicNames)
{
yield break;
}

foreach (ResourceSymbol resource in model.Root.ResourceDeclarations)
{
if (GetPropertiesIfOuterScopedDeployment(resource) is ObjectSyntax propertiesObject)
Expand Down Expand Up @@ -80,7 +87,7 @@ override public IEnumerable<IDiagnostic> AnalyzeInternal(SemanticModel model, Di
return null;
}

if (!resourceType.TypeReference.FormatType().Equals("microsoft.resources/deployments", LanguageConstants.ResourceTypeComparison))
if (!resourceType.TypeReference.FormatType().Equals(AzResourceTypeProvider.ResourceTypeDeployments, LanguageConstants.ResourceTypeComparison))
{
// Not a deployment resource
return null;
Expand Down
32 changes: 27 additions & 5 deletions src/Bicep.Core/CoreResources.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/Bicep.Core/CoreResources.resx
Original file line number Diff line number Diff line change
Expand Up @@ -485,4 +485,10 @@
<data name="ExperimentalFeatureNames_MicrosoftGraphPreview" xml:space="preserve">
<value>MicrosoftGraph Preview</value>
</data>
<data name="NoSymbolicReferencesInInnerScopedDeploymentResourcesDescription" xml:space="preserve">
<value>Nested deployment resources cannot refer to top-level symbols from within the 'template' property when inner-scoped evaluation is used.</value>
</data>
<data name="NoSymbolicReferencesInInnerScopedDeploymentResourcesMessageFormat" xml:space="preserve">
<value>The symbol "{0}" is declared in the context of the outer deployment and cannot be accessed by expressions within a nested deployment template that uses inner scoping for expression evaluation.</value>
</data>
</root>
Loading

0 comments on commit 5f83b12

Please sign in to comment.