Support setting LocalAddr in peer communication - with e2e tests #17661

flawedmatrix · 2024-03-27T23:08:32Z

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.

k8s-ci-robot · 2024-03-27T23:08:41Z

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.

server/embed/config.go

ahrtr · 2024-03-28T10:15:07Z

/ok-to-test

server/embed/config.go

server/embed/etcd.go

server/etcdmain/help.go

tests/e2e/utils.go

tests/e2e/etcd_config_test.go

server/embed/config_test.go

flawedmatrix · 2024-04-01T22:17:13Z

Thank you @ahrtr for the very thorough review. I've made the suggested changes to my PR.

server/embed/config_test.go

ahrtr · 2024-04-03T15:32:09Z

cc @fuweid @jmhbnz @serathius PTAL

flawedmatrix · 2024-04-03T20:07:17Z

/retest

tests/e2e/etcd_config_test.go

ahrtr · 2024-04-11T18:22:36Z

@vivekpatani @ivanvc @fuweid @jmhbnz @serathius Please take a look at this PR. It resolve a real production issue #17068

jmhbnz

Thanks for working on this @flawedmatrix and @ahrtr. Some questions below.

server/embed/config_test.go

jmhbnz · 2024-04-11T20:20:18Z

server/etcdmain/help.go

@@ -107,6 +107,8 @@ Member:
 Clustering:
  --initial-advertise-peer-urls 'http://localhost:2380'
    List of this member's peer URLs to advertise to the rest of the cluster.
+  --set-member-localaddr 'false'


Note so we don't forget to do follow-up pr for website for this new clustering flag in https://etcd.io/docs/v3.6/op-guide/configuration.

jmhbnz · 2024-04-11T20:24:25Z

server/etcdmain/help.go

@@ -107,6 +107,8 @@ Member:
 Clustering:
  --initial-advertise-peer-urls 'http://localhost:2380'
    List of this member's peer URLs to advertise to the rest of the cluster.
+  --set-member-localaddr 'false'
+    Enable to have etcd use the first specified and non-loopback host from initial-advertise-peer-urls as the local address when communicating with a peer.


Would it make more sense to say local address rather than host? Basing this on the test case above where local IPv4 is prioritised over hostname.

Suggested change

Enable to have etcd use the first specified and non-loopback host from initial-advertise-peer-urls as the local address when communicating with a peer.

Enable to have etcd use the first specified and non-loopback local address from initial-advertise-peer-urls as the local address when communicating with a peer.

Alternatively we might just want to state what the priority order is so users aren't left to figure this out on their own.

I'm not sure if it's just me, but I feel like mentioning etcd in this case is redundant (without addressing the priority order) something like:

Enable using the first specified and non-loopback local address from initial-advertise-peer-urls as the local address when communicating with a peer.

Sounds more straightforward.

ivanvc

Thanks for invoking me. I left a couple of comments. I hope they're relevant ✌️

server/embed/config.go

server/embed/config_test.go

ivanvc · 2024-04-12T05:39:49Z

server/etcdmain/help.go

@@ -107,6 +107,8 @@ Member:
 Clustering:
  --initial-advertise-peer-urls 'http://localhost:2380'
    List of this member's peer URLs to advertise to the rest of the cluster.
+  --set-member-localaddr 'false'
+    Enable to have etcd use the first specified and non-loopback host from initial-advertise-peer-urls as the local address when communicating with a peer.


I'm not sure if it's just me, but I feel like mentioning etcd in this case is redundant (without addressing the priority order) something like:

Enable using the first specified and non-loopback local address from initial-advertise-peer-urls as the local address when communicating with a peer.

Sounds more straightforward.

server/embed/config_test.go

server/embed/config.go

flawedmatrix · 2024-04-12T23:43:25Z

Thanks all for the suggestions. I've made the changes, added more unit test cases and added some logging to InferLocalAddr.

flawedmatrix · 2024-04-13T04:30:19Z

/retest

flawedmatrix · 2024-05-01T22:29:06Z

Hi @ahrtr, I just rebased again to fix the merge conflict. Could you please take a look?

ivanvc · 2024-05-03T02:35:48Z

Hi @flawedmatrix, I suggest squashing the commits in preparation for merging this PR.

flawedmatrix · 2024-05-03T22:25:10Z

Hi @ivanvc, I've squashed the commits that were my contributions. Thanks.

server/embed/config.go

ivanvc

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.

jmhbnz

LGTM - Thanks for your work on this @flawedmatrix

ahrtr · 2024-05-24T09:43:10Z

tests/e2e/etcd_config_test.go

