From 44806809e3fb6d1bf5ca4073c1439bf371259a6f Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Tue, 5 Nov 2024 19:59:52 +0000 Subject: [PATCH 1/6] manage routes for instances in multiple vpcs in a single region --- cloud/linode/cloud.go | 43 ++------- cloud/linode/instances.go | 29 +++++-- cloud/linode/route_controller.go | 120 +++++++++++++++----------- cloud/linode/route_controller_test.go | 53 ++++++------ cloud/linode/vpc.go | 30 ++++++- deploy/chart/templates/daemonset.yaml | 7 +- deploy/chart/values.yaml | 3 +- main.go | 3 +- 8 files changed, 160 insertions(+), 128 deletions(-) diff --git a/cloud/linode/cloud.go b/cloud/linode/cloud.go index 46230bbf..8d2b6cf2 100644 --- a/cloud/linode/cloud.go +++ b/cloud/linode/cloud.go @@ -6,13 +6,13 @@ import ( "net" "os" "strconv" - "sync" "time" "github.com/spf13/pflag" "golang.org/x/exp/slices" "k8s.io/client-go/informers" cloudprovider "k8s.io/cloud-provider" + "k8s.io/klog/v2" "github.com/linode/linode-cloud-controller-manager/cloud/linode/client" ) @@ -35,41 +35,14 @@ var Options struct { KubeconfigFlag *pflag.Flag LinodeGoDebug bool EnableRouteController bool + // deprecated: use VPCNames instead VPCName string + VPCNames string LoadBalancerType string BGPNodeSelector string LinodeExternalNetwork *net.IPNet } -// vpcDetails is set when VPCName options flag is set. -// We use it to list instances running within the VPC if set -type vpcDetails struct { - mu sync.RWMutex - id int - name string -} - -func (v *vpcDetails) setDetails(client client.Client, name string) error { - v.mu.Lock() - defer v.mu.Unlock() - - id, err := getVPCID(client, Options.VPCName) - if err != nil { - return fmt.Errorf("failed finding VPC ID: %w", err) - } - v.id = id - v.name = name - return nil -} - -func (v *vpcDetails) getID() int { - v.mu.Lock() - defer v.mu.Unlock() - return v.id -} - -var vpcInfo vpcDetails = vpcDetails{id: 0, name: ""} - type linodeCloud struct { client client.Client instances cloudprovider.InstancesV2 @@ -114,11 +87,13 @@ func newCloud() (cloudprovider.Interface, error) { linodeClient.SetDebug(true) } + if Options.VPCName != "" && Options.VPCNames != "" { + return nil, fmt.Errorf("cannot have both vpc-name and vpc-names set") + } + if Options.VPCName != "" { - err := vpcInfo.setDetails(linodeClient, Options.VPCName) - if err != nil { - return nil, fmt.Errorf("failed finding VPC ID: %w", err) - } + klog.Warningf("vpc-name flag is deprecated. Use vpc-names instead") + Options.VPCNames = Options.VPCName } routes, err := newRoutes(linodeClient) diff --git a/cloud/linode/instances.go b/cloud/linode/instances.go index f5cc449a..80c4853f 100644 --- a/cloud/linode/instances.go +++ b/cloud/linode/instances.go @@ -79,17 +79,28 @@ func (nc *nodeCache) refreshInstances(ctx context.Context, client client.Client) // If running within VPC, find instances and store their ips vpcNodes := map[int][]string{} - vpcID := vpcInfo.getID() - if vpcID != 0 { - resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) + vpcNames := strings.Split(Options.VPCNames, ",") + for _, v := range vpcNames { + vpcName := strings.TrimSpace(v) + if vpcName == "" { + continue + } + vpcID, err := GetVPCID(client, strings.TrimSpace(vpcName)) if err != nil { - return err + klog.Errorf("failed updating instances cache for VPC %s. Error: %s", vpcName, err.Error()) + continue } - for _, r := range resp { - if r.Address == nil { - continue + if vpcID != 0 { + resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) + if err != nil { + return err + } + for _, r := range resp { + if r.Address == nil { + continue + } + vpcNodes[r.LinodeID] = append(vpcNodes[r.LinodeID], *r.Address) } - vpcNodes[r.LinodeID] = append(vpcNodes[r.LinodeID], *r.Address) } } @@ -97,7 +108,7 @@ func (nc *nodeCache) refreshInstances(ctx context.Context, client client.Client) for i, instance := range instances { // if running within VPC, only store instances in cache which are part of VPC - if vpcID != 0 && len(vpcNodes[instance.ID]) == 0 { + if Options.VPCNames != "" && len(vpcNodes[instance.ID]) == 0 { continue } node := linodeInstance{ diff --git a/cloud/linode/route_controller.go b/cloud/linode/route_controller.go index 909ea76f..fbfe9bcb 100644 --- a/cloud/linode/route_controller.go +++ b/cloud/linode/route_controller.go @@ -5,6 +5,7 @@ import ( "fmt" "os" "strconv" + "strings" "sync" "time" @@ -19,28 +20,40 @@ import ( ) type routeCache struct { - sync.RWMutex + Mu sync.RWMutex routes map[int][]linodego.VPCIP lastUpdate time.Time ttl time.Duration } +// RefreshCache checks if cache has expired and updates it accordingly func (rc *routeCache) refreshRoutes(ctx context.Context, client client.Client) error { - rc.Lock() - defer rc.Unlock() + rc.Mu.Lock() + defer rc.Mu.Unlock() if time.Since(rc.lastUpdate) < rc.ttl { return nil } vpcNodes := map[int][]linodego.VPCIP{} - vpcID := vpcInfo.getID() - resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) - if err != nil { - return err - } - for _, r := range resp { - vpcNodes[r.LinodeID] = append(vpcNodes[r.LinodeID], r) + vpcNames := strings.Split(Options.VPCNames, ",") + for _, v := range vpcNames { + vpcName := strings.TrimSpace(v) + if vpcName == "" { + continue + } + vpcID, err := GetVPCID(client, strings.TrimSpace(vpcName)) + if err != nil { + klog.Errorf("failed updating cache for VPC %s. Error: %s", vpcName, err.Error()) + continue + } + resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) + if err != nil { + return err + } + for _, r := range resp { + vpcNodes[r.LinodeID] = append(vpcNodes[r.LinodeID], r) + } } rc.routes = vpcNodes @@ -49,7 +62,6 @@ func (rc *routeCache) refreshRoutes(ctx context.Context, client client.Client) e } type routes struct { - vpcid int client client.Client instances *instances routeCache *routeCache @@ -64,13 +76,11 @@ func newRoutes(client client.Client) (cloudprovider.Routes, error) { } klog.V(3).Infof("TTL for routeCache set to %d seconds", timeout) - vpcid := vpcInfo.getID() - if Options.EnableRouteController && vpcid == 0 { - return nil, fmt.Errorf("cannot enable route controller as vpc [%s] not found", Options.VPCName) + if Options.EnableRouteController && Options.VPCNames == "" { + return nil, fmt.Errorf("cannot enable route controller as vpc-names is empty") } return &routes{ - vpcid: vpcid, client: client, instances: newInstances(client), routeCache: &routeCache{ @@ -82,8 +92,8 @@ func newRoutes(client client.Client) (cloudprovider.Routes, error) { // instanceRoutesByID returns routes for given instance id func (r *routes) instanceRoutesByID(id int) ([]linodego.VPCIP, error) { - r.routeCache.RLock() - defer r.routeCache.RUnlock() + r.routeCache.Mu.RLock() + defer r.routeCache.Mu.RUnlock() instanceRoutes, ok := r.routeCache.routes[id] if !ok { return nil, fmt.Errorf("no routes found for instance %d", id) @@ -135,22 +145,25 @@ func (r *routes) CreateRoute(ctx context.Context, clusterName string, nameHint s // check already configured routes intfRoutes := []string{} intfVPCIP := linodego.VPCIP{} - for _, ir := range instanceRoutes { - if ir.VPCID != r.vpcid { - continue - } - if ir.Address != nil { - intfVPCIP = ir - continue - } + for _, vpcid := range GetAllVPCIDs() { + for _, ir := range instanceRoutes { + if ir.VPCID != vpcid { + continue + } - if ir.AddressRange != nil && *ir.AddressRange == route.DestinationCIDR { - klog.V(4).Infof("Route already exists for node %s", route.TargetNode) - return nil - } + if ir.Address != nil { + intfVPCIP = ir + continue + } - intfRoutes = append(intfRoutes, *ir.AddressRange) + if ir.AddressRange != nil && *ir.AddressRange == route.DestinationCIDR { + klog.V(4).Infof("Route already exists for node %s", route.TargetNode) + return nil + } + + intfRoutes = append(intfRoutes, *ir.AddressRange) + } } if intfVPCIP.Address == nil { @@ -185,21 +198,24 @@ func (r *routes) DeleteRoute(ctx context.Context, clusterName string, route *clo // check already configured routes intfRoutes := []string{} intfVPCIP := linodego.VPCIP{} - for _, ir := range instanceRoutes { - if ir.VPCID != r.vpcid { - continue - } - if ir.Address != nil { - intfVPCIP = ir - continue - } + for _, vpcid := range GetAllVPCIDs() { + for _, ir := range instanceRoutes { + if ir.VPCID != vpcid { + continue + } - if ir.AddressRange != nil && *ir.AddressRange == route.DestinationCIDR { - continue - } + if ir.Address != nil { + intfVPCIP = ir + continue + } + + if ir.AddressRange != nil && *ir.AddressRange == route.DestinationCIDR { + continue + } - intfRoutes = append(intfRoutes, *ir.AddressRange) + intfRoutes = append(intfRoutes, *ir.AddressRange) + } } if intfVPCIP.Address == nil { @@ -234,17 +250,19 @@ func (r *routes) ListRoutes(ctx context.Context, clusterName string) ([]*cloudpr } // check for configured routes - for _, ir := range instanceRoutes { - if ir.Address != nil || ir.VPCID != r.vpcid { - continue - } + for _, vpcid := range GetAllVPCIDs() { + for _, ir := range instanceRoutes { + if ir.Address != nil || ir.VPCID != vpcid { + continue + } - if ir.AddressRange != nil { - route := &cloudprovider.Route{ - TargetNode: types.NodeName(instance.Label), - DestinationCIDR: *ir.AddressRange, + if ir.AddressRange != nil { + route := &cloudprovider.Route{ + TargetNode: types.NodeName(instance.Label), + DestinationCIDR: *ir.AddressRange, + } + configuredRoutes = append(configuredRoutes, route) } - configuredRoutes = append(configuredRoutes, route) } } } diff --git a/cloud/linode/route_controller_test.go b/cloud/linode/route_controller_test.go index 2235b18c..cf862340 100644 --- a/cloud/linode/route_controller_test.go +++ b/cloud/linode/route_controller_test.go @@ -10,17 +10,16 @@ import ( "github.com/stretchr/testify/assert" "k8s.io/apimachinery/pkg/types" cloudprovider "k8s.io/cloud-provider" + "k8s.io/utils/ptr" "github.com/linode/linode-cloud-controller-manager/cloud/linode/client/mocks" ) func TestListRoutes(t *testing.T) { - Options.VPCName = "test" + Options.VPCNames = "test" + vpcIDs["test"] = 1 Options.EnableRouteController = true - vpcInfo.id = 1 - vpcid := vpcInfo.getID() - nodeID := 123 name := "mock-instance" publicIPv4 := net.ParseIP("45.76.101.25") @@ -38,7 +37,7 @@ func TestListRoutes(t *testing.T) { client.EXPECT().ListInstances(gomock.Any(), gomock.Any()).Times(1).Return([]linodego.Instance{}, nil) client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return([]linodego.VPCIP{}, nil) - routes, err := routeController.ListRoutes(ctx, "abc") + routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) }) @@ -61,7 +60,7 @@ func TestListRoutes(t *testing.T) { client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return([]linodego.VPCIP{}, nil) - routes, err := routeController.ListRoutes(ctx, "abc") + routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) }) @@ -71,7 +70,7 @@ func TestListRoutes(t *testing.T) { { Address: &vpcIP, AddressRange: nil, - VPCID: vpcid, + VPCID: vpcIDs["test"], NAT1To1: nil, LinodeID: nodeID, }, @@ -87,7 +86,7 @@ func TestListRoutes(t *testing.T) { client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return(noRoutesInVPC, nil) - routes, err := routeController.ListRoutes(ctx, "abc") + routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) }) @@ -98,21 +97,21 @@ func TestListRoutes(t *testing.T) { { Address: &vpcIP, AddressRange: nil, - VPCID: vpcid, + VPCID: vpcIDs["test"], NAT1To1: nil, LinodeID: nodeID, }, { Address: nil, AddressRange: &addressRange1, - VPCID: vpcid, + VPCID: vpcIDs["test"], NAT1To1: nil, LinodeID: nodeID, }, { Address: nil, AddressRange: &addressRange2, - VPCID: vpcid, + VPCID: vpcIDs["test"], NAT1To1: nil, LinodeID: nodeID, }, @@ -128,7 +127,7 @@ func TestListRoutes(t *testing.T) { client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return(routesInVPC, nil) - routes, err := routeController.ListRoutes(ctx, "abc") + routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.NotEmpty(t, routes) assert.Equal(t, addressRange1, routes[0].DestinationCIDR) @@ -169,7 +168,7 @@ func TestListRoutes(t *testing.T) { client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return(routesInDifferentVPC, nil) - routes, err := routeController.ListRoutes(ctx, "abc") + routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) }) @@ -177,12 +176,10 @@ func TestListRoutes(t *testing.T) { func TestCreateRoute(t *testing.T) { ctx := context.Background() - Options.VPCName = "test" + Options.VPCNames = "dummy" + vpcIDs["dummy"] = 1 Options.EnableRouteController = true - vpcInfo.id = 1 - vpcid := vpcInfo.getID() - nodeID := 123 name := "mock-instance" publicIPv4 := net.ParseIP("45.76.101.25") @@ -202,14 +199,14 @@ func TestCreateRoute(t *testing.T) { { Address: &vpcIP, AddressRange: nil, - VPCID: vpcid, + VPCID: vpcIDs["dummy"], NAT1To1: nil, LinodeID: nodeID, }, } instanceConfigIntfWithVPCAndRoute := linodego.InstanceConfigInterface{ - VPCID: &vpcid, + VPCID: ptr.To(vpcIDs["dummy"]), IPv4: &linodego.VPCIPv4{VPC: vpcIP}, IPRanges: []string{"10.10.10.0/24"}, } @@ -238,14 +235,14 @@ func TestCreateRoute(t *testing.T) { { Address: &vpcIP, AddressRange: nil, - VPCID: vpcid, + VPCID: vpcIDs["dummy"], NAT1To1: nil, LinodeID: nodeID, }, { Address: nil, AddressRange: &addressRange1, - VPCID: vpcid, + VPCID: vpcIDs["dummy"], NAT1To1: nil, LinodeID: nodeID, }, @@ -279,14 +276,12 @@ func TestCreateRoute(t *testing.T) { } func TestDeleteRoute(t *testing.T) { - Options.VPCName = "test" + Options.VPCNames = "dummy" + vpcIDs["dummy"] = 1 Options.EnableRouteController = true ctx := context.Background() - vpcInfo.id = 1 - vpcid := vpcInfo.getID() - nodeID := 123 name := "mock-instance" publicIPv4 := net.ParseIP("45.76.101.25") @@ -326,14 +321,14 @@ func TestDeleteRoute(t *testing.T) { { Address: &vpcIP, AddressRange: nil, - VPCID: vpcid, + VPCID: vpcIDs["dummy"], NAT1To1: nil, LinodeID: nodeID, }, } instanceConfigIntfWithVPCAndNoRoute := linodego.InstanceConfigInterface{ - VPCID: &vpcid, + VPCID: ptr.To(vpcIDs["dummy"]), IPv4: &linodego.VPCIPv4{VPC: vpcIP}, IPRanges: []string{}, } @@ -356,14 +351,14 @@ func TestDeleteRoute(t *testing.T) { { Address: &vpcIP, AddressRange: nil, - VPCID: vpcid, + VPCID: vpcIDs["dummy"], NAT1To1: nil, LinodeID: nodeID, }, { Address: nil, AddressRange: &addressRange1, - VPCID: vpcid, + VPCID: vpcIDs["dummy"], NAT1To1: nil, LinodeID: nodeID, }, diff --git a/cloud/linode/vpc.go b/cloud/linode/vpc.go index fd40e731..fa505523 100644 --- a/cloud/linode/vpc.go +++ b/cloud/linode/vpc.go @@ -3,11 +3,18 @@ package linode import ( "context" "fmt" + "sync" "github.com/linode/linode-cloud-controller-manager/cloud/linode/client" "github.com/linode/linodego" ) +var ( + Mu sync.RWMutex + // vpcIDs map stores vpc id's for given vpc labels + vpcIDs = make(map[string]int, 0) +) + type vpcLookupError struct { value string } @@ -16,14 +23,33 @@ func (e vpcLookupError) Error() string { return fmt.Sprintf("failed to find VPC: %q", e.value) } -// getVPCID returns the VPC id using the VPC label -func getVPCID(client client.Client, vpcName string) (int, error) { +// GetAllVPCIDs returns vpc ids stored in map +func GetAllVPCIDs() []int { + Mu.Lock() + defer Mu.Unlock() + values := make([]int, 0, len(vpcIDs)) + for _, v := range vpcIDs { + values = append(values, v) + } + return values +} + +// GetVPCID returns the VPC id of given VPC label +func GetVPCID(client client.Client, vpcName string) (int, error) { + Mu.Lock() + defer Mu.Unlock() + + // check if map contains vpc id for given label + if vpcid, ok := vpcIDs[vpcName]; ok { + return vpcid, nil + } vpcs, err := client.ListVPCs(context.TODO(), &linodego.ListOptions{}) if err != nil { return 0, err } for _, vpc := range vpcs { if vpc.Label == vpcName { + vpcIDs[vpcName] = vpc.ID return vpc.ID, nil } } diff --git a/deploy/chart/templates/daemonset.yaml b/deploy/chart/templates/daemonset.yaml index f8ed0397..fe812f0a 100644 --- a/deploy/chart/templates/daemonset.yaml +++ b/deploy/chart/templates/daemonset.yaml @@ -38,7 +38,12 @@ spec: {{- end }} {{- if .Values.routeController }} - --enable-route-controller=true - - --vpc-name={{ required "A valid .Values.routeController.vpcName is required" .Values.routeController.vpcName }} + {{- if .Values.routeController.vpcName }} + - --vpc-name={{ "A valid .Values.routeController.vpcName is required" .Values.routeController.vpcName }} + {{- end }} + {{- if .Values.routeController.vpcNames }} + - --vpc-names={{ "A valid .Values.routeController.vpcNames is required" .Values.routeController.vpcNames }} + {{- end }} - --configure-cloud-routes={{ default true .Values.routeController.configureCloudRoutes }} - --cluster-cidr={{ required "A valid .Values.routeController.clusterCIDR is required" .Values.routeController.clusterCIDR }} {{- if .Values.routeController.routeReconciliationPeriod }} diff --git a/deploy/chart/values.yaml b/deploy/chart/values.yaml index d360beef..5bd4546c 100644 --- a/deploy/chart/values.yaml +++ b/deploy/chart/values.yaml @@ -51,7 +51,8 @@ tolerations: # This section adds ability to enable route-controller for ccm # routeController: -# vpcName: +# vpcName: [Deprecated: use vpcNames instead] +# vpcNames: # clusterCIDR: 10.0.0.0/8 # configureCloudRoutes: true diff --git a/main.go b/main.go index f8b3c1ee..455c9475 100644 --- a/main.go +++ b/main.go @@ -81,7 +81,8 @@ func main() { // Add Linode-specific flags command.Flags().BoolVar(&linode.Options.LinodeGoDebug, "linodego-debug", false, "enables debug output for the LinodeAPI wrapper") command.Flags().BoolVar(&linode.Options.EnableRouteController, "enable-route-controller", false, "enables route_controller for ccm") - command.Flags().StringVar(&linode.Options.VPCName, "vpc-name", "", "vpc name whose routes will be managed by route-controller") + command.Flags().StringVar(&linode.Options.VPCName, "vpc-name", "", "[deprecated] vpc name whose routes will be managed by route-controller") + command.Flags().StringVar(&linode.Options.VPCNames, "vpc-names", "", "comma separated vpc names whose routes will be managed by route-controller") command.Flags().StringVar(&linode.Options.LoadBalancerType, "load-balancer-type", "nodebalancer", "configures which type of load-balancing to use for LoadBalancer Services (options: nodebalancer, cilium-bgp)") command.Flags().StringVar(&linode.Options.BGPNodeSelector, "bgp-node-selector", "", "node selector to use to perform shared IP fail-over with BGP (e.g. cilium-bgp-peering=true") From 43489cb75c1f7a0ce40b0b8bf3d56aa2064681e3 Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Wed, 6 Nov 2024 15:27:15 +0000 Subject: [PATCH 2/6] fix readme and helm chart --- README.md | 2 +- deploy/chart/templates/daemonset.yaml | 4 ++-- main.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 957de726..93aefe00 100644 --- a/README.md +++ b/README.md @@ -176,7 +176,7 @@ When running k8s clusters within VPC, node specific podCIDRs need to be allowed ##### Example usage in values.yaml ```yaml routeController: - vpcName: + vpcNames: clusterCIDR: 10.0.0.0/8 configureCloudRoutes: true ``` diff --git a/deploy/chart/templates/daemonset.yaml b/deploy/chart/templates/daemonset.yaml index fe812f0a..fcfff9d9 100644 --- a/deploy/chart/templates/daemonset.yaml +++ b/deploy/chart/templates/daemonset.yaml @@ -39,10 +39,10 @@ spec: {{- if .Values.routeController }} - --enable-route-controller=true {{- if .Values.routeController.vpcName }} - - --vpc-name={{ "A valid .Values.routeController.vpcName is required" .Values.routeController.vpcName }} + - --vpc-name={{ .Values.routeController.vpcName }} {{- end }} {{- if .Values.routeController.vpcNames }} - - --vpc-names={{ "A valid .Values.routeController.vpcNames is required" .Values.routeController.vpcNames }} + - --vpc-names={{ .Values.routeController.vpcNames }} {{- end }} - --configure-cloud-routes={{ default true .Values.routeController.configureCloudRoutes }} - --cluster-cidr={{ required "A valid .Values.routeController.clusterCIDR is required" .Values.routeController.clusterCIDR }} diff --git a/main.go b/main.go index 455c9475..2ef42d02 100644 --- a/main.go +++ b/main.go @@ -81,7 +81,7 @@ func main() { // Add Linode-specific flags command.Flags().BoolVar(&linode.Options.LinodeGoDebug, "linodego-debug", false, "enables debug output for the LinodeAPI wrapper") command.Flags().BoolVar(&linode.Options.EnableRouteController, "enable-route-controller", false, "enables route_controller for ccm") - command.Flags().StringVar(&linode.Options.VPCName, "vpc-name", "", "[deprecated] vpc name whose routes will be managed by route-controller") + command.Flags().StringVar(&linode.Options.VPCName, "vpc-name", "", "[deprecated: use vpc-names instead] vpc name whose routes will be managed by route-controller") command.Flags().StringVar(&linode.Options.VPCNames, "vpc-names", "", "comma separated vpc names whose routes will be managed by route-controller") command.Flags().StringVar(&linode.Options.LoadBalancerType, "load-balancer-type", "nodebalancer", "configures which type of load-balancing to use for LoadBalancer Services (options: nodebalancer, cilium-bgp)") command.Flags().StringVar(&linode.Options.BGPNodeSelector, "bgp-node-selector", "", "node selector to use to perform shared IP fail-over with BGP (e.g. cilium-bgp-peering=true") From c2e541bec62f938dc5583e9044393b5ba852a94b Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Fri, 8 Nov 2024 04:43:28 +0000 Subject: [PATCH 3/6] address review comments --- cloud/linode/cloud.go | 2 +- deploy/chart/templates/daemonset.yaml | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cloud/linode/cloud.go b/cloud/linode/cloud.go index 8d2b6cf2..f6ec2f7c 100644 --- a/cloud/linode/cloud.go +++ b/cloud/linode/cloud.go @@ -35,7 +35,7 @@ var Options struct { KubeconfigFlag *pflag.Flag LinodeGoDebug bool EnableRouteController bool - // deprecated: use VPCNames instead + // Deprecated: use VPCNames instead VPCName string VPCNames string LoadBalancerType string diff --git a/deploy/chart/templates/daemonset.yaml b/deploy/chart/templates/daemonset.yaml index fcfff9d9..0a46c517 100644 --- a/deploy/chart/templates/daemonset.yaml +++ b/deploy/chart/templates/daemonset.yaml @@ -38,6 +38,12 @@ spec: {{- end }} {{- if .Values.routeController }} - --enable-route-controller=true + {{- if and .Values.routeController.vpcName .Values.routeController.vpcNames }} + {{- fail "Both vpcName and vpcNames are set. Please use only vpcNames." }} + {{- end }} + {{- if not (or .Values.routeController.vpcName .Values.routeController.vpcNames) }} + {{- fail "Neither vpcName nor vpcNames is set. Please set one of them." }} + {{- end }} {{- if .Values.routeController.vpcName }} - --vpc-name={{ .Values.routeController.vpcName }} {{- end }} From 4527c56b1742a79af4f1dbd2bbf781419d3624ce Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Tue, 12 Nov 2024 06:09:22 +0000 Subject: [PATCH 4/6] add test for multiple vpcs --- cloud/linode/route_controller_test.go | 75 ++++++++++++++++++++++++--- 1 file changed, 69 insertions(+), 6 deletions(-) diff --git a/cloud/linode/route_controller_test.go b/cloud/linode/route_controller_test.go index cf862340..6b2efc64 100644 --- a/cloud/linode/route_controller_test.go +++ b/cloud/linode/route_controller_test.go @@ -16,8 +16,9 @@ import ( ) func TestListRoutes(t *testing.T) { - Options.VPCNames = "test" + Options.VPCNames = "test,abc" vpcIDs["test"] = 1 + vpcIDs["abc"] = 2 Options.EnableRouteController = true nodeID := 123 @@ -36,7 +37,7 @@ func TestListRoutes(t *testing.T) { assert.NoError(t, err) client.EXPECT().ListInstances(gomock.Any(), gomock.Any()).Times(1).Return([]linodego.Instance{}, nil) - client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return([]linodego.VPCIP{}, nil) + client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return([]linodego.VPCIP{}, nil) routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) @@ -59,7 +60,7 @@ func TestListRoutes(t *testing.T) { assert.NoError(t, err) client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) - client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return([]linodego.VPCIP{}, nil) + client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return([]linodego.VPCIP{}, nil) routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) @@ -85,7 +86,7 @@ func TestListRoutes(t *testing.T) { assert.NoError(t, err) client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) - client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return(noRoutesInVPC, nil) + client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(4).Return(noRoutesInVPC, nil) routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) @@ -126,7 +127,7 @@ func TestListRoutes(t *testing.T) { assert.NoError(t, err) client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) - client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return(routesInVPC, nil) + client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(4).Return(routesInVPC, nil) routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.NotEmpty(t, routes) @@ -167,11 +168,73 @@ func TestListRoutes(t *testing.T) { assert.NoError(t, err) client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance}, nil) - client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(2).Return(routesInDifferentVPC, nil) + client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(4).Return(routesInDifferentVPC, nil) routes, err := routeController.ListRoutes(ctx, "test") assert.NoError(t, err) assert.Empty(t, routes) }) + + t.Run("should return routes if multiple instances exists, connected to VPCs and ip_ranges configured", func(t *testing.T) { + ctx := context.Background() + ctrl := gomock.NewController(t) + defer ctrl.Finish() + client := mocks.NewMockClient(ctrl) + routeController, err := newRoutes(client) + assert.NoError(t, err) + + vpcIP2 := "10.0.0.3" + addressRange3 := "10.192.40.0/24" + addressRange4 := "10.192.50.0/24" + + validInstance2 := linodego.Instance{ + ID: 124, + Label: "mock-instance2", + Type: linodeType, + Region: region, + IPv4: []*net.IP{&publicIPv4, &privateIPv4}, + } + + routesInVPC2 := []linodego.VPCIP{ + { + Address: &vpcIP2, + AddressRange: nil, + VPCID: vpcIDs["abc"], + NAT1To1: nil, + LinodeID: 124, + }, + { + Address: nil, + AddressRange: &addressRange3, + VPCID: vpcIDs["abc"], + NAT1To1: nil, + LinodeID: 124, + }, + { + Address: nil, + AddressRange: &addressRange4, + VPCID: vpcIDs["abc"], + NAT1To1: nil, + LinodeID: 124, + }, + } + + client.EXPECT().ListInstances(gomock.Any(), nil).Times(1).Return([]linodego.Instance{validInstance, validInstance2}, nil) + c1 := client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).Times(1).Return(routesInVPC, nil) + c2 := client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).After(c1).Times(1).Return(routesInVPC2, nil) + c3 := client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).After(c2).Times(1).Return(routesInVPC, nil) + client.EXPECT().ListVPCIPAddresses(gomock.Any(), gomock.Any(), gomock.Any()).After(c3).Times(1).Return(routesInVPC2, nil) + routes, err := routeController.ListRoutes(ctx, "test") + assert.NoError(t, err) + assert.NotEmpty(t, routes) + cidrs := make([]string, len(routes)) + for i, value := range routes { + cidrs[i] = value.DestinationCIDR + } + assert.Contains(t, cidrs, addressRange1) + assert.Contains(t, cidrs, addressRange2) + assert.Contains(t, cidrs, addressRange3) + assert.Contains(t, cidrs, addressRange4) + }) } func TestCreateRoute(t *testing.T) { From f95b9bd9ae072f81548d23b27a9ae3db5d82de7a Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Wed, 13 Nov 2024 07:29:09 +0000 Subject: [PATCH 5/6] remove vpc from cache if it doesn't exist --- cloud/linode/instances.go | 16 +++++----------- cloud/linode/route_controller.go | 16 ++++------------ cloud/linode/vpc.go | 25 +++++++++++++++++++++++-- 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/cloud/linode/instances.go b/cloud/linode/instances.go index 80c4853f..358aa569 100644 --- a/cloud/linode/instances.go +++ b/cloud/linode/instances.go @@ -85,22 +85,16 @@ func (nc *nodeCache) refreshInstances(ctx context.Context, client client.Client) if vpcName == "" { continue } - vpcID, err := GetVPCID(client, strings.TrimSpace(vpcName)) + resp, err := GetVPCIPAddresses(ctx, client, vpcName) if err != nil { klog.Errorf("failed updating instances cache for VPC %s. Error: %s", vpcName, err.Error()) continue } - if vpcID != 0 { - resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) - if err != nil { - return err - } - for _, r := range resp { - if r.Address == nil { - continue - } - vpcNodes[r.LinodeID] = append(vpcNodes[r.LinodeID], *r.Address) + for _, r := range resp { + if r.Address == nil { + continue } + vpcNodes[r.LinodeID] = append(vpcNodes[r.LinodeID], *r.Address) } } diff --git a/cloud/linode/route_controller.go b/cloud/linode/route_controller.go index fbfe9bcb..8b3bdf47 100644 --- a/cloud/linode/route_controller.go +++ b/cloud/linode/route_controller.go @@ -27,12 +27,12 @@ type routeCache struct { } // RefreshCache checks if cache has expired and updates it accordingly -func (rc *routeCache) refreshRoutes(ctx context.Context, client client.Client) error { +func (rc *routeCache) refreshRoutes(ctx context.Context, client client.Client) { rc.Mu.Lock() defer rc.Mu.Unlock() if time.Since(rc.lastUpdate) < rc.ttl { - return nil + return } vpcNodes := map[int][]linodego.VPCIP{} @@ -42,15 +42,11 @@ func (rc *routeCache) refreshRoutes(ctx context.Context, client client.Client) e if vpcName == "" { continue } - vpcID, err := GetVPCID(client, strings.TrimSpace(vpcName)) + resp, err := GetVPCIPAddresses(ctx, client, vpcName) if err != nil { klog.Errorf("failed updating cache for VPC %s. Error: %s", vpcName, err.Error()) continue } - resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) - if err != nil { - return err - } for _, r := range resp { vpcNodes[r.LinodeID] = append(vpcNodes[r.LinodeID], r) } @@ -58,7 +54,6 @@ func (rc *routeCache) refreshRoutes(ctx context.Context, client client.Client) e rc.routes = vpcNodes rc.lastUpdate = time.Now() - return nil } type routes struct { @@ -104,10 +99,7 @@ func (r *routes) instanceRoutesByID(id int) ([]linodego.VPCIP, error) { // getInstanceRoutes returns routes for given instance id // It refreshes routeCache if it has expired func (r *routes) getInstanceRoutes(ctx context.Context, id int) ([]linodego.VPCIP, error) { - if err := r.routeCache.refreshRoutes(ctx, r.client); err != nil { - return nil, err - } - + r.routeCache.refreshRoutes(ctx, r.client) return r.instanceRoutesByID(id) } diff --git a/cloud/linode/vpc.go b/cloud/linode/vpc.go index fa505523..274904aa 100644 --- a/cloud/linode/vpc.go +++ b/cloud/linode/vpc.go @@ -3,10 +3,12 @@ package linode import ( "context" "fmt" + "strings" "sync" "github.com/linode/linode-cloud-controller-manager/cloud/linode/client" "github.com/linode/linodego" + "k8s.io/klog/v2" ) var ( @@ -35,7 +37,7 @@ func GetAllVPCIDs() []int { } // GetVPCID returns the VPC id of given VPC label -func GetVPCID(client client.Client, vpcName string) (int, error) { +func GetVPCID(ctx context.Context, client client.Client, vpcName string) (int, error) { Mu.Lock() defer Mu.Unlock() @@ -43,7 +45,7 @@ func GetVPCID(client client.Client, vpcName string) (int, error) { if vpcid, ok := vpcIDs[vpcName]; ok { return vpcid, nil } - vpcs, err := client.ListVPCs(context.TODO(), &linodego.ListOptions{}) + vpcs, err := client.ListVPCs(ctx, &linodego.ListOptions{}) if err != nil { return 0, err } @@ -55,3 +57,22 @@ func GetVPCID(client client.Client, vpcName string) (int, error) { } return 0, vpcLookupError{vpcName} } + +// GetVPCIPAddresses returns vpc ip's for given VPC label +func GetVPCIPAddresses(ctx context.Context, client client.Client, vpcName string) ([]linodego.VPCIP, error) { + vpcID, err := GetVPCID(ctx, client, strings.TrimSpace(vpcName)) + if err != nil { + return nil, err + } + resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) + if err != nil { + if strings.Contains(err.Error(), "Not found") { + Mu.Lock() + defer Mu.Unlock() + klog.Errorf("vpc %s not found. Deleting entry from cache", vpcName) + delete(vpcIDs, vpcName) + } + return nil, err + } + return resp, nil +} From 51944067ebab8f44af062bee72d7655320ecdd6b Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Wed, 13 Nov 2024 20:11:58 +0000 Subject: [PATCH 6/6] address review comments --- cloud/linode/vpc.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cloud/linode/vpc.go b/cloud/linode/vpc.go index 274904aa..01c1ed14 100644 --- a/cloud/linode/vpc.go +++ b/cloud/linode/vpc.go @@ -3,6 +3,7 @@ package linode import ( "context" "fmt" + "net/http" "strings" "sync" @@ -66,7 +67,7 @@ func GetVPCIPAddresses(ctx context.Context, client client.Client, vpcName string } resp, err := client.ListVPCIPAddresses(ctx, vpcID, linodego.NewListOptions(0, "")) if err != nil { - if strings.Contains(err.Error(), "Not found") { + if linodego.ErrHasStatus(err, http.StatusNotFound) { Mu.Lock() defer Mu.Unlock() klog.Errorf("vpc %s not found. Deleting entry from cache", vpcName)