Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Abstract sockets #470

Merged
merged 6 commits into from
Dec 21, 2021
Merged

Abstract sockets #470

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8037a7a
Rework memif to use abstract sockets
Oct 25, 2021
0f5bf22
Add tests for proxy
Oct 27, 2021
e87be57
Rework memif to new api, add WithExternalVPP option
Nov 3, 2021
e6df795
refactoring of init()
Mixaster995 Dec 16, 2021
3c68e61
refactored to remove global variables
Mixaster995 Dec 17, 2021
78e713d
refactoring for not using shared variables
Mixaster995 Dec 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .github/workflows/ci.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ jobs:
- name: Build
run: go build -race ./...
- name: Test
if: matrix.os != 'windows-latest'
run: sudo -E PATH="$PATH" bash -c "go test -race ./..."
- name: Test
if: matrix.os == 'windows-latest'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain why this change?

Copy link
Contributor Author

@Mixaster995 Mixaster995 Dec 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test https://github.com/Mixaster995/sdk-vpp/blob/abstract-sockets/pkg/tools/proxy/proxy_test.go#L44
failes without these lines
The reason is creation of a new network namespace throws operation not permitted on linux, so we have to use sudo, and this construction in ci passes correct envs to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it :)

run: go test -race ./...
golangci-lint:
name: golangci-lint
Expand Down
3 changes: 3 additions & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ require (
github.com/stretchr/testify v1.7.0
github.com/thanhpk/randstr v1.0.4
github.com/vishvananda/netlink v1.1.0
github.com/vishvananda/netns v0.0.0-20200728191858-db3c7e526aae
go.uber.org/goleak v1.1.10
golang.org/x/sys v0.0.0-20210630005230-0f9fa26af87c
golang.zx2c4.com/wireguard/wgctrl v0.0.0-20200609130330-bd2cb7843e1b
google.golang.org/grpc v1.35.0
google.golang.org/protobuf v1.25.0
Expand Down
11 changes: 9 additions & 2 deletions pkg/networkservice/chains/forwarder/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ func NewServer(ctx context.Context, name string, authzServer networkservice.Netw
)
nsClient := registryclient.NewNetworkServiceRegistryClient(ctx, clientURL, registryclient.WithDialOptions(clientDialOptions...))

netNsInfo := memif.NewNetNSInfo()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mixaster995 Why are we doing this outside the memif.NewServer() ? If this can be safely done inside memif.NewServer it vastly simplifies the use of memif.NewServer...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@edwarnicke i was under impression that it was required for some logic, but was mistaken. Made some refactoring and tested all changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mixaster995 Its all good. When reworking someone else's work, its sometimes hard to figure out if something that looks weird is essential or not.

rv := &xconnectNSServer{}
additionalFunctionality := []networkservice.NetworkServiceServer{
recvfd.NewServer(),
Expand All @@ -93,7 +94,10 @@ func NewServer(ctx context.Context, name string, authzServer networkservice.Netw
tag.NewServer(ctx, vppConn),
mtu.NewServer(vppConn),
mechanisms.NewServer(map[string]networkservice.NetworkServiceServer{
memif.MECHANISM: memif.NewServer(vppConn, memif.WithDirectMemif()),
memif.MECHANISM: memif.NewServer(ctx, vppConn, netNsInfo,
memif.WithDirectMemif(),
memif.WithChangeNetNS(),
memif.WithExternalVPP()),
kernel.MECHANISM: kernel.NewServer(vppConn),
vxlan.MECHANISM: vxlan.NewServer(vppConn, tunnelIP, vxlan.WithVniPort(tunnelPort)),
wireguard.MECHANISM: wireguard.NewServer(vppConn, tunnelIP),
Expand All @@ -113,7 +117,10 @@ func NewServer(ctx context.Context, name string, authzServer networkservice.Netw
mtu.NewClient(vppConn),
tag.NewClient(ctx, vppConn),
// mechanisms
memif.NewClient(vppConn),
memif.NewClient(vppConn, netNsInfo,
memif.WithChangeNetNS(),
memif.WithExternalVPP(),
),
kernel.NewClient(vppConn),
vxlan.NewClient(vppConn, tunnelIP, vxlan.WithVniPort(tunnelPort)),
wireguard.NewClient(vppConn, tunnelIP),
Expand Down
61 changes: 31 additions & 30 deletions pkg/networkservice/mechanisms/memif/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

// +build !windows
//+build linux

package memif

Expand All @@ -28,39 +28,38 @@ import (
"github.com/pkg/errors"
"google.golang.org/grpc"

"github.com/networkservicemesh/sdk-vpp/pkg/networkservice/mechanisms/memif/memifproxy"

"github.com/networkservicemesh/sdk/pkg/networkservice/common/switchcase"
"github.com/networkservicemesh/sdk/pkg/networkservice/core/chain"

"github.com/networkservicemesh/api/pkg/api/networkservice"
"github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/cls"
"github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/memif"

"github.com/networkservicemesh/sdk/pkg/networkservice/core/chain"
"github.com/networkservicemesh/sdk/pkg/networkservice/core/next"
"github.com/networkservicemesh/sdk/pkg/networkservice/utils/metadata"
"github.com/networkservicemesh/sdk/pkg/tools/postpone"

"github.com/networkservicemesh/sdk-vpp/pkg/networkservice/mechanisms/memif/memifproxy"
)

type memifClient struct {
vppConn api.Connection
vppConn *vppConnection
changeNetNs bool
nsInfo NetNSInfo
}

// NewClient provides a NetworkServiceClient chain elements that support the memif Mechanism
func NewClient(vppConn api.Connection) networkservice.NetworkServiceClient {
m := &memifClient{
vppConn: vppConn,
func NewClient(vppConn api.Connection, nsInfo NetNSInfo, options ...Option) networkservice.NetworkServiceClient {
opts := &memifOptions{}
for _, o := range options {
o(opts)
}

return chain.NewNetworkServiceClient(
m,
switchcase.NewClient(&switchcase.ClientCase{
Condition: func(ctx context.Context, conn *networkservice.Connection) bool {
_, ok := loadDirectMemifInfo(ctx)
return !ok
&memifClient{
vppConn: &vppConnection{
isExternal: opts.isVPPExternal,
Connection: vppConn,
},
Client: memifproxy.New(),
}),
changeNetNs: opts.changeNetNS,
nsInfo: nsInfo,
},
)
}

Expand All @@ -75,11 +74,11 @@ func mechanismsContain(list []*networkservice.Mechanism, t string) bool {

func (m *memifClient) Request(ctx context.Context, request *networkservice.NetworkServiceRequest, opts ...grpc.CallOption) (*networkservice.Connection, error) {
if !mechanismsContain(request.MechanismPreferences, memif.MECHANISM) {
request.MechanismPreferences = append(request.MechanismPreferences, &networkservice.Mechanism{
Cls: cls.LOCAL,
Type: memif.MECHANISM,
Parameters: make(map[string]string),
})
mechanism := memif.ToMechanism(memif.NewAbstract(m.nsInfo.netNSPath))
if m.changeNetNs {
mechanism.SetNetNSURL("")
}
request.MechanismPreferences = append(request.MechanismPreferences, mechanism.Mechanism)
}

postponeCtxFunc := postpone.ContextWithValues(ctx)
Expand All @@ -89,14 +88,16 @@ func (m *memifClient) Request(ctx context.Context, request *networkservice.Netwo
return nil, err
}

// if direct memif enabled save socket filename to metadata
_, ok := loadDirectMemifInfo(ctx)
if mechanism := memif.ToMechanism(conn.GetMechanism()); mechanism != nil && ok {
storeDirectMemifInfo(ctx, directMemifInfo{socketURL: mechanism.GetSocketFileURL()})
return conn, nil
// If direct memif case store mechanism to metadata and return.
if info, ok := memifproxy.LoadInfo(ctx); ok {
if mechanism := memif.ToMechanism(conn.GetMechanism()); mechanism != nil && ok {
info.NSURL = mechanism.GetNetNSURL()
info.SocketFile = mechanism.GetSocketFilename()
return conn, nil
}
}

if err := create(ctx, conn, m.vppConn, metadata.IsClient(m)); err != nil {
if err = create(ctx, conn, m.vppConn, metadata.IsClient(m), m.nsInfo.netNS); err != nil {
closeCtx, cancelClose := postponeCtxFunc()
defer cancelClose()

Expand Down
6 changes: 3 additions & 3 deletions pkg/networkservice/mechanisms/memif/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
// See the License for the specific language governing permissions and
// limitations under the License.

// +build !windows
// +build linux

package memif_test

Expand All @@ -33,7 +33,7 @@ import (
)

func Test_MemifClient_ShouldAppendMechanismIfMemifMechanismMissed(t *testing.T) {
c := chain.NewNetworkServiceClient(metadata.NewClient(), memif.NewClient(nil))
c := chain.NewNetworkServiceClient(metadata.NewClient(), memif.NewClient(nil, memif.NetNSInfo{}))

req := &networkservice.NetworkServiceRequest{
MechanismPreferences: []*networkservice.Mechanism{},
Expand All @@ -55,7 +55,7 @@ func Test_MemifClient_ShouldAppendMechanismIfMemifMechanismMissed(t *testing.T)
}

func Test_MemifClient_ShouldNotDuplicateMechanisms(t *testing.T) {
c := chain.NewNetworkServiceClient(metadata.NewClient(), memif.NewClient(nil))
c := chain.NewNetworkServiceClient(metadata.NewClient(), memif.NewClient(nil, memif.NetNSInfo{}))

req := &networkservice.NetworkServiceRequest{
MechanismPreferences: []*networkservice.Mechanism{
Expand Down
Loading