From 7ea3b3d4e6aa794ff0d9650abb600d14616560a9 Mon Sep 17 00:00:00 2001 From: denis-tingaikin Date: Wed, 3 Jul 2024 09:29:53 +0300 Subject: [PATCH 1/3] Revert "Add peer cleanup on Requests and Closes from begin (#1636)" This reverts commit c01f9e1008513cb1a49aea56f1b74ce240f9c3ba. --- .../common/begin/event_factory.go | 2 - .../common/mechanisms/recvfd/server_test.go | 86 +------------------ .../checks/checkcontextonreturn/server.go | 59 ------------- 3 files changed, 1 insertion(+), 146 deletions(-) delete mode 100644 pkg/networkservice/utils/checks/checkcontextonreturn/server.go diff --git a/pkg/networkservice/common/begin/event_factory.go b/pkg/networkservice/common/begin/event_factory.go index ae92164e9..b1b96ec25 100644 --- a/pkg/networkservice/common/begin/event_factory.go +++ b/pkg/networkservice/common/begin/event_factory.go @@ -22,7 +22,6 @@ import ( "github.com/edwarnicke/serialize" "github.com/networkservicemesh/api/pkg/api/networkservice" "google.golang.org/grpc" - "google.golang.org/grpc/peer" "github.com/networkservicemesh/sdk/pkg/tools/extend" "github.com/networkservicemesh/sdk/pkg/tools/postpone" @@ -180,7 +179,6 @@ func (f *eventFactoryServer) updateContext(valueCtx context.Context) { f.ctxFunc = func() (context.Context, context.CancelFunc) { eventCtx, cancel := f.initialCtxFunc() eventCtx = extend.WithValuesFromContext(eventCtx, valueCtx) - eventCtx = peer.NewContext(eventCtx, &peer.Peer{}) return withEventFactory(eventCtx, f), cancel } } diff --git a/pkg/networkservice/common/mechanisms/recvfd/server_test.go b/pkg/networkservice/common/mechanisms/recvfd/server_test.go index 0b3ab3baa..77ec0e62a 100644 --- a/pkg/networkservice/common/mechanisms/recvfd/server_test.go +++ b/pkg/networkservice/common/mechanisms/recvfd/server_test.go @@ -1,6 +1,6 @@ // Copyright (c) 2021-2022 Doc.ai and/or its affiliates. // -// Copyright (c) 2023-2024 Cisco and/or its affiliates. +// Copyright (c) 2023 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 // @@ -27,9 +27,7 @@ import ( "net/url" "os" "path" - "path/filepath" "runtime" - "sync" "testing" "time" @@ -38,8 +36,6 @@ import ( "github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/cls" "github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/common" "github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/kernel" - "github.com/pkg/errors" - "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" "go.uber.org/goleak" "google.golang.org/grpc" @@ -51,8 +47,6 @@ import ( "github.com/networkservicemesh/sdk/pkg/networkservice/common/mechanisms/sendfd" "github.com/networkservicemesh/sdk/pkg/networkservice/core/chain" "github.com/networkservicemesh/sdk/pkg/networkservice/utils/checks/checkcontext" - "github.com/networkservicemesh/sdk/pkg/networkservice/utils/checks/checkcontextonreturn" - "github.com/networkservicemesh/sdk/pkg/networkservice/utils/inject/injecterror" "github.com/networkservicemesh/sdk/pkg/tools/grpcfdutils" "github.com/networkservicemesh/sdk/pkg/tools/grpcutils" "github.com/networkservicemesh/sdk/pkg/tools/sandbox" @@ -226,81 +220,3 @@ func (s *checkRecvfdTestSuite) TestRecvfdClosesMultipleFiles() { }, time.Second, time.Millisecond*100) } } - -func TestRecvfdDoesntWaitForAnyFilesOnRequestsFromBegin(t *testing.T) { - ctx, cancel := context.WithTimeout(context.Background(), time.Second) - - t.Cleanup(func() { - cancel() - goleak.VerifyNone(t) - }) - - eventFactoryCh := make(chan begin.EventFactory, 1) - var once sync.Once - // Create a server - server := chain.NewNetworkServiceServer( - begin.NewServer(), - checkcontextonreturn.NewServer(t, func(t *testing.T, ctx context.Context) { - once.Do(func() { - eventFactoryCh <- begin.FromContext(ctx) - close(eventFactoryCh) - }) - }), - recvfd.NewServer(), - injecterror.NewServer( - injecterror.WithError(errors.New("error")), - injecterror.WithRequestErrorTimes(1), - injecterror.WithCloseErrorTimes(1)), - ) - - tempDir := t.TempDir() - sock, err := os.Create(filepath.Clean(path.Join(tempDir, "test.sock"))) - require.NoError(t, err) - - serveURL := &url.URL{Scheme: "unix", Path: sock.Name()} - grpcServer := grpc.NewServer(grpc.Creds(grpcfd.TransportCredentials(insecure.NewCredentials()))) - networkservice.RegisterNetworkServiceServer(grpcServer, server) - errCh := grpcutils.ListenAndServe(ctx, serveURL, grpcServer) - require.Len(t, errCh, 0) - - // Create a client - c := createClient(ctx, serveURL) - - // Create a file to send - testFileName := filepath.Clean(path.Join(tempDir, "TestRecvfdDoesntWaitForAnyFilesOnRequestsFromBegin.test")) - f, err := os.Create(testFileName) - require.NoErrorf(t, err, "Failed to create and open a file: %v", err) - err = f.Close() - require.NoErrorf(t, err, "Failed to close file: %v", err) - - // Create a request - request := &networkservice.NetworkServiceRequest{ - Connection: &networkservice.Connection{ - Id: "id", - Mechanism: &networkservice.Mechanism{ - Cls: cls.LOCAL, - Type: kernel.MECHANISM, - Parameters: map[string]string{ - common.InodeURL: "file:" + testFileName, - }, - }, - }, - } - - // Make the first request from the client to send files - conn, err := c.Request(ctx, request) - require.NoError(t, err) - request.Connection = conn.Clone() - - // Make the second request that return an error. - // It should make recvfd close all the files. - _, err = c.Request(ctx, request) - require.Error(t, err) - - // Send Close. Recvfd shouldn't freeze trying to read files - // from the client because we send Close from begin. - eventFactory := <-eventFactoryCh - ch := eventFactory.Close() - err = <-ch - require.NoError(t, err) -} diff --git a/pkg/networkservice/utils/checks/checkcontextonreturn/server.go b/pkg/networkservice/utils/checks/checkcontextonreturn/server.go deleted file mode 100644 index b659a7e7e..000000000 --- a/pkg/networkservice/utils/checks/checkcontextonreturn/server.go +++ /dev/null @@ -1,59 +0,0 @@ -// Copyright (c) 2020-2024 Cisco and/or its affiliates. -// -// SPDX-License-Identifier: Apache-2.0 -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at: -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -// Package checkcontextonreturn - provides a NetworkServiceClient chain element for checking the state of the context.Context -// -// after the next element in the chain has returned -package checkcontextonreturn - -import ( - "context" - "testing" - - "github.com/golang/protobuf/ptypes/empty" - "github.com/networkservicemesh/api/pkg/api/networkservice" - - "github.com/networkservicemesh/sdk/pkg/networkservice/core/next" -) - -type checkContextOnReturnServer struct { - *testing.T - check func(t *testing.T, ctx context.Context) -} - -// NewServer - returns a NetworkServiceServer chain element for checking the state of the context.Context -// -// after the next element in the chain has returned -// t - *testing.T for doing the checks -// check - function for checking the context.Context -func NewServer(t *testing.T, check func(t *testing.T, ctx context.Context)) networkservice.NetworkServiceServer { - return &checkContextOnReturnServer{ - T: t, - check: check, - } -} - -func (t *checkContextOnReturnServer) Request(ctx context.Context, request *networkservice.NetworkServiceRequest) (*networkservice.Connection, error) { - conn, err := next.Server(ctx).Request(ctx, request) - t.check(t.T, ctx) - return conn, err -} - -func (t *checkContextOnReturnServer) Close(ctx context.Context, conn *networkservice.Connection) (*empty.Empty, error) { - e, err := next.Server(ctx).Close(ctx, conn) - t.check(t.T, ctx) - return e, err -} From dfa406317960035c94e3808eef8f827c396d3821 Mon Sep 17 00:00:00 2001 From: denis-tingaikin Date: Wed, 3 Jul 2024 09:30:00 +0300 Subject: [PATCH 2/3] Revert "fix authorize (#1637)" This reverts commit 50c0908267dec982170bcf65cdf4bbb41c3bb51f. --- pkg/networkservice/common/authorize/server.go | 3 +-- pkg/networkservice/common/authorize/server_test.go | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/pkg/networkservice/common/authorize/server.go b/pkg/networkservice/common/authorize/server.go index b38ee2af6..eb7a68a10 100644 --- a/pkg/networkservice/common/authorize/server.go +++ b/pkg/networkservice/common/authorize/server.go @@ -129,8 +129,7 @@ func (a *authorizeServer) Close(ctx context.Context, conn *networkservice.Connec a.spiffeIDConnectionMap.Store(spiffeID, ids) } } - - if p, ok := peer.FromContext(ctx); ok && p != nil && *p != (peer.Peer{}) { + if _, ok := peer.FromContext(ctx); ok { if err := a.policies.check(ctx, leftSide); err != nil { return nil, err } diff --git a/pkg/networkservice/common/authorize/server_test.go b/pkg/networkservice/common/authorize/server_test.go index faf93b901..50790b34a 100644 --- a/pkg/networkservice/common/authorize/server_test.go +++ b/pkg/networkservice/common/authorize/server_test.go @@ -26,7 +26,6 @@ import ( "crypto/tls" "crypto/x509" "math/big" - "net" "net/url" "os" "path" @@ -182,7 +181,7 @@ func TestAuthzEndpoint(t *testing.T) { require.Equal(t, s.Code(), codes.PermissionDenied, "wrong error status code") } - ctx := peer.NewContext(context.Background(), &peer.Peer{Addr: &net.IPAddr{}}) + ctx := peer.NewContext(context.Background(), &peer.Peer{}) _, err := srv.Request(ctx, s.request) checkResult(err) From d108231640e8d3e82b6ad3e83e4f3c1ccf3dce33 Mon Sep 17 00:00:00 2001 From: denis-tingaikin Date: Wed, 3 Jul 2024 09:48:15 +0300 Subject: [PATCH 3/3] fix ci issue Signed-off-by: denis-tingaikin --- pkg/networkservice/common/mechanisms/recvfd/server_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/networkservice/common/mechanisms/recvfd/server_test.go b/pkg/networkservice/common/mechanisms/recvfd/server_test.go index 77ec0e62a..df0dedb68 100644 --- a/pkg/networkservice/common/mechanisms/recvfd/server_test.go +++ b/pkg/networkservice/common/mechanisms/recvfd/server_test.go @@ -1,6 +1,6 @@ // Copyright (c) 2021-2022 Doc.ai and/or its affiliates. // -// Copyright (c) 2023 Cisco and/or its affiliates. +// Copyright (c) 2023-2024 Cisco and/or its affiliates. // // SPDX-License-Identifier: Apache-2.0 //