Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure Azure.AKS.CNISubnetSize rule uses AzureCNI selector #1177

Merged
merged 5 commits into from
Dec 28, 2021
Merged

Ensure Azure.AKS.CNISubnetSize rule uses AzureCNI selector #1177

merged 5 commits into from
Dec 28, 2021

Conversation

ArmaanMcleod
Copy link
Contributor

@ArmaanMcleod ArmaanMcleod commented Dec 27, 2021 •
edited
Loading

PR Summary

Fix #1178

Small fix. Added Azure.AKS.Azure.CNI selector to Azure.AKS.CNISubnetSize rule. Makes sense to make sure this rule just checks CNI clusters.

PR Checklist

  • PR has a meaningful title
  • Summarized changes
  • Change is not breaking
  • This PR is ready to merge and is not Work in Progress
  • Rule changes
    • Unit tests created/ updated
    • Rule documentation created/ updated
    • Link to a filed issue
    • Change log has been updated with change under unreleased section
  • Other code changes
    • Unit tests created/ updated
    • Link to a filed issue
    • Change log has been updated with change under unreleased section

@ArmaanMcleod ArmaanMcleod requested a review from a team as a code owner December 27, 2021 11:45
BernieWhite
BernieWhite reviewed Dec 27, 2021
View reviewed changes
Copy link
Collaborator

@BernieWhite BernieWhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArmaanMcleod Makes sense to me. It's a bug. Let's log this and add a change log so if anyone encounters this issue they know it's been fixed.

@ArmaanMcleod
Copy link
Contributor Author

ArmaanMcleod commented Dec 27, 2021 •
edited
Loading

@ArmaanMcleod Makes sense to me. It's a bug. Let's log this and add a change log so if anyone encounters this issue they know it's been fixed.

@BernieWhite No worries, I can link an issue with this.

I've also noticed Export-AzRuleTemplateData doesn't include Microsoft.ContainerService/managedClusters/agentPools resources like https://github.com/Azure/PSRule.Rules.Azure/blob/main/tests/PSRule.Rules.Azure.Tests/Resources.AKS.json#L448, which this change would affect as the selector only checks for Microsoft.ContainerService/managedClusters. Do you think that would be a problem?

Thats probably why I needed to delete a test in this change. Not sure of a good way to have the selector include Microsoft.ContainerService/managedClusters/agentPools resources, as there is not reliable way to check for Azure CNI, unless we check it has a subnet sub resource or something?

I'm wondering since this rule is only evaluating exported resources, maybe this is fine and we can remove Microsoft.ContainerService/managedClusters/agentPools as a type for this rule.

@ArmaanMcleod ArmaanMcleod changed the title Ensure Azure.AKS.CNISubnetSize uses CNI selector Ensure Azure.AKS.CNISubnetSize uses AzureCNI selector Dec 27, 2021
@ArmaanMcleod ArmaanMcleod changed the title Ensure Azure.AKS.CNISubnetSize uses AzureCNI selector Ensure Azure.AKS.CNISubnetSize rule uses AzureCNI selector Dec 27, 2021
BernieWhite
BernieWhite approved these changes Dec 28, 2021
View reviewed changes
Copy link
Collaborator

@BernieWhite BernieWhite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ArmaanMcleod One cleanup suggestion. Otherwise all good to merge.

src/PSRule.Rules.Azure/rules/Azure.AKS.Rule.ps1 Outdated Show resolved Hide resolved
@BernieWhite
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ArmaanMcleod @BernieWhite
Remove type filter
009c228 
Co-authored-by: Bernie White <bewhite@microsoft.com>
@ArmaanMcleod
Copy link
Contributor Author

@ArmaanMcleod One cleanup suggestion. Otherwise all good to merge.

Thanks @BernieWhite. Have removed the type filter.

@ArmaanMcleod ArmaanMcleod merged commit 76a4aba into Azure:main Dec 28, 2021
@ArmaanMcleod ArmaanMcleod deleted the aks-cni-subnet-size-selector branch December 28, 2021 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BernieWhite BernieWhite BernieWhite approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ensure Azure.AKS.CNISubnetSize rule uses AzureCNI selector
2 participants
@ArmaanMcleod @BernieWhite