-
Notifications
You must be signed in to change notification settings - Fork 1.4k
🐛 Fix a panic in conditions.Delete method if the sources condition list is empty #13048
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
🐛 Fix a panic in conditions.Delete method if the sources condition list is empty #13048
Conversation
do not allocate a smaller array for the deleteConditions method because it panics if the existing condition list has length 0. The smaller allocation is also wrong if the condition to be deleted is not part of the conditions list.
|
Welcome @Arakos! |
|
Hi @Arakos. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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. |
|
Thx! /lgtm |
|
/cherry-pick release-1.12 |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions 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. |
|
LGTM label has been added. Git tree hash: 43ad16b9552934b7638aeb4eb0952d2ec740a6ed
|
|
/cherry-pick release-1.11 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cherry-pick release-1.10 |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions 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. |
|
@sbueringer: once the present PR merges, I will cherry-pick it on top of In response to this:
Instructions 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. |
|
@sbueringer: new pull request created: #13053 In response to this:
Instructions 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. |
|
@sbueringer: new pull request created: #13054 In response to this:
Instructions 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. |
|
@sbueringer: #13048 failed to apply on top of branch "release-1.10": In response to this:
Instructions 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. |
What this PR does / why we need it:
The PR fixes a panic in the current conditions.Delete method implementation.
There the new conditions list is allocated with the old condition lists size - 1, but there is no check on the conditions lists size.
So if the old condition list has size 0 the allocation tries to allocate an array of length -1 and panics.
The solution is to just reserve a list of the same size as the current conditions list (as was the case in previous versions of the method).
Note: the smaller allocation might also be wrong if the condition to be deleted is not part of the conditions list, so the performance improvement one might expected with this optimization is neglegible.