-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Support multiple values for allowed client and peer TLS identities #18015
base: main
Are you sure you want to change the base?
Conversation
Hi @lhy1024. Thanks for your PR. I'm waiting for a etcd-io 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. |
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.
Hi @lhy1024 - Thanks for proposing this. It looks like some of the commits are not signed. Can you please take a look and ensure all commits are signed so the developer certificate of origin check passes?
In this case it may involve changing author or re-doing some of the commits to ensure you can sign them off.
/ok-to-test |
/test pull-etcd-e2e-amd64 |
/retest-required |
/retest |
/test pull-etcd-e2e-amd64 |
/test pull-etcd-unit-test |
/retest-required |
@jmhbnz PTAL. BTW, could this pr be picked to 3.5 or 3.4? |
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 resurrecting this @lhy1024. I think it would be helpful if you could expand on your reasons/motivations for wanting this in the pr description so the why is clear.
Overall I think the changes themselves seem reasonable, besides the potential breaking change decision mentioned below. However to set expectations given this is TLS we need wider careful review which can take a while.
cc @ahrtr, @serathius to please also take a look at this.
I am part of the team at PingCAP, a company dedicated to open-source database solutions. Our database comprises three core components: TiDB, TiKV, and PD. Currently, we are working to enhance our database's capabilities by supporting multiple CNs for SSL/TLS certificates, which is crucial for many of our users' security configurations. However, we've encountered a limitation with PD, which relies on an embedded etcd. Unfortunately, etcd does not currently support multiple CNs, which has prompted the need for this pull request. By merging this PR, we aim to overcome this limitation, thereby aligning TiDB with the security flexibility required by modern distributed systems. Additionally, I noticed that Datadog, another prominent player in the industry, has also implemented a similar feature by picking old PRs for their fork. This observation suggests that there is a broader demand for such a feature, indicating its potential utility and impact within the community. Given the nature of this update and its implications on TLS configurations, I understand and appreciate the need for a thorough and careful review process. I look forward to the feedback from the community and thank you in advance for your careful consideration. |
PTAL @jmhbnz |
Overall looks good with a couple of minor comments. Thanks |
Please squash the commits into one commit. |
Support multiple values for allowed client and peer TLS identities Signed-off-by: lhy1024 <admin@liudos.us> fix test Signed-off-by: lhy1024 <admin@liudos.us> keep compatible Signed-off-by: lhy1024 <admin@liudos.us> address comments Signed-off-by: lhy1024 <admin@liudos.us>
/test pull-etcd-unit-test |
/test pull-etcd-integration-1-cpu-amd64 |
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.
LGTM
Please also add a changelog item for 3.6, in this PR or in a separate PR.
OK, I will do it in another PR. BTW, could this PR be picked to 3.5 or other release? |
I have no objection on it. |
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.
LGTM - Nice work @lhy1024
Thank you for reviewing the changes. Could you please let me know when this PR might be merged? Is there anything else I need to do to speed up this process? |
cc @serathius for additional review. |
Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.
From #13460
We also need the similiar features, and hope etcd can support multiple values for allowed client. Ref tikv/pd#5134
Fixes #11728