konnectivity-client: ensure Close() is called for grpc.ClientConn in CreateSingleUseGrpcTunnel

[andrewsykim]

As part of the on-going investigation in #276.

If grpcClient.Proxy(ctx) from CreateSingleUseGrpcTunnel errors, then we never call Close() on the client connection.

Signed-off-by: Andrew Sy Kim andrewsy@google.com

[andrewsykim]

I removed this test to avoid changing our go modules for now, but here's the test that reproduces the goroutine leak without the fix:

func TestCreateSingleUseGrpcTunnel_NoLeakOnFailure(t *testing.T) {
    defer goleak.VerifyNone(t, goleak.IgnoreCurrent())
    
     _, err := CreateSingleUseGrpcTunnel(context.Background(), "127.0.0.1:12345", grpc.WithInsecure())
    if err == nil {
        t.Fatal("expected error when calling CreateSingleUseGrpcTunnel")
    }
}

$ go test ./...
I0223 02:03:46.766980   27663 client.go:104] "stream read failure"
I0223 02:03:46.767325   27663 client.go:104] "stream read failure"
--- FAIL: TestCreateSingleUseGrpcTunnel_NoLeakOnFailure (0.45s)
    leaks.go:78: found unexpected goroutines:
        [Goroutine 25 in state select, with google.golang.org/grpc.(*ccBalancerWrapper).watcher on top of the stack:
        goroutine 25 [select]:
        google.golang.org/grpc.(*ccBalancerWrapper).watcher(0xc0001ae4c0)
                /....pkg/mod/google.golang.org/grpc@v1.27.1/balancer_conn_wrappers.go:69 +0x8f
        created by google.golang.org/grpc.newCCBalancerWrapper
                /.../pkg/mod/google.golang.org/grpc@v1.27.1/balancer_conn_wrappers.go:60 +0x1ca
        
         Goroutine 26 in state select, with google.golang.org/grpc.(*addrConn).resetTransport on top of the stack:
        goroutine 26 [select]:
        google.golang.org/grpc.(*addrConn).resetTransport(0xc0001fe000)
                /.../pkg/mod/google.golang.org/grpc@v1.27.1/clientconn.go:1149 +0x5f7
        created by google.golang.org/grpc.(*addrConn).connect
                /.../pkg/mod/google.golang.org/grpc@v1.27.1/clientconn.go:815 +0x145
        ]
FAIL
FAIL    sigs.k8s.io/apiserver-network-proxy/konnectivity-client/pkg/client      0.496s
FAIL

[andrewsykim]

^ consistent with pprof data collected here kubernetes/kubernetes#108215 (comment)

[rata]

@andrewsykim IMHO it will be great to add this test back. We had a bunch of fixes in the konnectivity-client and each fix there means we need to backport it to k8s, which is painful and slow to rollout to customers.

IMHO, it seems nice to keep it, the deps didn't seem like so many. What do you think?

[andrewsykim]

yeah I'll look into adding it back, it'll mostly depend on how it effects deps which I can look into

[rata]

@andrewsykim it seems it might not be an issue with go 1.17 if we just use it in tests? See the last paragraph in the selected answer: https://stackoverflow.com/questions/64071364/best-way-to-use-test-dependencies-in-go-but-prevent-export-them

Not sure if that will do the trick. Also, kubernetes 1.21 to 1.23 is using go 1.16 so, even if it does help, not right now. The other option might be to move the tests to another pkg, but it is not great either :(

[andrewsykim]

Created #333 to follow-up, I'll open a PR to add these back after I test the module update in k/k

[rata]

Thanks!

[andrewsykim]

/assign @cheftako

[andrewsykim]

/retest

[andrewsykim]

c.Close() was only called from tunnel.serve() below. If grpc.Client.Proxy() errors, which is possible when there are no client connections available, we would never call c.Close()

[wojtek-t]

good catch

[andrewsykim]

/hold

(for reviews)

[cheftako]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, cheftako

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [andrewsykim,cheftako]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[andrewsykim]

/hold cancel