review(sdk): addressed comments

Author: joe0BAB

internal/ipc/design.md

@@ -69,12 +69,14 @@ Also connect rpc is part of CNCF ([source](https://www.cncf.io/projects/connect-
 
 Potential drawbacks: Performance
 
-Using [nri/net/multiplex](https://github.com/containerd/nri/tree/main/pkg/net/multiplex) with [ttrpc](https://github.com/containerd/ttrpc) probably would be the most performance optimised solution. 
-It re-uses one stream over the multiplexed socket per direction and has overhead of the http protocol as protobuf gets streamed directly over the multiplexer. 
-However, it has stopped evolving and e.g. has not caught up on latest improvements on protobuf. 
-Another major downside is that it's mainly Go only, i.e., supporting alternative languages for plugins would come at a high cost.
-We argue that in our use case, as the networking only happens locally so the overhead of grpc over http and the cost of opening a new yamux stream are negligible.
-In addition, the main performance bottleneck will be within the actual plugins, e.g., because authentication needs to happen, disk access or because (slow) external networking access is required.
+Using [nri/net/multiplex](https://github.com/containerd/nri/tree/main/pkg/net/multiplex) with [ttrpc](https://github.com/containerd/ttrpc) probably would be the most performant solution.
+It re-uses one stream over the multiplexed socket per direction and does not have the overhead of the HTTP protocol as Protobuf gets streamed directly over the multiplexer.
+Although lightweight, it has stopped evolving and has not caught up to the latest improvements on Protobuf.
+Another major downside is that it's mainly Go only. 
+Plugins written in a different language would come at a high cost.
+
+We argue that in our use case since the networking only happens locally the overhead of GRPC over HTTP and the cost of opening a new yamux stream per API request are negligible. 
+In addition, the main performance bottleneck will be within the actual plugins due to IO operations, additional upstream network requests and potentially authentication.
 
 ---
 

internal/ipc/ipc.go

@@ -147,6 +147,10 @@ func createYamuxedClient(session *yamux.Session) *http.Client {
 		DialContext: func(context.Context, string, string) (net.Conn, error) {
 			return session.Open()
 		},
+		// We don't want to use http keepalive because
+		// - we keep re-using the underlying socket anyway
+		// - it allows clean bookkeeping of yamux session / any yamux session means IPC happening and not stale keepalive connections
+		// -> we can check for session.NumStreams() on shutdown
 		DisableKeepAlives: true,
 	}
 	return &http.Client{Transport: transport}

pkg/adaptation/plugin.go

@@ -47,7 +47,7 @@ type plugin struct {
 	close          func() error
 }
 
-// newExternalPlugin Create a plugin (stub) for an accepted external plugin connection.
+// newExternalPlugin creates a plugin (stub) for an accepted external plugin connection.
 func newExternalPlugin(conn net.Conn, v setupValidator) (*plugin, error) {
 	r, err := setup(conn, v)
 	if err != nil {

pkg/adaptation/plugin_test.go

@@ -80,13 +80,14 @@ func Test_newExternalPlugin(t *testing.T) {
 		{
 			name: "create external plugin",
 			test: func(t *testing.T, l net.Listener, conn net.Conn) {
+				t.Helper()
 				m := &mockExternalRuntime{l: l, done: make(chan struct{})}
 				go m.run()
 
 				s, err := p.New(newMockedPlugin(), p.WithPluginName("my-plugin"), p.WithConnection(conn))
 				require.NoError(t, err)
 				runErr, cancel := runAsyncWithTimeout(t.Context(), s.Run)
-				defer cancel()
+				t.Cleanup(cancel)
 
 				runtime, err := m.getRuntime()
 				assert.NoError(t, err)
@@ -102,13 +103,14 @@ func Test_newExternalPlugin(t *testing.T) {
 		{
 			name: "plugin returns error on GetSecret",
 			test: func(t *testing.T, l net.Listener, conn net.Conn) {
+				t.Helper()
 				m := &mockExternalRuntime{l: l, done: make(chan struct{})}
 				go m.run()
 
 				s, err := p.New(newMockedPlugin(WithID("rewrite-id")), p.WithPluginName("my-plugin"), p.WithConnection(conn))
 				require.NoError(t, err)
 				runErr, cancel := runAsyncWithTimeout(t.Context(), s.Run)
-				defer cancel()
+				t.Cleanup(cancel)
 
 				runtime, err := m.getRuntime()
 				assert.NoError(t, err)
@@ -123,6 +125,7 @@ func Test_newExternalPlugin(t *testing.T) {
 		{
 			name: "cancelling plugin.run() shuts down the runtime",
 			test: func(t *testing.T, l net.Listener, conn net.Conn) {
+				t.Helper()
 				m := &mockExternalRuntime{l: l, done: make(chan struct{})}
 				go m.run()
 
@@ -149,6 +152,7 @@ func Test_newExternalPlugin(t *testing.T) {
 		{
 			name: "plugins with invalid patterns are rejected",
 			test: func(t *testing.T, l net.Listener, conn net.Conn) {
+				t.Helper()
 				doneRuntime := make(chan struct{})
 				go func() {
 					conn, err := l.Accept()