Merge pull request #590 from glazychev-art/fix_codereview

pinggrouprange: fix code review comments

pkg/kernel/networkservice/pinggrouprange/client.go

@@ -24,6 +24,7 @@ import (
 	"google.golang.org/grpc"
 
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
+	"github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/kernel"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
 	"github.com/networkservicemesh/sdk/pkg/tools/postpone"
 )
@@ -41,15 +42,16 @@ func (p *pinggrouprangeClient) Request(ctx context.Context, request *networkserv
 	if err != nil {
 		return nil, err
 	}
-
-	if err := set(ctx, conn); err != nil {
-		closeCtx, cancelClose := postponeCtxFunc()
-		defer cancelClose()
-
-		if _, closeErr := p.Close(closeCtx, conn, opts...); closeErr != nil {
-			err = errors.Wrapf(err, "connection closed with error: %s", closeErr.Error())
+	if mechanism := kernel.ToMechanism(conn.GetMechanism()); mechanism != nil && mechanism.GetVLAN() == 0 {
+		if err := applyPingGroupRange(ctx, mechanism); err != nil {
+			closeCtx, cancelClose := postponeCtxFunc()
+			defer cancelClose()
+
+			if _, closeErr := p.Close(closeCtx, conn, opts...); closeErr != nil {
+				err = errors.Wrapf(err, "connection closed with error: %s", closeErr.Error())
+			}
+			return nil, err
 		}
-		return nil, err
 	}
 	return conn, nil
 }

pkg/kernel/networkservice/pinggrouprange/common.go

@@ -23,7 +23,6 @@ import (
 	"github.com/pkg/errors"
 	"github.com/vishvananda/netns"
 
-	"github.com/networkservicemesh/api/pkg/api/networkservice"
 	"github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/kernel"
 
 	"github.com/networkservicemesh/sdk/pkg/tools/log"
@@ -32,30 +31,30 @@ import (
 )
 
 // See https://github.com/go-ping/ping#linux
-const groupRange = "0 2147483647"
-
-func set(ctx context.Context, conn *networkservice.Connection) error {
-	if mechanism := kernel.ToMechanism(conn.GetMechanism()); mechanism != nil && mechanism.GetVLAN() == 0 {
-		forwarderNetNS, err := nshandle.Current()
-		if err != nil {
-			return err
-		}
-		defer func() { _ = forwarderNetNS.Close() }()
-
-		var targetNetNS netns.NsHandle
-		targetNetNS, err = nshandle.FromURL(mechanism.GetNetNSURL())
-		if err != nil {
-			return err
-		}
-		defer func() { _ = targetNetNS.Close() }()
-
-		pingGroupRangeFilename := "/proc/sys/net/ipv4/ping_group_range"
-		if err = nshandle.RunIn(forwarderNetNS, targetNetNS, func() error {
-			return ioutil.WriteFile(pingGroupRangeFilename, []byte(groupRange), 0o600)
-		}); err != nil {
-			return errors.Wrapf(err, "failed to set %s = %s", pingGroupRangeFilename, groupRange)
-		}
-		log.FromContext(ctx).Infof("%s was set to %s", pingGroupRangeFilename, groupRange)
+const (
+	pingGroupRangeFilename = "/proc/sys/net/ipv4/ping_group_range"
+	groupRange             = "0 2147483647"
+)
+
+func applyPingGroupRange(ctx context.Context, mech *kernel.Mechanism) error {
+	forwarderNetNS, err := nshandle.Current()
+	if err != nil {
+		return err
+	}
+	defer func() { _ = forwarderNetNS.Close() }()
+
+	var targetNetNS netns.NsHandle
+	targetNetNS, err = nshandle.FromURL(mech.GetNetNSURL())
+	if err != nil {
+		return err
+	}
+	defer func() { _ = targetNetNS.Close() }()
+
+	if err = nshandle.RunIn(forwarderNetNS, targetNetNS, func() error {
+		return ioutil.WriteFile(pingGroupRangeFilename, []byte(groupRange), 0o600)
+	}); err != nil {
+		return errors.Wrapf(err, "failed to set %s = %s", pingGroupRangeFilename, groupRange)
 	}
+	log.FromContext(ctx).Infof("%s was set to %s", pingGroupRangeFilename, groupRange)
 	return nil
 }

pkg/kernel/networkservice/pinggrouprange/server.go

@@ -23,6 +23,7 @@ import (
 	"github.com/pkg/errors"
 
 	"github.com/networkservicemesh/api/pkg/api/networkservice"
+	"github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/kernel"
 	"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
 	"github.com/networkservicemesh/sdk/pkg/tools/postpone"
 )
@@ -40,15 +41,16 @@ func (p *pinggrouprangeServer) Request(ctx context.Context, request *networkserv
 	if err != nil {
 		return nil, err
 	}
-
-	if err := set(ctx, conn); err != nil {
-		closeCtx, cancelClose := postponeCtxFunc()
-		defer cancelClose()
-
-		if _, closeErr := p.Close(closeCtx, conn); closeErr != nil {
-			err = errors.Wrapf(err, "connection closed with error: %s", closeErr.Error())
+	if mechanism := kernel.ToMechanism(conn.GetMechanism()); mechanism != nil && mechanism.GetVLAN() == 0 {
+		if err := applyPingGroupRange(ctx, mechanism); err != nil {
+			closeCtx, cancelClose := postponeCtxFunc()
+			defer cancelClose()
+
+			if _, closeErr := p.Close(closeCtx, conn); closeErr != nil {
+				err = errors.Wrapf(err, "connection closed with error: %s", closeErr.Error())
+			}
+			return nil, err
 		}
-		return nil, err
 	}
 	return conn, nil
 }