embedetcd: Allow passing in a *tls.Config object for the embedded ETCD Server #17130
Conversation
|
Hi @RaphSku. 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/test-infra repository. |
|
Has there been any movement on this? I'd very much like to see this be merged |
I'm on it, just had not much time at hand during new year transition but I'm picking up again, hope I will submit a valid PR in approx. 2 weeks. |
|
Oh that's totally fine! Thank you so much for your work, it's geniuely very helpful. |
|
Hey everyone, I needed to refactor and restructure a lot of the code in the embed code. Its passing the tests but I will probably still need to add a few tests, at least another integration test but I wanted to get feedback from you all whether this is going into the right direction or if I am over-engineering it. Thank you all a lot! |
|
I unfortunately dont have the ability to test this right at this second. But I am pinging this thread as I very much want this to be part of the etcd project as it will remove a lot of projects using the antipattern of generating 10 year long certs and enables integration with acme. |
|
cc @awadmhamad |
|
Discussed during sig-etcd triage meeting, @RaphSku do you have capacity to continue this work? If so can you please rebase this pr. Thanks |
|
@jmhbnz I have rebased the PR. Thank you for asking. Yes, I have capacity to work on it. - I still have to fix a few errors that I have introduced by rebasing |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@jmhbnz I have fixed the rebase changes. The tests are green again. But as I said, would be good if someone could take a look and point me to the right direction. |
|
/ok-to-test |
only allow transport.TLSInfo but also tls.Config Fields CustomClientTLSConfig and CustomPeerTLSConfig added to embed.Config to allow for the injection of programmatically generated TLS configurations. Therefore, the above mentioned functions were refactored to allow the usage of both. Also, two unit tests were being added to config_test.go. Signed-off-by: Raphael Skuza <[email protected]>
…D server Fields CustomClientTLSConfig and CustomPeerTLSConfig added to embed. Config to allow for the injection of programmatically generated TLS configurations. Therefore, refactoring of the embed ETCD code is necessary. Signed-off-by: Raphael Skuza <[email protected]>
…D server Fields CustomClientTLSConfig and CustomPeerTLSConfig added to embed Config to allow for the injection of programmatically generated TLS configuration. Added a new interface called tlsConfigConstraint that allows in to pass tls.Config and transport.TLSInfo such that we do not have to change the function signature. Signed-off-by: Raphael Skuza <[email protected]>
…D server Added filterOutInsecureCipherSuitesByID function to filter out all insecure ciphers that would be passed to tls.Config in updateCipherSuites function. Signed-off-by: Raphael Skuza <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: RaphSku The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…D server Removed the update of TLS Cipher Suites if tls Config is passed since tls CipherSuites are secure by default if tls.Config.CipherSuites is nil. Signed-off-by: Raphael Skuza <[email protected]>
|
/retest |
1 similar comment
|
/retest |
|
@jmhbnz All checks have passed, is there someone available that would have time to review this PR? |
|
PR needs rebase. 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. |
|
Is anyone able to give this a look at merge it? |
The idea is that not only paths to certificate file paths should be possible to define in
*embed.Config.ClientTLSInfoand*embed.Config.PeerTLSInfobut also*tls.Configwhere the certificates could be generated programmatically.For more details, see the issue: #16339