+	// node 0 (127.0.0.1) does not set localaddr, while nodes 1 and nodes 2 (both use host's IP) do.
+	// The other two nodes will reject connections from node 0 warning that node 0's certificate is valid only for
+	// 127.0.0.1, not the host IP, since node 0 will try to connect to the other peers with the host IP
+	// as the client address.
+	// Node 0 will not reject connections from the other nodes since they will
+	// use the host's IP to connect (due to --experimental-set-member-localaddr)


Suggested change

// node 0 (127.0.0.1) does not set localaddr, while nodes 1 and nodes 2 (both use host's IP) do.

// The other two nodes will reject connections from node 0 warning that node 0's certificate is valid only for

// 127.0.0.1, not the host IP, since node 0 will try to connect to the other peers with the host IP

// as the client address.

// Node 0 will not reject connections from the other nodes since they will

// use the host's IP to connect (due to --experimental-set-member-localaddr)

// node 0 (127.0.0.1) does not set `--experimental-set-member-localaddr`,

// while nodes 1 and nodes 2 do.

//

// node 0's peer certificate is signed on 127.0.0.1, but it uses host IP

// (by default) to communicate with peers, so they don't match, accordingly

// other peers will reject connections from node 0.

//

// Both node 1 and node 2's peer certificates are signed on host IP, and

// they also communicate with peers using host IP (explicitly set using

// flag `--experimental-set-member-localaddr`), so there is no issue.

//

// Refer to https://github.com/etcd-io/etcd/issues/17068.

Thanks, I've updated the comment now.

ahrtr · 2024-05-24T09:44:56Z

Overall looks good to me with a minor comment, to ensure the e2e test is easier to understand.

Which sets the LocalAddr to an IP address from --initial-advertise-peer-urls. Also adds e2e test that requires this flag to succeed. Co-authored-by: HighPon <s.shiraki.business@gmail.com> Signed-off-by: Edwin Xie <edwin.xie@broadcom.com>

flawedmatrix · 2024-05-24T18:20:12Z

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.

ahrtr

LGTM

Thanks for the fix.

…s https/unixs Backports a657f06, 22f20a8, 497f1a4 and 3e86af6 from etcd-io#18186. Also backports required test util elements of 4c77726 from etcd-io#17661. Signed-off-by: James Blair <mail@jamesblair.net>

k8s-ci-robot added the needs-ok-to-test label Mar 27, 2024

lubronzhan reviewed Mar 27, 2024

View reviewed changes

server/embed/config.go Outdated Show resolved Hide resolved

flawedmatrix force-pushed the fix/17068 branch 2 times, most recently from 1ddb114 to 8c46359 Compare March 27, 2024 23:58

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Mar 28, 2024

ahrtr reviewed Mar 29, 2024

View reviewed changes

server/embed/config_test.go Outdated Show resolved Hide resolved

flawedmatrix force-pushed the fix/17068 branch from 8c46359 to f71d029 Compare April 1, 2024 22:16

flawedmatrix force-pushed the fix/17068 branch from f71d029 to 71663a3 Compare April 1, 2024 22:20

ahrtr reviewed Apr 3, 2024

View reviewed changes

server/embed/config_test.go Outdated Show resolved Hide resolved

ahrtr reviewed Apr 8, 2024

View reviewed changes

tests/e2e/etcd_config_test.go Show resolved Hide resolved

jmhbnz reviewed Apr 11, 2024

View reviewed changes

ivanvc reviewed Apr 12, 2024

View reviewed changes

fuweid reviewed Apr 12, 2024

View reviewed changes

server/embed/config.go Show resolved Hide resolved

flawedmatrix force-pushed the fix/17068 branch 2 times, most recently from 5a19043 to 4ea35aa Compare April 12, 2024 23:38

flawedmatrix force-pushed the fix/17068 branch 2 times, most recently from d5f26d0 to 39453ca Compare May 1, 2024 20:48

flawedmatrix force-pushed the fix/17068 branch from 39453ca to a028ebf Compare May 3, 2024 21:51

flawedmatrix force-pushed the fix/17068 branch from a028ebf to c2e26d8 Compare May 3, 2024 21:52

ahrtr mentioned this pull request May 9, 2024

Fix Blackhole implementation for e2e tests #17938

Closed

ahrtr reviewed May 23, 2024

View reviewed changes

server/embed/config.go Outdated Show resolved Hide resolved

flawedmatrix force-pushed the fix/17068 branch from c2e26d8 to 482da3f Compare May 23, 2024 18:40

k8s-ci-robot added the area/testing label May 23, 2024

ivanvc approved these changes May 24, 2024

View reviewed changes

jmhbnz approved these changes May 24, 2024

View reviewed changes

ahrtr reviewed May 24, 2024

View reviewed changes

flawedmatrix force-pushed the fix/17068 branch from 482da3f to e536520 Compare May 24, 2024 18:17

