fix: include priority 4096 in managed security rule allocation range - #10815
fix: include priority 4096 in managed security rule allocation range#10815pujitha24 wants to merge 1 commit into
Conversation
Motivation: IsManagedSecurityRule (pkg/provider/securitygroup/securityrule.go) treats the managed priority range as [500, 4096], inclusive of the maximum priority 4096. However, RuleHelper.nextRulePriority (pkg/provider/securitygroup/securitygroup.go), which allocates priorities for rules the cloud-provider creates, used an exclusive upper bound and could only ever assign priorities in [500, 4095] in both allocation directions (from start and from end). Azure's valid NSG rule priority range is 100-4096, so priority 4096 is a legitimate allocatable value; excluding it from allocation while still treating it as "managed" is an inconsistency between the two functions. Approach: Rewrite nextRulePriority's loop to use inclusive bounds on both ends instead of the previous exclusive-bounds trick (init != end), so it can now allocate priority 4096 for both rulePriorityPreferFromStart and rulePriorityPreferFromEnd, matching IsManagedSecurityRule's range. Validation: - go build ./... passes. - Added TestRuleHelper_NextRulePriority_IncludesMaximumPriority in pkg/provider/securitygroup/securityrule_test.go, which fills every priority except 4096 and asserts nextRulePriority can still find it in both directions. This test fails on the pre-fix code (returns "security rule priority exhausted") and passes after the fix. - go test ./pkg/provider/securitygroup/... passes, including the updated TestSecurityGroupHelper_AddRuleForDenyAll, whose expected priority for a deny-all rule allocated into an otherwise-empty managed range changes from the old (buggy) 4095 to the correct 4096. - golangci-lint run ./pkg/provider/securitygroup/... shows only pre-existing goconst warnings in the test file, unchanged from master. - This is a narrow allocation off-by-one affecting only the single boundary priority 4096; it does not change behavior for any existing rule or any priority below 4096, and is not a crash or data-loss issue. Report: kubernetes-sigs#9906 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pujitha24 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @pujitha24. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/assign @nilo19 Would appreciate a review when you have a chance. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
IsManagedSecurityRule(pkg/provider/securitygroup/securityrule.go) treats themanaged security rule priority range as
[500, 4096], inclusive of the maximumpriority 4096. However,
RuleHelper.nextRulePriority(pkg/provider/securitygroup/securitygroup.go), which allocates priorities for
rules the cloud-provider creates, used an exclusive upper bound and could only
ever assign priorities in
[500, 4095]in both allocation directions (fromstart and from end). Azure's valid NSG rule priority range is 100-4096, so
priority 4096 is a legitimate allocatable value; excluding it from allocation
while still treating it as "managed" was an inconsistency between the two
functions.
This PR rewrites
nextRulePriority's loop to use inclusive bounds on bothends, so it can now allocate priority 4096, matching
IsManagedSecurityRule'srange in both allocation directions.
This is a narrow allocation off-by-one affecting only the single boundary
priority 4096; it does not change behavior for any existing rule or any
priority below 4096, and is not a crash or data-loss issue.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Validation performed locally:
go build ./...passes.TestRuleHelper_NextRulePriority_IncludesMaximumPriorityinpkg/provider/securitygroup/securityrule_test.go, which fills every priority
except 4096 and asserts
nextRulePrioritycan still find it in bothdirections. This test fails on the pre-fix code (returns "security rule
priority exhausted") and passes after the fix.
go test ./pkg/provider/securitygroup/...passes, including the updatedTestSecurityGroupHelper_AddRuleForDenyAll, whose expected priority for adeny-all rule allocated into an otherwise-empty managed range changes from
the old (buggy) 4095 to the correct 4096.
golangci-lint run ./pkg/provider/securitygroup/...shows onlypre-existing
goconstwarnings in the test file, unchanged from master(verified via
git stash).Note:
pkg/provider/azure_standard.gohas agetNextAvailablePriorityfunction with the same exclusive-upper-bound pattern, but it appears to be
unreferenced outside its own unit test (dead code), so it was left out of
this narrowly-scoped fix.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
Fixes #9906