Fix Blackhole implementation for e2e tests

[henrybear327]

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

Based on Fu Wei's idea discussed in the issue, we employ the blocking on L7 but without using external tools.

Problem

A peer will
(a) receive traffic from its peers
(b) initiate connections to its peers (via stream and pipeline).

Thus, the current mechanism of only blocking peer traffic via the peer's existing proxy is insufficient, since only scenario (a) is handled, and scenario (b) is not blocked at all.

Solution - main idea

We introduce an "HTTP proxy" for each peer, which will be proxying all the connections initiated from a peer to its peers.

The modified architecture will look something like this:

A -- A's forward proxy ----- B's (currently existing) reverse proxy - B
     ^ newly introduced   ^ in the original codebase

By adding this HTTP proxy, we can block all in and out traffic that is initiated from a peer to others, without having to resort to external tools, such as iptables. It's verified that the blocking of traffic is complete, compared to previous solutions [2][3].

Implementation

The main subtasks are

• set up an environment variable FORWARD_PROXY: please read Fix Blackhole implementation for e2e tests #17938 (comment)
• implement HTTP proxy by extending the existing proxy server code
• implement enable/disable of the HTTP proxy in the e2e test

The result is that for every peer, we will have the arch like this

A -- A's HTTP proxy (connections initiated from A will be forwarded from this proxy)
 |   ^ covers case (b)
 |
 --- A's (currently existing) proxy (advertised to other peers where the connection should come in from)
     ^ covers case (a)

Test

• make gofail-enable && make build && make gofail-disable && \ go test -timeout 60s -run ^TestBlackholeByMockingPartitionLeader$ go.etcd.io/etcd/tests/v3/e2e -v -count=1
• make gofail-enable && make build && make gofail-disable && \ go test -timeout 60s -run ^TestBlackholeByMockingPartitionFollower$ go.etcd.io/etcd/tests/v3/e2e -v -count=1

Known issue

• I run into context deadline exceeded sometimes

    etcd_mix_versions_test.go:175: 
                Error Trace:    /Users/henrybear327/go/src/etcd/tests/e2e/etcd_mix_versions_test.go:175
                                                        /Users/henrybear327/go/src/etcd/tests/e2e/blackhole_test.go:75
                                                        /Users/henrybear327/go/src/etcd/tests/e2e/blackhole_test.go:31
                Error:          Received unexpected error:
                                [/Users/henrybear327/go/src/etcd/bin/etcdctl --endpoints=http://localhost:20006 put key-0 value-0] match not found.  Set EXPECT_DEBUG for more info Errs: [unexpected exit code [1] after running [/Users/henrybear327/go/src/etcd/bin/etcdctl --endpoints=http://localhost:20006 put key-0 value-0]], last lines:
                                {"level":"warn","ts":"2024-05-05T23:02:36.809726+0800","logger":"etcd-client","caller":"v3@v3.6.0-alpha.0/retry_interceptor.go:65","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0x140001ee960/localhost:20006","method":"/etcdserverpb.KV/Put","attempt":0,"error":"rpc error: code = DeadlineExceeded desc = context deadline exceeded"}
                                Error: context deadline exceeded
                                 (expected "OK", got []). Try EXPECT_DEBUG=TRUE
                Test:           TestBlackholeByMockingPartitionLeader
                Messages:       failed to put "key-0", error: [/Users/henrybear327/go/src/etcd/bin/etcdctl --endpoints=http://localhost:20006 put key-0 value-0] match not found.  Set EXPECT_DEBUG for more info Errs: [unexpected exit code [1] after running [/Users/henrybear327/go/src/etcd/bin/etcdctl --endpoints=http://localhost:20006 put key-0 value-0]], last lines:
                                {"level":"warn","ts":"2024-05-05T23:02:36.809726+0800","logger":"etcd-client","caller":"v3@v3.6.0-alpha.0/retry_interceptor.go:65","msg":"retrying of unary invoker failed","target":"etcd-endpoints://0x140001ee960/localhost:20006","method":"/etcdserverpb.KV/Put","attempt":0,"error":"rpc error: code = DeadlineExceeded desc = context deadline exceeded"}
                                Error: context deadline exceeded
                                 (expected "OK", got []). Try EXPECT_DEBUG=TRUE

[References]
[1] issue #17737
[2] PR (V1) https://github.com/henrybear327/etcd/tree/fix/e2e_blackhole
[3] PR (V2) #17891

[k8s-ci-robot]

Hi @henrybear327. 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-sigs/prow repository.

[henrybear327]

/cc @siyuanfoundation @fuweid

[ivanvc]

/retitle Fix Blackhole implementation for e2e tests

[serathius]

Not a networking expert, would like to get some help.

cc @aojea
Can you help review this?

[serathius]

Talked with @aojea, proposed L7 forward proxy is an improvement over the previous faulty approach, however it might not be necessarily what we want. We want to simulate a realistic network disruptions, and there is no way to do that using a L7 proxy. This is currently impossible with current setup where etcd processes communicate via localhost, to modify packets we need them to go through kernel network stack, and that's not possible if they share the same network stack.

Unfortunately setting dedicated network stack this is pretty complicated, @aojea suggest using docker which sets it up for each container, however I'm worried about how much using docker will complicate etcd e2e testing. I would prefer to set up network ourselves, however this makes it less portable. Main benefit of using docker is portability across platforms.

As we won't be able to build such solution anytime soon, I'm ok with still using L7 http proxy in the meantime.

[serathius]

Don't know if we want to support a new environment variable FORWARD_PROXY for proxying.

I know that http.ProxyFromEnvironment(req) will ignore HTTP_PROXY when it URL targets localhost, but what about just changing the LookupEnv to check for HTTP_PROXY and return early?

cc @ahrtr

[henrybear327]

The reason I did it this way is that I don't want to interfere with the default Golang behavior, since this code change will be reflected in the normally built binaries and I am not sure if this will break some other parts of the system or existing user assumption/usage. (my main concern is that HTTP_PROXY or HTTPS_PROXY parsing behavior will change if we use LookupEnv to intercept the HTTP_PROXY or HTTPS_PROXY and if it's set with localhost or 127.0.0.1)

I am too new to judge so I need @ahrtr 's help to confirm this!

[ahrtr]

I suggest that we do not change this very basic common function. Instead, we can extend its functionality on top of it in test only using the Decorator pattern.

Specifically, you can add a similar function NewTransport in test framework, it calls this function, but it also lookup the environment variable "HTTP_PROXY" or "HTTPS_PROXY", and set the proxy no matter it's localhost or not.

[ahrtr]

The other option is to enhance the test framework to enable contributors to use a non localhost IP, so that you can use environment variable "HTTP_PROXY" or "HTTPS_PROXY" to set a proxy directly, and no need to change this common function NewTransport, similar to what #17661 does.

[henrybear327]

I will look into those 2 approaches :) Thank you @ahrtr

