etcd
etcd copied to clipboard
etcd-io
Support setting LocalAddr in peer communication - with e2e tests
This PR builds upon #17085 in addressing issue #17068.
The e2e test added includes a positive test that --set-member-localaddr is needed for an etcd node to come up, and a test that without it, etcd will fail to connect to peers due to the certificate being issued for a different IP.
I did have to add in some cert generation to create certificates for the host IP where the test is running.
There is an implementation detail here that --set-member-localaddr will set the LocalAddr only if there is a specified non-loopback IP address in the --initial-advertise-peer-urls list. But I don't know if this is too restrictive.
Hi @flawedmatrix. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test label.
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.
Thank you @ahrtr for the very thorough review. I've made the suggested changes to my PR.
@vivekpatani @ivanvc @fuweid @jmhbnz @serathius Please take a look at this PR. It resolve a real production issue https://github.com/etcd-io/etcd/issues/17068
Thanks all for the suggestions. I've made the changes, added more unit test cases and added some logging to InferLocalAddr.
Hi @ahrtr, I just rebased again to fix the merge conflict. Could you please take a look?
Hi @flawedmatrix, I suggest squashing the commits in preparation for merging this PR.
Hi @ivanvc, I've squashed the commits that were my contributions. Thanks.
Overall looks good to me with a minor comment, to ensure the e2e test is easier to understand.
LGTM. If we are considering backporting this PR, I think it will be easier if you squash the commits into one. You can still co-author the commit with the original contributor (refer to https://docs.github.com/en/pull-requests/committing-changes-to-your-project/creating-and-editing-commits/creating-a-commit-with-multiple-authors). Thanks, @flawedmatrix.
Thanks @ivanvc. I didn't initially intend on backporting this change, but to future-proof this PR I've decided to squash the commits anyways.