Go-based custom dataplane with the new control plane APIs

[praveingk]

Initial basic go dataplane which is integrated with the new control plane xds and APIs.
Based on #37

[orozery]

First of all, great work!
Having all of this work in a few days is very impressive!

Some general comments:

• Each commit should be buildable and testable on its own. In this PR you have commits which depend on future commits.
• Having both go-dataplane and dataplane as well as cl-dataplane is confusing to me. We can keep dataplane for the old dataplane which will be deprecated once we switch to xDS. But I would change the new xDS go dataplane to be co-located with the envoy one (cl-dataplane). Simply add a new CLI flag which will allow choosing between the two.

nits:

• I advise we use a uniform commit message format. < component >: < Short description of change >/n/n< Long description of change >/n/nSigned-off-by: Your Name your@email.com
• Many error logging can be removed since they are redundant.
• IMO Connection-level logging should be debug instead of info.
• Look for extra spaces in strings and log lines missing an ending ..

Since this is a relatively big PR, I didn't thoroughly reviewed every line, but instead did a quick-pass.
I plan to do a more through pass when this gets close to be merged.

[orozery]

Suggested change

	func (f *forwarder) start() {
	func (f *forwarder) run() error {

I would propagate errors from the spawned go-routines.

[praveingk]

There are two errors from the dataplane forwarder. The errors might not be needed downstream since closure of one socket by the application results in an error.

[orozery]

You should make sure that if one go-routine fails, the other should also quit.

[praveingk]

Yes, just verified that closeConnections should be called within the the routines so that the functions quit and properly terminate the connection. What I meant was that the error from the two routines doesn't need to be propagated since any normal closure of client connection will eventually lead to an error in connection due to closed socket.

[praveingk]

First of all, great work! Having all of this work in a few days is very impressive!

Some general comments:

• Each commit should be buildable and testable on its own. In this PR you have commits which depend on future commits.
• Having both go-dataplane and dataplane as well as cl-dataplane is confusing to me. We can keep dataplane for the old dataplane which will be deprecated once we switch to xDS. But I would change the new xDS go dataplane to be co-located with the envoy one (cl-dataplane). Simply add a new CLI flag which will allow choosing between the two.

nits:

• I advise we use a uniform commit message format. < component >: < Short description of change >/n/n< Long description of change >/n/nSigned-off-by: Your Name your@email.com
• Many error logging can be removed since they are redundant.
• IMO Connection-level logging should be debug instead of info.
• Look for extra spaces in strings and log lines missing an ending ..

Since this is a relatively big PR, I didn't thoroughly reviewed every line, but instead did a quick-pass. I plan to do a more through pass when this gets close to be merged.

Thanks Or for the detailed comments. I started the changes from your repo. Hence, the commits initially were in a bulk :) .
I will incorporate the comments over the next days.

[praveingk]

The tests work for me. @kfirtoledo @orozery : Can you please check if the e2e test configuration is correct?

+ kubectl exec -it gwctl -- gwctl create import --name foo --host bla --port 9999
Imported service created successfully
+ kubectl exec -it gwctl -- gwctl create binding --import foo --peer peer1
Binding created successfully
++ kubectl exec -it gwctl -- /bin/bash -c 'gwctl get import --name foo | jq '\''.Status.Listener.Port'\'' | tr -d '\''\n'\'''
+ PORT=41089
+ kubectl expose deployment cl-dataplane --name=bla --port=9999 --target-port=41089
service/bla exposed
+ kubectl exec -it gwctl -- timeout 30 sh -c 'until nc -z $0 $1; do sleep 0.1; done' bla 9999
+ kubectl exec -it gwctl -- timeout 30 sh -c 'until nc -z $0 $1; do sleep 0.1; done' gwctl 1234
+ kubectl exec -it gwctl -- iperf3 -c bla -p 9999 -t 1
Connecting to host bla, port 9999
[  5] local 10.244.0.7 port 34648 connected to 10.96.46.4 port 9999
[ ID] Interval           Transfer     Bitrate         Retr  Cwnd
[  5]   0.00-1.00   sec   466 MBytes  3.91 Gbits/sec    0   1.07 MBytes       
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval           Transfer     Bitrate         Retr
[  5]   0.00-1.00   sec   466 MBytes  3.91 Gbits/sec    0             sender
[  5]   0.00-1.01   sec   461 MBytes  3.82 Gbits/sec                  receiver

iperf Done.

[praveingk]

@orozery I have addressed most of the comments. Would you mind if I do the dataplane + controller reorg as part of another PR? I just want to make sure the old dataplane code is removed before I make the changes.

[elevran]

@praveingk please take a look at my last commit intended to make the linting pass.

While fixing the double import of logrus is reasonable, I'm less sure of the other two changes:

• fetcher.go:93:17: newFetcher - ctx is unused: seems more reasonable to derive in the context in the function from the parameter than to keep the linter happy by setting the context parameter to _
• client/xds.go:28:32: (*XDSClient).runFetcher - f is unused: the passed in fetcher is unused and the function always calls newFetcher in it. There is no point in passing in a fetcher in that case.

Please consider if the commit makes the right changes or revert/amend appropriately

[praveingk]

@elevran Thanks for noticing the ctx and logger. I have fixed it to use the ctx returned and to use logger available in the fetcher in this commit