[ahrtr]

Clarify on my previous comment #17938 (comment),

We can enable users to inject a customized NewTransport, similar to

etcd/client/pkg/srv/srv.go

Lines 29 to 30 in e1244f1

	lookupSRV = net.LookupSRV // net.DefaultResolver.LookupSRV when ctxs don't conflict
	resolveTCPAddr = net.ResolveTCPAddr

[henrybear327]

Thanks @ahrtr for the code snippets!

After looking into the suggestions, I think it's hard to obtain a non-localhost IP and use it as a proxy during e2e testing, so I went on to look into injecting a customized NewTransport.

My understanding is that the e2e test is not like a unit test where we can change the function pointers during the execution of the unit test dynamically (like #17938 (comment)) because they are in the same module. Instead, the client code is compiled before the e2e test is run. Therefore, some sort of communication channel is required for the e2e test to indicate its need to change the code path during testing (not as easy as swapping lookupSRV), so I use the ENV E2E_TEST_FORWARD_PROXY_IP as a way to pass in the proxy address.

My current attempt is c93e3fc. The idea is kind of borrowed from #17938 (comment), where the HTTP proxy parsing function in use will be determined during the first Transport initialization. I currently can't come up with a cleaner way without having to use a custom ENV though... :( I am not using HTTP_PROXY or HTTPS_PROXY as we would like to not affect the default behavior of http.ProxyFromEnvironment (not parse localhost addresses), and only use a custom NewTransport during e2e testing only when E2E_TEST_FORWARD_PROXY_IP is set.

Not sure if this makes sense to you! Happy to know if there is a better way of implementing this. :)

[henrybear327]

Talked with @aojea, proposed L7 forward proxy is an improvement over the previous faulty approach, however it might not be necessarily what we want. We want to simulate a realistic network disruptions, and there is no way to do that using a L7 proxy. This is currently impossible with current setup where etcd processes communicate via localhost, to modify packets we need them to go through kernel network stack, and that's not possible if they share the same network stack.
Unfortunately setting dedicated network stack this is pretty complicated, @aojea suggest using docker which sets it up for each container, however I'm worried about how much using docker will complicate etcd e2e testing. I would prefer to set up network ourselves, however this makes it less portable. Main benefit of using docker is portability across platforms.
As we won't be able to build such solution anytime soon, I'm ok with still using L7 http proxy in the meantime.

Hey @serathius, thanks for the detailed write-up! I agree that what we want to do eventually, like dropping HTTP packets without having to resort to HTTPS, reordering of the packets, etc., isn't doable on L7. I will finish this PR properly, and then research the potential setup using docker and submit a PoC afterward!

@fuweid and I have discussed my other PR changes #17950 (comment), and I am currently working on implementing it. By doing it this way, we will not need to have both forward and reverse proxies, just 1 forward proxy per peer, and we should be good to go!

WDYT? :)

This PR is forward + reverse proxy, and #17985 (a PoC) is keeping only the forward proxy.

This current PR is easier to blackhole traffic, since for the peer we would like to blackhole, we just need to drop all the traffic to and from its forward and reverse proxies, whereas if we only have the reverse proxy, we need to configure proxies from all peers to perform traffic filtering. But if we only keep the forward proxy, we can reduce the hop count on our e2e testing network and make our set up slimmer and cleaner.

[henrybear327]

@fuweid Based on the discussion here, I implemented the L7 proxy via ServeHTTP function, and now only keeping 1 forward proxy per peer.

I did not have a unified implementation for this PR because the current proxy implementation has "pause/delay accepting connections" features built-in, and these can only be done at L4 instead of L7.

I checked the codebase and "pause accepting connections" isn't used, and "delay accepting connections" is only used during the proxy server's unit test, so in a sense, we can remove the L4 server altogether if we remove those 2 features. But if we ever need those 2 features again, we will need to reimplement the L4 proxy...