Closed BenjaminEngeset closed 3 months ago
Ready for review @BernieWhite. This one was not straight forward.
The reason for not wanting the default
configuration is because they all can exist and by configuring default wrongly, you might get some unexpected behavior. The documentation recommends using these two explicitly, even tho all three can exist at the same time. Let me know what you think about this approach.
Thank you for the excellent feedback and suggestions, @BernieWhite. I have now implemented the requested changes.
Given that our rule specifically targets scenarios where the child resource is nested within the parent resource, will this rule consistently fail for clusters where it’s declared outside the parent? That will at least make it clear for customers explicitly, but might be confusing that the rule still fails after they declare it. This approach seems to be the only viable solution currently anyways.
Thank you for the excellent feedback and suggestions, @BernieWhite. I have now implemented the requested changes.
Given that our rule specifically targets scenarios where the child resource is nested within the parent resource, will this rule consistently fail for clusters where it’s declared outside the parent? That will at least make it clear for customers explicitly, but might be confusing that the rule still fails after they declare it. This approach seems to be the only viable solution currently anyways.
PSRule automatically nests the sub-resource into the parent during expansion so it shouldn't be an issue, if you define a cluster then the expectation is somewhere in the same deployment (although could be a separate module) you also define maintenance config. If IaC will be flagged as an issue.
The case that's PSRule doesn't handle is if you define a cluster in code then separately through a different process you add maintenance config, like deploy if not exists case with Azure Policy. In this case IaC is going to flag an issue with the code, which could be suppressed or excluded.
Thank you for the excellent feedback and suggestions, @BernieWhite. I have now implemented the requested changes. Given that our rule specifically targets scenarios where the child resource is nested within the parent resource, will this rule consistently fail for clusters where it’s declared outside the parent? That will at least make it clear for customers explicitly, but might be confusing that the rule still fails after they declare it. This approach seems to be the only viable solution currently anyways.
PSRule automatically nests the sub-resource into the parent during expansion so it shouldn't be an issue, if you define a cluster then the expectation is somewhere in the same deployment (although could be a separate module) you also define maintenance config. If IaC will be flagged as an issue.
The case that's PSRule doesn't handle is if you define a cluster in code then separately through a different process you add maintenance config, like deploy if not exists case with Azure Policy. In this case IaC is going to flag an issue with the code, which could be suppressed or excluded.
Thanks for the information, @BernieWhite. Given this, I have a broader question about a design pattern we’ve been using for several years. Specifically, I’m curious about why, in certain cases, we provide the child as the value to -TargetType
.
My understanding has been that PSRule wouldn’t correctly handle scenarios where the child is declared outside its parent, hence having to add the child as the value to -TargetType
to cover these scenarios. I now understand that PSRule will handle that as long it's within the same deployment and nest them accordingly, but what about the following underneath:
For reference, here’s an example:
Is using -TargetType Microsoft.ContainerService/managedClusters/agentPools
for the following scenarios as these I have here?
The AKS cluster declared in another deployment, the pool in another deployment, e.g
1.
resource aks 'Microsoft.ContainerService/managedClusters@2024-03-02-preview' existing = {
name: 'cluster'
}
resource pool 'Microsoft.ContainerService/managedClusters/agentPools@2022-07-01' = {
parent: aks
name: poolName
properties: {
count: minCount
vmSize: vmSize
osDiskSizeGB: 60
osType: 'Linux'
osDiskType: 'Ephemeral'
maxPods: 50
mode: 'User'
}
}
2.
resource aks 'Microsoft.ContainerService/managedClusters@2024-03-02-preview' existing = {
name: 'cluster'
scope: resourceGroup(exampleRG)
}
resource pool 'Microsoft.ContainerService/managedClusters/agentPools@2022-07-01' = {
parent: aks
name: poolName
properties: {
count: minCount
vmSize: vmSize
osDiskSizeGB: 60
osType: 'Linux'
osDiskType: 'Ephemeral'
maxPods: 50
mode: 'User'
}
}
3.
resource aks 'Microsoft.ContainerService/managedClusters@2024-03-02-preview'' existing = {
name: 'cluster'
scope: resourceGroup(exampleRG)
}
module pool '../create-pool/main.bicep' = {
name: 'deploy'
scope: 'aks'
}
Could you clarify this design?
Thanks @BenjaminEngeset! All good to merge if you're happy with this explanation.
Also, I added the in-flight export to this PR.
Thank you! All good.
Azure.AKS.EphemeralOSDisk
Thanks for the question @BenjaminEngeset.
For the rule Azure.AKS.MaintenanceWindow
we are checking the state of Microsoft.ContainerService/managedClusters
by checking it's sub-resources.
For the rule Azure.AKS.EphemeralOSDisk
we are checking the state of Microsoft.ContainerService/managedClusters/agentPools
which probably (most of the time) is in the cluster deployment.
So when will agentPools not be automatically nested under a parent resource by PSRule?
You're existing
resource is the main case :) an existing
resource is not a deployment of a Microsoft.ContainerService/managedClusters
resource. It is a reference to a cluster that was deployed at some point previously.
The deployment for this doesn't have to be in the same deployment where the agent pool is created. It could for all intents be manually deployed by someone clicking in the portal, as long as it exists at the time the agent pool deployment references it.
Because there is no cluster defined, PSRule won't nest the sub-resource, it will be left as is. That said, the reason this happens is not because you used an existing
reference, it's because in the deployment the cluster is not defined at all. If you used existing
in a child module but another modules part of the deployment defined the cluster it would be nested.
Azure.AKS.EphemeralOSDisk
Thanks for the question @BenjaminEngeset.
For the rule
Azure.AKS.MaintenanceWindow
we are checking the state ofMicrosoft.ContainerService/managedClusters
by checking it's sub-resources.
- If the sub-resources exist in the designed configuration it passes.
- If the sub-resources don't exist then it fails because the Azure default don't configure these sub-resources, and as a result the cluster is not in the desired state.
For the rule
Azure.AKS.EphemeralOSDisk
we are checking the state ofMicrosoft.ContainerService/managedClusters/agentPools
which probably (most of the time) is in the cluster deployment.
- If the agent pools are in the desired state they pass.
- If no agent pools exist, the rule still passes.
So when will agentPools not be automatically nested under a parent resource by PSRule?
You're
existing
resource is the main case :) anexisting
resource is not a deployment of aMicrosoft.ContainerService/managedClusters
resource. It is a reference to a cluster that was deployed at some point previously.The deployment for this doesn't have to be in the same deployment where the agent pool is created. It could for all intents be manually deployed by someone clicking in the portal, as long as it exists at the time the agent pool deployment references it.
Because there is no cluster defined, PSRule won't nest the sub-resource, it will be left as is. That said, the reason this happens is not because you used an
existing
reference, it's because in the deployment the cluster is not defined at all. If you usedexisting
in a child module but another modules part of the deployment defined the cluster it would be nested.
Thank you for taking your time to give this explanation, @BernieWhite. Truly grateful for that. It makes sense.
PR Summary
Fixes #2444
Added Azure.AKS.MaintenanceWindow.
PR Checklist