k8s-ci-robot added area/documentation github_actions Pull requests that update GitHub Actions code labels May 24, 2024

flawedmatrix force-pushed the fix/17068 branch from e536520 to 4c77726 Compare May 24, 2024 18:19

ahrtr approved these changes May 24, 2024

View reviewed changes

ahrtr merged commit a529268 into etcd-io:main May 24, 2024
43 checks passed

ivanvc mentioned this pull request Jun 6, 2024

Supporting setting LocalAddr in peer communication #17085

Closed

jmhbnz mentioned this pull request Jul 6, 2024

[3.5] Backport fix(server/embed) enforce non-empty client TLS if scheme is https/unixs #18289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support setting LocalAddr in peer communication - with e2e tests #17661

Support setting LocalAddr in peer communication - with e2e tests #17661

flawedmatrix commented Mar 27, 2024

k8s-ci-robot commented Mar 27, 2024

ahrtr commented Mar 28, 2024

flawedmatrix commented Apr 1, 2024

ahrtr commented Apr 3, 2024

flawedmatrix commented Apr 3, 2024

ahrtr commented Apr 11, 2024

jmhbnz left a comment

jmhbnz Apr 11, 2024

jmhbnz Apr 11, 2024

ivanvc Apr 12, 2024

ivanvc left a comment

ivanvc Apr 12, 2024

flawedmatrix commented Apr 12, 2024

flawedmatrix commented Apr 13, 2024

flawedmatrix commented May 1, 2024

ivanvc commented May 3, 2024

flawedmatrix commented May 3, 2024

ivanvc left a comment

jmhbnz left a comment

ahrtr May 24, 2024

flawedmatrix May 24, 2024

ahrtr commented May 24, 2024

flawedmatrix commented May 24, 2024

ahrtr left a comment

	Enable to have etcd use the first specified and non-loopback host from initial-advertise-peer-urls as the local address when communicating with a peer.
	Enable to have etcd use the first specified and non-loopback local address from initial-advertise-peer-urls as the local address when communicating with a peer.

-	// node 0 (127.0.0.1) does not set localaddr, while nodes 1 and nodes 2 (both use host's IP) do.
-	// The other two nodes will reject connections from node 0 warning that node 0's certificate is valid only for
-	// 127.0.0.1, not the host IP, since node 0 will try to connect to the other peers with the host IP
-	// as the client address.
-	// Node 0 will not reject connections from the other nodes since they will
-	// use the host's IP to connect (due to --experimental-set-member-localaddr)
+	// node 0 (127.0.0.1) does not set `--experimental-set-member-localaddr`,
+	// while nodes 1 and nodes 2 do.
+	//
+	// node 0's peer certificate is signed on 127.0.0.1, but it uses host IP
+	// (by default) to communicate with peers, so they don't match, accordingly
+	// other peers will reject connections from node 0.
+	//
+	// Both node 1 and node 2's peer certificates are signed on host IP, and
+	// they also communicate with peers using host IP (explicitly set using
+	// flag `--experimental-set-member-localaddr`), so there is no issue.
+	//
+	// Refer to https://github.com/etcd-io/etcd/issues/17068.

Support setting LocalAddr in peer communication - with e2e tests #17661

Support setting LocalAddr in peer communication - with e2e tests #17661

Conversation

flawedmatrix commented Mar 27, 2024

k8s-ci-robot commented Mar 27, 2024

ahrtr commented Mar 28, 2024

flawedmatrix commented Apr 1, 2024

ahrtr commented Apr 3, 2024

flawedmatrix commented Apr 3, 2024

ahrtr commented Apr 11, 2024

jmhbnz left a comment

Choose a reason for hiding this comment

jmhbnz Apr 11, 2024

Choose a reason for hiding this comment

jmhbnz Apr 11, 2024

Choose a reason for hiding this comment

ivanvc Apr 12, 2024

Choose a reason for hiding this comment

ivanvc left a comment

Choose a reason for hiding this comment

ivanvc Apr 12, 2024

Choose a reason for hiding this comment

flawedmatrix commented Apr 12, 2024

flawedmatrix commented Apr 13, 2024

flawedmatrix commented May 1, 2024

ivanvc commented May 3, 2024

flawedmatrix commented May 3, 2024

ivanvc left a comment

Choose a reason for hiding this comment

jmhbnz left a comment

Choose a reason for hiding this comment

ahrtr May 24, 2024

Choose a reason for hiding this comment

flawedmatrix May 24, 2024

Choose a reason for hiding this comment

ahrtr commented May 24, 2024

flawedmatrix commented May 24, 2024

ahrtr left a comment

Choose a reason for hiding this comment