-
Notifications
You must be signed in to change notification settings - Fork 742
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
Linter rule to use stable VM image #4883
Conversation
this.parent = parent; | ||
} | ||
|
||
public override void VisitResourceDeclarationSyntax(ResourceDeclarationSyntax syntax) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
public override void VisitObjectPropertySyntax(ObjectPropertySyntax syntax) | ||
{ | ||
string? propertyName = syntax.TryGetKeyText(); | ||
// Check all properties with the name 'imageReference', regardless of casing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
{ | ||
AddDiagnosticsIfImageReferencePropertiesContainPreview(objectSyntax); | ||
} | ||
else if (syntax.Value is VariableAccessSyntax && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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!
foreach (ResourceMetadata resource in model.AllResources) | ||
{ | ||
ResourceDeclarationSyntax resourceSyntax = resource.Symbol.DeclaringResource; | ||
if (resourceSyntax.TryGetBody()?.SafeGetPropertyByName("properties") is ObjectPropertySyntax propertiesSyntax |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just use SafeGetPropertyByNameRecursive
here for all 3 properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
{
if (objectSyntax.SafeGetPropertyByName(property) is ObjectPropertySyntax objectPropertySyntax && | ||
objectPropertySyntax.Value is StringSyntax valueSyntax && | ||
valueSyntax.TryGetLiteralValue() is string value && | ||
value.Contains("preview")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Should we check insensitively (ContainsOrdinalInsensitively("preview")
)?
{ | ||
public new const string Code = "use-stable-vm-image"; | ||
|
||
private readonly HashSet<string> imageReferenceProperties = new HashSet<string> { "offer", "sku", "version" }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ImmutableHashSet
here (in general - default any shared state to Immutable
unless there's a good reason not to)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
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":