Rework client, connect, to simplify and prepare for simpler healing.

[edwarnicke]

Previously we had a single 'client' per grpc.ClientConnInterface.
This created a lot of complexity, particularly in connect.

This rework of client moves to a single 'client' that can handle
multiple grpc.ClientConnInterfaces.

This vastly simplified both the client chain and connect chain.

Finally, heal has been temporarily removed in preparation for a rework
of heal focused on back propogating Connection 'DOWN' events via
monitor. The net net at the end of the process should be:

1. Refresh is only originated by actual clients, not the 'client' part
   of passthrough NSEs
2. Heal will only be originated by actual clients, not the 'client' part
   pf passthrough NSEs.

Signed-off-by: Ed Warnicke [email protected]

Description

Issue link

How Has This Been Tested?

• Added unit testing to cover
• Tested manually
• Tested by integration testing
• Have not tested

Types of changes

• Bug fix
• New functionallity
• Documentation
• Refactoring
• CI

[edwarnicke]

@Bolodya1997 I removed this line because without it

sdk/pkg/networkservice/chains/nsmgr/suite_test.go

Lines 310 to 356 in 07ced33

	func (s *nsmgrSuite) Test_ConnectToDeadNSEUsecase() {
	t := s.T()
	ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
	defer cancel()
	nsReg, err := s.nsRegistryClient.Register(ctx, defaultRegistryService())
	require.NoError(t, err)
	nseReg := defaultRegistryEndpoint(nsReg.Name)
	counter := new(count.Server)
	nse := s.domain.Nodes[0].NewEndpoint(ctx, nseReg, sandbox.GenerateTestToken, counter)
	nsc := s.domain.Nodes[0].NewClient(ctx, sandbox.GenerateTestToken)
	request := defaultRequest(nsReg.Name)
	reqCtx, reqCancel := context.WithTimeout(ctx, time.Second)
	defer reqCancel()
	conn, err := nsc.Request(reqCtx, request.Clone())
	require.NoError(t, err)
	require.NotNil(t, conn)
	require.Equal(t, 1, counter.Requests())
	require.Equal(t, 5, len(conn.Path.PathSegments))
	nse.Cancel()
	// Simulate refresh from client
	refreshRequest := request.Clone()
	refreshRequest.Connection = conn.Clone()
	refreshReqCtx, refreshReqCancel := context.WithTimeout(ctx, time.Second)
	defer refreshReqCancel()
	_, err = nsc.Request(refreshReqCtx, refreshRequest)
	require.Error(t, err)
	require.NoError(t, reqCtx.Err())
	// Close
	_, _ = nsc.Close(ctx, conn)
	// Endpoint unregister
	_, err = s.domain.Nodes[0].NSMgr.NetworkServiceEndpointRegistryServer().Unregister(ctx, nseReg)
	require.NoError(t, err)
	}

fails. In digging into it, it was failing because we had WaitForReady set to true... no idea why it didn't fail before. Any insights? Anything I missed here?

[denis-tingaikin]

What the error do you see in Test_ConnectToDeadNSEUsecase with grpc.WaitForReady(true) ?

[Bolodya1997]

I suppose that there are some reasons why we may want to use WaitForReady flag, but I am not actually sure because we most probably had started using it before I have started working on NSM :)
There was an issue with Request hanging in such case and so it is the reason why do we have such test.
We solved this issue in connect several times with several approaches during the heal development. Currently connect uses heal client (or some other monitoring client) to understand that remote side is dead and so close the gRPC connection to prevent Request hanging with WaitForReady flag set.

[Bolodya1997]

@edwarnicke
Any update for this?

[edwarnicke]

I've restored the grpc.WaitForReady(true)

[denis-tingaikin]

I like parts with simplifications related to dial/clientconn, but it this PR also removes heal. So it seems to me to merge this PR we also need to do one of these options:

1. Remove/Disable all heal tests.
2. Wait for heal rework.

[denis-tingaikin]

Why do we need to remove heal tests?

[edwarnicke]

Because we don't have the heal replacement yet :) Will revive them when that comes in.

[denis-tingaikin]

I like idea on adding golang examples,
Do we need to add some asserts and outputs?

[edwarnicke]

That would make it more of a test wouldn't it?

[denis-tingaikin]

These tests cover many corner cases. Can we keep them?

[Bolodya1997]

I totally agree with Denis, most of our connect tests have been motivated by some painful issues. Removing them will most probably lead us back to the old problems.

[edwarnicke]

@Bolodya1997 Added the tests back... and yes, they caught some issues :)

[Bolodya1997]

I have added few missed goleak checks to connect tests (#1087), please can you rebase your tests?

[edwarnicke]

Done

[denis-tingaikin]

What the error do you see in Test_ConnectToDeadNSEUsecase with grpc.WaitForReady(true) ?

[Bolodya1997]

What if we have different token timeout for the different applications?

NSC(1h) -> NSMgr(30m) -> Forwarder(15m) -> NSMgr(30m) -> NSE

If we use such option on NSMgr, Forwarder we should probably compute the least token timeout in refresh on NSC to become sure that there will be no timeouts in the chain.

[Bolodya1997]

Probably it can be some rare case for the local scenario, because most probably all of the applications share the same spire server with same SVID TTL for all of them, but it also can be an interdomain scenario with different spire federations and so different SVID TTLs for local and remote applications.

[edwarnicke]

@Bolodya1997 You are completely correct we need to watch out for this. We could do as you suggested and compute that in refresh. I'm tempted though to do it in updatetoken by clamping down expiretime to the smallest in the path on the return direction of the path.