Linter rule to use stable VM image

[bhsubra]

Fixes #3972

Adds below linter rule:
Virtual machines shouldn't use preview images. We will check below properties under "imageReference" and fail the linter rule if they contain string "preview":

• offer
• sku
• version

[majastrz]

VisitResourceDeclarationSyntax

We should check that the resource type is a VM, VMSS, or any other relevant type before running the second visitor. Most resource types will not have that property, so we can save some time by short circuiting. Secondly, the imageReference property on non-compute resources could mean something completely different (but still have a similar structure) which could lead to us logging a misleading diagnostic.

[majastrz]

all properties

I don't think we should be checking all the properties called imageReference. We should check only the ones that expect image refs and ignore all others.

Compute types allow open objects in a few places with arbitrary structures - we shouldn't analyze those. Also, the user could have extra imageReferences properties in the wrong places - we should ignore those as well (those would get flagged by the type checks).

[StephenWeatherford]

I'm inclined towards the approach I just took in https://github.com/Azure/bicep/pull/4925/files, where I loop through resources, and look for properties in a specific place in the hierarchy using a modified version of SafeGetPropertyByNameRecursively.

I'm mixed on whether to restrict to specific types only - in the other PR, I did not do that, but did verify a different property that was common among the 3 types I currently know of. New types are sometimes added which we would have to know about in order for the rule to pick up the new types (this happened with the other PR - HybridComputing was added after the rule was written). But we do run the risk of accidentally catching on unexpected resources, esp. with the new extensibility. @majastrz I'd be interested in your thoughts on the other PR, too.)

[majastrz]

+1 on the SafeGetPropertyByNameRecursively approach. It should be more readable and require less code in general. It also has the benefit of only looking at the properties at an explicit JPath. The other benefit is that if the user misplaces a property, we won't look at it.

I don't see a realistic way for us to be 100% perfect, so I think it's just about choosing the least bad option here. In my mind, not covering some brand new resource type with a rule is better than flagging the wrong resource type. There's also a marginal perf benefit as most resource types won't have those properties so checking type upfront prevents us from walking those trees unnecessarily. On top of that it's a trivial fix to add a type to an inclusion list if the new type is discovered/reported. (Can easily be done by the community as well.)

[majastrz]

@anthony-c-martin With extensibility in place, do we need to assert on the "provider" as well as the type when doing these type comparisons?

[StephenWeatherford]

@anthony-c-martin @majastrz I've been wanting to have a discussion about this. In particular, how far do we go in dataflow analysis for checking for specific property values? E.g.:

var v1 = {
  imageReference: {
    offer: 'WindowsServer'
    sku: '2019-Datacenter'
    version: 'latest' 
  }
}
resource vm ... {
  properties: {
    imageReference: v1.imageReference
  }
}

I do think this happens quite a lot, so I don't think we can ignore it but not sure how far it should go. It's pretty easy to check for resolved literal strings in values like I do in AdminUsernameShouldNotBeLiteral:

                    var type = model.GetTypeInfo(syntax.Value);
                    if (type is StringLiteralType stringType) {
                        diagnostics.Add(parent.CreateDiagnosticForSpan(syntax.Value.Span, stringType.RawStringValue));

and even check the RawStringValue against the value we're searching for. I think this approach is better than what Bhavya's doing here and also easier to implement.

Do you guys agree? Is this enough? Is there a better way? Thanks!

[anthony-c-martin]

Can we just use SafeGetPropertyByNameRecursive here for all 3 properties?

[anthony-c-martin]

Sorry - I don't think I explained this properly - what I was wondering was: Is there a reason we can't just replace:

ResourceDeclarationSyntax resourceSyntax = resource.Symbol.DeclaringResource;
if (resourceSyntax.TryGetBody()?.SafeGetPropertyByName("properties") is ObjectPropertySyntax propertiesSyntax
    && propertiesSyntax.Value is ObjectSyntax properties)
{                
    if (properties.SafeGetPropertyByNameRecursive("storageProfile", "imageReference") is ObjectPropertySyntax imageReferenceSyntax)
    {

with:

ResourceDeclarationSyntax resourceSyntax = resource.Symbol.DeclaringResource;
if (resourceSyntax.TryGetBody()?.SafeGetPropertyByNameRecursive("properties", "storageProfile", "imageReference") is {} imageReferenceSyntax)
{

[anthony-c-martin]

[nit] Should we check insensitively (ContainsOrdinalInsensitively("preview"))?

[anthony-c-martin]

Use ImmutableHashSet here (in general - default any shared state to Immutable unless there's a good reason not to)

[anthony-c-martin]

Looks good!