-
Notifications
You must be signed in to change notification settings - Fork 65
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
Sort certificates in bundles to ensure deterministic behaviour #380
Conversation
Hi @jabdoa2. Thanks for your PR. I'm waiting for a cert-manager 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. |
/ok-to-test |
Should be good to go now. I adjusted the certificate order in smoke and integration test. |
@erikgb is there a chance to get this merged & released soonith? we would prefer to forward fix our issue instead of rolling everything back. |
@jabdoa2 Yes, it should be possible to merge and release this improvement soonish. I am going to review it today, but I think we should get #375 merged first. The other PR changes overlapping area of code to this PR, and has been open for a while. So you'd have to be prepared for a rebase. 😉 |
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.
You can merge it as it is then I can re-write code to remediate all remarks as well as introducing |
Thanks @arsenalzp! |
How it will look like: my PR is the first one then #380 is the second one? |
/lgtm We should maybe squash commits before/on merge? |
Signed-off-by: Tim Ramlot <[email protected]> Signed-off-by: Jan Kantert <[email protected]>
I squashed everything into one commit |
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.
Thanks for fixing this serious issue, @jabdoa2! 🚀
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: erikgb 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 |
I spent Friday afternoon manually testing the bug with label selectors. I was able to reproduce it, and I was able to test that your PR fixes the issue. I have documented the testing I did in the Hackmd page PR 380: Manually Testing that ConfigMaps are No Longer Unexpectedly Updated By The ConfigMap Label Selector Bug. By the way, would there be by any chance someone relying the fact that the ordering in |
Order doesn't matter here IMO. The order must be consistent, but I don't think a particular order is required. |
This should be very unlikely for trust bundles. Those are not usually ordered and order should not matter. The story would be different for PEMs containing a certificate chain. For instance, OpenSSL is well known for behaving badly if the first cert is not the leaf cert. Most other libraries will behave well though. Also that is not the usecase for trust-manager. |
fix #310