Skip to content

Commit

Permalink
Adding unit test for PD fallback
Browse files Browse the repository at this point in the history
Signed-off-by: Hemanth Malla <hemanth.malla@datadoghq.com>
  • Loading branch information
hemanthmalla authored and ldelossa committed Mar 1, 2024
1 parent 5a487b5 commit 0007e35
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 16 deletions.
12 changes: 12 additions & 0 deletions pkg/aws/ec2/ec2.go
Expand Up @@ -27,6 +27,18 @@ import (
"github.com/cilium/cilium/pkg/spanstat"
)

const (
SubnetFullErrMsgStr = "There aren't sufficient free Ipv4 addresses or prefixes"

// InsufficientPrefixesInSubnetStr AWS error code for insufficient /28 prefixes in a subnet, possibly due to
// fragmentation
InsufficientPrefixesInSubnetStr = "InsufficientCidrBlocks"

// InvalidParameterValueStr sort of catch-all error code from AWS to indicate request params are invalid. Often,
// requires looking at the error message to get the actual reason. See SubnetFullErrMsgStr for example.
InvalidParameterValueStr = "InvalidParameterValue"
)

// Client represents an EC2 API client
type Client struct {
ec2Client *ec2.Client
Expand Down
7 changes: 6 additions & 1 deletion pkg/aws/ec2/mock/mock.go
Expand Up @@ -9,6 +9,7 @@ import (
"net"

"github.com/cilium/cilium/pkg/api/helpers"
"github.com/cilium/cilium/pkg/aws/ec2"
eniTypes "github.com/cilium/cilium/pkg/aws/eni/types"
"github.com/cilium/cilium/pkg/aws/types"
"github.com/cilium/cilium/pkg/ip"
Expand All @@ -20,6 +21,7 @@ import (
"github.com/cilium/cilium/pkg/time"

ec2_types "github.com/aws/aws-sdk-go-v2/service/ec2/types"
"github.com/aws/smithy-go"
"github.com/google/uuid"
log "github.com/sirupsen/logrus"
"golang.org/x/time/rate"
Expand Down Expand Up @@ -433,7 +435,10 @@ func assignPrefixToENI(e *API, eni *eniTypes.ENI, prefixes int32) error {
}

if int(prefixes)*option.ENIPDBlockSizeIPv4 > subnet.AvailableAddresses {
return fmt.Errorf("subnet %s has not enough addresses available", eni.Subnet.ID)
return &smithy.GenericAPIError{
Code: ec2.InvalidParameterValueStr,
Message: ec2.SubnetFullErrMsgStr,
}
}

for i := int32(0); i < prefixes; i++ {
Expand Down
19 changes: 5 additions & 14 deletions pkg/aws/eni/node.go
Expand Up @@ -19,6 +19,7 @@ import (
"github.com/sirupsen/logrus"

operatorOption "github.com/cilium/cilium/operator/option"
"github.com/cilium/cilium/pkg/aws/ec2"
"github.com/cilium/cilium/pkg/aws/eni/limits"
eniTypes "github.com/cilium/cilium/pkg/aws/eni/types"
"github.com/cilium/cilium/pkg/defaults"
Expand All @@ -39,16 +40,6 @@ const (

getMaximumAllocatableIPv4FailureWarningStr = "maximum allocatable ipv4 addresses will be 0 (unlimited)" +
" this could lead to ip allocation overflows if the max-allocate flag is not set"

subnetFullErrMsgStr = "There aren't sufficient free Ipv4 addresses or prefixes"

// insufficientPrefixesInSubnetStr AWS error code for insufficient /28 prefixes in a subnet, possibly due to
// fragmentation
insufficientPrefixesInSubnetStr = "InsufficientCidrBlocks"

// invalidParameterValueStr sort of catch-all error code from AWS to indicate request params are invalid. Often,
// requires looking at the error message to get the actual reason. See subnetFullErrMsgStr for example.
invalidParameterValueStr = "InvalidParameterValue"
)

type ipamNodeActions interface {
Expand Down Expand Up @@ -273,9 +264,9 @@ func (n *Node) PrepareIPAllocation(scopedLog *logrus.Entry) (a *ipam.AllocationA
func isSubnetAtPrefixCapacity(err error) bool {
var apiErr smithy.APIError
if errors.As(err, &apiErr) {
return apiErr.ErrorCode() == insufficientPrefixesInSubnetStr ||
(apiErr.ErrorCode() == invalidParameterValueStr &&
strings.Contains(apiErr.ErrorMessage(), subnetFullErrMsgStr))
return apiErr.ErrorCode() == ec2.InsufficientPrefixesInSubnetStr ||
(apiErr.ErrorCode() == ec2.InvalidParameterValueStr &&
strings.Contains(apiErr.ErrorMessage(), ec2.SubnetFullErrMsgStr))
}
return false
}
Expand Down Expand Up @@ -361,7 +352,7 @@ func (n *Node) errorInstanceNotRunning(err error) (notRunning bool) {
func isAttachmentIndexConflict(err error) bool {
var apiErr smithy.APIError
if errors.As(err, &apiErr) {
return apiErr.ErrorCode() == invalidParameterValueStr &&
return apiErr.ErrorCode() == ec2.InvalidParameterValueStr &&
strings.Contains(apiErr.ErrorMessage(), "interface attached at device")
}
return false
Expand Down
20 changes: 19 additions & 1 deletion pkg/aws/eni/node_manager_test.go
Expand Up @@ -160,7 +160,8 @@ func (e *ENISuite) TestNodeManagerDefaultAllocation(c *check.C) {
func (e *ENISuite) TestNodeManagerPrefixDelegation(c *check.C) {
const instanceID = "i-testNodeManagerDefaultAllocation-0"

ec2api := ec2mock.NewAPI([]*ipamTypes.Subnet{testSubnet}, []*ipamTypes.VirtualNetwork{testVpc}, testSecurityGroups)
pdTestSubnet := *testSubnet
ec2api := ec2mock.NewAPI([]*ipamTypes.Subnet{&pdTestSubnet}, []*ipamTypes.VirtualNetwork{testVpc}, testSecurityGroups)
instances := NewInstancesManager(ec2api)
c.Assert(instances, check.Not(check.IsNil))
eniID1, _, err := ec2api.CreateNetworkInterface(context.TODO(), 0, "s-1", "desc", []string{"sg1", "sg2"}, true)
Expand Down Expand Up @@ -198,6 +199,23 @@ func (e *ENISuite) TestNodeManagerPrefixDelegation(c *check.C) {
totalPrefixes += len(eni.Prefixes)
}
c.Assert(totalPrefixes, check.Equals, 2)

// Test fallback to /32 IPs when /28 blocks aren't available
//
// Set available IPs to a value insufficient to allocate a /28 block, but enough for /32 IPs to resolve
// pre-allocate deficit.
pdTestSubnet.AvailableAddresses = 15
ec2api.UpdateSubnets([]*ipamTypes.Subnet{&pdTestSubnet})

// Use 25 out of 32 IPs
mngr.Upsert(updateCiliumNode(cn, 32, 25))
c.Assert(testutils.WaitUntil(func() bool { return reachedAddressesNeeded(mngr, "node1", 0) }, 5*time.Second), check.IsNil)

node = mngr.Get("node1")
c.Assert(node, check.Not(check.IsNil))
// Should allocate only 1 additional IP after fallback, not an entire /28 prefix
c.Assert(node.Stats().AvailableIPs, check.Equals, 33)
c.Assert(node.Stats().UsedIPs, check.Equals, 25)
}

// TestNodeManagerENIWithSGTags tests ENI allocation + association with a SG based on tags
Expand Down

0 comments on commit 0007e35

Please sign in to comment.