Don't cache dynamic VPC IPv4 CIDR info

Rather than cache and then update the cache - just don't cache :)

Author: anguslees

pkg/awsutils/awsutils.go

@@ -129,7 +129,7 @@ type APIs interface {
 	DeallocIPAddresses(eniID string, ips []string) error
 
 	// GetVPCIPv4CIDRs returns VPC's CIDRs from instance metadata
-	GetVPCIPv4CIDRs() []string
+	GetVPCIPv4CIDRs() ([]string, error)
 
 	// GetLocalIPv4 returns the primary IP address on the primary ENI interface
 	GetLocalIPv4() net.IP
@@ -164,7 +164,6 @@ type EC2InstanceMetadataCache struct {
 	localIPv4           net.IP
 	instanceID          string
 	instanceType        string
-	vpcIPv4CIDRs        StringSet
 	primaryENI          string
 	primaryENImac       string
 	availabilityZone    string
@@ -401,16 +400,9 @@ func (cache *EC2InstanceMetadataCache) initWithEC2Metadata(ctx context.Context)
 		return err
 	}
 
-	// retrieve VPC IPv4 CIDR blocks
-	err = cache.refreshVPCIPv4CIDRs(mac)
-	if err != nil {
-		return err
-	}
-
 	// Refresh security groups and VPC CIDR blocks in the background
 	// Ignoring errors since we will retry in 30s
 	go wait.Forever(func() { _ = cache.refreshSGIDs(mac) }, 30*time.Second)
-	go wait.Forever(func() { _ = cache.refreshVPCIPv4CIDRs(mac) }, 30*time.Second)
 
 	// We use the ctx here for testing, since we spawn go-routines above which will run forever.
 	select {
@@ -484,36 +476,6 @@ func (cache *EC2InstanceMetadataCache) refreshSGIDs(mac string) error {
 	return nil
 }
 
-// refreshVPCIPv4CIDRs retrieves VPC IPv4 CIDR blocks
-func (cache *EC2InstanceMetadataCache) refreshVPCIPv4CIDRs(mac string) error {
-	ctx := context.TODO()
-
-	ipnets, err := cache.imds.GetVPCIPv4CIDRBlocks(ctx, mac)
-	if err != nil {
-		return err
-	}
-
-	// TODO: keep as net.IPNet and remove this round-trip to/from string
-	vpcIPv4CIDRs := make([]string, len(ipnets))
-	for i, ipnet := range ipnets {
-		vpcIPv4CIDRs[i] = ipnet.String()
-	}
-
-	newVpcIPv4CIDRs := StringSet{}
-	newVpcIPv4CIDRs.Set(vpcIPv4CIDRs)
-	addedVpcIPv4CIDRs := newVpcIPv4CIDRs.Difference(&cache.vpcIPv4CIDRs)
-	deletedVpcIPv4CIDRs := cache.vpcIPv4CIDRs.Difference(&newVpcIPv4CIDRs)
-
-	for _, vpcIPv4CIDR := range addedVpcIPv4CIDRs.SortedList() {
-		log.Infof("Found %s, added to ipamd cache", vpcIPv4CIDR)
-	}
-	for _, vpcIPv4CIDR := range deletedVpcIPv4CIDRs.SortedList() {
-		log.Infof("Removed %s from ipamd cache", vpcIPv4CIDR)
-	}
-	cache.vpcIPv4CIDRs.Set(vpcIPv4CIDRs)
-	return nil
-}
-
 // GetAttachedENIs retrieves ENI information from meta data service
 func (cache *EC2InstanceMetadataCache) GetAttachedENIs() (eniList []ENIMetadata, err error) {
 	ctx := context.TODO()
@@ -1454,8 +1416,21 @@ func (cache *EC2InstanceMetadataCache) getFilteredListOfNetworkInterfaces() ([]*
 }
 
 // GetVPCIPv4CIDRs returns VPC CIDRs
-func (cache *EC2InstanceMetadataCache) GetVPCIPv4CIDRs() []string {
-	return cache.vpcIPv4CIDRs.SortedList()
+func (cache *EC2InstanceMetadataCache) GetVPCIPv4CIDRs() ([]string, error) {
+	ctx := context.TODO()
+
+	ipnets, err := cache.imds.GetVPCIPv4CIDRBlocks(ctx, cache.primaryENImac)
+	if err != nil {
+		return nil, err
+	}
+
+	// TODO: keep as net.IPNet and remove this round-trip to/from string
+	asStrs := make([]string, len(ipnets))
+	for i, ipnet := range ipnets {
+		asStrs[i] = ipnet.String()
+	}
+
+	return asStrs, nil
 }
 
 // GetLocalIPv4 returns the primary IP address on the primary interface

pkg/awsutils/awsutils_test.go

@@ -124,7 +124,6 @@ func TestInitWithEC2metadata(t *testing.T) {
 		assert.Equal(t, ins.primaryENI, primaryeniID)
 		assert.Equal(t, len(ins.securityGroups.SortedList()), 2)
 		assert.Equal(t, subnetID, ins.subnetID)
-		assert.Equal(t, len(ins.vpcIPv4CIDRs.SortedList()), 2)
 	}
 }
 

pkg/awsutils/mocks/awsutils_mocks.go

@@ -17,7 +17,6 @@ import (
 	"fmt"
 	"net"
 	"os"
-	"reflect"
 	"strconv"
 	"strings"
 	"sync"
@@ -36,6 +35,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/pkg/errors"
 	"github.com/prometheus/client_golang/prometheus"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/client-go/kubernetes"
 )
@@ -332,7 +332,10 @@ func (c *IPAMContext) nodeInit() error {
 		return err
 	}
 
-	vpcCIDRs := c.awsClient.GetVPCIPv4CIDRs()
+	vpcCIDRs, err := c.awsClient.GetVPCIPv4CIDRs()
+	if err != nil {
+		return err
+	}
 	primaryIP := c.awsClient.GetLocalIPv4()
 	err = c.networkClient.SetupHostNetwork(vpcCIDRs, c.awsClient.GetPrimaryENImac(), &primaryIP, c.enablePodENI)
 	if err != nil {
@@ -382,6 +385,10 @@ func (c *IPAMContext) nodeInit() error {
 	if err = c.configureIPRulesForPods(vpcCIDRs); err != nil {
 		return err
 	}
+	// Spawning updateCIDRsRulesOnChange go-routine
+	go wait.Forever(func() {
+		vpcCIDRs = c.updateCIDRsRulesOnChange(vpcCIDRs)
+	}, 30*time.Second)
 
 	if c.useCustomNetworking && c.eniConfig.Getter().MyENI != "default" {
 		// Signal to VPC Resource Controller that the node is using custom networking
@@ -424,8 +431,6 @@ func (c *IPAMContext) nodeInit() error {
 		return err
 	}
 
-	// Spawning updateCIDRsRulesOnChange go-routine
-	go wait.Forever(func() { vpcCIDRs = c.updateCIDRsRulesOnChange(vpcCIDRs) }, 30*time.Second)
 	return nil
 }
 
@@ -450,10 +455,16 @@ func (c *IPAMContext) configureIPRulesForPods(pbVPCcidrs []string) error {
 	return nil
 }
 
-func (c *IPAMContext) updateCIDRsRulesOnChange(oldVPCCidrs []string) []string {
-	newVPCCIDRs := c.awsClient.GetVPCIPv4CIDRs()
+func (c *IPAMContext) updateCIDRsRulesOnChange(oldVPCCIDRs []string) []string {
+	newVPCCIDRs, err := c.awsClient.GetVPCIPv4CIDRs()
+	if err != nil {
+		log.Warnf("skipping periodic update to VPC CIDRs due to error: %v", err)
+		return oldVPCCIDRs
+	}
 
-	if len(oldVPCCidrs) != len(newVPCCIDRs) || !reflect.DeepEqual(oldVPCCidrs, newVPCCIDRs) {
+	old := sets.NewString(oldVPCCIDRs...)
+	new := sets.NewString(newVPCCIDRs...)
+	if !old.Equal(new) {
 		_ = c.configureIPRulesForPods(newVPCCIDRs)
 	}
 	return newVPCCIDRs

pkg/ipamd/ipamd.go

@@ -112,7 +112,7 @@ func TestNodeInit(t *testing.T) {
 	m.awsutils.EXPECT().IsUnmanagedENI(eni2.ENIID).Return(false).AnyTimes()
 
 	primaryIP := net.ParseIP(ipaddr01)
-	m.awsutils.EXPECT().GetVPCIPv4CIDRs().AnyTimes().Return(cidrs)
+	m.awsutils.EXPECT().GetVPCIPv4CIDRs().AnyTimes().Return(cidrs, nil)
 	m.awsutils.EXPECT().GetPrimaryENImac().Return("")
 	m.network.EXPECT().SetupHostNetwork(cidrs, "", &primaryIP, false).Return(nil)
 

pkg/ipamd/ipamd_test.go

@@ -140,7 +140,10 @@ func (s *server) AddNetwork(ctx context.Context, in *rpc.AddNetworkRequest) (*rp
 		}
 		addr, deviceNumber, err = s.ipamContext.dataStore.AssignPodIPv4Address(ipamKey)
 	}
-	pbVPCcidrs := s.ipamContext.awsClient.GetVPCIPv4CIDRs()
+	pbVPCcidrs, err := s.ipamContext.awsClient.GetVPCIPv4CIDRs()
+	if err != nil {
+		return nil, err
+	}
 	for _, cidr := range pbVPCcidrs {
 		log.Debugf("VPC CIDR %s", cidr)
 	}

pkg/ipamd/rpc_handler.go

@@ -37,7 +37,7 @@ func TestServer_VersionCheck(t *testing.T) {
 		networkClient: m.network,
 		dataStore:     datastore.NewDataStore(log, datastore.NullCheckpoint{}),
 	}
-	m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return([]string{})
+	m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return([]string{}, nil)
 	m.network.EXPECT().UseExternalSNAT().Return(true)
 
 	rpcServer := server{
@@ -126,18 +126,19 @@ func TestServer_AddNetwork(t *testing.T) {
 		},
 	}
 	for _, tc := range testCases {
-		m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return(tc.vpcCIDRs)
+		m.awsutils.EXPECT().GetVPCIPv4CIDRs().Return(tc.vpcCIDRs, nil)
 		m.network.EXPECT().UseExternalSNAT().Return(tc.useExternalSNAT)
 		if !tc.useExternalSNAT {
 			m.network.EXPECT().GetExcludeSNATCIDRs().Return(tc.snatExclusionCIDRs)
 		}
 
 		addNetworkReply, err := rpcServer.AddNetwork(context.TODO(), addNetworkRequest)
-		assert.NoError(t, err, tc.name)
+		if assert.NoError(t, err, tc.name) {
 
-		assert.Equal(t, tc.useExternalSNAT, addNetworkReply.UseExternalSNAT, tc.name)
+			assert.Equal(t, tc.useExternalSNAT, addNetworkReply.UseExternalSNAT, tc.name)
 
-		expectedCIDRs := append([]string{vpcCIDR}, tc.snatExclusionCIDRs...)
-		assert.Equal(t, expectedCIDRs, addNetworkReply.VPCcidrs, tc.name)
+			expectedCIDRs := append([]string{vpcCIDR}, tc.snatExclusionCIDRs...)
+			assert.Equal(t, expectedCIDRs, addNetworkReply.VPCcidrs, tc.name)
+		}
 	}
 }