Add Import Policy for Service VIPs (#721)

* rename export policies to make it direction independent

* split creating neighborsets and prefixsets from applying export policy

* add bgp import policy to deny service VIPs

* add tests for addition of import policy

Author: aauren

pkg/controllers/routing/bgp_peers.go

@@ -350,9 +350,9 @@ func (nrc *NetworkRoutingController) OnNodeUpdate(obj interface{}) {
 	}
 
 	// update export policies so that NeighborSet gets updated with new set of nodes
-	err := nrc.addExportPolicies()
+	err := nrc.AddPolicies()
 	if err != nil {
-		glog.Errorf("Error adding BGP export policies: %s", err.Error())
+		glog.Errorf("Error adding BGP policies: %s", err.Error())
 	}
 
 	if nrc.bgpEnableInternal {

pkg/controllers/routing/export_policies.go renamed to pkg/controllers/routing/bgp_policies.go

@@ -10,22 +10,10 @@ import (
 	v1core "k8s.io/api/core/v1"
 )
 
-// BGP export policies are added so that following conditions are met
-//
-// - by default export of all routes from the RIB to the neighbour's is denied, and explicity statements are added i
-//   to permit the desired routes to be exported
-// - each node is allowed to advertise its assigned pod CIDR's to all of its iBGP peer neighbours with same ASN if --enable-ibgp=true
-// - each node is allowed to advertise its assigned pod CIDR's to all of its external BGP peer neighbours
-//   only if --advertise-pod-cidr flag is set to true
-// - each node is NOT allowed to advertise its assigned pod CIDR's to all of its external BGP peer neighbours
-//   only if --advertise-pod-cidr flag is set to false
-// - each node is allowed to advertise service VIP's (cluster ip, load balancer ip, external IP) ONLY to external
-//   BGP peers
-// - each node is NOT allowed to advertise service VIP's (cluster ip, load balancer ip, external IP) to
-//   iBGP peers
-// - an option to allow overriding the next-hop-address with the outgoing ip for external bgp peers
-func (nrc *NetworkRoutingController) addExportPolicies() error {
-
+// First create all prefix and neighbor sets
+// Then apply export policies
+// Then apply import policies
+func (nrc *NetworkRoutingController) AddPolicies() error {
 	// we are rr server do not add export policies
 	if nrc.bgpRRServer {
 		return nil
@@ -51,30 +39,18 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 	}
 
 	// creates prefix set to represent all the advertisable IP associated with the services
-	advIpPrefixList := make([]config.Prefix, 0)
+	advIPPrefixList := make([]config.Prefix, 0)
 	advIps, _, _ := nrc.getAllVIPs()
 	for _, ip := range advIps {
-		advIpPrefixList = append(advIpPrefixList, config.Prefix{IpPrefix: ip + "/32"})
+		advIPPrefixList = append(advIPPrefixList, config.Prefix{IpPrefix: ip + "/32"})
 	}
-	clusterIpPrefixSet, err := table.NewPrefixSet(config.PrefixSet{
+	clusterIPPrefixSet, err := table.NewPrefixSet(config.PrefixSet{
 		PrefixSetName: "clusteripprefixset",
-		PrefixList:    advIpPrefixList,
+		PrefixList:    advIPPrefixList,
 	})
-	err = nrc.bgpServer.ReplaceDefinedSet(clusterIpPrefixSet)
+	err = nrc.bgpServer.ReplaceDefinedSet(clusterIPPrefixSet)
 	if err != nil {
-		nrc.bgpServer.AddDefinedSet(clusterIpPrefixSet)
-	}
-
-	statements := make([]config.Statement, 0)
-
-	var bgpActions config.BgpActions
-	if nrc.pathPrepend {
-		bgpActions = config.BgpActions{
-			SetAsPathPrepend: config.SetAsPathPrepend{
-				As:      nrc.pathPrependAS,
-				RepeatN: nrc.pathPrependCount,
-			},
-		}
+		nrc.bgpServer.AddDefinedSet(clusterIPPrefixSet)
 	}
 
 	if nrc.bgpEnableInternal {
@@ -93,10 +69,75 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 			NeighborSetName:  "iBGPpeerset",
 			NeighborInfoList: iBGPPeers,
 		})
-		err = nrc.bgpServer.ReplaceDefinedSet(iBGPPeerNS)
+		err := nrc.bgpServer.ReplaceDefinedSet(iBGPPeerNS)
 		if err != nil {
 			nrc.bgpServer.AddDefinedSet(iBGPPeerNS)
 		}
+	}
+
+	externalBgpPeers := make([]string, 0)
+	if len(nrc.globalPeerRouters) > 0 {
+		for _, peer := range nrc.globalPeerRouters {
+			externalBgpPeers = append(externalBgpPeers, peer.Config.NeighborAddress)
+		}
+	}
+	if len(nrc.nodePeerRouters) > 0 {
+		for _, peer := range nrc.nodePeerRouters {
+			externalBgpPeers = append(externalBgpPeers, peer)
+		}
+	}
+	if len(externalBgpPeers) > 0 {
+		ns, _ := table.NewNeighborSet(config.NeighborSet{
+			NeighborSetName:  "externalpeerset",
+			NeighborInfoList: externalBgpPeers,
+		})
+		err := nrc.bgpServer.ReplaceDefinedSet(ns)
+		if err != nil {
+			nrc.bgpServer.AddDefinedSet(ns)
+		}
+	}
+
+	err = nrc.addExportPolicies()
+	if err != nil {
+		return err
+	}
+
+	err = nrc.addImportPolicies()
+	if err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// BGP export policies are added so that following conditions are met:
+//
+// - by default export of all routes from the RIB to the neighbour's is denied, and explicity statements are added i
+//   to permit the desired routes to be exported
+// - each node is allowed to advertise its assigned pod CIDR's to all of its iBGP peer neighbours with same ASN if --enable-ibgp=true
+// - each node is allowed to advertise its assigned pod CIDR's to all of its external BGP peer neighbours
+//   only if --advertise-pod-cidr flag is set to true
+// - each node is NOT allowed to advertise its assigned pod CIDR's to all of its external BGP peer neighbours
+//   only if --advertise-pod-cidr flag is set to false
+// - each node is allowed to advertise service VIP's (cluster ip, load balancer ip, external IP) ONLY to external
+//   BGP peers
+// - each node is NOT allowed to advertise service VIP's (cluster ip, load balancer ip, external IP) to
+//   iBGP peers
+// - an option to allow overriding the next-hop-address with the outgoing ip for external bgp peers
+func (nrc *NetworkRoutingController) addExportPolicies() error {
+	statements := make([]config.Statement, 0)
+
+	var bgpActions config.BgpActions
+	if nrc.pathPrepend {
+		bgpActions = config.BgpActions{
+			SetAsPathPrepend: config.SetAsPathPrepend{
+				As:      nrc.pathPrependAS,
+				RepeatN: nrc.pathPrependCount,
+			},
+		}
+	}
+
+	if nrc.bgpEnableInternal {
 		actions := config.Actions{
 			RouteDisposition: config.ROUTE_DISPOSITION_ACCEPT_ROUTE,
 		}
@@ -118,26 +159,7 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 			})
 	}
 
-	externalBgpPeers := make([]string, 0)
-	if len(nrc.globalPeerRouters) != 0 {
-		for _, peer := range nrc.globalPeerRouters {
-			externalBgpPeers = append(externalBgpPeers, peer.Config.NeighborAddress)
-		}
-	}
-	if len(nrc.nodePeerRouters) != 0 {
-		for _, peer := range nrc.nodePeerRouters {
-			externalBgpPeers = append(externalBgpPeers, peer)
-		}
-	}
-	if len(externalBgpPeers) > 0 {
-		ns, _ := table.NewNeighborSet(config.NeighborSet{
-			NeighborSetName:  "externalpeerset",
-			NeighborInfoList: externalBgpPeers,
-		})
-		err = nrc.bgpServer.ReplaceDefinedSet(ns)
-		if err != nil {
-			nrc.bgpServer.AddDefinedSet(ns)
-		}
+	if len(nrc.globalPeerRouters) > 0 || len(nrc.nodePeerRouters) > 0 {
 		if nrc.overrideNextHop {
 			bgpActions.SetNextHop = "self"
 		}
@@ -179,7 +201,7 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 	}
 
 	definition := config.PolicyDefinition{
-		Name:       "kube_router",
+		Name:       "kube_router_export",
 		Statements: statements,
 	}
 
@@ -191,7 +213,7 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 	policyAlreadyExists := false
 	policyList := nrc.bgpServer.GetPolicy()
 	for _, existingPolicy := range policyList {
-		if existingPolicy.Name == "kube_router" {
+		if existingPolicy.Name == "kube_router_export" {
 			policyAlreadyExists = true
 		}
 	}
@@ -207,7 +229,7 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 	_, existingPolicyAssignments, err := nrc.bgpServer.GetPolicyAssignment("", table.POLICY_DIRECTION_EXPORT)
 	if err == nil {
 		for _, existingPolicyAssignment := range existingPolicyAssignments {
-			if existingPolicyAssignment.Name == "kube_router" {
+			if existingPolicyAssignment.Name == "kube_router_export" {
 				policyAssignmentExists = true
 			}
 		}
@@ -234,3 +256,76 @@ func (nrc *NetworkRoutingController) addExportPolicies() error {
 
 	return nil
 }
+
+// BGP import policies are added so that the following conditions are met:
+// - do not import Service VIPs at all, instead traffic to service VIPs should be sent to the gateway and ECMPed from there
+func (nrc *NetworkRoutingController) addImportPolicies() error {
+	statements := make([]config.Statement, 0)
+
+	statements = append(statements, config.Statement{
+		Conditions: config.Conditions{
+			MatchPrefixSet: config.MatchPrefixSet{
+				PrefixSet: "clusteripprefixset",
+			},
+		},
+		Actions: config.Actions{
+			RouteDisposition: config.ROUTE_DISPOSITION_REJECT_ROUTE,
+		},
+	})
+
+	definition := config.PolicyDefinition{
+		Name:       "kube_router_import",
+		Statements: statements,
+	}
+
+	policy, err := table.NewPolicy(definition)
+	if err != nil {
+		return errors.New("Failed to create new policy: " + err.Error())
+	}
+
+	policyAlreadyExists := false
+	policyList := nrc.bgpServer.GetPolicy()
+	for _, existingPolicy := range policyList {
+		if existingPolicy.Name == "kube_router_import" {
+			policyAlreadyExists = true
+		}
+	}
+
+	if !policyAlreadyExists {
+		err = nrc.bgpServer.AddPolicy(policy, false)
+		if err != nil {
+			return errors.New("Failed to add policy: " + err.Error())
+		}
+	}
+
+	policyAssignmentExists := false
+	_, existingPolicyAssignments, err := nrc.bgpServer.GetPolicyAssignment("", table.POLICY_DIRECTION_IMPORT)
+	if err == nil {
+		for _, existingPolicyAssignment := range existingPolicyAssignments {
+			if existingPolicyAssignment.Name == "kube_router_import" {
+				policyAssignmentExists = true
+			}
+		}
+	}
+
+	// Default policy is to accept
+	if !policyAssignmentExists {
+		err = nrc.bgpServer.AddPolicyAssignment("",
+			table.POLICY_DIRECTION_IMPORT,
+			[]*config.PolicyDefinition{&definition},
+			table.ROUTE_TYPE_ACCEPT)
+		if err != nil {
+			return errors.New("Failed to add policy assignment: " + err.Error())
+		}
+	} else {
+		err = nrc.bgpServer.ReplacePolicyAssignment("",
+			table.POLICY_DIRECTION_IMPORT,
+			[]*config.PolicyDefinition{&definition},
+			table.ROUTE_TYPE_ACCEPT)
+		if err != nil {
+			return errors.New("Failed to replace policy assignment: " + err.Error())
+		}
+	}
+
+	return nil
+}

pkg/controllers/routing/ecmp_vip.go

@@ -94,9 +94,9 @@ func (nrc *NetworkRoutingController) handleServiceUpdate(svc *v1core.Service) {
 	}
 
 	// update export policies so that new VIP's gets addedd to clusteripprefixsit and vip gets advertised to peers
-	err = nrc.addExportPolicies()
+	err = nrc.AddPolicies()
 	if err != nil {
-		glog.Errorf("Error adding BGP export policies: %s", err.Error())
+		glog.Errorf("Error adding BGP policies: %s", err.Error())
 	}
 
 	nrc.advertiseVIPs(toAdvertise)
@@ -162,7 +162,7 @@ func (nrc *NetworkRoutingController) newEndpointsEventHandler() cache.ResourceEv
 
 // OnEndpointsAdd handles endpoint add events from apiserver
 // This method calls OnEndpointsUpdate with the addition of updating BGP export policies
-// Calling addExportPolicies here covers the edge case where addExportPolicies fails in
+// Calling AddPolicies here covers the edge case where AddPolicies fails in
 // OnServiceUpdate because the corresponding Endpoint resource for the
 // Service was not created yet.
 func (nrc *NetworkRoutingController) OnEndpointsAdd(obj interface{}) {
@@ -171,9 +171,9 @@ func (nrc *NetworkRoutingController) OnEndpointsAdd(obj interface{}) {
 		return
 	}
 
-	err := nrc.addExportPolicies()
+	err := nrc.AddPolicies()
 	if err != nil {
-		glog.Errorf("error adding BGP export policies: %s", err)
+		glog.Errorf("error adding BGP policies: %s", err)
 	}
 
 	nrc.OnEndpointsUpdate(obj)

pkg/controllers/routing/network_routes_controller.go

@@ -286,9 +286,9 @@ func (nrc *NetworkRoutingController) Run(healthChan chan<- *healthcheck.Controll
 			glog.Errorf("Error advertising route: %s", err.Error())
 		}
 
-		err = nrc.addExportPolicies()
+		err = nrc.AddPolicies()
 		if err != nil {
-			glog.Errorf("Error adding BGP export policies: %s", err.Error())
+			glog.Errorf("Error adding BGP policies: %s", err.Error())
 		}
 
 		if nrc.bgpEnableInternal {