From ee80de00dc9fbd6aac1d0cef33ea7ad318f21658 Mon Sep 17 00:00:00 2001 From: Lionel Jouin Date: Thu, 10 Mar 2022 16:57:15 +0100 Subject: [PATCH 1/3] VIP implementation in TAPA replaced by NSM IP and policy update * The initial NSM request does not contain any information about the ip context. Once the conduit connected (sucessful NSM request), the VIP watcher will call SetVIPs which will update (new request) the NSM connection (SrcIPs and policies). * the initial SrcIPs are saved in the conduit struct (to not mix the target IPs and VIPs) * The previous code has been removed --- pkg/ambassador/tap/conduit/conduit.go | 109 +++++++++++++++----------- pkg/ambassador/tap/conduit/vip.go | 74 ----------------- 2 files changed, 62 insertions(+), 121 deletions(-) delete mode 100644 pkg/ambassador/tap/conduit/vip.go diff --git a/pkg/ambassador/tap/conduit/conduit.go b/pkg/ambassador/tap/conduit/conduit.go index 6c5934ec..485e1fa7 100644 --- a/pkg/ambassador/tap/conduit/conduit.go +++ b/pkg/ambassador/tap/conduit/conduit.go @@ -20,6 +20,8 @@ import ( "context" "errors" "fmt" + "net" + "strings" "sync" "github.com/networkservicemesh/api/pkg/api/networkservice" @@ -53,8 +55,7 @@ type Conduit struct { StreamFactory StreamFactory connection *networkservice.Connection mu sync.Mutex - vips []*virtualIP - tableID int + localIPs []string } // New is the constructor of Conduit. @@ -76,8 +77,7 @@ func New(conduit *ambassadorAPI.Conduit, NetworkServiceClient: networkServiceClient, NetUtils: netUtils, connection: nil, - vips: []*virtualIP{}, - tableID: 1, + localIPs: []string{}, } c.StreamFactory = stream.NewFactory(targetRegistryClient, stream.MaxNumberOfTargets, c) c.StreamManager = NewStreamManager(configurationManagerClient, targetRegistryClient, streamRegistry, c.StreamFactory, PendingTime) @@ -117,6 +117,7 @@ func (c *Conduit) Connect(ctx context.Context) error { } logrus.Infof("Conduit connected: %v", c.Conduit) c.connection = connection + c.localIPs = c.connection.GetContext().GetIpContext().GetSrcIpAddrs() c.Configuration.Watch() @@ -132,9 +133,6 @@ func (c *Conduit) Disconnect(ctx context.Context) error { logrus.Infof("Disconnect from conduit: %v", c.Conduit) // Stops the configuration c.Configuration.Stop() - // reset the VIPs related configuration - c.deleteVIPs(c.vips) - c.tableID = 1 var errFinal error // Stop the stream manager (close the streams) errFinal = c.StreamManager.Stop(ctx) @@ -174,7 +172,7 @@ func (c *Conduit) GetConduit() *ambassadorAPI.Conduit { // GetStreams returns the local IPs for this conduit func (c *Conduit) GetIPs() []string { if c.connection != nil { - return c.connection.GetContext().GetIpContext().GetSrcIpAddrs() + return c.localIPs } return []string{} } @@ -186,34 +184,65 @@ func (c *Conduit) SetVIPs(vips []string) error { if !c.isConnected() { return nil } - currentVIPs := make(map[string]*virtualIP) - for _, vip := range c.vips { - currentVIPs[vip.prefix] = vip + // prepare SrcIpAddrs (IPs allocated by the proxy + VIPs) + c.connection.Context.IpContext.SrcIpAddrs = append(c.GetIPs(), vips...) + // prepare the routes (nexthops = proxy bridge IPs) + ipv4Nexthops := []*networkservice.Route{} + ipv6Nexthops := []*networkservice.Route{} + for _, nexthop := range c.getGateways() { + gw, _, err := net.ParseCIDR(nexthop) + if err != nil { + continue + } + route := &networkservice.Route{ + NextHop: gw.String(), + } + if isIPv6(nexthop) { + route.Prefix = "::/0" + ipv6Nexthops = append(ipv6Nexthops, route) + } else { + route.Prefix = "0.0.0.0/0" + ipv4Nexthops = append(ipv4Nexthops, route) + } } + // prepare the policies (only based on VIP address for now) + c.connection.Context.IpContext.Policies = []*networkservice.PolicyRoute{} for _, vip := range vips { - if _, ok := currentVIPs[vip]; !ok { - newVIP, err := newVirtualIP(vip, c.tableID, c.NetUtils) - if err != nil { - logrus.Errorf("SimpleTarget: Error adding SourceBaseRoute: %v", err) // todo: err handling - continue - } - c.tableID++ - c.vips = append(c.vips, newVIP) - for _, nexthop := range c.getGateways() { - err = newVIP.AddNexthop(nexthop) - if err != nil { - logrus.Errorf("Client: Error adding nexthop: %v", err) // todo: err handling - } - } + nexthops := ipv4Nexthops + if isIPv6(vip) { + nexthops = ipv6Nexthops } - delete(currentVIPs, vip) + newPolicyRoute := &networkservice.PolicyRoute{ + From: vip, + Routes: nexthops, + } + c.connection.Context.IpContext.Policies = append(c.connection.Context.IpContext.Policies, newPolicyRoute) } - // delete remaining vips - vipsSlice := []*virtualIP{} - for _, vip := range currentVIPs { - vipsSlice = append(vipsSlice, vip) + var err error + // update the NSM connection + // TODO: retry if error returned? + c.connection, err = c.NetworkServiceClient.Request(context.TODO(), + &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{ + Id: c.connection.GetId(), + NetworkService: c.connection.GetNetworkService(), + Labels: c.connection.GetLabels(), + Payload: c.connection.GetPayload(), + Context: &networkservice.ConnectionContext{ + IpContext: c.connection.GetContext().GetIpContext(), + }, + }, + MechanismPreferences: []*networkservice.Mechanism{ + { + Cls: cls.LOCAL, + Type: kernelmech.MECHANISM, + }, + }, + }) + if err != nil { + return fmt.Errorf("error updating the VIPs in conduit: %v - %v", c.Conduit, err) } - c.deleteVIPs(vipsSlice) + logrus.Infof("VIPs in conduit updated: %v - %v", c.Conduit, vips) return nil } @@ -226,22 +255,8 @@ func (c *Conduit) isConnected() bool { return c.connection != nil } -func (c *Conduit) deleteVIPs(vips []*virtualIP) { - vipsMap := make(map[string]*virtualIP) - for _, vip := range vips { - vipsMap[vip.prefix] = vip - } - for index := 0; index < len(c.vips); index++ { - vip := c.vips[index] - if _, ok := vipsMap[vip.prefix]; ok { - c.vips = append(c.vips[:index], c.vips[index+1:]...) - index-- - err := vip.Delete() - if err != nil { - logrus.Errorf("Client: Error deleting vip: %v", err) // todo: err handling - } - } - } +func isIPv6(address string) bool { + return strings.Count(address, ":") >= 2 } // TODO: Requires the IPs of the bridge diff --git a/pkg/ambassador/tap/conduit/vip.go b/pkg/ambassador/tap/conduit/vip.go deleted file mode 100644 index 72876054..00000000 --- a/pkg/ambassador/tap/conduit/vip.go +++ /dev/null @@ -1,74 +0,0 @@ -/* -Copyright (c) 2021-2022 Nordix Foundation - -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 conduit - -import ( - "github.com/nordix/meridio/pkg/networking" - "github.com/sirupsen/logrus" -) - -type virtualIP struct { - sourceBasedRoute networking.SourceBasedRoute - prefix string - netUtils networking.Utils -} - -func (vip *virtualIP) Delete() error { - err := vip.netUtils.DeleteVIP(vip.prefix) - if err != nil { - return err - } - return vip.removeSourceBaseRoute() -} - -func (vip *virtualIP) AddNexthop(ip string) error { - return vip.sourceBasedRoute.AddNexthop(ip) -} - -func (vip *virtualIP) RemoveNexthop(ip string) error { - return vip.sourceBasedRoute.RemoveNexthop(ip) -} - -func (vip *virtualIP) createSourceBaseRoute(tableID int) error { - var err error - vip.sourceBasedRoute, err = vip.netUtils.NewSourceBasedRoute(tableID, vip.prefix) - logrus.Infof("VIP Simple target: sourceBasedRoute index - vip: %v - %v", tableID, vip.prefix) - if err != nil { - return err - } - return nil -} - -func (vip *virtualIP) removeSourceBaseRoute() error { - return vip.sourceBasedRoute.Delete() -} - -func newVirtualIP(prefix string, tableID int, netUtils networking.Utils) (*virtualIP, error) { - vip := &virtualIP{ - prefix: prefix, - netUtils: netUtils, - } - err := vip.createSourceBaseRoute(tableID) - if err != nil { - return nil, err - } - err = vip.netUtils.AddVIP(vip.prefix) - if err != nil { - return nil, err - } - return vip, nil -} From 2ef245f147e10fffe6aa7648f1c91ff41503d546 Mon Sep 17 00:00:00 2001 From: Lionel Jouin Date: Thu, 10 Mar 2022 17:02:21 +0100 Subject: [PATCH 2/3] TAPA privileges removed * The last privileges required (NET_ADMIN) has been removed --- examples/target/helm/templates/target.yaml | 3 --- 1 file changed, 3 deletions(-) diff --git a/examples/target/helm/templates/target.yaml b/examples/target/helm/templates/target.yaml index aa915d1d..9743a4a7 100644 --- a/examples/target/helm/templates/target.yaml +++ b/examples/target/helm/templates/target.yaml @@ -84,9 +84,6 @@ spec: - name: meridio-socket mountPath: /var/lib/meridio readOnly: false - securityContext: - capabilities: - add: ["NET_ADMIN"] volumes: - name: spire-agent-socket hostPath: From 99b2dced9eb399fef94306073773e4941b70432f Mon Sep 17 00:00:00 2001 From: Lionel Jouin Date: Mon, 11 Apr 2022 22:29:04 +0200 Subject: [PATCH 3/3] Retry request on VIP update and cancel request from Disconnect --- pkg/ambassador/tap/conduit/conduit.go | 56 ++++++++++++++++----------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/pkg/ambassador/tap/conduit/conduit.go b/pkg/ambassador/tap/conduit/conduit.go index 485e1fa7..e34e76f3 100644 --- a/pkg/ambassador/tap/conduit/conduit.go +++ b/pkg/ambassador/tap/conduit/conduit.go @@ -23,6 +23,7 @@ import ( "net" "strings" "sync" + "time" "github.com/networkservicemesh/api/pkg/api/networkservice" "github.com/networkservicemesh/api/pkg/api/networkservice/mechanisms/cls" @@ -34,6 +35,7 @@ import ( "github.com/nordix/meridio/pkg/ambassador/tap/types" "github.com/nordix/meridio/pkg/conduit" "github.com/nordix/meridio/pkg/networking" + "github.com/nordix/meridio/pkg/retry" "github.com/sirupsen/logrus" ) @@ -56,6 +58,8 @@ type Conduit struct { connection *networkservice.Connection mu sync.Mutex localIPs []string + ctx context.Context + cancel context.CancelFunc } // New is the constructor of Conduit. @@ -93,9 +97,10 @@ func (c *Conduit) Connect(ctx context.Context) error { if c.isConnected() { return nil } + c.ctx, c.cancel = context.WithCancel(ctx) logrus.Infof("Attempt to connect conduit: %v", c.Conduit) nsName := conduit.GetNetworkServiceNameWithProxy(c.Conduit.GetName(), c.Conduit.GetTrench().GetName(), c.Namespace) - connection, err := c.NetworkServiceClient.Request(ctx, + connection, err := c.NetworkServiceClient.Request(c.ctx, &networkservice.NetworkServiceRequest{ Connection: &networkservice.Connection{ Id: fmt.Sprintf("%s-%s-%d", c.TargetName, nsName, 0), @@ -128,6 +133,9 @@ func (c *Conduit) Connect(ctx context.Context) error { // Disconnect closes the connection from NSM, closes all streams // and stop the VIP watcher func (c *Conduit) Disconnect(ctx context.Context) error { + if c.cancel != nil { + c.cancel() + } c.mu.Lock() defer c.mu.Unlock() logrus.Infof("Disconnect from conduit: %v", c.Conduit) @@ -169,7 +177,7 @@ func (c *Conduit) GetConduit() *ambassadorAPI.Conduit { return c.Conduit } -// GetStreams returns the local IPs for this conduit +// GetIPs returns the local IPs for this conduit func (c *Conduit) GetIPs() []string { if c.connection != nil { return c.localIPs @@ -219,29 +227,33 @@ func (c *Conduit) SetVIPs(vips []string) error { c.connection.Context.IpContext.Policies = append(c.connection.Context.IpContext.Policies, newPolicyRoute) } var err error + c.ctx, c.cancel = context.WithCancel(context.TODO()) // update the NSM connection - // TODO: retry if error returned? - c.connection, err = c.NetworkServiceClient.Request(context.TODO(), - &networkservice.NetworkServiceRequest{ - Connection: &networkservice.Connection{ - Id: c.connection.GetId(), - NetworkService: c.connection.GetNetworkService(), - Labels: c.connection.GetLabels(), - Payload: c.connection.GetPayload(), - Context: &networkservice.ConnectionContext{ - IpContext: c.connection.GetContext().GetIpContext(), + err = retry.Do(func() error { + c.connection, err = c.NetworkServiceClient.Request(c.ctx, + &networkservice.NetworkServiceRequest{ + Connection: &networkservice.Connection{ + Id: c.connection.GetId(), + NetworkService: c.connection.GetNetworkService(), + Labels: c.connection.GetLabels(), + Payload: c.connection.GetPayload(), + Context: &networkservice.ConnectionContext{ + IpContext: c.connection.GetContext().GetIpContext(), + }, }, - }, - MechanismPreferences: []*networkservice.Mechanism{ - { - Cls: cls.LOCAL, - Type: kernelmech.MECHANISM, + MechanismPreferences: []*networkservice.Mechanism{ + { + Cls: cls.LOCAL, + Type: kernelmech.MECHANISM, + }, }, - }, - }) - if err != nil { - return fmt.Errorf("error updating the VIPs in conduit: %v - %v", c.Conduit, err) - } + }) + if err != nil { + return fmt.Errorf("error updating the VIPs in conduit: %v - %v", c.Conduit, err) + } + return nil + }, retry.WithContext(c.ctx), + retry.WithDelay(500*time.Millisecond)) logrus.Infof("VIPs in conduit updated: %v - %v", c.Conduit, vips) return nil }