Fix connect

[Bolodya1997]

Issue

Existing connect.NewServer() chain elements provides some request, connection translation which should be different for the different cases (ex: NSMgr, interpose NSE, pass through NSE) #552.

Solution

Add translationClientFactory to extract all translation logic from the connect.NewServer(), add translation package with common translation clients translation.NewNSMgrClient()..., andtranslation.Builder to create custom ones.
Add NewCrossConnectClientFactory for the cross connect (interpose NSE, pass through NSE) cases, rework connect.

[edwarnicke]

One other potentially thought here... rather than having translation factories as arguments to connect ... we could have connect store its client connection in metadata and then appropriate subsequent chain elements could handle the translation without having to complicate the connect logic with translation.

[Bolodya1997]

One other potentially thought here... rather than having translation factories as arguments to connect ... we could have connect store its client connection in metadata and then appropriate subsequent chain elements could handle the translation without having to complicate the connect logic with translation.

Yep, it can be very helpful to store some additional info about the server+client connections in s single place, but we need to perform translation logic before the client chain:

connect.NewServer(
	ctx,
	client.NewClientFactory(
		name,
		// What to call onHeal
		addressof.NetworkServiceClient(adapters.NewServerToClient(rv)),
		tokenGenerator,
		// <-- all these chain elements comes after the updatepath, refresh, updatetoken...
		connectioncontextkernel.NewClient(),
		tag.NewClient(ctx, vppConn),
		// mechanisms
		memif.NewClient(vppConn, &lastSocketID),
		kernel.NewClient(vppConn),
		recvfd.NewClient(),
	),
	clientDialOptions...,
),

so if we want to perform some translation, we should do it before the client.NewClientFactory() chain. And it is actually the reason why we need to pass translationFactory to the connection server.

[Bolodya1997]

One other potentially thought here... rather than having translation factories as arguments to connect ... we could have connect store its client connection in metadata and then appropriate subsequent chain elements could handle the translation without having to complicate the connect logic with translation.

There are actually 2 different approaches we can use to implement server request -> client request and client connection -> server connection translations.

Use metadata and stored serverRequest, clientConnection

endpoint := chain.NewNetworkServiceServer(
    ...
    customtranslator.NewRequestServer(), // <-- creates `clientRequest` from request/metadata, stores `request` 
                                         //     as `serverRequest` in metadata, replaces `request` with
                                         //     `clientRequest`
    connection.NewServer( // <-- already uses `clientRequest`, no need for translation
        client.NewFactory(clientChain), // <-- same
    ),
    customtranslator.NewConnectionServer(), // <-- updates `clientRequest` with `conn` (`clientConn`), stores
                                            //     it in metadata, replaces `request` with `serverRequest`
    ...
)

We can't add translation as a part of clientChain because it is processed after all default client chain elements:

sdk/pkg/networkservice/chains/client/client.go

Lines 59 to 70 in 332daa2

	rv = chain.NewNetworkServiceClient(
	append(
	append([]networkservice.NetworkServiceClient{
	authorize.NewClient(),
	updatepath.NewClient(name),
	heal.NewClient(ctx, networkservice.NewMonitorConnectionClient(cc), onHeal),
	refresh.NewClient(ctx),
	}, additionalFunctionality...),
	injectpeer.NewClient(),
	updatetoken.NewClient(tokenGenerator),
	networkservice.NewNetworkServiceClient(cc),
	)...)

so it should be performed in server chain and so we need 2 translation elements:

1. to translate server request before starting processing client chain
2. to translate client connection after the client chain
   Another one thing I really don't like is that NewRequestServer, connection.NewServer and NewConnectionServer are the part of the server chain but they are processing client request.

Use translationFactory

endpoint := chain.NewNetworkServiceServer(
    ...
    connection.NewServer( // <-- uses only `request`
        translation.NewClient, // <-- is inserted before the `clientChain`, replaces `request` with
                               //     `clientRequest`, returns `serverConn` instead of `clientConn`
        client.NewFactory(clientChain), // <-- already uses `clientRequest`, no need for translation
    ),
    ...
)

Here we have only 1 chain element incapsulating all translation logic and so it is only one who is processing both server and client request. No one in the server chain knows about the client request/connection, same for the client chain and the server request/connection.

[denis-tingaikin]

Other things look for me over desined.
But I tihnk this PR can be merged as bug fixing.

I would suggest to solve this with something like: Create two separte client factories:

1. For nsmgr and so on.
2. For forwarders.

Something like

nsmgr.NewClientFactory
forwarder.NewClientFactory

[edwarnicke]

Use translationFactory

endpoint := chain.NewNetworkServiceServer(
    ...
    connection.NewServer( // <-- uses only `request`
        translation.NewClient, // <-- is inserted before the `clientChain`, replaces `request` with
                               //     `clientRequest`, returns `serverConn` instead of `clientConn`
        client.NewFactory(clientChain), // <-- already uses `clientRequest`, no need for translation
    ),
    ...
)

Here we have only 1 chain element incapsulating all translation logic and so it is only one who is processing both server and client request. No one in the server chain knows about the client request/connection, same for the client chain and the server request/connection.

I like this approach very much :)

[denis-tingaikin]

Looks good

[denis-tingaikin]

@Bolodya1997 Could you also update vpp-forwarder and vppagent-forwarder?

[denis-tingaikin]

@edwarnicke Let us know if you have something in mind related to this changes.