From d72a94d1a329309665f96e96c161c236230094a0 Mon Sep 17 00:00:00 2001 From: Hao Zhou Date: Sat, 13 Jul 2024 16:34:52 +0000 Subject: [PATCH 01/12] add finalizer handler in v1.4 cr: https://code.amazon.com/reviews/CR-137885549 --- controllers/core/node_controller.go | 21 ++++++++++-- controllers/core/node_controller_test.go | 41 ++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/controllers/core/node_controller.go b/controllers/core/node_controller.go index 8a1f8b0e..8afdf74a 100644 --- a/controllers/core/node_controller.go +++ b/controllers/core/node_controller.go @@ -33,6 +33,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/healthz" ) @@ -43,6 +44,7 @@ import ( // when the controller has to be restarted for various reasons. const ( MaxNodeConcurrentReconciles = 10 + NodeTerminationFinalizer = "networking.k8s.aws/resource-cleanup" ) // NodeReconciler reconciles a Node object @@ -73,13 +75,26 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } node := &corev1.Node{} - var err error logger := r.Log.WithValues("node", req.NamespacedName) if err := r.Client.Get(ctx, req.NamespacedName, node); err != nil { if errors.IsNotFound(err) { - r.Log.V(1).Info("the requested node couldn't be found by k8s client", "Node", req.NamespacedName) + // clean up CNINode finalizer + cniNode := &v1alpha1.CNINode{} + if err = r.Client.Get(ctx, req.NamespacedName, cniNode); err == nil { + if yes := controllerutil.ContainsFinalizer(cniNode, NodeTerminationFinalizer); yes { + updated := cniNode.DeepCopy() + if yes = controllerutil.RemoveFinalizer(updated, NodeTerminationFinalizer); yes { + if err := r.Client.Patch(ctx, updated, client.MergeFrom(cniNode)); err != nil { + return ctrl.Result{}, err + } + r.Log.Info("removed leaked CNINode resource's finalizer", "cninode", cniNode.Name) + } + } + } + + // clean up local cached nodes _, found := r.Manager.GetNode(req.Name) if found { err := r.Manager.DeleteNode(req.Name) @@ -94,6 +109,8 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. return ctrl.Result{}, client.IgnoreNotFound(err) } + var err error + _, found := r.Manager.GetNode(req.Name) if found { logger.V(1).Info("updating node") diff --git a/controllers/core/node_controller_test.go b/controllers/core/node_controller_test.go index c592dccc..311a35b6 100644 --- a/controllers/core/node_controller_test.go +++ b/controllers/core/node_controller_test.go @@ -18,6 +18,7 @@ import ( "testing" "time" + "github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1" mock_condition "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/condition" mock_node "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node" mock_manager "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node/manager" @@ -25,11 +26,13 @@ import ( "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" fakeClient "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -61,6 +64,7 @@ func NewNodeMock(ctrl *gomock.Controller, mockObjects ...client.Object) NodeMock scheme := runtime.NewScheme() _ = corev1.AddToScheme(scheme) + _ = v1alpha1.AddToScheme(scheme) client := fakeClient.NewClientBuilder().WithScheme(scheme).WithObjects(mockObjects...).Build() return NodeMock{ @@ -139,6 +143,43 @@ func TestNodeReconciler_Reconcile_DeleteNonExistentNode(t *testing.T) { assert.Equal(t, res, reconcile.Result{}) } +func TestNodeReconciler_Reconcile_DeleteNonExistentNodesCNINode(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mock := NewNodeMock(ctrl) + cniNode := &v1alpha1.CNINode{ + ObjectMeta: v1.ObjectMeta{ + Name: mockNodeName, + Finalizers: []string{NodeTerminationFinalizer}, + }, + } + mock.Reconciler.Client = fakeClient.NewClientBuilder().WithScheme(mock.Reconciler.Scheme).WithObjects(cniNode).Build() + + mock.Conditions.EXPECT().GetPodDataStoreSyncStatus().Return(true) + mock.Manager.EXPECT().GetNode(mockNodeName).Return(mock.MockNode, false) + + original := &v1alpha1.CNINode{} + err := mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, original) + assert.NoError(t, err) + assert.True(t, controllerutil.ContainsFinalizer(original, NodeTerminationFinalizer), "the CNINode has finalizer") + + res, err := mock.Reconciler.Reconcile(context.TODO(), reconcileRequest) + assert.NoError(t, err) + assert.Equal(t, res, reconcile.Result{}) + + node := &corev1.Node{} + updated := &v1alpha1.CNINode{} + err = mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, node) + assert.Error(t, err, "the node shouldn't existing") + assert.True(t, errors.IsNotFound(err)) + + err = mock.Reconciler.Client.Get(context.TODO(), types.NamespacedName{Name: cniNode.Name}, updated) + assert.NoError(t, err) + assert.True(t, updated.Name == mockNodeName, "the CNINode should existing and waiting for finalizer removal") + assert.False(t, controllerutil.ContainsFinalizer(updated, NodeTerminationFinalizer), "CNINode finalizer should be removed when the node is gone") +} + func TestNodeReconciler_Reconcile_DeleteNonExistentUnmanagedNode(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() From 22745e6abbfc472e2746611aa2a28550f6a6cb22 Mon Sep 17 00:00:00 2001 From: Hao Zhou Date: Fri, 2 Aug 2024 20:33:43 +0000 Subject: [PATCH 02/12] fix an err variable cr: https://code.amazon.com/reviews/CR-141063909 --- controllers/core/node_controller.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/controllers/core/node_controller.go b/controllers/core/node_controller.go index 8afdf74a..92885de9 100644 --- a/controllers/core/node_controller.go +++ b/controllers/core/node_controller.go @@ -78,11 +78,11 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. logger := r.Log.WithValues("node", req.NamespacedName) - if err := r.Client.Get(ctx, req.NamespacedName, node); err != nil { - if errors.IsNotFound(err) { + if nodeErr := r.Client.Get(ctx, req.NamespacedName, node); nodeErr != nil { + if errors.IsNotFound(nodeErr) { // clean up CNINode finalizer cniNode := &v1alpha1.CNINode{} - if err = r.Client.Get(ctx, req.NamespacedName, cniNode); err == nil { + if cninodeErr := r.Client.Get(ctx, req.NamespacedName, cniNode); cninodeErr == nil { if yes := controllerutil.ContainsFinalizer(cniNode, NodeTerminationFinalizer); yes { updated := cniNode.DeepCopy() if yes = controllerutil.RemoveFinalizer(updated, NodeTerminationFinalizer); yes { @@ -92,21 +92,23 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. r.Log.Info("removed leaked CNINode resource's finalizer", "cninode", cniNode.Name) } } + } else if !errors.IsNotFound(cninodeErr) { + return ctrl.Result{}, cninodeErr } // clean up local cached nodes _, found := r.Manager.GetNode(req.Name) if found { - err := r.Manager.DeleteNode(req.Name) - if err != nil { + cacheErr := r.Manager.DeleteNode(req.Name) + if cacheErr != nil { // The request is not retryable so not returning the error - logger.Error(err, "failed to delete node from manager") + logger.Error(cacheErr, "failed to delete node from manager") return ctrl.Result{}, nil } logger.V(1).Info("deleted the node from manager") } } - return ctrl.Result{}, client.IgnoreNotFound(err) + return ctrl.Result{}, client.IgnoreNotFound(nodeErr) } var err error From c2fd513dad0b3ea0f569746f26d5043b89ee30fd Mon Sep 17 00:00:00 2001 From: Hao Zhou Date: Fri, 2 Aug 2024 04:25:41 +0000 Subject: [PATCH 03/12] adding logs for mismatched CNINode cr: https://code.amazon.com/reviews/CR-141072119 --- controllers/core/node_controller.go | 3 +- .../pkg/aws/ec2/mock_instance.go | 15 +++++++++ pkg/aws/ec2/instance.go | 8 +++++ pkg/node/manager/manager.go | 2 +- pkg/provider/branch/provider.go | 2 +- pkg/provider/branch/trunk/trunk.go | 31 +++++++++++++++++++ pkg/provider/branch/trunk/trunk_test.go | 2 ++ 7 files changed, 60 insertions(+), 3 deletions(-) diff --git a/controllers/core/node_controller.go b/controllers/core/node_controller.go index 92885de9..d7548948 100644 --- a/controllers/core/node_controller.go +++ b/controllers/core/node_controller.go @@ -15,6 +15,7 @@ package controllers import ( "context" + "fmt" "net/http" "time" @@ -93,7 +94,7 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } } } else if !errors.IsNotFound(cninodeErr) { - return ctrl.Result{}, cninodeErr + return ctrl.Result{}, fmt.Errorf("failed getting CNINode %s from cached client, %w", cniNode.Name, cninodeErr) } // clean up local cached nodes diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/mock_instance.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/mock_instance.go index d287cff4..92015b49 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/mock_instance.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/mock_instance.go @@ -73,6 +73,21 @@ func (mr *MockEC2InstanceMockRecorder) FreeDeviceIndex(arg0 interface{}) *gomock return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "FreeDeviceIndex", reflect.TypeOf((*MockEC2Instance)(nil).FreeDeviceIndex), arg0) } +// GetCustomNetworkingSpec mocks base method. +func (m *MockEC2Instance) GetCustomNetworkingSpec() (string, []string) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetCustomNetworkingSpec") + ret0, _ := ret[0].(string) + ret1, _ := ret[1].([]string) + return ret0, ret1 +} + +// GetCustomNetworkingSpec indicates an expected call of GetCustomNetworkingSpec. +func (mr *MockEC2InstanceMockRecorder) GetCustomNetworkingSpec() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCustomNetworkingSpec", reflect.TypeOf((*MockEC2Instance)(nil).GetCustomNetworkingSpec)) +} + // GetHighestUnusedDeviceIndex mocks base method. func (m *MockEC2Instance) GetHighestUnusedDeviceIndex() (int64, error) { m.ctrl.T.Helper() diff --git a/pkg/aws/ec2/instance.go b/pkg/aws/ec2/instance.go index adde9058..a513e871 100644 --- a/pkg/aws/ec2/instance.go +++ b/pkg/aws/ec2/instance.go @@ -79,6 +79,7 @@ type EC2Instance interface { PrimaryNetworkInterfaceID() string CurrentInstanceSecurityGroups() []string SetNewCustomNetworkingSpec(subnetID string, securityGroup []string) + GetCustomNetworkingSpec() (subnetID string, securityGroup []string) UpdateCurrentSubnetAndCidrBlock(helper api.EC2APIHelper) error } @@ -311,3 +312,10 @@ func (i *ec2Instance) updateCurrentSubnetAndCidrBlock(ec2APIHelper api.EC2APIHel return nil } + +func (i *ec2Instance) GetCustomNetworkingSpec() (subnetID string, securityGroup []string) { + i.lock.RLock() + defer i.lock.RUnlock() + + return i.newCustomNetworkingSubnetID, i.newCustomNetworkingSecurityGroups +} diff --git a/pkg/node/manager/manager.go b/pkg/node/manager/manager.go index c9b6f9c2..2759e775 100644 --- a/pkg/node/manager/manager.go +++ b/pkg/node/manager/manager.go @@ -228,7 +228,7 @@ func (m *manager) CreateCNINodeIfNotExisting(node *v1.Node) error { } return err } else { - m.Log.V(1).Info("The CNINode is already existing", "CNINode", cniNode) + m.Log.Info("The CNINode is already existing", "cninode", cniNode.Name, "features", cniNode.Spec.Features) return nil } } diff --git a/pkg/provider/branch/provider.go b/pkg/provider/branch/provider.go index 0b525a33..4bb3cb36 100644 --- a/pkg/provider/branch/provider.go +++ b/pkg/provider/branch/provider.go @@ -139,7 +139,7 @@ func timeSinceMs(start time.Time) float64 { // cache for use in future Create/Delete Requests func (b *branchENIProvider) InitResource(instance ec2.EC2Instance) error { nodeName := instance.Name() - log := b.log.WithValues("node name", nodeName) + log := b.log.WithValues("nodeName", nodeName) trunkENI := trunk.NewTrunkENI(log, instance, b.apiWrapper.EC2API) // Initialize the Trunk ENI diff --git a/pkg/provider/branch/trunk/trunk.go b/pkg/provider/branch/trunk/trunk.go index 1a5e1dd3..370a24ed 100644 --- a/pkg/provider/branch/trunk/trunk.go +++ b/pkg/provider/branch/trunk/trunk.go @@ -16,6 +16,7 @@ package trunk import ( "encoding/json" "fmt" + "slices" "strconv" "strings" "sync" @@ -27,6 +28,7 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/vpc" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" + "github.com/samber/lo" "github.com/aws/aws-sdk-go/aws" awsEC2 "github.com/aws/aws-sdk-go/service/ec2" @@ -192,6 +194,7 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error { return err } + var trunk awsEC2.InstanceNetworkInterface // Get trunk network interface for _, nwInterface := range nwInterfaces { // It's possible to get an empty network interface response if the instance is being deleted. @@ -206,6 +209,7 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error { } else { return fmt.Errorf("failed to verify network interface status attached for %v", *nwInterface.NetworkInterfaceId) } + trunk = *nwInterface } } @@ -231,6 +235,33 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error { return nil } + // the node already have trunk, let's check if its SGs and Subnets match with expected + expectedSubnetID, expectedSecurityGroups := t.instance.GetCustomNetworkingSpec() + if len(expectedSecurityGroups) > 0 || expectedSubnetID != "" { + slices.Sort(expectedSecurityGroups) + trunkSGs := lo.Map(trunk.Groups, func(g *awsEC2.GroupIdentifier, _ int) string { + return lo.FromPtr(g.GroupId) + }) + slices.Sort(trunkSGs) + + mismatchedSubnets := expectedSubnetID != lo.FromPtr(trunk.SubnetId) + mismatchedSGs := !slices.Equal(expectedSecurityGroups, trunkSGs) + + extraSGsInTrunk, missingSGsInTrunk := lo.Difference(trunkSGs, expectedSecurityGroups) + t.log.Info("Observed trunk ENI config", + "instanceID", t.instance.InstanceID(), + "trunkENIID", lo.FromPtr(trunk.NetworkInterfaceId), + "configuredTrunkSGs", trunkSGs, + "configuredTrunkSubnet", lo.FromPtr(trunk.SubnetId), + "desiredTrunkSGs", expectedSecurityGroups, + "desiredTrunkSubnet", expectedSubnetID, + "mismatchedSGs", mismatchedSGs, + "mismatchedSubnets", mismatchedSubnets, + "missingSGs", missingSGsInTrunk, + "extraSGs", extraSGsInTrunk, + ) + } + // Get the list of branch ENIs branchInterfaces, err := t.ec2ApiHelper.GetBranchNetworkInterface(&t.trunkENIId, aws.String(t.instance.SubnetID())) if err != nil { diff --git a/pkg/provider/branch/trunk/trunk_test.go b/pkg/provider/branch/trunk/trunk_test.go index 2f0eed90..49dcaf0d 100644 --- a/pkg/provider/branch/trunk/trunk_test.go +++ b/pkg/provider/branch/trunk/trunk_test.go @@ -645,6 +645,7 @@ func TestTrunkENI_InitTrunk(t *testing.T) { name: "TrunkExists_WithBranches, verifies no error when trunk exists with branches", prepare: func(f *fields) { f.mockInstance.EXPECT().InstanceID().Return(InstanceId) + f.mockInstance.EXPECT().GetCustomNetworkingSpec().Return("", []string{}) f.mockEC2APIHelper.EXPECT().GetInstanceNetworkInterface(&InstanceId).Return(instanceNwInterfaces, nil) f.mockEC2APIHelper.EXPECT().WaitForNetworkInterfaceStatusChange(&trunkId, awsEc2.AttachmentStatusAttached).Return(nil) f.mockInstance.EXPECT().SubnetID().Return(SubnetId) @@ -674,6 +675,7 @@ func TestTrunkENI_InitTrunk(t *testing.T) { name: "TrunkExists_DanglingENIs, verifies ENIs are pushed to delete queue if no pod exists", prepare: func(f *fields) { f.mockInstance.EXPECT().InstanceID().Return(InstanceId) + f.mockInstance.EXPECT().GetCustomNetworkingSpec().Return("", []string{}) f.mockEC2APIHelper.EXPECT().GetInstanceNetworkInterface(&InstanceId).Return(instanceNwInterfaces, nil) f.mockEC2APIHelper.EXPECT().WaitForNetworkInterfaceStatusChange(&trunkId, awsEc2.AttachmentStatusAttached).Return(nil) f.mockInstance.EXPECT().SubnetID().Return(SubnetId) From 1757e1e900769935bbd3f70bc58280310032e9ce Mon Sep 17 00:00:00 2001 From: Hao Zhou Date: Fri, 2 Aug 2024 23:40:10 +0000 Subject: [PATCH 04/12] add metrics for mismatches cr: https://code.amazon.com/reviews/CR-141092975 --- controllers/core/node_controller.go | 24 ++++++++++++++++++++++++ pkg/provider/branch/trunk/trunk.go | 16 ++++++++++++++++ 2 files changed, 40 insertions(+) diff --git a/controllers/core/node_controller.go b/controllers/core/node_controller.go index d7548948..6e750eec 100644 --- a/controllers/core/node_controller.go +++ b/controllers/core/node_controller.go @@ -25,6 +25,8 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/node/manager" "github.com/google/uuid" + "github.com/prometheus/client_golang/prometheus" + "sigs.k8s.io/controller-runtime/pkg/metrics" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" @@ -38,6 +40,17 @@ import ( "sigs.k8s.io/controller-runtime/pkg/healthz" ) +var ( + leakedCNINodeResourceCount = prometheus.NewCounter( + prometheus.CounterOpts{ + Name: "orphaned_cninode_objects", + Help: "The number of leaked cninode resources", + }, + ) + + prometheusRegistered = false +) + // MaxNodeConcurrentReconciles is the number of go routines that can invoke // Reconcile in parallel. Since Node Reconciler, performs local operation // on cache only a single go routine should be sufficient. Using more than @@ -92,6 +105,7 @@ func (r *NodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } r.Log.Info("removed leaked CNINode resource's finalizer", "cninode", cniNode.Name) } + leakedCNINodeResourceCount.Inc() } } else if !errors.IsNotFound(cninodeErr) { return ctrl.Result{}, fmt.Errorf("failed getting CNINode %s from cached client, %w", cniNode.Name, cninodeErr) @@ -135,6 +149,8 @@ func (r *NodeReconciler) SetupWithManager(mgr ctrl.Manager, healthzHandler *rcHe map[string]healthz.Checker{"health-node-controller": r.Check()}, ) + prometheusRegister() + return ctrl.NewControllerManagedBy(mgr). For(&corev1.Node{}). WithOptions(controller.Options{MaxConcurrentReconciles: MaxNodeConcurrentReconciles}). @@ -172,3 +188,11 @@ func (r *NodeReconciler) Check() healthz.Checker { return err } } + +func prometheusRegister() { + if !prometheusRegistered { + metrics.Registry.MustRegister(leakedCNINodeResourceCount) + + prometheusRegistered = true + } +} diff --git a/pkg/provider/branch/trunk/trunk.go b/pkg/provider/branch/trunk/trunk.go index 370a24ed..b4b9a699 100644 --- a/pkg/provider/branch/trunk/trunk.go +++ b/pkg/provider/branch/trunk/trunk.go @@ -64,6 +64,13 @@ var ( }, []string{"operation"}, ) + unreconciledTrunkENICount = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "unreconciled_trunk_network_interfaces", + Help: "The number of unreconciled trunk network interfaces", + }, + []string{"attribute"}, + ) branchENIOperationsSuccessCount = prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "branch_eni_opeartions_success_count", @@ -175,6 +182,7 @@ func NewTrunkENI(logger logr.Logger, instance ec2.EC2Instance, helper api.EC2API func PrometheusRegister() { if !prometheusRegistered { metrics.Registry.MustRegister(trunkENIOperationsErrCount) + metrics.Registry.MustRegister(unreconciledTrunkENICount) metrics.Registry.MustRegister(branchENIOperationsSuccessCount) metrics.Registry.MustRegister(branchENIOperationsFailureCount) @@ -260,6 +268,14 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error { "missingSGs", missingSGsInTrunk, "extraSGs", extraSGsInTrunk, ) + + if mismatchedSGs { + unreconciledTrunkENICount.WithLabelValues("security_groups").Inc() + } + + if mismatchedSubnets { + unreconciledTrunkENICount.WithLabelValues("subnet").Inc() + } } // Get the list of branch ENIs From 4bb81caa64a833c6b9f5aea13127dd8c776823ec Mon Sep 17 00:00:00 2001 From: Hao Zhou Date: Sat, 3 Aug 2024 02:04:04 +0000 Subject: [PATCH 05/12] update EC2 instance types --- controllers/core/node_controller.go | 2 +- pkg/aws/vpc/limits.go | 352 ++++++++++++++++++++++++---- pkg/provider/branch/trunk/trunk.go | 8 +- 3 files changed, 317 insertions(+), 45 deletions(-) diff --git a/controllers/core/node_controller.go b/controllers/core/node_controller.go index 6e750eec..7440939b 100644 --- a/controllers/core/node_controller.go +++ b/controllers/core/node_controller.go @@ -44,7 +44,7 @@ var ( leakedCNINodeResourceCount = prometheus.NewCounter( prometheus.CounterOpts{ Name: "orphaned_cninode_objects", - Help: "The number of leaked cninode resources", + Help: "The number of CNINode objects that do not have a parent Node object (likely indicating a leak from a deleted node)", }, ) diff --git a/pkg/aws/vpc/limits.go b/pkg/aws/vpc/limits.go index dbc17475..59bee69c 100644 --- a/pkg/aws/vpc/limits.go +++ b/pkg/aws/vpc/limits.go @@ -17,7 +17,7 @@ // so we can get this information at runtime. // Code generated by go generate; DO NOT EDIT. -// This file was generated at 2024-04-04T20:24:15Z +// This file was generated at 2024-08-03T00:54:51Z // WARNING: please add @ellistarn, @bwagner5, or @jonathan-innis from aws/karpenter to reviewers // if you are updating this file since Karpenter is depending on this file to calculate max pods. @@ -1846,19 +1846,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "c6in.32xlarge": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -1911,19 +1911,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "c6in.metal": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -6416,19 +6416,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "m6idn.32xlarge": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -6481,19 +6481,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "m6idn.metal": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -6576,19 +6576,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "m6in.32xlarge": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -6641,19 +6641,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "m6in.metal": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -7380,6 +7380,21 @@ var Limits = map[string]*VPCLimits{ Hypervisor: "", IsBareMetal: true, }, + "mac2-m1ultra.metal": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 6, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "", + IsBareMetal: true, + }, "mac2-m2.metal": { Interface: 8, IPv4PerInterface: 30, @@ -9606,19 +9621,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "r6idn.32xlarge": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -9671,19 +9686,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "r6idn.metal": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -9766,19 +9781,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "r6in.32xlarge": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -9831,19 +9846,19 @@ var Limits = map[string]*VPCLimits{ IsBareMetal: false, }, "r6in.metal": { - Interface: 14, + Interface: 16, IPv4PerInterface: 50, IsTrunkingCompatible: true, - BranchInterface: 108, + BranchInterface: 106, DefaultNetworkCardIndex: 0, NetworkCards: []NetworkCard{ { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 0, }, { - MaximumNetworkInterfaces: 7, + MaximumNetworkInterfaces: 8, NetworkCardIndex: 1, }, }, @@ -10630,6 +10645,186 @@ var Limits = map[string]*VPCLimits{ Hypervisor: "nitro", IsBareMetal: false, }, + "r8g.12xlarge": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 54, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.16xlarge": { + Interface: 15, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 107, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 15, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.24xlarge": { + Interface: 15, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 107, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 15, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.2xlarge": { + Interface: 4, + IPv4PerInterface: 15, + IsTrunkingCompatible: true, + BranchInterface: 38, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 4, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.48xlarge": { + Interface: 15, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 107, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 15, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.4xlarge": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 54, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.8xlarge": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 54, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.large": { + Interface: 3, + IPv4PerInterface: 10, + IsTrunkingCompatible: true, + BranchInterface: 9, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 3, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.medium": { + Interface: 2, + IPv4PerInterface: 4, + IsTrunkingCompatible: true, + BranchInterface: 4, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 2, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "r8g.metal-24xl": { + Interface: 15, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 107, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 15, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "", + IsBareMetal: true, + }, + "r8g.metal-48xl": { + Interface: 15, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 107, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 15, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "", + IsBareMetal: true, + }, + "r8g.xlarge": { + Interface: 4, + IPv4PerInterface: 15, + IsTrunkingCompatible: true, + BranchInterface: 18, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 4, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, "t1.micro": { Interface: 2, IPv4PerInterface: 2, @@ -11325,6 +11520,81 @@ var Limits = map[string]*VPCLimits{ Hypervisor: "nitro", IsBareMetal: false, }, + "u7i-12tb.224xlarge": { + Interface: 15, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 107, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 15, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "u7in-16tb.224xlarge": { + Interface: 16, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 106, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 1, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "u7in-24tb.224xlarge": { + Interface: 16, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 106, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 1, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "u7in-32tb.224xlarge": { + Interface: 16, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 106, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 1, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, "vt1.24xlarge": { Interface: 15, IPv4PerInterface: 50, diff --git a/pkg/provider/branch/trunk/trunk.go b/pkg/provider/branch/trunk/trunk.go index b4b9a699..71de6991 100644 --- a/pkg/provider/branch/trunk/trunk.go +++ b/pkg/provider/branch/trunk/trunk.go @@ -42,7 +42,9 @@ const ( // MaxAllocatableVlanIds is the maximum number of Vlan Ids that can be allocated per trunk. MaxAllocatableVlanIds = 121 // MaxDeleteRetries is the maximum number of times the ENI will be retried before being removed from the delete queue - MaxDeleteRetries = 3 + MaxDeleteRetries = 3 + SubnetLabel = "subnet" + SecurityGroupsLabel = "security_groups" ) var ( @@ -270,11 +272,11 @@ func (t *trunkENI) InitTrunk(instance ec2.EC2Instance, podList []v1.Pod) error { ) if mismatchedSGs { - unreconciledTrunkENICount.WithLabelValues("security_groups").Inc() + unreconciledTrunkENICount.WithLabelValues(SecurityGroupsLabel).Inc() } if mismatchedSubnets { - unreconciledTrunkENICount.WithLabelValues("subnet").Inc() + unreconciledTrunkENICount.WithLabelValues(SubnetLabel).Inc() } } From 35f0eccbe5f14d3592acfaa425af3d1d7e5591cb Mon Sep 17 00:00:00 2001 From: Jay Deokar <23660509+jaydeokar@users.noreply.github.com> Date: Wed, 11 Sep 2024 18:38:01 -0700 Subject: [PATCH 06/12] Update aws-sdk-go and change way to get regional sts endpoint (#466) --- go.mod | 6 +-- go.sum | 8 ++-- pkg/aws/ec2/api/wrapper.go | 50 ++++++++++++++++++++----- pkg/aws/ec2/api/wrapper_test.go | 65 +++++++++++++++++++++++++++++++++ pkg/utils/helper.go | 10 ++--- pkg/utils/helper_test.go | 15 +++++--- 6 files changed, 128 insertions(+), 26 deletions(-) create mode 100644 pkg/aws/ec2/api/wrapper_test.go diff --git a/go.mod b/go.mod index 1c748137..1eb0528c 100644 --- a/go.mod +++ b/go.mod @@ -3,9 +3,9 @@ module github.com/aws/amazon-vpc-resource-controller-k8s go 1.21 require ( - github.com/aws/amazon-vpc-cni-k8s v1.17.1 - github.com/aws/aws-sdk-go v1.51.12 - github.com/go-logr/logr v1.4.1 + github.com/aws/amazon-vpc-cni-k8s v1.18.1 + github.com/aws/aws-sdk-go v1.55.5 + github.com/go-logr/logr v1.4.2 github.com/go-logr/zapr v1.3.0 github.com/golang/mock v1.6.0 github.com/google/uuid v1.6.0 diff --git a/go.sum b/go.sum index f15bc12f..df69f536 100644 --- a/go.sum +++ b/go.sum @@ -1,9 +1,9 @@ github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5 h1:0CwZNZbxp69SHPdPJAN/hZIm0C4OItdklCFmMRWYpio= github.com/armon/go-socks5 v0.0.0-20160902184237-e75332964ef5/go.mod h1:wHh0iHkYZB8zMSxRWpUBQtwG5a7fFgvEO+odwuTv2gs= -github.com/aws/amazon-vpc-cni-k8s v1.17.1 h1:pF+AmlGbgK8/e58LbtOsLUzDy2hqI8Ug/D8Xxx7+Sis= -github.com/aws/amazon-vpc-cni-k8s v1.17.1/go.mod h1:fNfKsEUNrAj+046SGML0UQWLcsF7hAsKRqnvwIcflvw= -github.com/aws/aws-sdk-go v1.51.12 h1:DvuhIHZXwnjaR1/Gu19gUe1EGPw4J0qSJw4Qs/5PA8g= -github.com/aws/aws-sdk-go v1.51.12/go.mod h1:LF8svs817+Nz+DmiMQKTO3ubZ/6IaTpq3TjupRn3Eqk= +github.com/aws/amazon-vpc-cni-k8s v1.18.1 h1:u/OeBgnUUX6f3PCEOpA4dbG0+iZ71CnY6tEljjrl3iw= +github.com/aws/amazon-vpc-cni-k8s v1.18.1/go.mod h1:m/J5GsxF0Th2iQTOE3ww4W9LFvwdC0tGyA9dIL4h6iQ= +github.com/aws/aws-sdk-go v1.55.5 h1:KKUZBfBoyqy5d3swXyiC7Q76ic40rYcbqH7qjh59kzU= +github.com/aws/aws-sdk-go v1.55.5/go.mod h1:eRwEWoyTWFMVYVQzKMNHWP5/RV4xIUGMQfXQHfHkpNU= github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= diff --git a/pkg/aws/ec2/api/wrapper.go b/pkg/aws/ec2/api/wrapper.go index 81ca97a3..99c4a42f 100644 --- a/pkg/aws/ec2/api/wrapper.go +++ b/pkg/aws/ec2/api/wrapper.go @@ -455,21 +455,21 @@ func (e *ec2Wrapper) getClientUsingAssumedRole(instanceRegion, roleARN, clusterN } e.log.Info("created rate limited http client", "qps", qps, "burst", burst) - // Get the regional sts end point - regionalSTSEndpoint, err := endpoints.DefaultResolver(). - EndpointFor("sts", aws.StringValue(userStsSession.Config.Region), endpoints.STSRegionalEndpointOption) - if err != nil { - return nil, fmt.Errorf("failed to get the regional sts endoint for region %s: %v", - *userStsSession.Config.Region, err) - } - + // GetPartition ID, SourceAccount and SourceARN roleARN = strings.Trim(roleARN, "\"") - sourceAcct, sourceArn, err := utils.GetSourceAcctAndArn(roleARN, region, clusterName) + sourceAcct, partitionID, sourceArn, err := utils.GetSourceAcctAndArn(roleARN, region, clusterName) if err != nil { return nil, err } + // Get the regional sts end point + regionalSTSEndpoint, err := e.getRegionalStsEndpoint(partitionID, region) + if err != nil { + return nil, fmt.Errorf("failed to get the regional sts endpoint for region %s: %v %v", + *userStsSession.Config.Region, err, partitionID) + } + regionalProvider := &stscreds.AssumeRoleProvider{ Client: e.createSTSClient(userStsSession, client, regionalSTSEndpoint, sourceAcct, sourceArn), RoleARN: roleARN, @@ -789,3 +789,35 @@ func (e *ec2Wrapper) CreateNetworkInterfacePermission(input *ec2.CreateNetworkIn return output, err } + +func (e *ec2Wrapper) getRegionalStsEndpoint(partitionID, region string) (endpoints.ResolvedEndpoint, error) { + var partition *endpoints.Partition + var stsServiceID = "sts" + for _, p := range endpoints.DefaultPartitions() { + if partitionID == p.ID() { + partition = &p + break + } + } + if partition == nil { + return endpoints.ResolvedEndpoint{}, fmt.Errorf("partition %s not valid", partitionID) + } + + stsSvc, ok := partition.Services()[stsServiceID] + if !ok { + e.log.Info("STS service not found in partition, generating default endpoint.", "Partition:", partitionID) + // Add the host of the current instances region if the service doesn't already exists in the partition + // so we don't fail if the service is not present in the go sdk but matches the instances region. + res, err := partition.EndpointFor(stsServiceID, region, endpoints.STSRegionalEndpointOption, endpoints.ResolveUnknownServiceOption) + if err != nil { + return endpoints.ResolvedEndpoint{}, fmt.Errorf("error resolving endpoint for %s in partition %s. err: %v", region, partition.ID(), err) + } + return res, nil + } + + res, err := stsSvc.ResolveEndpoint(region, endpoints.STSRegionalEndpointOption) + if err != nil { + return endpoints.ResolvedEndpoint{}, fmt.Errorf("error resolving endpoint for %s in partition %s. err: %v", region, partition.ID(), err) + } + return res, nil +} diff --git a/pkg/aws/ec2/api/wrapper_test.go b/pkg/aws/ec2/api/wrapper_test.go new file mode 100644 index 00000000..281a8d59 --- /dev/null +++ b/pkg/aws/ec2/api/wrapper_test.go @@ -0,0 +1,65 @@ +package api + +import ( + "testing" +) + +func getMockEC2Wrapper() ec2Wrapper { + return ec2Wrapper{} +} +func Test_getRegionalStsEndpoint(t *testing.T) { + + ec2Wapper := getMockEC2Wrapper() + + type args struct { + partitionID string + region string + } + + tests := []struct { + name string + args args + want string + wantErr bool + }{ + { + name: "service doesn't exist in partition", + args: args{ + partitionID: "aws-iso-f", + region: "testregions", + }, + want: "https://sts.testregions.csp.hci.ic.gov", + wantErr: false, + }, + { + name: "region doesn't exist in partition", + args: args{ + partitionID: "aws", + region: "us-test-2", + }, + want: "https://sts.us-test-2.amazonaws.com", + wantErr: false, + }, + { + name: "region and service exist in partition", + args: args{ + partitionID: "aws", + region: "us-west-2", + }, + want: "https://sts.us-west-2.amazonaws.com", + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ec2Wapper.getRegionalStsEndpoint(tt.args.partitionID, tt.args.region) + if (err != nil) != tt.wantErr { + t.Errorf("getRegionalStsEndpoint() error = %v, wantErr %v", err, tt.wantErr) + return + } + if got.URL != tt.want { + t.Errorf("getRegionalStsEndpoint() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/pkg/utils/helper.go b/pkg/utils/helper.go index d6b6eeac..14fda665 100644 --- a/pkg/utils/helper.go +++ b/pkg/utils/helper.go @@ -213,22 +213,22 @@ func IsNitroInstance(instanceType string) (bool, error) { } // GetSourceAcctAndArn constructs source acct and arn and return them for use -func GetSourceAcctAndArn(roleARN, region, clusterName string) (string, string, error) { +func GetSourceAcctAndArn(roleARN, region, clusterName string) (string, string, string, error) { // ARN format (https://docs.aws.amazon.com/IAM/latest/UserGuide/reference-arns.html) // arn:partition:service:region:account-id:resource-type/resource-id // IAM format, region is always blank // arn:aws:iam::account:role/role-name-with-path if !arn.IsARN(roleARN) { - return "", "", fmt.Errorf("incorrect ARN format for role %s", roleARN) + return "", "", "", fmt.Errorf("incorrect ARN format for role %s", roleARN) } else if region == "" { - return "", "", nil + return "", "", "", nil } parsedArn, err := arn.Parse(roleARN) if err != nil { - return "", "", err + return "", "", "", err } sourceArn := fmt.Sprintf("arn:%s:eks:%s:%s:cluster/%s", parsedArn.Partition, region, parsedArn.AccountID, clusterName) - return parsedArn.AccountID, sourceArn, nil + return parsedArn.AccountID, parsedArn.Partition, sourceArn, nil } diff --git a/pkg/utils/helper_test.go b/pkg/utils/helper_test.go index 34a4e4d2..aee3659b 100644 --- a/pkg/utils/helper_test.go +++ b/pkg/utils/helper_test.go @@ -538,26 +538,29 @@ func TestGetSourceAcctAndArn(t *testing.T) { clusterName := "test-cluster" region := "us-west-2" clusterARN := "arn:aws:eks:us-west-2:123456789876:cluster/test-cluster" - + partition := "aws" roleARN := "arn:aws:iam::123456789876:role/test-cluster" // test correct inputs - acct, arn, err := GetSourceAcctAndArn(roleARN, region, clusterName) + acct, part, arn, err := GetSourceAcctAndArn(roleARN, region, clusterName) assert.NoError(t, err, "no error should be returned with accurate role arn") + assert.Equal(t, partition, part, "correct partition should be retrieved") assert.Equal(t, accountID, acct, "correct account ID should be retrieved") assert.Equal(t, clusterARN, arn, "correct cluster arn should be retrieved") region = "us-gov-west-1" roleARN = "arn:aws-us-gov:iam::123456789876:role/test-cluster" clusterARN = "arn:aws-us-gov:eks:us-gov-west-1:123456789876:cluster/test-cluster" - acct, arn, err = GetSourceAcctAndArn(roleARN, region, clusterName) + partition = "aws-us-gov" + acct, part, arn, err = GetSourceAcctAndArn(roleARN, region, clusterName) assert.NoError(t, err, "no error should be returned with accurate aws-us-gov partition role arn") assert.Equal(t, accountID, acct, "correct account ID should be retrieved") + assert.Equal(t, partition, part, "correct patition should be retrieved") assert.Equal(t, clusterARN, arn, "correct gov partition cluster arn should be retrieved") // test error handling roleARN = "arn:aws:iam::123456789876" - _, _, err = GetSourceAcctAndArn(roleARN, region, clusterName) + _, _, _, err = GetSourceAcctAndArn(roleARN, region, clusterName) assert.Error(t, err, "error should be returned with inaccurate role arn is given") } @@ -569,8 +572,10 @@ func TestGetSourceAcctAndArn_NoRegion(t *testing.T) { roleARN := "arn:aws:iam::123456789876:role/test-cluster" // test correct inputs - acct, arn, err := GetSourceAcctAndArn(roleARN, region, clusterName) + acct, part, arn, err := GetSourceAcctAndArn(roleARN, region, clusterName) assert.NoError(t, err, "no error should be returned with accurate role arn") assert.Equal(t, "", acct, "correct account ID should be retrieved") assert.Equal(t, "", arn, "correct cluster arn should be retrieved") + assert.Equal(t, "", part, "correct partiton should be retrieved") + } From 4e02e3521b09967985857f142fd221056e02920a Mon Sep 17 00:00:00 2001 From: Jay Deokar Date: Thu, 12 Sep 2024 03:27:29 +0000 Subject: [PATCH 07/12] Missing dependency update --- go.mod | 10 +++++----- go.sum | 24 ++++++++++++------------ 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/go.mod b/go.mod index 1eb0528c..b436af3d 100644 --- a/go.mod +++ b/go.mod @@ -9,13 +9,13 @@ require ( github.com/go-logr/zapr v1.3.0 github.com/golang/mock v1.6.0 github.com/google/uuid v1.6.0 - github.com/onsi/ginkgo/v2 v2.15.0 + github.com/onsi/ginkgo/v2 v2.17.1 github.com/onsi/gomega v1.31.1 github.com/pkg/errors v0.9.1 github.com/prometheus/client_golang v1.19.0 github.com/prometheus/client_model v0.6.0 - github.com/prometheus/common v0.51.1 - github.com/stretchr/testify v1.8.4 + github.com/prometheus/common v0.52.2 + github.com/stretchr/testify v1.9.0 go.uber.org/zap v1.26.0 golang.org/x/time v0.5.0 gomodules.xyz/jsonpatch/v2 v2.4.0 @@ -63,13 +63,13 @@ require ( github.com/samber/lo v1.39.0 github.com/spf13/pflag v1.0.5 // indirect go.uber.org/multierr v1.11.0 // indirect - golang.org/x/exp v0.0.0-20230315142452-642cacee5cc0 + golang.org/x/exp v0.0.0-20231006140011-7918f672742d golang.org/x/net v0.23.0 // indirect golang.org/x/oauth2 v0.18.0 // indirect golang.org/x/sys v0.18.0 // indirect golang.org/x/term v0.18.0 // indirect golang.org/x/text v0.14.0 // indirect - golang.org/x/tools v0.16.1 // indirect + golang.org/x/tools v0.17.0 // indirect google.golang.org/appengine v1.6.8 // indirect google.golang.org/protobuf v1.33.0 // indirect gopkg.in/inf.v0 v0.9.1 // indirect diff --git a/go.sum b/go.sum index df69f536..eda6752f 100644 --- a/go.sum +++ b/go.sum @@ -21,8 +21,8 @@ github.com/evanphx/json-patch/v5 v5.8.0/go.mod h1:VNkHZ/282BpEyt/tObQO8s5CMPmYYq github.com/fsnotify/fsnotify v1.7.0 h1:8JEhPFa5W2WU7YfeZzPNqzMP6Lwt7L2715Ggo0nosvA= github.com/fsnotify/fsnotify v1.7.0/go.mod h1:40Bi/Hjc2AVfZrqy+aj+yEI+/bRxZnMJyTJwOpGvigM= github.com/go-logr/logr v1.3.0/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= -github.com/go-logr/logr v1.4.1 h1:pKouT5E8xu9zeFC39JXRDukb6JFQPXM5p5I91188VAQ= -github.com/go-logr/logr v1.4.1/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= +github.com/go-logr/logr v1.4.2 h1:6pFjapn8bFcIbiKo3XT4j/BhANplGihG6tvd+8rYgrY= +github.com/go-logr/logr v1.4.2/go.mod h1:9T104GzyrTigFIr8wt5mBrctHMim0Nb2HLGrmQ40KvY= github.com/go-logr/zapr v1.3.0 h1:XGdV8XW8zdwFiwOA2Dryh1gj2KRQyOOoNmBy4EplIcQ= github.com/go-logr/zapr v1.3.0/go.mod h1:YKepepNBd1u/oyhd/yQmtjVXmm9uML4IXUgMOwR8/Gg= github.com/go-openapi/jsonpointer v0.19.6 h1:eCs3fxoIi3Wh6vtgmLTOjdhSpiqphQ+DaPn38N2ZdrE= @@ -91,8 +91,8 @@ github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f h1:y5//uYreIhSUg3J1GEMiLbxo1LJaP8RfCpH6pymGZus= github.com/mxk/go-flowrate v0.0.0-20140419014527-cca7078d478f/go.mod h1:ZdcZmHo+o7JKHSa8/e818NopupXU1YMK5fe1lsApnBw= -github.com/onsi/ginkgo/v2 v2.15.0 h1:79HwNRBAZHOEwrczrgSOPy+eFTTlIGELKy5as+ClttY= -github.com/onsi/ginkgo/v2 v2.15.0/go.mod h1:HlxMHtYF57y6Dpf+mc5529KKmSq9h2FpCF+/ZkwUxKM= +github.com/onsi/ginkgo/v2 v2.17.1 h1:V++EzdbhI4ZV4ev0UTIj0PzhzOcReJFyJaLjtSF55M8= +github.com/onsi/ginkgo/v2 v2.17.1/go.mod h1:llBI3WDLL9Z6taip6f33H76YcWtJv+7R3HigUjbIBOs= github.com/onsi/gomega v1.31.1 h1:KYppCUK+bUgAZwHOu7EXVBKyQA6ILvOESHkn/tgoqvo= github.com/onsi/gomega v1.31.1/go.mod h1:y40C95dwAD1Nz36SsEnxvfFe8FFfNxzI5eJ0EYGyAy0= github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= @@ -103,8 +103,8 @@ github.com/prometheus/client_golang v1.19.0 h1:ygXvpU1AoN1MhdzckN+PyD9QJOSD4x7km github.com/prometheus/client_golang v1.19.0/go.mod h1:ZRM9uEAypZakd+q/x7+gmsvXdURP+DABIEIjnmDdp+k= github.com/prometheus/client_model v0.6.0 h1:k1v3CzpSRUTrKMppY35TLwPvxHqBu0bYgxZzqGIgaos= github.com/prometheus/client_model v0.6.0/go.mod h1:NTQHnmxFpouOD0DpvP4XujX3CdOAGQPoaGhyTchlyt8= -github.com/prometheus/common v0.51.1 h1:eIjN50Bwglz6a/c3hAgSMcofL3nD+nFQkV6Dd4DsQCw= -github.com/prometheus/common v0.51.1/go.mod h1:lrWtQx+iDfn2mbH5GUzlH9TSHyfZpHkSiG1W7y3sF2Q= +github.com/prometheus/common v0.52.2 h1:LW8Vk7BccEdONfrJBDffQGRtpSzi5CQaRZGtboOO2ck= +github.com/prometheus/common v0.52.2/go.mod h1:lrWtQx+iDfn2mbH5GUzlH9TSHyfZpHkSiG1W7y3sF2Q= github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo= github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= @@ -121,8 +121,8 @@ github.com/stretchr/testify v1.6.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/ github.com/stretchr/testify v1.7.1/go.mod h1:6Fq8oRcR53rry900zMqJjRRixrwX3KX962/h/Wwjteg= github.com/stretchr/testify v1.8.0/go.mod h1:yNjHg4UonilssWZ8iaSj1OCr/vHnekPRkoO+kdMU+MU= github.com/stretchr/testify v1.8.1/go.mod h1:w2LPCIKwWwSfY2zedu0+kehJoqGctiVI29o6fzry7u4= -github.com/stretchr/testify v1.8.4 h1:CcVxjf3Q8PM0mHUKJCdn+eZZtm5yQwehR5yeSVQQcUk= -github.com/stretchr/testify v1.8.4/go.mod h1:sz/lmYIOXD/1dqDmKjjqLyZ2RngseejIcXlSw2iwfAo= +github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= +github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= github.com/yuin/goldmark v1.1.27/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k= @@ -137,8 +137,8 @@ golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACk golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= -golang.org/x/exp v0.0.0-20230315142452-642cacee5cc0 h1:pVgRXcIictcr+lBQIFeiwuwtDIs4eL21OuM9nyAADmo= -golang.org/x/exp v0.0.0-20230315142452-642cacee5cc0/go.mod h1:CxIveKay+FTh1D0yPZemJVgC/95VzuuOLq5Qi4xnoYc= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d h1:jtJma62tbqLibJ5sFQz8bKtEM8rJBtfilJ2qTU199MI= +golang.org/x/exp v0.0.0-20231006140011-7918f672742d/go.mod h1:ldy0pHrwJyGW56pPQzzkH36rKxoZW1tw7ZJpeKx+hdo= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.4.2/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= @@ -188,8 +188,8 @@ golang.org/x/tools v0.0.0-20200619180055-7c47624df98f/go.mod h1:EkVYQZoAsY45+roY golang.org/x/tools v0.0.0-20210106214847-113979e3529a/go.mod h1:emZCQorbCU4vsT4fOWvOPXz4eW1wZW4PmDk9uLelYpA= golang.org/x/tools v0.1.1/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= -golang.org/x/tools v0.16.1 h1:TLyB3WofjdOEepBHAU20JdNC1Zbg87elYofWYAY5oZA= -golang.org/x/tools v0.16.1/go.mod h1:kYVVN6I1mBNoB1OX+noeBjbRk4IUEPa7JJ+TJMEooJ0= +golang.org/x/tools v0.17.0 h1:FvmRgNOcs3kOa+T20R1uhfP9F6HgG2mfxDv1vrx1Htc= +golang.org/x/tools v0.17.0/go.mod h1:xsh6VxdV005rRVaS6SSAf9oiAqljS7UZUacMZ8Bnsps= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= From c944e38714ebeb19dcc1038ed837b26e97915047 Mon Sep 17 00:00:00 2001 From: Jay Deokar <23660509+jaydeokar@users.noreply.github.com> Date: Wed, 11 Sep 2024 22:58:12 -0700 Subject: [PATCH 08/12] Remove hard failure for not getting global STS endpoint (#467) --- pkg/aws/ec2/api/wrapper.go | 23 ++++++++++++----------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/pkg/aws/ec2/api/wrapper.go b/pkg/aws/ec2/api/wrapper.go index 99c4a42f..9be5a773 100644 --- a/pkg/aws/ec2/api/wrapper.go +++ b/pkg/aws/ec2/api/wrapper.go @@ -482,22 +482,23 @@ func (e *ec2Wrapper) getClientUsingAssumedRole(instanceRegion, roleARN, clusterN // TODO: we should revisit the global sts endpoint and check if we should remove global endpoint // we are not using it since the concern on availability and performance // https://docs.aws.amazon.com/IAM/latest/UserGuide/id_credentials_temp_enable-regions.html + globalSTSEndpoint, err := endpoints.DefaultResolver(). EndpointFor("sts", aws.StringValue(userStsSession.Config.Region)) if err != nil { - return nil, fmt.Errorf("failed to get the global sts endoint for region %s: %v", - *userStsSession.Config.Region, err) - } - - // If the regional STS endpoint is different than the global STS endpoint then add the global sts endpoint - if regionalSTSEndpoint.URL != globalSTSEndpoint.URL { - globalProvider := &stscreds.AssumeRoleProvider{ - Client: e.createSTSClient(userStsSession, client, regionalSTSEndpoint, sourceAcct, sourceArn), - RoleARN: roleARN, - Duration: time.Minute * 60, + e.log.Info("failed to get the global STS Endpoint, ignoring", "roleARN", roleARN) + } else { + // If the regional STS endpoint is different than the global STS endpoint then add the global sts endpoint + if regionalSTSEndpoint.URL != globalSTSEndpoint.URL { + globalProvider := &stscreds.AssumeRoleProvider{ + Client: e.createSTSClient(userStsSession, client, regionalSTSEndpoint, sourceAcct, sourceArn), + RoleARN: roleARN, + Duration: time.Minute * 60, + } + providers = append(providers, globalProvider) } - providers = append(providers, globalProvider) } + e.log.Info("initialized the regional/global providers", "roleARN", roleARN) userStsSession.Config.Credentials = credentials.NewChainCredentials(providers) From d7c639a8e929165ac8becb08aae40e17a789f038 Mon Sep 17 00:00:00 2001 From: Hao Zhou Date: Mon, 19 Aug 2024 10:33:42 -0700 Subject: [PATCH 09/12] update ec2 supported instance types (#454) --- pkg/aws/vpc/limits.go | 217 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 216 insertions(+), 1 deletion(-) diff --git a/pkg/aws/vpc/limits.go b/pkg/aws/vpc/limits.go index 59bee69c..c6c55129 100644 --- a/pkg/aws/vpc/limits.go +++ b/pkg/aws/vpc/limits.go @@ -17,7 +17,7 @@ // so we can get this information at runtime. // Code generated by go generate; DO NOT EDIT. -// This file was generated at 2024-08-03T00:54:51Z +// This file was generated at 2024-08-19T17:13:10Z // WARNING: please add @ellistarn, @bwagner5, or @jonathan-innis from aws/karpenter to reviewers // if you are updating this file since Karpenter is depending on this file to calculate max pods. @@ -2530,6 +2530,81 @@ var Limits = map[string]*VPCLimits{ Hypervisor: "nitro", IsBareMetal: false, }, + "c7i-flex.2xlarge": { + Interface: 4, + IPv4PerInterface: 15, + IsTrunkingCompatible: true, + BranchInterface: 18, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 4, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "c7i-flex.4xlarge": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 34, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "c7i-flex.8xlarge": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 54, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "c7i-flex.large": { + Interface: 3, + IPv4PerInterface: 10, + IsTrunkingCompatible: true, + BranchInterface: 3, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 3, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "c7i-flex.xlarge": { + Interface: 4, + IPv4PerInterface: 15, + IsTrunkingCompatible: true, + BranchInterface: 8, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 4, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, "c7i.12xlarge": { Interface: 8, IPv4PerInterface: 30, @@ -3565,6 +3640,146 @@ var Limits = map[string]*VPCLimits{ Hypervisor: "nitro", IsBareMetal: false, }, + "g6e.12xlarge": { + Interface: 10, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 112, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 10, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "g6e.16xlarge": { + Interface: 15, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 107, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 15, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "g6e.24xlarge": { + Interface: 20, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 102, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 10, + NetworkCardIndex: 0, + }, + + { + MaximumNetworkInterfaces: 10, + NetworkCardIndex: 1, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "g6e.2xlarge": { + Interface: 4, + IPv4PerInterface: 15, + IsTrunkingCompatible: true, + BranchInterface: 38, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 4, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "g6e.48xlarge": { + Interface: 40, + IPv4PerInterface: 50, + IsTrunkingCompatible: true, + BranchInterface: 82, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 10, + NetworkCardIndex: 0, + }, + + { + MaximumNetworkInterfaces: 10, + NetworkCardIndex: 1, + }, + + { + MaximumNetworkInterfaces: 10, + NetworkCardIndex: 2, + }, + + { + MaximumNetworkInterfaces: 10, + NetworkCardIndex: 3, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "g6e.4xlarge": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 54, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "g6e.8xlarge": { + Interface: 8, + IPv4PerInterface: 30, + IsTrunkingCompatible: true, + BranchInterface: 84, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 8, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, + "g6e.xlarge": { + Interface: 4, + IPv4PerInterface: 15, + IsTrunkingCompatible: true, + BranchInterface: 18, + DefaultNetworkCardIndex: 0, + NetworkCards: []NetworkCard{ + { + MaximumNetworkInterfaces: 4, + NetworkCardIndex: 0, + }, + }, + Hypervisor: "nitro", + IsBareMetal: false, + }, "gr6.4xlarge": { Interface: 8, IPv4PerInterface: 30, From a6174da3a0339512a02b149e449f597e811c444e Mon Sep 17 00:00:00 2001 From: Tatenda Zifudzi Date: Wed, 4 Sep 2024 15:53:42 -0700 Subject: [PATCH 10/12] Add Windows secondary IP mode configurable options for managing IP address allocation (#443) * Add Windows secondary IP mode configurable options (#443) https://github.com/aws/amazon-vpc-resource-controller-k8s/pull/443 * Various code fixes for PR feedback https://github.com/aws/amazon-vpc-resource-controller-k8s/pull/443 --- controllers/core/configmap_controller.go | 40 +- controllers/core/configmap_controller_test.go | 65 ++- .../secondary_ip_mode_config_options.md | 63 +++ pkg/config/loader.go | 123 ++++-- pkg/config/loader_test.go | 251 ++++++++++- pkg/config/type.go | 1 + pkg/pool/pool.go | 55 ++- pkg/pool/pool_test.go | 393 ++++++++++++++++-- pkg/pool/utils.go | 39 ++ pkg/pool/utils_test.go | 112 +++++ pkg/provider/branch/provider.go | 3 +- pkg/provider/ip/provider.go | 6 +- pkg/provider/ip/provider_test.go | 47 ++- pkg/provider/prefix/provider.go | 19 +- pkg/provider/prefix/provider_test.go | 21 +- pkg/provider/provider.go | 5 +- scripts/test/run-integration-tests.sh | 3 +- test/README.md | 2 +- test/integration/windows/windows_test.go | 334 +++++++++++++-- 19 files changed, 1389 insertions(+), 193 deletions(-) create mode 100644 docs/windows/secondary_ip_mode_config_options.md create mode 100644 pkg/pool/utils.go create mode 100644 pkg/pool/utils_test.go diff --git a/controllers/core/configmap_controller.go b/controllers/core/configmap_controller.go index e56e3847..e28037cd 100644 --- a/controllers/core/configmap_controller.go +++ b/controllers/core/configmap_controller.go @@ -46,8 +46,8 @@ type ConfigMapReconciler struct { Condition condition.Conditions curWinIPAMEnabledCond bool curWinPrefixDelegationEnabledCond bool - curWinPDWarmIPTarget int - curWinPDMinIPTarget int + curWinWarmIPTarget int + curWinMinIPTarget int curWinPDWarmPrefixTarget int Context context.Context } @@ -116,21 +116,33 @@ func (r *ConfigMapReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( isPrefixFlagUpdated = true } - // Check if configurations for Windows prefix delegation have changed - var isPDConfigUpdated bool - warmIPTarget, minIPTarget, warmPrefixTarget := config.ParseWinPDTargets(r.Log, configmap) - if r.curWinPDWarmIPTarget != warmIPTarget || r.curWinPDMinIPTarget != minIPTarget || r.curWinPDWarmPrefixTarget != warmPrefixTarget { - r.curWinPDWarmIPTarget = warmIPTarget - r.curWinPDMinIPTarget = minIPTarget + // Check if Windows IP target configurations in ConfigMap have changed + var isWinIPConfigsUpdated bool + + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := config.ParseWinIPTargetConfigs(r.Log, configmap) + var winMinIPTargetUpdated = r.curWinMinIPTarget != minIPTarget + var winWarmIPTargetUpdated = r.curWinWarmIPTarget != warmIPTarget + var winPDWarmPrefixTargetUpdated = r.curWinPDWarmPrefixTarget != warmPrefixTarget + if winWarmIPTargetUpdated || winMinIPTargetUpdated { + r.curWinWarmIPTarget = warmIPTarget + r.curWinMinIPTarget = minIPTarget + isWinIPConfigsUpdated = true + } + if isPDEnabled && winPDWarmPrefixTargetUpdated { r.curWinPDWarmPrefixTarget = warmPrefixTarget - logger.Info("updated PD configs from configmap", config.WarmIPTarget, r.curWinPDWarmIPTarget, - config.MinimumIPTarget, r.curWinPDMinIPTarget, config.WarmPrefixTarget, r.curWinPDWarmPrefixTarget) - - isPDConfigUpdated = true + isWinIPConfigsUpdated = true + } + if isWinIPConfigsUpdated { + logger.Info( + "Detected update in Windows IP configuration parameter values in ConfigMap", + config.WinWarmIPTarget, r.curWinWarmIPTarget, + config.WinMinimumIPTarget, r.curWinMinIPTarget, + config.WinWarmPrefixTarget, r.curWinPDWarmPrefixTarget, + config.EnableWindowsPrefixDelegationKey, isPDEnabled, + ) } - // Flag is updated, update all nodes - if isIPAMFlagUpdated || isPrefixFlagUpdated || isPDConfigUpdated { + if isIPAMFlagUpdated || isPrefixFlagUpdated || isWinIPConfigsUpdated { err := UpdateNodesOnConfigMapChanges(r.K8sAPI, r.NodeManager) if err != nil { // Error in updating nodes diff --git a/controllers/core/configmap_controller_test.go b/controllers/core/configmap_controller_test.go index 34635b3c..9372ce71 100644 --- a/controllers/core/configmap_controller_test.go +++ b/controllers/core/configmap_controller_test.go @@ -16,14 +16,9 @@ package controllers import ( "context" "errors" + "strconv" "testing" - mock_condition "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/condition" - mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s" - mock_node "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node" - mock_manager "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node/manager" - "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" - cooldown "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" corev1 "k8s.io/api/core/v1" @@ -35,18 +30,36 @@ import ( fakeClient "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + mock_condition "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/condition" + mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s" + mock_node "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node" + mock_manager "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/node/manager" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + cooldown "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" ) var ( mockConfigMap = &corev1.ConfigMap{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{Name: config.VpcCniConfigMapName, Namespace: config.KubeSystemNamespace}, - Data: map[string]string{config.EnableWindowsIPAMKey: "true", config.EnableWindowsPrefixDelegationKey: "true"}, + Data: map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "false", + config.WinMinimumIPTarget: strconv.Itoa(config.IPv4DefaultWinMinIPTarget), + config.WinWarmIPTarget: strconv.Itoa(config.IPv4DefaultWinWarmIPTarget), + }, } mockConfigMapPD = &corev1.ConfigMap{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{Name: config.VpcCniConfigMapName, Namespace: config.KubeSystemNamespace}, - Data: map[string]string{config.EnableWindowsIPAMKey: "false", config.EnableWindowsPrefixDelegationKey: "true"}, + Data: map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "true", + config.WinMinimumIPTarget: strconv.Itoa(config.IPv4PDDefaultMinIPTargetSize), + config.WinWarmIPTarget: strconv.Itoa(config.IPv4PDDefaultWarmIPTargetSize), + config.WinWarmPrefixTarget: strconv.Itoa(config.IPv4PDDefaultWarmPrefixTargetSize), + }, } mockConfigMapReq = reconcile.Request{ NamespacedName: types.NamespacedName{ @@ -89,11 +102,13 @@ func NewConfigMapMock(ctrl *gomock.Controller, mockObjects ...client.Object) Con return ConfigMapMock{ MockNodeManager: mockNodeManager, ConfigMapReconciler: &ConfigMapReconciler{ - Client: client, - Log: zap.New(), - NodeManager: mockNodeManager, - K8sAPI: mockK8sWrapper, - Condition: mockCondition, + Client: client, + Log: zap.New(), + NodeManager: mockNodeManager, + K8sAPI: mockK8sWrapper, + Condition: mockCondition, + curWinMinIPTarget: config.IPv4DefaultWinMinIPTarget, + curWinWarmIPTarget: config.IPv4DefaultWinWarmIPTarget, }, MockNode: mockNode, MockK8sAPI: mockK8sWrapper, @@ -103,13 +118,13 @@ func NewConfigMapMock(ctrl *gomock.Controller, mockObjects ...client.Object) Con } } -func Test_Reconcile_ConfigMap_Updated(t *testing.T) { +func Test_Reconcile_ConfigMap_Updated_Secondary_IP(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() mock := NewConfigMapMock(ctrl, mockConfigMap) mock.MockCondition.EXPECT().IsWindowsIPAMEnabled().Return(true) - mock.MockCondition.EXPECT().IsWindowsPrefixDelegationEnabled().Return(true) + mock.MockCondition.EXPECT().IsWindowsPrefixDelegationEnabled().Return(false) mock.MockK8sAPI.EXPECT().ListNodes().Return(nodeList, nil) mock.MockNodeManager.EXPECT().GetNode(mockNodeName).Return(mock.MockNode, true) mock.MockNodeManager.EXPECT().UpdateNode(mockNodeName).Return(nil) @@ -120,14 +135,32 @@ func Test_Reconcile_ConfigMap_Updated(t *testing.T) { res, err := mock.ConfigMapReconciler.Reconcile(context.TODO(), mockConfigMapReq) assert.NoError(t, err) assert.Equal(t, res, reconcile.Result{}) +} +func Test_Reconcile_ConfigMap_Updated_PD(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mock := NewConfigMapMock(ctrl, mockConfigMapPD) + mock.MockCondition.EXPECT().IsWindowsIPAMEnabled().Return(true) + mock.MockCondition.EXPECT().IsWindowsPrefixDelegationEnabled().Return(true) + mock.MockK8sAPI.EXPECT().ListNodes().Return(nodeList, nil) + mock.MockNodeManager.EXPECT().GetNode(mockNodeName).Return(mock.MockNode, true) + mock.MockNodeManager.EXPECT().UpdateNode(mockNodeName).Return(nil) + + mock.MockK8sAPI.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(createCoolDownMockCM("30"), nil).AnyTimes() + + cooldown.InitCoolDownPeriod(mock.MockK8sAPI, zap.New(zap.UseDevMode(true)).WithName("cooldown")) + res, err := mock.ConfigMapReconciler.Reconcile(context.TODO(), mockConfigMapReq) + assert.NoError(t, err) + assert.Equal(t, res, reconcile.Result{}) } func Test_Reconcile_ConfigMap_PD_Disabled_If_IPAM_Disabled(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() - mock := NewConfigMapMock(ctrl, mockConfigMapPD) + mock := NewConfigMapMock(ctrl, mockConfigMap) mock.MockCondition.EXPECT().IsWindowsIPAMEnabled().Return(false) mock.MockCondition.EXPECT().IsWindowsPrefixDelegationEnabled().Return(false) mock.MockK8sAPI.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(createCoolDownMockCM("30"), nil).AnyTimes() diff --git a/docs/windows/secondary_ip_mode_config_options.md b/docs/windows/secondary_ip_mode_config_options.md new file mode 100644 index 00000000..2f552d76 --- /dev/null +++ b/docs/windows/secondary_ip_mode_config_options.md @@ -0,0 +1,63 @@ +# Configuration options when using secondary IP addresses Windows + +We provide multiple configuration options that allow you to fine-tune the IP address allocation behavior on Windows +nodes using the secondary IP address mode. These configuration options can be set in the `amazon-vpc-cni` ConfigMap in +the `kube-system` namespace. + +- `windows-warm-ip-target` → The total number of IP addresses that should be allocated to each Windows node in excess of + the current need at any given time. The excess IPs can be used by newly launched pods, which aids in faster pod + startup times since there is no wait time for additional IP addresses to be allocated. The VPC Resource Controller + will attempt to ensure that this excess desired threshold is always met. + + Defaults to 3 if unspecified or invalid. Must be greater than or equal to 1. + + For example, if no pods were running on a given Windows node, and if you set `windows-warm-ip-target` to 5, the VPC + Resource Controller will aim to ensure that each Windows node always has at least 5 IP addresses in excess, ready for + use, allocated to its ENI. If 2 pods are scheduled on the node, the controller will allocate 2 additional IP addresses + to the ENI, maintaining the 5 warm IP address target. + +- `windows-minimum-ip-target` → Defaults to 3 if unspecified or invalid. The minimum number of IP addresses, both in use + by running pods and available as warm IPs, that should be allocated to each Windows node at any given time. The + controller will attempt to ensure that this minimum threshold is always met. + + Defaults to 3 if unspecified or invalid. Must be greater than or equal to 0. + + For example, if no pods were running on a given Windows node, and if you set `windows-minimum-ip-target` to 10, the + VPC Resource Controller will aim to ensure that the total number of IP addresses on the Windows node should be at + least 10. Therefore, before pods are scheduled, there should be at least 10 IP addresses available. If 5 pods are + scheduled on a given node, they will consume 5 of the 10 available IPs. The VPC Resource Controller will keep 5 the + remaining available IPs available in addition to the 5 already in use to meet the target of 10. + +### Considerations while using the above configuration options + +- These configuration options only apply when the VPC Resource Controller is operating in the secondary IP mode. They do + not affect the prefix delegation mode. More explicitly, if `enable-windows-prefix-delegation` is set to false, or is + not specified, then the VPC Resource Controller operates in secondary IP mode. +- Setting either `windows-warm-ip-target` or `windows-minimum-ip-target` to a negative value will result in the + respective default value being used. +- If the values of `windows-warm-ip-target` or `windows-minimum-ip-target` are set such that the maximum node IP + capacity would be exceeded, the controller will limit the allocation to the maximum capacity possible. +- The `warm-prefix-target` configuration option will be ignored when using the secondary IP mode, as it only applies to + the prefix delegation mode. +- If `windows-warm-ip-target` is set to 0, the system will implicitly set `windows-warm-ip-target` to 1. This is + because on-demand IP allocation whereby an IP is allocated on the Windows node as the pods are scheduled is currently + not supported. Implicitly Setting `windows-warm-ip-target` to 1 ensures the minimum acceptable non-zero value is set + since the `windows-warm-ip-target` should always be at least 1. +- The configuration options `warm-ip-target` and `minimum-ip-target` are deprecated in favor of the new + options `windows-warm-ip-target` and `windows-minimum-ip-target`. + +### Examples + +| `windows-warm-ip-target` | `windows-minimum-ip-target` | Running Pods | Total Allocated IPs | Warm IPs | +|--------------------------|-----------------------------|--------------|---------------------|----------| +| 1 | 0 | 0 | 1 | 1 | +| 1 | 0 | 5 | 6 | 1 | +| 5 | 0 | 0 | 5 | 5 | +| 1 | 1 | 0 | 1 | 1 | +| 1 | 1 | 1 | 2 | 1 | +| 1 | 3 | 3 | 4 | 1 | +| 1 | 3 | 5 | 6 | 1 | +| 5 | 10 | 0 | 10 | 10 | +| 10 | 10 | 0 | 10 | 10 | +| 10 | 10 | 10 | 20 | 10 | +| 15 | 10 | 10 | 25 | 15 | diff --git a/pkg/config/loader.go b/pkg/config/loader.go index 90d1b61d..94f36ee0 100644 --- a/pkg/config/loader.go +++ b/pkg/config/loader.go @@ -28,13 +28,14 @@ const ( // Default Configuration for Pod ENI resource type PodENIDefaultWorker = 30 - // Default Configuration for IPv4 resource type - IPv4DefaultWorker = 2 - IPv4DefaultWPSize = 3 - IPv4DefaultMaxDev = 1 - IPv4DefaultResSize = 0 - - // Default Configuration for IPv4 prefix resource type + // Default Windows Configuration for IPv4 resource type + IPv4DefaultWinWorkerCount = 2 + IPv4DefaultWinWarmIPTarget = 3 + IPv4DefaultWinMinIPTarget = 3 + IPv4DefaultWinMaxDev = 0 + IPv4DefaultWinResSize = 0 + + // Default Windows Configuration for IPv4 prefix resource type IPv4PDDefaultWorker = 2 IPv4PDDefaultWPSize = 1 IPv4PDDefaultMaxDev = 0 @@ -69,26 +70,44 @@ func LoadResourceConfig() map[string]ResourceConfig { func LoadResourceConfigFromConfigMap(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) map[string]ResourceConfig { resourceConfig := getDefaultResourceConfig() - warmIPTarget, minIPTarget, warmPrefixTarget := ParseWinPDTargets(log, vpcCniConfigMap) + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCniConfigMap) // If no PD configuration is set in configMap or none is valid, return default resource config if warmIPTarget == 0 && minIPTarget == 0 && warmPrefixTarget == 0 { return resourceConfig } - resourceConfig[ResourceNameIPAddressFromPrefix].WarmPoolConfig.WarmIPTarget = warmIPTarget - resourceConfig[ResourceNameIPAddressFromPrefix].WarmPoolConfig.MinIPTarget = minIPTarget - resourceConfig[ResourceNameIPAddressFromPrefix].WarmPoolConfig.WarmPrefixTarget = warmPrefixTarget + if isPDEnabled { + resourceConfig[ResourceNameIPAddressFromPrefix].WarmPoolConfig.WarmIPTarget = warmIPTarget + resourceConfig[ResourceNameIPAddressFromPrefix].WarmPoolConfig.MinIPTarget = minIPTarget + resourceConfig[ResourceNameIPAddressFromPrefix].WarmPoolConfig.WarmPrefixTarget = warmPrefixTarget + } else { + resourceConfig[ResourceNameIPAddress].WarmPoolConfig.WarmIPTarget = warmIPTarget + resourceConfig[ResourceNameIPAddress].WarmPoolConfig.MinIPTarget = minIPTarget + } return resourceConfig } -// ParseWinPDTargets parses config map for Windows prefix delegation configurations set by users -func ParseWinPDTargets(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (warmIPTarget int, minIPTarget int, warmPrefixTarget int) { - warmIPTarget, minIPTarget, warmPrefixTarget = 0, 0, 0 - +// ParseWinIPTargetConfigs parses Windows IP target configuration parameters in the amazon-vpc-cni ConfigMap +// If all three config parameter values (warm-ip-target, min-ip-target, warm-prefix-target) are 0 or unset, or config map does not exist, +// then default values for warm-ip-target and min-ip-target will be set. +func ParseWinIPTargetConfigs(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (warmIPTarget int, minIPTarget int, warmPrefixTarget int, isPDEnabled bool) { if vpcCniConfigMap.Data == nil { - return warmIPTarget, minIPTarget, warmPrefixTarget + warmIPTarget = IPv4DefaultWinWarmIPTarget + minIPTarget = IPv4DefaultWinMinIPTarget + log.Info( + "No ConfigMap data found, falling back to using default values", + "minIPTarget", minIPTarget, + "warmIPTarget", warmIPTarget, + ) + return warmIPTarget, minIPTarget, 0, false + } + + isPDEnabled, err := strconv.ParseBool(vpcCniConfigMap.Data[EnableWindowsPrefixDelegationKey]) + if err != nil { + log.Info("Could not parse prefix delegation flag from ConfigMap, falling back to using secondary IP mode") + isPDEnabled = false } warmIPTargetStr, foundWarmIP := vpcCniConfigMap.Data[WarmIPTarget] @@ -104,36 +123,60 @@ func ParseWinPDTargets(log logr.Logger, vpcCniConfigMap *v1.ConfigMap) (warmIPTa warmPrefixTargetStr, foundWarmPrefix = vpcCniConfigMap.Data[WinWarmPrefixTarget] } - // If no configuration is found, return 0 - if !foundWarmIP && !foundMinIP && !foundWarmPrefix { - return warmIPTarget, minIPTarget, warmPrefixTarget - } - + // If warm IP target config value is not found, or there is an error parsing it, the value will be set to zero if foundWarmIP { - warmIPTargetInt, err := strconv.Atoi(warmIPTargetStr) + warmIPTarget, err = strconv.Atoi(warmIPTargetStr) if err != nil { - log.Error(err, "failed to parse warm ip target", "warm ip target", warmIPTargetStr) - } else { - warmIPTarget = warmIPTargetInt + log.Info("Could not parse warm ip target, defaulting to zero", "warm ip target", warmIPTargetStr) + } else if !isPDEnabled && warmIPTarget == 0 { + // Handle secondary IP mode scenario where WarmIPTarget is explicitly configured to zero + // In such a case there must always be 1 warm IP to ensure that the warmpool is never empty + log.Info("Explicitly setting WarmIPTarget zero value not supported in secondary IP mode, will override with 1") + warmIPTarget = 1 } + } else { + log.Info("could not find warm ip target in ConfigMap, defaulting to zero") + warmIPTarget = 0 } + + // If min IP target config value is not found, or there is an error parsing it, the value will be set to zero if foundMinIP { - minIPTargetInt, err := strconv.Atoi(minIPTargetStr) + minIPTarget, err = strconv.Atoi(minIPTargetStr) if err != nil { - log.Error(err, "failed to parse minimum ip target", "minimum ip target", minIPTargetStr) - } else { - minIPTarget = minIPTargetInt + log.Info("Could not parse minimum ip target, defaulting to zero", "minimum ip target", minIPTargetStr) } + } else { + log.Info("could not find minimum ip target in ConfigMap, defaulting to zero") + minIPTarget = 0 } - if foundWarmPrefix { - warmPrefixTargetInt, err := strconv.Atoi(warmPrefixTargetStr) + + warmPrefixTarget = 0 + if isPDEnabled && foundWarmPrefix { + warmPrefixTarget, err = strconv.Atoi(warmPrefixTargetStr) if err != nil { - log.Error(err, "failed to parse warm prefix target", "warm prefix target", warmPrefixTargetStr) - } else { - warmPrefixTarget = warmPrefixTargetInt + log.Info("Could not parse warm prefix target, defaulting to zero", "warm prefix target", warmPrefixTargetStr) + } + } + + if warmIPTarget == 0 && minIPTarget == 0 { + if isPDEnabled && warmPrefixTarget == 0 { + minIPTarget = IPv4PDDefaultMinIPTargetSize + warmIPTarget = IPv4PDDefaultWarmIPTargetSize + warmPrefixTarget = IPv4PDDefaultWarmPrefixTargetSize + } else if !isPDEnabled { + minIPTarget = IPv4DefaultWinMinIPTarget + warmIPTarget = IPv4DefaultWinWarmIPTarget } + log.Info( + "Encountered zero values for warm-ip-target, min-ip-target and warm-prefix-target in ConfigMap data, falling back to using default values since on demand IP allocation is not supported", + "minIPTarget", minIPTarget, + "warmIPTarget", warmIPTarget, + "warmPrefixTarget", warmPrefixTarget, + "isPDEnabled", isPDEnabled, + ) } - return warmIPTarget, minIPTarget, warmPrefixTarget + + return warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled } // getDefaultResourceConfig returns the default Resource Configuration. @@ -152,13 +195,15 @@ func getDefaultResourceConfig() map[string]ResourceConfig { // Create default configuration for IPv4 Resource ipV4WarmPoolConfig := WarmPoolConfig{ - DesiredSize: IPv4DefaultWPSize, - MaxDeviation: IPv4DefaultMaxDev, - ReservedSize: IPv4DefaultResSize, + DesiredSize: IPv4DefaultWinWarmIPTarget, + WarmIPTarget: IPv4DefaultWinWarmIPTarget, + MinIPTarget: IPv4DefaultWinMinIPTarget, + MaxDeviation: IPv4DefaultWinMaxDev, + ReservedSize: IPv4DefaultWinResSize, } ipV4Config := ResourceConfig{ Name: ResourceNameIPAddress, - WorkerCount: IPv4DefaultWorker, + WorkerCount: IPv4DefaultWinWorkerCount, SupportedOS: map[string]bool{OSWindows: true, OSLinux: false}, WarmPoolConfig: &ipV4WarmPoolConfig, } diff --git a/pkg/config/loader_test.go b/pkg/config/loader_test.go index 88fa4b33..e246eb75 100644 --- a/pkg/config/loader_test.go +++ b/pkg/config/loader_test.go @@ -36,14 +36,15 @@ func TestLoadResourceConfig(t *testing.T) { // Verify default resource configuration for resource IPv4 Address ipV4Config := defaultResourceConfig[ResourceNameIPAddress] assert.Equal(t, ResourceNameIPAddress, ipV4Config.Name) - assert.Equal(t, IPv4DefaultWorker, ipV4Config.WorkerCount) + assert.Equal(t, IPv4DefaultWinWorkerCount, ipV4Config.WorkerCount) assert.Equal(t, map[string]bool{OSLinux: false, OSWindows: true}, ipV4Config.SupportedOS) // Verify default Warm pool configuration for IPv4 Address ipV4WPConfig := ipV4Config.WarmPoolConfig - assert.Equal(t, IPv4DefaultWPSize, ipV4WPConfig.DesiredSize) - assert.Equal(t, IPv4DefaultMaxDev, ipV4WPConfig.MaxDeviation) - assert.Equal(t, IPv4DefaultResSize, ipV4WPConfig.ReservedSize) + assert.Equal(t, IPv4DefaultWinWarmIPTarget, ipV4WPConfig.DesiredSize) + assert.Equal(t, IPv4DefaultWinMinIPTarget, ipV4WPConfig.MinIPTarget) + assert.Equal(t, IPv4DefaultWinMaxDev, ipV4WPConfig.MaxDeviation) + assert.Equal(t, IPv4DefaultWinResSize, ipV4WPConfig.ReservedSize) // Verify default resource configuration for prefix-deconstructed IPv4 Address prefixIPv4Config := defaultResourceConfig[ResourceNameIPAddressFromPrefix] @@ -61,8 +62,8 @@ func TestLoadResourceConfig(t *testing.T) { assert.Equal(t, IPv4PDDefaultWarmPrefixTargetSize, prefixIPv4WPConfig.WarmPrefixTarget) } -// TestParseWinPDTargets parses prefix delegation configurations from a vpc cni config map -func TestParseWinPDTargets(t *testing.T) { +// TestParseWinIPTargetConfigs_PDEnabledWithDefaultTargets parses prefix delegation configurations from a vpc cni config map +func TestParseWinIPTargetConfigs_PDEnabledWithDefaultTargets(t *testing.T) { log := zap.New(zap.UseDevMode(true)).WithName("loader test") vpcCNIConfig := &v1.ConfigMap{ @@ -74,14 +75,109 @@ func TestParseWinPDTargets(t *testing.T) { WarmPrefixTarget: strconv.Itoa(IPv4PDDefaultWarmPrefixTargetSize), }, } - warmIPTarget, minIPTarget, warmPrefixTarget := ParseWinPDTargets(log, vpcCNIConfig) + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) assert.Equal(t, IPv4PDDefaultWarmIPTargetSize, warmIPTarget) assert.Equal(t, IPv4PDDefaultMinIPTargetSize, minIPTarget) assert.Equal(t, IPv4PDDefaultWarmPrefixTargetSize, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) +} + +func TestParseWinIPTargetConfigs_PDDisabledWithAllZeroTargets(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "false", + WarmIPTarget: strconv.Itoa(0), + MinimumIPTarget: strconv.Itoa(0), + }, + } + warmIPTarget, minIPTarget, _, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, 1, warmIPTarget) + assert.Equal(t, 0, minIPTarget) + assert.Equal(t, false, isPDEnabled) +} + +func TestParseWinIPTargetConfigs_PDEnabledWithAllZeroTargets(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "true", + WarmIPTarget: strconv.Itoa(0), + MinimumIPTarget: strconv.Itoa(0), + WarmPrefixTarget: strconv.Itoa(0), + }, + } + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, IPv4PDDefaultWarmIPTargetSize, warmIPTarget) + assert.Equal(t, IPv4PDDefaultMinIPTargetSize, minIPTarget) + assert.Equal(t, IPv4PDDefaultWarmPrefixTargetSize, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) +} + +func TestParseWinIPTargetConfigs_PDDisabledWithDefaultTargets(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + expectedWarmIPTarget := IPv4DefaultWinWarmIPTarget + expectedMinIPTarget := IPv4DefaultWinMinIPTarget + expectedWarmPrefixTarget := 0 + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "false", + WarmIPTarget: strconv.Itoa(expectedWarmIPTarget), + MinimumIPTarget: strconv.Itoa(expectedMinIPTarget), + }, + } + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, expectedWarmIPTarget, warmIPTarget) + assert.Equal(t, expectedMinIPTarget, minIPTarget) + assert.Equal(t, expectedWarmPrefixTarget, warmPrefixTarget) + assert.Equal(t, false, isPDEnabled) +} + +func TestParseWinIPTargetConfigs_PDDisabledAndInvalidConfig_ReturnsDefault(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "false", + WarmIPTarget: "Invalid string", + MinimumIPTarget: "Invalid string", + }, + } + + warmIPTarget, minIPTarget, _, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.False(t, isPDEnabled) + assert.Equal(t, IPv4DefaultWinWarmIPTarget, warmIPTarget) + assert.Equal(t, IPv4DefaultWinMinIPTarget, minIPTarget) +} + +// negative values are still read in but processed accordingly when it's used in the warm pool +func TestParseWinIPTargetConfigs_PDDisabledAndNegativeConfig_ReturnsOriginal(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "false", + WarmIPTarget: strconv.Itoa(-5), + MinimumIPTarget: strconv.Itoa(-5), + }, + } + + warmIPTarget, minIPTarget, _, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.False(t, isPDEnabled) + assert.Equal(t, -5, warmIPTarget) + assert.Equal(t, -5, minIPTarget) } // TestParseWinPDTargets parses prefix delegation configurations with negative values and returns the same -func TestParseWinPDTargets_Negative(t *testing.T) { +func TestParseWinIPTargetConfigs_PDEnabled_Negative(t *testing.T) { log := zap.New(zap.UseDevMode(true)).WithName("loader test") vpcCNIConfig := &v1.ConfigMap{ @@ -93,15 +189,60 @@ func TestParseWinPDTargets_Negative(t *testing.T) { WarmPrefixTarget: strconv.Itoa(0), }, } - warmIPTarget, minIPTarget, warmPrefixTarget := ParseWinPDTargets(log, vpcCNIConfig) + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) // negative values are still read in but processed when it's used in the warm pool assert.Equal(t, -10, warmIPTarget) assert.Equal(t, -100, minIPTarget) assert.Equal(t, 0, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) +} + +func TestParseWinIPTargetConfigs_OnlyWindowsIPAM_EffectsDefaults(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + }, + } + + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, IPv4DefaultWinWarmIPTarget, warmIPTarget) + assert.Equal(t, IPv4DefaultWinMinIPTarget, minIPTarget) + assert.Equal(t, 0, warmPrefixTarget) + assert.Equal(t, false, isPDEnabled) } -// TestParseWinPDTargets_Invalid parses prefix delegation configurations with invalid values and returns 0s as targets -func TestParseWinPDTargets_Invalid(t *testing.T) { +func TestParseWinIPTargetConfigs_PartiallyEmptyTargets_EffectsSpecified(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + WinWarmIPTarget: "1", + }, + } + + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, 1, warmIPTarget) + assert.Equal(t, 0, minIPTarget) + assert.Equal(t, 0, warmPrefixTarget) + assert.Equal(t, false, isPDEnabled) +} + +// TestParseWinIPTargetConfigs_EmptyConfigMap_ReturnsDefaultsWithSecondaryIP parses configurations with empty configmap +func TestParseWinIPTargetConfigs_EmptyConfigMap_ReturnsDefaultsWithSecondaryIP(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{} + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, IPv4DefaultWinWarmIPTarget, warmIPTarget) + assert.Equal(t, IPv4DefaultWinMinIPTarget, minIPTarget) + assert.Equal(t, 0, warmPrefixTarget) + assert.Equal(t, false, isPDEnabled) +} + +// TestParseWinIPTargetConfigs_PDEnabled_Invalid parses prefix delegation configurations with invalid values and returns 0s as targets +func TestParseWinIPTargetConfigs_PDEnabled_Invalid(t *testing.T) { log := zap.New(zap.UseDevMode(true)).WithName("loader test") vpcCNIConfig := &v1.ConfigMap{ @@ -113,10 +254,88 @@ func TestParseWinPDTargets_Invalid(t *testing.T) { WarmPrefixTarget: "can't parse", }, } - warmIPTarget, minIPTarget, warmPrefixTarget := ParseWinPDTargets(log, vpcCNIConfig) + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, IPv4PDDefaultWarmIPTargetSize, warmIPTarget) + assert.Equal(t, IPv4PDDefaultMinIPTargetSize, minIPTarget) + assert.Equal(t, 0, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) +} + +// TestParseWinIPTargetConfigs_PDEnabledAndWarmPrefixInvalid parses prefix delegation configurations with warm prefix being invalid +func TestParseWinIPTargetConfigs_PDEnabledAndWarmPrefixInvalid(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "true", + WarmIPTarget: "2", + MinimumIPTarget: "2", + WarmPrefixTarget: "invalid value", + }, + } + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, 2, warmIPTarget) + assert.Equal(t, 2, minIPTarget) + assert.Equal(t, 0, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) +} + +// TestParseWinIPTargetConfigs_PDEnabledAndWarmAndMinimumIPInvalid parses prefix delegation configurations with only warm prefix being valid +func TestParseWinIPTargetConfigs_PDEnabledAndWarmAndMinimumIPInvalid(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "true", + WarmIPTarget: "invalid value", + MinimumIPTarget: "invalid value", + WarmPrefixTarget: "1", + }, + } + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) assert.Equal(t, 0, warmIPTarget) assert.Equal(t, 0, minIPTarget) + assert.Equal(t, 1, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) +} + +// TestParseWinIPTargetConfigs_PDEnabledAndWarmAndOnlyWarmPrefixSet parses prefix delegation configurations with only warm prefix set +func TestParseWinIPTargetConfigs_PDEnabledAndWarmAndOnlyWarmPrefixSet(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "true", + WarmPrefixTarget: "1", + }, + } + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, 0, warmIPTarget) + assert.Equal(t, 0, minIPTarget) + assert.Equal(t, 1, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) +} + +// TestParseWinIPTargetConfigs_PDEnabledAndWarmAndOnlyWarmPrefixNotSet parses prefix delegation configurations with only warm prefix not set +func TestParseWinIPTargetConfigs_PDEnabledAndWarmAndOnlyWarmPrefixNotSet(t *testing.T) { + log := zap.New(zap.UseDevMode(true)).WithName("loader test") + + vpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + EnableWindowsIPAMKey: "true", + EnableWindowsPrefixDelegationKey: "true", + WarmIPTarget: "2", + MinimumIPTarget: "2", + }, + } + warmIPTarget, minIPTarget, warmPrefixTarget, isPDEnabled := ParseWinIPTargetConfigs(log, vpcCNIConfig) + assert.Equal(t, 2, warmIPTarget) + assert.Equal(t, 2, minIPTarget) assert.Equal(t, 0, warmPrefixTarget) + assert.Equal(t, true, isPDEnabled) } // TestLoadResourceConfigFromConfigMap tests the custom configuration for PD is loaded correctly from config map @@ -148,14 +367,14 @@ func TestLoadResourceConfigFromConfigMap(t *testing.T) { // Verify default resource configuration for resource IPv4 Address ipV4Config := resourceConfig[ResourceNameIPAddress] assert.Equal(t, ResourceNameIPAddress, ipV4Config.Name) - assert.Equal(t, IPv4DefaultWorker, ipV4Config.WorkerCount) + assert.Equal(t, IPv4DefaultWinWorkerCount, ipV4Config.WorkerCount) assert.Equal(t, map[string]bool{OSLinux: false, OSWindows: true}, ipV4Config.SupportedOS) // Verify default Warm pool configuration for IPv4 Address ipV4WPConfig := ipV4Config.WarmPoolConfig - assert.Equal(t, IPv4DefaultWPSize, ipV4WPConfig.DesiredSize) - assert.Equal(t, IPv4DefaultMaxDev, ipV4WPConfig.MaxDeviation) - assert.Equal(t, IPv4DefaultResSize, ipV4WPConfig.ReservedSize) + assert.Equal(t, IPv4DefaultWinWarmIPTarget, ipV4WPConfig.DesiredSize) + assert.Equal(t, IPv4DefaultWinMaxDev, ipV4WPConfig.MaxDeviation) + assert.Equal(t, IPv4DefaultWinResSize, ipV4WPConfig.ReservedSize) // Verify default resource configuration for prefix-deconstructed IPv4 Address prefixIPv4Config := resourceConfig[ResourceNameIPAddressFromPrefix] diff --git a/pkg/config/type.go b/pkg/config/type.go index d7673640..a8bac5d9 100644 --- a/pkg/config/type.go +++ b/pkg/config/type.go @@ -140,6 +140,7 @@ type ResourceConfig struct { // WarmPoolConfig is the configuration of Warm Pool of a resource type WarmPoolConfig struct { + // TODO: Deprecate DesiredSize in favour of using WarmIPTarget since historically they served the same purpose // Number of resources to keep in warm pool per node; for prefix IP pool, this is used to check if pool is active DesiredSize int // Number of resources not to use in the warm pool diff --git a/pkg/pool/pool.go b/pkg/pool/pool.go index fcb24df7..193312dd 100644 --- a/pkg/pool/pool.go +++ b/pkg/pool/pool.go @@ -18,10 +18,11 @@ import ( "sync" "time" + "github.com/go-logr/logr" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/worker" - "github.com/go-logr/logr" ) var ( @@ -442,9 +443,14 @@ func (p *pool) ReconcilePool() *worker.WarmPoolJob { } // Consider pending create as well so we don't create multiple subsequent create request - deviation := p.warmPoolConfig.DesiredSize - (numWarmResources + p.pendingCreate) + // deviation represents the difference between the desired number of resources and the current state + // A negative deviation means IP resources need to be deleted to reach the desired state + // A positive deviation means IP resources need to be created to reach the desired state + var deviation int if p.isPDPool { deviation = p.getPDDeviation() + } else { + deviation = p.calculateSecondaryIPDeviation() } // Need to create more resources for warm pool @@ -715,6 +721,51 @@ func (p *pool) getPDDeviation() int { return deviationPrefix * NumIPv4AddrPerPrefix } +// calculateSecondaryIPDeviation calculates the deviation required to meet the desired state for secondary IP mode +// Returns a number of IPv4 addresses by taking into account the MinIPTarget and WarmIPTarget +func (p *pool) calculateSecondaryIPDeviation() int { + numWarmResources := numResourcesFromMap(p.warmResources) + numUsedResources := len(p.usedResources) + numAssignedResources := numUsedResources + numWarmResources + p.pendingCreate + len(p.coolDownQueue) + + // warm pool is in draining state, set targets to zero + if p.warmPoolConfig.DesiredSize == 0 { + p.warmPoolConfig.WarmIPTarget = 0 + p.warmPoolConfig.MinIPTarget = 0 + p.warmPoolConfig.WarmPrefixTarget = 0 + } + + isMinIPTargetInvalid := p.warmPoolConfig.MinIPTarget < 0 + isWarmIPTargetInvalid := p.warmPoolConfig.WarmIPTarget < 0 + // Handle scenario where MinIPTarget is configured to negative integer which is invalid + if isMinIPTargetInvalid { + p.warmPoolConfig.MinIPTarget = config.IPv4DefaultWinMinIPTarget + } + // Handle scenario where WarmIPTarget is configured to negative integer which is invalid + if isWarmIPTargetInvalid { + p.warmPoolConfig.WarmIPTarget = config.IPv4DefaultWinWarmIPTarget + } + + availableResources := numWarmResources + p.pendingCreate + + // Calculate how many IPs we're short of the warm target + resourcesShort := max(p.warmPoolConfig.WarmIPTarget-availableResources, 0) + + // Adjust short based on the minimum IP target + resourcesShort = max(resourcesShort, p.warmPoolConfig.MinIPTarget-numAssignedResources) + + // Calculate how many IPs we're over the warm target + resourcesOver := max(availableResources-p.warmPoolConfig.WarmIPTarget, 0) + + // Adjust over to not go below the minimum IP target + resourcesOver = max(min(resourcesOver, numAssignedResources-p.warmPoolConfig.MinIPTarget), 0) + + // The final deviation is the difference between short and over + deviation := resourcesShort - resourcesOver + + return deviation +} + // numResourcesFromMap returns total number of resources from a map of list of resources indexed by group id func numResourcesFromMap(resourceGroups map[string][]Resource) int { count := 0 diff --git a/pkg/pool/pool_test.go b/pkg/pool/pool_test.go index ec28c525..ef15acc3 100644 --- a/pkg/pool/pool_test.go +++ b/pkg/pool/pool_test.go @@ -25,11 +25,25 @@ import ( "sigs.k8s.io/controller-runtime/pkg/log/zap" ) +type deviationCalcTestCase struct { + warmPoolConfig *config.WarmPoolConfig + warmResources map[string][]Resource + usedResources map[string]Resource + capacity int + expectedWarmIPSize int + expectedMinIPSize int + expectedDeviation int + isPDPool bool + pendingCreate int +} + var ( poolConfig = &config.WarmPoolConfig{ - DesiredSize: 2, + DesiredSize: 1, ReservedSize: 1, - MaxDeviation: 1, + MaxDeviation: config.IPv4DefaultWinMaxDev, + WarmIPTarget: config.IPv4DefaultWinWarmIPTarget, + MinIPTarget: config.IPv4DefaultWinMinIPTarget, } nodeName = "node-name" @@ -345,57 +359,84 @@ func TestPool_ReconcilePool_MaxCapacity(t *testing.T) { assert.Equal(t, &worker.WarmPoolJob{Operations: worker.OperationReconcileNotRequired}, job) } -// TestPool_ReconcilePool_NotRequired tests if the deviation form warm pool is equal to or less than the max deviation +// TestPool_ReconcilePool_NotRequired tests if the deviation from warm pool is equal to or less than the max deviation // then reconciliation is not triggered func TestPool_ReconcilePool_NotRequired(t *testing.T) { - warmPool := getMockPool(poolConfig, usedResources, map[string][]Resource{}, 7, false) - warmPool.pendingCreate = 1 + usedResourcesEmpty := map[string]Resource{} + warmPoolResourcesEmpty := map[string][]Resource{} + poolConfig.WarmIPTarget = 2 + poolConfig.MinIPTarget = 0 + warmPool := getMockPool(poolConfig, usedResourcesEmpty, warmPoolResourcesEmpty, 7, false) + warmPool.pendingCreate = 2 job := warmPool.ReconcilePool() - // deviation = 2(desired WP) - 1(actual WP + pending create) = 1, (deviation)1 > (max deviation)1 => false, + // deviation = 2(warmIPTarget) - 2(actual warmpool size + pending create) = 0 + // 0(deviation) > 0(max deviation) => false, // so no need create right now assert.Equal(t, &worker.WarmPoolJob{Operations: worker.OperationReconcileNotRequired}, job) } // TestPool_ReconcilePool tests job with operation type create is returned when the warm pool deviates form max deviation func TestPool_ReconcilePool_Create(t *testing.T) { - warmPool := getMockPool(poolConfig, usedResources, map[string][]Resource{}, 7, false) + usedResourcesEmpty := map[string]Resource{} + warmPoolResourcesEmpty := map[string][]Resource{} + poolConfig.WarmIPTarget = 2 + poolConfig.MinIPTarget = 0 + warmPool := getMockPool(poolConfig, usedResourcesEmpty, warmPoolResourcesEmpty, 7, false) job := warmPool.ReconcilePool() - // deviation = 2(desired WP) - 0(actual WP + pending create) = 0, (deviation)0 > (max deviation)1 => true, + // deviation = 2(warmIPTarget) - 0(actual warm pool size + pending create) = 0 + // 2 (deviation) >= 0 (max deviation) => true, so need to create 2 resources // create (deviation)2 resources assert.Equal(t, &worker.WarmPoolJob{Operations: worker.OperationCreate, ResourceCount: 2}, job) - assert.Equal(t, warmPool.pendingCreate, 2) + assert.Equal(t, 2, warmPool.pendingCreate) } // TestPool_ReconcilePool_Create_LimitByMaxCapacity tests when the warm pool deviates from max deviation and the deviation // is greater than the capacity of the pool, then only resources upto the max capacity are created func TestPool_ReconcilePool_Create_LimitByMaxCapacity(t *testing.T) { - warmPool := getMockPool(poolConfig, usedResources, map[string][]Resource{}, 7, false) - warmPool.pendingDelete = 4 + usedResources6 := map[string]Resource{ + res1: {GroupID: grp1, ResourceID: res1}, + res2: {GroupID: grp2, ResourceID: res2}, + res3: {GroupID: grp3, ResourceID: res3}, + res4: {GroupID: grp4, ResourceID: res4}, + res5: {GroupID: grp5, ResourceID: res5}, + res6: {GroupID: grp6, ResourceID: res6}, + } + warmPoolResourcesEmpty := map[string][]Resource{} + poolConfig.WarmIPTarget = 2 + poolConfig.MinIPTarget = 0 + warmPool := getMockPool(poolConfig, usedResources6, warmPoolResourcesEmpty, 7, false) job := warmPool.ReconcilePool() - // deviation = 2(desired WP) - 0(actual WP + pending create) = 2, (deviation)2 >= (max deviation)1 => true, so - // need to create (deviation)2 resources. But since remaining capacity is just 1, so we create 1 resource instead + // deviation = 2(warmIPTarget) - 0(actual warmpool size + pending create) = 2 + // 2 (deviation) >= 0 (max deviation) => true, so need to create 2 resources + // 6 resources are already pending creation when the ENI has a capacity of 7 + // Since the remaining capacity is just 1, so we create 1 resource instead assert.Equal(t, &worker.WarmPoolJob{Operations: worker.OperationCreate, ResourceCount: 1}, job) - assert.Equal(t, warmPool.pendingCreate, 1) + assert.Equal(t, 1, warmPool.pendingCreate) } // TestPool_ReconcilePool_Delete_NotRequired tests that if the warm pool is over the desired warm pool size but has not // exceeded the max deviation then we don't return a delete job func TestPool_ReconcilePool_Delete_NotRequired(t *testing.T) { - warmResources := make(map[string][]Resource) - warmResources[res3] = []Resource{{GroupID: res3, ResourceID: res3}} - warmResources[res4] = []Resource{{GroupID: res4, ResourceID: res4}} - warmResources[res5] = []Resource{{GroupID: res5, ResourceID: res5}} - warmPool := getMockPool(poolConfig, usedResources, warmResources, 7, false) + usedResourcesEmpty := map[string]Resource{} + poolConfig.WarmIPTarget = 2 + poolConfig.MinIPTarget = 0 + poolConfig.MaxDeviation = 1 + warmResources3 := make(map[string][]Resource) + warmResources3[res3] = []Resource{{GroupID: res3, ResourceID: res3}} + warmResources3[res4] = []Resource{{GroupID: res4, ResourceID: res4}} + warmResources3[res5] = []Resource{{GroupID: res5, ResourceID: res5}} + warmPool := getMockPool(poolConfig, usedResourcesEmpty, warmResources3, 7, false) job := warmPool.ReconcilePool() - // deviation = 2(desired WP) - 3(actual WP) = -1, (-deviation)1 > (max deviation)1 => false, so no need delete + // deviation = 2(warmIPTarget) - 3(actual warmpool size) = -1, + // -1 (deviation) > 1 (max deviation) => false, so no need delete assert.Equal(t, &worker.WarmPoolJob{Operations: worker.OperationReconcileNotRequired}, job) assert.Equal(t, warmPool.pendingDelete, 0) } @@ -403,15 +444,20 @@ func TestPool_ReconcilePool_Delete_NotRequired(t *testing.T) { // TestPool_ReconcilePool_Delete tests that if the warm pool is over the desired warm pool size and has exceed the max // deviation then we issue a return a delete job func TestPool_ReconcilePool_Delete(t *testing.T) { - warmResources := make(map[string][]Resource) - warmResources[res3] = []Resource{{GroupID: res3, ResourceID: res3}} - warmResources[res4] = []Resource{{GroupID: res4, ResourceID: res4}} - warmResources[res5] = []Resource{{GroupID: res5, ResourceID: res5}} - warmResources[res6] = []Resource{{GroupID: res6, ResourceID: res6}} - warmPool := getMockPool(poolConfig, usedResources, warmResources, 7, false) + usedResourcesEmpty := make(map[string]Resource) + poolConfig.WarmIPTarget = 2 + poolConfig.MinIPTarget = 0 + warmResources4 := map[string][]Resource{ + res1: {{GroupID: res3, ResourceID: res1}}, + res2: {{GroupID: res3, ResourceID: res2}}, + res3: {{GroupID: res3, ResourceID: res3}}, + res4: {{GroupID: res3, ResourceID: res4}}, + } + warmPool := getMockPool(poolConfig, usedResourcesEmpty, warmResources4, 7, false) job := warmPool.ReconcilePool() - // deviation = 2(desired WP) - 4(actual WP) = -2, (-deviation)2 > (max deviation)1 => true, need to delete + // deviation = 2(warmIPTarget) - 4(actual warmpool) = -2, + //|-2| (deviation) > 0(max deviation) => true, need to delete // since the warm resources is a map, there is no particular order to delete ip address from secondary ip pool, // we can't assert which two ips would get deleted here assert.Equal(t, 2, job.ResourceCount) @@ -532,12 +578,23 @@ func TestPool_Introspect(t *testing.T) { } func TestPool_SetToDraining_SecondaryIP_Pool(t *testing.T) { - warmPool := getMockPool(poolConfig, usedResources, warmPoolResources, 7, false) + usedResourcesEmpty := map[string]Resource{} + warmPoolResources2 := map[string][]Resource{ + res2: {{GroupID: res2, ResourceID: res2}}, + res3: {{GroupID: res3, ResourceID: res3}}, + } + poolConfig.WarmIPTarget = 1 + poolConfig.MinIPTarget = 0 + warmPool := getMockPool(poolConfig, usedResourcesEmpty, warmPoolResources2, 7, false) job := warmPool.SetToDraining() - // only 1 warm resource, i.e. secondary IP address - assert.Equal(t, &worker.WarmPoolJob{Operations: worker.OperationDeleted, Resources: []string{"res-3"}, ResourceCount: 1}, job) - assert.Equal(t, 1, warmPool.pendingDelete) + // Both 2 warm secondary IP addresses need to be deleted, warm pool should drain to zero + assert.Equal(t, worker.OperationDeleted, job.Operations) + expectedDeletedCount := 2 + assert.Equal(t, expectedDeletedCount, job.ResourceCount) + assert.Equal(t, expectedDeletedCount, len(job.Resources)) + assert.Equal(t, expectedDeletedCount, warmPool.pendingDelete) + assert.Equal(t, 0, warmPool.pendingCreate) } func TestPool_SetToDraining_PD_Pool(t *testing.T) { @@ -555,12 +612,22 @@ func TestPool_SetToDraining_PD_Pool(t *testing.T) { } func TestPool_SetToActive_SecondaryIP_Pool(t *testing.T) { - emptyConfig := &config.WarmPoolConfig{} - warmPool := getMockPool(emptyConfig, usedResources, nil, 7, false) - newConfig := &config.WarmPoolConfig{DesiredSize: config.IPv4DefaultWPSize, MaxDeviation: config.IPv4DefaultMaxDev} + usedResourcesEmpty := map[string]Resource{} + warmPoolResources1 := map[string][]Resource{ + res3: { + {GroupID: res3, ResourceID: res3}, + }, + } + poolConfig.WarmIPTarget = 1 + poolConfig.MinIPTarget = 0 + warmPool := getMockPool(poolConfig, usedResourcesEmpty, warmPoolResources1, 7, false) + newConfig := &config.WarmPoolConfig{ + DesiredSize: 4, + WarmIPTarget: 4, + } job := warmPool.SetToActive(newConfig) - // default desired size is 3 + // 3 secondary IP addresses need to be created assert.Equal(t, &worker.WarmPoolJob{Operations: worker.OperationCreate, ResourceCount: 3}, job) assert.Equal(t, 3, warmPool.pendingCreate) } @@ -809,3 +876,259 @@ func TestNumResourcesFromMap(t *testing.T) { count = numResourcesFromMap(map[string][]Resource{grp5: {}}) assert.Equal(t, 0, count) } + +// TestCalcSecondaryIPDeviation_PDDisabledAndInvalidZeroTargets tests the pool calculation when prefix delegation is disabled, and secondary IP mode is active +// Zero values for minIPTarget and warmIPTarget are permitted because the user should have the flexibility to configure as little as zero minimum and warm IP targets +// Note that at current time, implementation in 'loader.go' prevents these zero values from being propagated, and instead overrides warm-ip-target with a default value of 1 so warmpool always has an available IP +func TestCalcSecondaryIPDeviation_PDDisabledAndInvalidZeroTargets(t *testing.T) { + poolConfig.MinIPTarget = 0 + poolConfig.WarmIPTarget = 0 + pdPool := getMockPool(poolConfig, nil, nil, 7, false) + + deviation := pdPool.calculateSecondaryIPDeviation() + assert.Equal(t, 0, pdPool.warmPoolConfig.WarmIPTarget) + assert.Equal(t, 0, pdPool.warmPoolConfig.MinIPTarget) + assert.Equal(t, 0, deviation) +} + +func TestCalcSecondaryIPDeviation_InvalidMinIPTargetSet_UsesDefaultConfig(t *testing.T) { + createTestCase := func(minIPTarget int) deviationCalcTestCase { + return deviationCalcTestCase{ + warmPoolConfig: &config.WarmPoolConfig{ + DesiredSize: 1, + MinIPTarget: minIPTarget, + }, + isPDPool: false, + warmResources: map[string][]Resource{}, + usedResources: map[string]Resource{}, + capacity: 14, + expectedMinIPSize: config.IPv4DefaultWinMinIPTarget, + expectedWarmIPSize: config.IPv4DefaultWinWarmIPTarget, + expectedDeviation: 0, + } + } + testCases := map[string]deviationCalcTestCase{ + "InvalidNegativeValue1": createTestCase(-1), + "InvalidNegativeValue2": createTestCase(-10), + "InvalidNegativeValue3": createTestCase(-100), + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + sIPPool := getMockPool(tc.warmPoolConfig, tc.usedResources, tc.warmResources, tc.capacity, tc.isPDPool) + sIPPool.calculateSecondaryIPDeviation() + assert.Equal(t, tc.expectedMinIPSize, sIPPool.warmPoolConfig.MinIPTarget) + }) + } +} + +func TestCalcSecondaryIPDeviation_InvalidWarmIPTargetSet_UsesDefaultConfig(t *testing.T) { + createTestCase := func(warmIPTarget int) deviationCalcTestCase { + return deviationCalcTestCase{ + warmPoolConfig: &config.WarmPoolConfig{ + DesiredSize: 1, + WarmIPTarget: warmIPTarget, + }, + isPDPool: false, + warmResources: map[string][]Resource{}, + usedResources: map[string]Resource{}, + capacity: 14, + expectedMinIPSize: config.IPv4DefaultWinMinIPTarget, + expectedWarmIPSize: config.IPv4DefaultWinWarmIPTarget, + expectedDeviation: config.IPv4DefaultWinMinIPTarget, + } + } + testCases := map[string]deviationCalcTestCase{ + "InvalidNegativeValue1": createTestCase(-1), + "InvalidNegativeValue2": createTestCase(-10), + "InvalidNegativeValue3": createTestCase(-100), + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + sIPPool := getMockPool(tc.warmPoolConfig, tc.usedResources, tc.warmResources, tc.capacity, tc.isPDPool) + sIPPool.calculateSecondaryIPDeviation() + assert.Equal(t, tc.expectedWarmIPSize, sIPPool.warmPoolConfig.WarmIPTarget) + }) + } +} + +func TestCalcSecondaryIPDeviation_OnlyWarmIPTargetSetToNonZero_ExpectedDeviation(t *testing.T) { + createTestCase := func(warmIPTarget int, pendingCreate int, expectedDeviation int, warmResources map[string][]Resource, usedResources map[string]Resource) deviationCalcTestCase { + return deviationCalcTestCase{ + warmPoolConfig: &config.WarmPoolConfig{ + DesiredSize: 1, + WarmIPTarget: warmIPTarget, + MinIPTarget: 0, + }, + isPDPool: false, + pendingCreate: pendingCreate, + warmResources: warmResources, + usedResources: usedResources, + expectedWarmIPSize: warmIPTarget, + expectedMinIPSize: 0, + expectedDeviation: expectedDeviation, + } + } + + usedResourcesEmpty := map[string]Resource{} + usedResources1 := map[string]Resource{ + pod1: {GroupID: grp1, ResourceID: res1}, + } + usedResources2 := map[string]Resource{ + pod1: {GroupID: grp1, ResourceID: res1}, + pod2: {GroupID: grp2, ResourceID: res2}, + } + warmResourcesEmpty := make(map[string][]Resource) + warmResources1 := make(map[string][]Resource) + warmResources1[res1] = []Resource{{GroupID: res1, ResourceID: res1}} + warmResources2 := make(map[string][]Resource) + warmResources2[res1] = []Resource{{GroupID: res1, ResourceID: res1}, {GroupID: res2, ResourceID: res2}} + + testCases := map[string]deviationCalcTestCase{ + "ResourcesNeedToBeDeleted1": createTestCase(1, 0, -1, warmResources2, usedResourcesEmpty), + "NoResourcesInUseNoWarmNoPendingCreate": createTestCase(3, 0, 3, warmResourcesEmpty, usedResourcesEmpty), + "SomePendingResourcesScenario1": createTestCase(2, 2, 0, warmResourcesEmpty, usedResourcesEmpty), + "SomePendingResourcesScenario2": createTestCase(2, 1, 1, warmResourcesEmpty, usedResourcesEmpty), + "SomeWarmResourcesScenario1": createTestCase(2, 0, 1, warmResources1, usedResourcesEmpty), + "SomeWarmResourcesScenario2": createTestCase(2, 0, 0, warmResources2, usedResourcesEmpty), + "SomeUsedResourcesScenario1": createTestCase(2, 0, 2, warmResourcesEmpty, usedResources1), + "SomeUsedResourcesScenario2": createTestCase(2, 0, 2, warmResourcesEmpty, usedResources2), + "ComplexScenario1": createTestCase(3, 1, 0, warmResources2, usedResources1), + "ComplexScenario2": createTestCase(3, 2, 0, warmResources1, usedResources2), + "ComplexScenario3": createTestCase(3, 1, 0, warmResources2, usedResources2), + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + sIPPool := getMockPool(tc.warmPoolConfig, tc.usedResources, tc.warmResources, tc.capacity, tc.isPDPool) + sIPPool.pendingCreate = tc.pendingCreate + deviation := sIPPool.calculateSecondaryIPDeviation() + assert.Equal(t, tc.expectedWarmIPSize, sIPPool.warmPoolConfig.WarmIPTarget) + assert.Equal(t, tc.expectedMinIPSize, sIPPool.warmPoolConfig.MinIPTarget) + assert.Equal(t, tc.expectedDeviation, deviation) + }) + } +} + +func TestCalcSecondaryIPDeviation_OnlyMinIPTargetSetToNonZero_ExpectedDeviation(t *testing.T) { + createTestCase := func(minIPTarget int, pendingCreate int, expectedDeviation int, warmResources map[string][]Resource, usedResources map[string]Resource) deviationCalcTestCase { + return deviationCalcTestCase{ + warmPoolConfig: &config.WarmPoolConfig{ + DesiredSize: 1, + WarmIPTarget: 1, + MinIPTarget: minIPTarget, + }, + isPDPool: false, + pendingCreate: pendingCreate, + warmResources: warmResources, + usedResources: usedResources, + expectedMinIPSize: minIPTarget, + expectedWarmIPSize: 1, + expectedDeviation: expectedDeviation, + } + } + + usedResourcesEmpty := map[string]Resource{} + usedResources1 := map[string]Resource{ + pod1: {GroupID: grp1, ResourceID: res1}, + } + usedResources2 := map[string]Resource{ + pod1: {GroupID: grp1, ResourceID: res1}, + pod2: {GroupID: grp2, ResourceID: res2}, + } + warmResourcesEmpty := make(map[string][]Resource) + warmResources1 := make(map[string][]Resource) + warmResources1[res1] = []Resource{{GroupID: res1, ResourceID: res1}} + warmResources2 := make(map[string][]Resource) + warmResources2[res1] = []Resource{{GroupID: res1, ResourceID: res1}, {GroupID: res2, ResourceID: res2}} + + testCases := map[string]deviationCalcTestCase{ + "ResourcesNeedToBeDeleted": createTestCase(0, 0, -1, warmResources2, usedResourcesEmpty), + "NoResourcesInUseNoWarmNoPendingCreate": createTestCase(3, 0, 3, warmResourcesEmpty, usedResourcesEmpty), + "SomePendingResourcesScenario1": createTestCase(2, 2, 0, warmResourcesEmpty, usedResourcesEmpty), + "SomePendingResourcesScenario2": createTestCase(2, 1, 1, warmResourcesEmpty, usedResourcesEmpty), + "SomeWarmResourcesScenario1": createTestCase(2, 0, 1, warmResources1, usedResourcesEmpty), + "SomeWarmResourcesScenario2": createTestCase(2, 0, 0, warmResources2, usedResourcesEmpty), + "SomeUsedResourcesScenario1": createTestCase(2, 0, 1, warmResourcesEmpty, usedResources1), + "SomeUsedResourcesScenario2": createTestCase(2, 0, 1, warmResourcesEmpty, usedResources2), + "ComplexScenario1": createTestCase(5, 1, 2, warmResources1, usedResources1), + "ComplexScenario2": createTestCase(5, 3, -2, warmResources2, usedResources2), + "ComplexScenario3": createTestCase(5, 2, 0, warmResources2, usedResources1), + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + sIPPool := getMockPool(tc.warmPoolConfig, tc.usedResources, tc.warmResources, tc.capacity, tc.isPDPool) + sIPPool.pendingCreate = tc.pendingCreate + deviation := sIPPool.calculateSecondaryIPDeviation() + assert.Equal(t, tc.expectedWarmIPSize, sIPPool.warmPoolConfig.WarmIPTarget) + assert.Equal(t, tc.expectedMinIPSize, sIPPool.warmPoolConfig.MinIPTarget) + assert.Equal(t, tc.expectedDeviation, deviation) + }) + } +} + +func TestCalcSecondaryIPDeviation_MinIPTargetAndWarmIPTargetSet_ExpectedDeviation(t *testing.T) { + createTestCase := func(minIPTarget int, warmIPTarget int, pendingCreate int, expectedDeviation int, warmResources map[string][]Resource, usedResources map[string]Resource) deviationCalcTestCase { + return deviationCalcTestCase{ + warmPoolConfig: &config.WarmPoolConfig{ + DesiredSize: 1, + WarmIPTarget: warmIPTarget, + MinIPTarget: minIPTarget, + }, + isPDPool: false, + pendingCreate: pendingCreate, + warmResources: warmResources, + usedResources: usedResources, + expectedMinIPSize: minIPTarget, + expectedWarmIPSize: warmIPTarget, + expectedDeviation: expectedDeviation, + } + } + + usedResourcesEmpty := map[string]Resource{} + usedResources1 := map[string]Resource{ + pod1: {GroupID: grp1, ResourceID: res1}, + } + usedResources2 := map[string]Resource{ + pod1: {GroupID: grp1, ResourceID: res1}, + pod2: {GroupID: grp2, ResourceID: res2}, + } + warmResourcesEmpty := make(map[string][]Resource) + warmResources1 := make(map[string][]Resource) + warmResources1[res1] = []Resource{{GroupID: res1, ResourceID: res1}} + warmResources2 := make(map[string][]Resource) + warmResources2[res1] = []Resource{{GroupID: res1, ResourceID: res1}, {GroupID: res2, ResourceID: res2}} + warmResources5 := make(map[string][]Resource) + warmResources5[res1] = []Resource{ + {GroupID: res1, ResourceID: res1}, + {GroupID: res2, ResourceID: res2}, + {GroupID: res3, ResourceID: res3}, + {GroupID: res4, ResourceID: res4}, + {GroupID: res5, ResourceID: res5}, + } + + testCases := map[string]deviationCalcTestCase{ + "Scenario1": createTestCase(1, 1, 0, 1, warmResourcesEmpty, usedResourcesEmpty), + "Scenario2": createTestCase(5, 2, 0, 3, warmResources2, usedResourcesEmpty), + "Scenario3": createTestCase(5, 2, 0, 0, warmResources5, usedResourcesEmpty), + "Scenario4": createTestCase(0, 1, 0, -4, warmResources5, usedResourcesEmpty), + "Scenario5": createTestCase(4, 5, 2, 1, warmResources2, usedResources1), + "Scenario6": createTestCase(10, 5, 1, 7, warmResources1, usedResources1), + "Scenario7": createTestCase(12, 12, 5, 5, warmResources2, usedResources2), + "Scenario8": createTestCase(0, 1, 0, 0, warmResources1, usedResources1), + "Scenario9": createTestCase(2, 1, 0, -1, warmResources2, usedResources1), + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + sIPPool := getMockPool(tc.warmPoolConfig, tc.usedResources, tc.warmResources, tc.capacity, tc.isPDPool) + sIPPool.pendingCreate = tc.pendingCreate + deviation := sIPPool.calculateSecondaryIPDeviation() + assert.Equal(t, tc.expectedWarmIPSize, sIPPool.warmPoolConfig.WarmIPTarget) + assert.Equal(t, tc.expectedMinIPSize, sIPPool.warmPoolConfig.MinIPTarget) + assert.Equal(t, tc.expectedDeviation, deviation) + }) + } +} diff --git a/pkg/pool/utils.go b/pkg/pool/utils.go new file mode 100644 index 00000000..e729e081 --- /dev/null +++ b/pkg/pool/utils.go @@ -0,0 +1,39 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file 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 pool + +import ( + "github.com/go-logr/logr" + + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" +) + +// GetWinWarmPoolConfig retrieves Windows warmpool configuration from ConfigMap, falls back to using default values on failure +func GetWinWarmPoolConfig(log logr.Logger, w api.Wrapper, isPDEnabled bool) *config.WarmPoolConfig { + var resourceConfig map[string]config.ResourceConfig + vpcCniConfigMap, err := w.K8sAPI.GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace) + if err == nil { + resourceConfig = config.LoadResourceConfigFromConfigMap(log, vpcCniConfigMap) + } else { + log.Error(err, "failed to read from config map, will use default resource config") + resourceConfig = config.LoadResourceConfig() + } + + if isPDEnabled { + return resourceConfig[config.ResourceNameIPAddressFromPrefix].WarmPoolConfig + } else { + return resourceConfig[config.ResourceNameIPAddress].WarmPoolConfig + } +} diff --git a/pkg/pool/utils_test.go b/pkg/pool/utils_test.go new file mode 100644 index 00000000..e41d12bf --- /dev/null +++ b/pkg/pool/utils_test.go @@ -0,0 +1,112 @@ +// Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"). You may +// not use this file except in compliance with the License. A copy of the +// License is located at +// +// http://aws.amazon.com/apache2.0/ +// +// or in the "license" file accompanying this file. This file 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 pool + +import ( + "fmt" + "strconv" + "testing" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + v1 "k8s.io/api/core/v1" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + + mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" +) + +func TestGetWinWarmPoolConfig_PDDisabledAndAPICallSuccess_ReturnsConfig(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + log := zap.New(zap.UseDevMode(true)).WithName("provider test") + + configMapToReturn := &v1.ConfigMap{ + Data: map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "true", + config.WinWarmIPTarget: strconv.Itoa(config.IPv4DefaultWinWarmIPTarget), + config.WinMinimumIPTarget: strconv.Itoa(config.IPv4DefaultWinMinIPTarget), + }, + } + expectedWarmPoolConfig := &config.WarmPoolConfig{ + WarmIPTarget: config.IPv4DefaultWinWarmIPTarget, + MinIPTarget: config.IPv4DefaultWinMinIPTarget, + DesiredSize: config.IPv4DefaultWinWarmIPTarget, + } + + mockK8sWrapper := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sWrapper.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(configMapToReturn, nil) + apiWrapperMock := api.Wrapper{K8sAPI: mockK8sWrapper} + + actualWarmPoolConfig := GetWinWarmPoolConfig(log, apiWrapperMock, false) + assert.Equal(t, expectedWarmPoolConfig, actualWarmPoolConfig) +} + +func TestGetWinWarmPoolConfig_PDEnabledAndAPICallSuccess_ReturnsConfig(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + log := zap.New(zap.UseDevMode(true)).WithName("provider test") + + configMapToReturn := &v1.ConfigMap{ + Data: map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "true", + config.WinWarmIPTarget: strconv.Itoa(config.IPv4PDDefaultWarmIPTargetSize), + config.WinWarmPrefixTarget: strconv.Itoa(config.IPv4PDDefaultWarmPrefixTargetSize), + config.WinMinimumIPTarget: strconv.Itoa(config.IPv4PDDefaultMinIPTargetSize), + }, + } + expectedWarmPoolConfig := &config.WarmPoolConfig{ + WarmIPTarget: config.IPv4PDDefaultWarmIPTargetSize, + WarmPrefixTarget: config.IPv4PDDefaultWarmPrefixTargetSize, + MinIPTarget: config.IPv4PDDefaultMinIPTargetSize, + DesiredSize: config.IPv4PDDefaultWarmIPTargetSize, + } + + mockK8sWrapper := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sWrapper.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(configMapToReturn, nil) + apiWrapperMock := api.Wrapper{K8sAPI: mockK8sWrapper} + + actualWarmPoolConfig := GetWinWarmPoolConfig(log, apiWrapperMock, true) + assert.Equal(t, expectedWarmPoolConfig, actualWarmPoolConfig) +} + +func TestGetWinWarmPoolConfig_PDDisabledAndAPICallFailure_ReturnsDefaultConfig(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + log := zap.New(zap.UseDevMode(true)).WithName("provider test") + + var configMapToReturn *v1.ConfigMap = nil + errorToReturn := fmt.Errorf("Some error occurred while fetching config map") + expectedWarmPoolConfig := &config.WarmPoolConfig{ + WarmIPTarget: config.IPv4DefaultWinWarmIPTarget, + MinIPTarget: config.IPv4DefaultWinMinIPTarget, + DesiredSize: config.IPv4DefaultWinWarmIPTarget, + } + + mockK8sWrapper := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sWrapper.EXPECT().GetConfigMap( + config.VpcCniConfigMapName, + config.KubeSystemNamespace, + ).Return( + configMapToReturn, + errorToReturn, + ) + apiWrapperMock := api.Wrapper{K8sAPI: mockK8sWrapper} + + actualWarmPoolConfig := GetWinWarmPoolConfig(log, apiWrapperMock, false) + assert.Equal(t, expectedWarmPoolConfig, actualWarmPoolConfig) +} diff --git a/pkg/provider/branch/provider.go b/pkg/provider/branch/provider.go index 4bb3cb36..749b4bdc 100644 --- a/pkg/provider/branch/provider.go +++ b/pkg/provider/branch/provider.go @@ -22,6 +22,8 @@ import ( "sync" "time" + "github.com/google/uuid" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/vpc" @@ -33,7 +35,6 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/trunk" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/worker" - "github.com/google/uuid" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/go-logr/logr" diff --git a/pkg/provider/ip/provider.go b/pkg/provider/ip/provider.go index f3bba704..e33c2853 100644 --- a/pkg/provider/ip/provider.go +++ b/pkg/provider/ip/provider.go @@ -153,9 +153,11 @@ func (p *ipv4Provider) InitResource(instance ec2.EC2Instance) error { // Expected node capacity based on instance type in secondary IP mode nodeCapacity := getCapacity(instance.Type(), instance.Os()) + isPDEnabled := p.conditions.IsWindowsPrefixDelegationEnabled() + p.config = pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isPDEnabled) + // Set warm pool config to empty config if PD is enabled secondaryIPWPConfig := p.config - isPDEnabled := p.conditions.IsWindowsPrefixDelegationEnabled() if isPDEnabled { secondaryIPWPConfig = &config.WarmPoolConfig{} } else { @@ -239,6 +241,8 @@ func (p *ipv4Provider) UpdateResourceCapacity(instance ec2.EC2Instance) error { resourceProviderAndPool.isPrevPDEnabled = false + p.config = pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isCurrPDEnabled && isNitroInstance) + // Set the secondary IP provider pool state to active job := resourceProviderAndPool.resourcePool.SetToActive(p.config) if job.Operations != worker.OperationReconcileNotRequired { diff --git a/pkg/provider/ip/provider_test.go b/pkg/provider/ip/provider_test.go index efe77261..e05f42e5 100644 --- a/pkg/provider/ip/provider_test.go +++ b/pkg/provider/ip/provider_test.go @@ -16,8 +16,11 @@ package ip import ( "fmt" "reflect" + "strconv" "testing" + v1 "k8s.io/api/core/v1" + mock_ec2 "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2" mock_condition "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/condition" mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s" @@ -47,9 +50,11 @@ var ( nodeCapacity = 14 ipV4WarmPoolConfig = config.WarmPoolConfig{ - DesiredSize: config.IPv4DefaultWPSize, - MaxDeviation: config.IPv4DefaultMaxDev, - ReservedSize: config.IPv4DefaultResSize, + WarmIPTarget: config.IPv4DefaultWinWarmIPTarget, + MinIPTarget: config.IPv4DefaultWinMinIPTarget, + DesiredSize: config.IPv4DefaultWinWarmIPTarget, + MaxDeviation: config.IPv4DefaultWinMaxDev, + ReservedSize: config.IPv4DefaultWinResSize, } ) @@ -327,17 +332,28 @@ func TestIPv4Provider_UpdateResourceCapacity_FromFromPDToIP(t *testing.T) { mockConditions := mock_condition.NewMockConditions(ctrl) mockWorker := mock_worker.NewMockWorker(ctrl) ipV4WarmPoolConfig := config.WarmPoolConfig{ - DesiredSize: config.IPv4DefaultWPSize, - MaxDeviation: config.IPv4DefaultMaxDev, - ReservedSize: config.IPv4DefaultResSize, + WarmIPTarget: config.IPv4DefaultWinWarmIPTarget, + MinIPTarget: config.IPv4DefaultWinMinIPTarget, + DesiredSize: config.IPv4DefaultWinWarmIPTarget, + MaxDeviation: config.IPv4DefaultWinMaxDev, + ReservedSize: config.IPv4DefaultWinResSize, } ipv4Provider := ipv4Provider{apiWrapper: api.Wrapper{K8sAPI: mockK8sWrapper}, workerPool: mockWorker, config: &ipV4WarmPoolConfig, instanceProviderAndPool: map[string]*ResourceProviderAndPool{}, log: zap.New(zap.UseDevMode(true)).WithName("ip provider"), conditions: mockConditions} + expectedVpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "false", + config.WarmIPTarget: strconv.Itoa(config.IPv4DefaultWinWarmIPTarget), + config.MinimumIPTarget: strconv.Itoa(config.IPv4DefaultWinMinIPTarget), + }, + } mockPool := mock_pool.NewMockPool(ctrl) mockManager := mock_eni.NewMockENIManager(ctrl) ipv4Provider.putInstanceProviderAndPool(nodeName, mockPool, mockManager, nodeCapacity, true) mockConditions.EXPECT().IsWindowsPrefixDelegationEnabled().Return(false) + mockK8sWrapper.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(expectedVpcCNIConfig, nil) job := &worker.WarmPoolJob{Operations: worker.OperationCreate} mockPool.EXPECT().SetToActive(&ipV4WarmPoolConfig).Return(job) @@ -391,11 +407,21 @@ func TestIPv4Provider_UpdateResourceCapacity_FromFromIPToPD_NonNitro(t *testing. mockWorker := mock_worker.NewMockWorker(ctrl) ipv4Provider := ipv4Provider{apiWrapper: api.Wrapper{K8sAPI: mockK8sWrapper}, workerPool: mockWorker, config: &ipV4WarmPoolConfig, instanceProviderAndPool: map[string]*ResourceProviderAndPool{}, log: zap.New(zap.UseDevMode(true)).WithName("ip provider"), conditions: mockConditions} + expectedVpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "false", + config.WarmIPTarget: strconv.Itoa(config.IPv4DefaultWinWarmIPTarget), + config.MinimumIPTarget: strconv.Itoa(config.IPv4DefaultWinMinIPTarget), + config.WarmPrefixTarget: strconv.Itoa(0), + }, + } mockPool := mock_pool.NewMockPool(ctrl) mockManager := mock_eni.NewMockENIManager(ctrl) ipv4Provider.putInstanceProviderAndPool(nodeName, mockPool, mockManager, nodeCapacity, false) mockConditions.EXPECT().IsWindowsPrefixDelegationEnabled().Return(true) + mockK8sWrapper.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(expectedVpcCNIConfig, nil) job := &worker.WarmPoolJob{Operations: worker.OperationCreate} mockPool.EXPECT().SetToActive(&ipV4WarmPoolConfig).Return(job) @@ -442,11 +468,20 @@ func TestIPv4Provider_UpdateResourceCapacity_FromIPToIP(t *testing.T) { mockWorker := mock_worker.NewMockWorker(ctrl) ipv4Provider := ipv4Provider{apiWrapper: api.Wrapper{K8sAPI: mockK8sWrapper}, workerPool: mockWorker, config: &ipV4WarmPoolConfig, instanceProviderAndPool: map[string]*ResourceProviderAndPool{}, log: zap.New(zap.UseDevMode(true)).WithName("ip provider"), conditions: mockConditions} + expectedVpcCNIConfig := &v1.ConfigMap{ + Data: map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "false", + config.WarmIPTarget: strconv.Itoa(config.IPv4DefaultWinWarmIPTarget), + config.MinimumIPTarget: strconv.Itoa(config.IPv4DefaultWinMinIPTarget), + }, + } mockPool := mock_pool.NewMockPool(ctrl) mockManager := mock_eni.NewMockENIManager(ctrl) ipv4Provider.putInstanceProviderAndPool(nodeName, mockPool, mockManager, nodeCapacity, false) mockConditions.EXPECT().IsWindowsPrefixDelegationEnabled().Return(false) + mockK8sWrapper.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(expectedVpcCNIConfig, nil) job := &worker.WarmPoolJob{Operations: worker.OperationCreate} mockPool.EXPECT().SetToActive(&ipV4WarmPoolConfig).Return(job) diff --git a/pkg/provider/prefix/provider.go b/pkg/provider/prefix/provider.go index 3cb22613..ffcbcb7d 100644 --- a/pkg/provider/prefix/provider.go +++ b/pkg/provider/prefix/provider.go @@ -155,11 +155,11 @@ func (p *ipv4PrefixProvider) InitResource(instance ec2.EC2Instance) error { // Expected node capacity based on instance type in PD mode nodeCapacity := getCapacity(instance.Type(), instance.Os()) * pool.NumIPv4AddrPerPrefix - p.config = p.getPDWarmPoolConfig() + isPDEnabled := p.conditions.IsWindowsPrefixDelegationEnabled() + p.config = pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isPDEnabled) // Set warm pool config to empty if PD is not enabled prefixIPWPConfig := p.config - isPDEnabled := p.conditions.IsWindowsPrefixDelegationEnabled() if !isPDEnabled { prefixIPWPConfig = &config.WarmPoolConfig{} } else { @@ -234,7 +234,7 @@ func (p *ipv4PrefixProvider) UpdateResourceCapacity(instance ec2.EC2Instance) er resourceProviderAndPool.isPrevPDEnabled = true - warmPoolConfig := p.getPDWarmPoolConfig() + warmPoolConfig := pool.GetWinWarmPoolConfig(p.log, p.apiWrapper, isCurrPDEnabled) // Set the secondary IP provider pool state to active job := resourceProviderAndPool.resourcePool.SetToActive(warmPoolConfig) @@ -508,19 +508,6 @@ func getCapacity(instanceType string, instanceOs string) int { return capacity } -// Retrieve dynamic configuration for prefix delegation from config map, else use default warm pool config -func (p *ipv4PrefixProvider) getPDWarmPoolConfig() *config.WarmPoolConfig { - var resourceConfig map[string]config.ResourceConfig - vpcCniConfigMap, err := p.apiWrapper.K8sAPI.GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace) - if err == nil { - resourceConfig = config.LoadResourceConfigFromConfigMap(p.log, vpcCniConfigMap) - } else { - p.log.Error(err, "failed to read from config map, will use default resource config") - resourceConfig = config.LoadResourceConfig() - } - return resourceConfig[config.ResourceNameIPAddressFromPrefix].WarmPoolConfig -} - func (p *ipv4PrefixProvider) check() healthz.Checker { p.log.Info("IPv4 prefix provider's healthz subpath was added") return func(req *http.Request) error { diff --git a/pkg/provider/prefix/provider_test.go b/pkg/provider/prefix/provider_test.go index 3daea497..d30b95d0 100644 --- a/pkg/provider/prefix/provider_test.go +++ b/pkg/provider/prefix/provider_test.go @@ -19,6 +19,8 @@ import ( "strconv" "testing" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + mock_ec2 "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2" mock_condition "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/condition" mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s" @@ -31,7 +33,6 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/ip/eni" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/worker" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" @@ -543,24 +544,6 @@ func getMockIPv4PrefixProvider() ipv4PrefixProvider { log: zap.New(zap.UseDevMode(true)).WithName("prefix provider")} } -func TestGetPDWarmPoolConfig(t *testing.T) { - ctrl := gomock.NewController(t) - defer ctrl.Finish() - - mockK8sWrapper := mock_k8s.NewMockK8sWrapper(ctrl) - mockConditions := mock_condition.NewMockConditions(ctrl) - prefixProvider := ipv4PrefixProvider{apiWrapper: api.Wrapper{K8sAPI: mockK8sWrapper}, - instanceProviderAndPool: map[string]*ResourceProviderAndPool{}, - log: zap.New(zap.UseDevMode(true)).WithName("prefix provider"), conditions: mockConditions} - - for _, c := range []*v1.ConfigMap{vpcCNIConfig, vpcCNIConfigWindows} { - mockK8sWrapper.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(c, nil) - - config := prefixProvider.getPDWarmPoolConfig() - assert.Equal(t, pdWarmPoolConfig, config) - } -} - // TestIsInstanceSupported tests that if the instance type is nitro, return true func TestIsInstanceSupported(t *testing.T) { ctrl := gomock.NewController(t) diff --git a/pkg/provider/provider.go b/pkg/provider/provider.go index c450358f..b5c80a6a 100644 --- a/pkg/provider/provider.go +++ b/pkg/provider/provider.go @@ -14,10 +14,11 @@ package provider import ( - "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2" - "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/pool" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/healthz" + + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/pool" ) // ResourceProvider is the provider interface that each resource managed by the controller has to implement diff --git a/scripts/test/run-integration-tests.sh b/scripts/test/run-integration-tests.sh index 4f7b2105..b378a25e 100755 --- a/scripts/test/run-integration-tests.sh +++ b/scripts/test/run-integration-tests.sh @@ -42,11 +42,12 @@ function run_integration_tests(){ TEST_RESULT=success (cd $INTEGRATION_TEST_DIR/perpodsg && CGO_ENABLED=0 ginkgo --skip=LOCAL $EXTRA_GINKGO_FLAGS -v -timeout=35m -- -cluster-kubeconfig=$KUBE_CONFIG_PATH -cluster-name=$CLUSTER_NAME --aws-region=$REGION --aws-vpc-id $VPC_ID) || TEST_RESULT=fail if [[ -z "${SKIP_WINDOWS_TEST}" ]]; then - (cd $INTEGRATION_TEST_DIR/windows && CGO_ENABLED=0 ginkgo --skip=LOCAL $EXTRA_GINKGO_FLAGS -v -timeout=100m -- -cluster-kubeconfig=$KUBE_CONFIG_PATH -cluster-name=$CLUSTER_NAME --aws-region=$REGION --aws-vpc-id $VPC_ID) || TEST_RESULT=fail + (cd $INTEGRATION_TEST_DIR/windows && CGO_ENABLED=0 ginkgo --skip=LOCAL $EXTRA_GINKGO_FLAGS -v -timeout=150m -- -cluster-kubeconfig=$KUBE_CONFIG_PATH -cluster-name=$CLUSTER_NAME --aws-region=$REGION --aws-vpc-id $VPC_ID) || TEST_RESULT=fail else echo "skipping Windows tests" fi (cd $INTEGRATION_TEST_DIR/webhook && CGO_ENABLED=0 ginkgo --skip=LOCAL $EXTRA_GINKGO_FLAGS -v -timeout=5m -- -cluster-kubeconfig=$KUBE_CONFIG_PATH -cluster-name=$CLUSTER_NAME --aws-region=$REGION --aws-vpc-id $VPC_ID) || TEST_RESULT=fail + # (cd $INTEGRATION_TEST_DIR/cninode && CGO_ENABLED=0 ginkgo --skip=LOCAL $EXTRA_GINKGO_FLAGS -v -timeout=10m -- -cluster-kubeconfig=$KUBE_CONFIG_PATH -cluster-name=$CLUSTER_NAME --aws-region=$REGION --aws-vpc-id $VPC_ID) || TEST_RESULT=fail if [[ "$TEST_RESULT" == fail ]]; then exit 1 diff --git a/test/README.md b/test/README.md index bb9aa28c..a1ba4831 100644 --- a/test/README.md +++ b/test/README.md @@ -63,7 +63,7 @@ The Integration test suite provides the following focuses. echo "Running Security Group for Pods Integration Tests" (cd perpodsg && CGO_ENABLED=0 GOOS=$OS ginkgo -v --timeout 40m -- --cluster-kubeconfig=$KUBE_CONFIG_PATH --cluster-name=$CLUSTER_NAME --aws-region=$AWS_REGION --aws-vpc-id=$VPC_ID) echo "Running Windows Integration Tests" - (cd windows && CGO_ENABLED=0 GOOS=$OS ginkgo -v --timeout 40m -- --cluster-kubeconfig=$KUBE_CONFIG_PATH --cluster-name=$CLUSTER_NAME --aws-region=$AWS_REGION --aws-vpc-id=$VPC_ID) + (cd windows && CGO_ENABLED=0 GOOS=$OS ginkgo -v --timeout 150m -- --cluster-kubeconfig=$KUBE_CONFIG_PATH --cluster-name=$CLUSTER_NAME --aws-region=$AWS_REGION --aws-vpc-id=$VPC_ID) ``` #### Running Integration tests on Controller running on EKS Control Plane diff --git a/test/integration/windows/windows_test.go b/test/integration/windows/windows_test.go index 24962b4a..aadfaaf8 100644 --- a/test/integration/windows/windows_test.go +++ b/test/integration/windows/windows_test.go @@ -59,6 +59,12 @@ var _ = Describe("Windows Integration Test", func() { testConfigMap v1.ConfigMap ) + var sleepInfinityContainerCommands = []string{ + GetCommandToTestHostConnectivity( + "www.amazon.com", 80, 2, true, + ), + } + BeforeEach(func() { namespace = "windows-test" jobParallelism = 1 @@ -203,10 +209,240 @@ var _ = Describe("Windows Integration Test", func() { }) }) + Describe("configMap secondary IP mode tests", Label("windows-prefix-delegation-disabled"), func() { + // Test workflow when windows prefix delegation is disabled and secondary IP mode is active + // In secondary IP mode, pods must have secondary IPs assigned + var testPod, testPod2, testPod3 *v1.Pod + var instanceID string + var nodeName string + var bufferForCoolDown = config.CoolDownPeriod + (time.Second * 5) + var poolReconciliationWaitTime = time.Second * 5 + container := manifest.NewWindowsContainerBuilder().Args(sleepInfinityContainerCommands).Build() + + data = map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "false", + } + testerContainerCommands = []string{ + GetCommandToTestHostConnectivity("www.amazon.com", 80, 2, false), + } + + JustBeforeEach(func() { + windowsNodeList = node.GetNodeAndWaitTillCapacityPresent(frameWork.NodeManager, "windows", + config.ResourceNameIPAddress) + instanceID = manager.GetNodeInstanceID(&windowsNodeList.Items[0]) + nodeName = windowsNodeList.Items[0].Name + + testPod = generateTestPodSpec(1, container, nodeName) + testPod2 = generateTestPodSpec(2, container, nodeName) + testPod3 = generateTestPodSpec(3, container, nodeName) + + data[config.EnableWindowsIPAMKey] = "true" + data[config.EnableWindowsPrefixDelegationKey] = "false" + updateConfigMap(data, poolReconciliationWaitTime) + + GinkgoWriter.Printf("Waiting %d seconds for cooldown period...\n", int(bufferForCoolDown.Seconds())) + time.Sleep(bufferForCoolDown) + }) + + AfterEach(func() { + data = map[string]string{ + config.EnableWindowsIPAMKey: "true", + config.EnableWindowsPrefixDelegationKey: "false", + } + updateConfigMap(data, poolReconciliationWaitTime) + }) + + Context("when prefix delegation is disabled and secondary IP mode is active", func() { + Context("When windows-warm-prefix-target is set to non zero value", Label("windows-warm-prefix-target"), func() { + BeforeEach(func() { + data[config.WinWarmPrefixTarget] = "2" + }) + It("if windows-warm-prefix-target is 2 the value should be ignored and no prefixes should be assigned", func() { + By(fmt.Sprintf("creating 1 Windows pod and waiting until in ready status with timeout of %d seconds", int(utils.WindowsPodsCreationTimeout.Seconds()))) + createdPod1, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + verify.WindowsPodHaveIPv4Address(createdPod1) + + GinkgoWriter.Printf("Waiting %d seconds for warmpool to reconciliate after creating new pod(s)...\n", int(poolReconciliationWaitTime.Seconds())) + time.Sleep(poolReconciliationWaitTime) + + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(BeNumerically(">=", 1)) + Expect(len(prefixCount)).To(Equal(0)) + + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Context("When windows-warm-ip-target is set to non zero value", Label("windows-warm-ip-target"), func() { + BeforeEach(func() { + data[config.WinWarmIPTarget] = "1" + data[config.WinMinimumIPTarget] = "0" + }) + It("if windows-warm-ip-target is 1 should have 1 warm IPs available when 0 pods were running", func() { + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + + Expect(len(ipv4AddressCount)).To(Equal(1)) + Expect(len(prefixCount)).To(Equal(0)) + }) + }) + + Context("When windows-warm-ip-target is 5", Label("windows-warm-ip-target"), func() { + BeforeEach(func() { + data[config.WinWarmIPTarget] = "5" + }) + It("should have 7 warm IPs available when 2 pods were running", func() { + By(fmt.Sprintf("creating 2 Windows pods and waiting until in ready status with timeout of %d seconds", int(utils.WindowsPodsCreationTimeout.Seconds()))) + createdPod1, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + createdPod2, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod2, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + + verify.WindowsPodHaveIPv4Address(createdPod1) + verify.WindowsPodHaveIPv4Address(createdPod2) + + GinkgoWriter.Printf("Waiting %d seconds for warmpool to reconciliate after creating new pod(s)...\n", int(poolReconciliationWaitTime.Seconds())) + time.Sleep(poolReconciliationWaitTime) + + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(BeNumerically("==", 7)) + Expect(len(prefixCount)).To(Equal(0)) + + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) + Expect(err).ToNot(HaveOccurred()) + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod2) + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Context("When windows-minimum-ip-target is set to non zero value", Label("windows-minimum-ip-target"), func() { + BeforeEach(func() { + data[config.WinMinimumIPTarget] = "6" + }) + It("if windows-minimum-ip-target is 6 should have 6 warm IPs available when 0 pods were running", func() { + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(Equal(6)) + Expect(len(prefixCount)).To(Equal(0)) + }) + }) + + Context("When windows-minimum-ip-target is set to 6 with 3 pods running", Label("windows-minimum-ip-target"), func() { + BeforeEach(func() { + data[config.WinMinimumIPTarget] = "6" + }) + It("if windows-minimum-ip-target is 6 should have 6 IPs assigned to the node when 3 pods were running", func() { + By(fmt.Sprintf("creating 3 Windows pods and waiting until in ready status with timeout of %d seconds", int(utils.WindowsPodsCreationTimeout.Seconds()))) + createdPod1, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + createdPod2, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod2, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + createdPod3, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod3, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + + verify.WindowsPodHaveIPv4Address(createdPod1) + verify.WindowsPodHaveIPv4Address(createdPod2) + verify.WindowsPodHaveIPv4Address(createdPod3) + + GinkgoWriter.Printf("Waiting %d seconds for warmpool to reconciliate after creating new pod(s)...\n", int(poolReconciliationWaitTime.Seconds())) + time.Sleep(poolReconciliationWaitTime) + + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(Equal(6)) + Expect(len(prefixCount)).To(Equal(0)) + + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) + Expect(err).ToNot(HaveOccurred()) + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod2) + Expect(err).ToNot(HaveOccurred()) + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod3) + Expect(err).ToNot(HaveOccurred()) + }) + }) + + Context("When windows-warm-ip-target and windows-minimum-ip-target set to non zero values", Label("windows-minimum-ip-target"), func() { + Context("windows-minimum-ip-target=3 and windows-warm-ip-target=6", func() { + BeforeEach(func() { + data[config.WinMinimumIPTarget] = "3" + data[config.WinWarmIPTarget] = "6" + }) + It("if should have 6 IPs assigned and 6 warm IPs available when 0 pods were running", func() { + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(Equal(6)) + Expect(len(prefixCount)).To(Equal(0)) + }) + }) + + Context("windows-minimum-ip-target=8 and windows-warm-ip-target=4", func() { + BeforeEach(func() { + data[config.WinMinimumIPTarget] = "8" + data[config.WinWarmIPTarget] = "4" + }) + It("should have 8 IPs assigned and 8 warm IPs available when 0 pods were running", func() { + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(Equal(8)) + Expect(len(prefixCount)).To(Equal(0)) + }) + }) + Context("windows-minimum-ip-target=2 and windows-warm-ip-target=4", func() { + BeforeEach(func() { + data[config.WinMinimumIPTarget] = "2" + data[config.WarmIPTarget] = "4" + }) + It("should have 6 IPs assigned and 4 warm IPs available when 2 pods were running", func() { + By(fmt.Sprintf("creating 2 Windows pods and waiting until in ready status with timeout of %d seconds", int(utils.WindowsPodsCreationTimeout.Seconds()))) + createdPod1, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + createdPod2, err := frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod2, utils.WindowsPodsCreationTimeout) + Expect(err).ToNot(HaveOccurred()) + + verify.WindowsPodHaveIPv4Address(createdPod1) + verify.WindowsPodHaveIPv4Address(createdPod2) + + GinkgoWriter.Printf("Waiting %d seconds for warmpool to reconciliate after creating new pod(s)...\n", int(poolReconciliationWaitTime.Seconds())) + time.Sleep(poolReconciliationWaitTime) + + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(Equal(6)) + Expect(len(prefixCount)).To(Equal(0)) + + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) + Expect(err).ToNot(HaveOccurred()) + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod2) + Expect(err).ToNot(HaveOccurred()) + }) + }) + }) + + Context("When windows-warm-ip-target and windows-min-ip-target set to 0", Label("windows-warm-ip-target"), func() { + BeforeEach(func() { + data[config.WinMinimumIPTarget] = "0" + data[config.WinWarmIPTarget] = "0" + }) + + It("should result in warm-ip-target=1 and min-ip-target=0", func() { + ipv4AddressCount, prefixCount, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + Expect(err).ToNot(HaveOccurred()) + Expect(len(ipv4AddressCount)).To(Equal(1)) + Expect(len(prefixCount)).To(Equal(0)) + }) + }) + }) + }) + Describe("configMap enable-windows-prefix-delegation tests", Label("windows-prefix-delegation"), func() { // Test windows prefix delegation feature enable/disable. When feature enabled, pod must have // prefix ips assigned. Otherwise, pod must have secondary ip assigned. - var testPod, testPod2 *v1.Pod + var testPod, testPod2, testPodLongLiving *v1.Pod var createdPod *v1.Pod var instanceID string var nodeName string @@ -225,12 +461,13 @@ var _ = Describe("Windows Integration Test", func() { nodeName = windowsNodeList.Items[0].Name testerContainerCommands = []string{ - GetCommandToTestHostConnectivity("www.amazon.com", 80, 2), + GetCommandToTestHostConnectivity("www.amazon.com", 80, 2, false), } testerContainer = manifest.NewWindowsContainerBuilder(). Args(testerContainerCommands). Build() + testContainerLongLiving := manifest.NewWindowsContainerBuilder().Args(sleepInfinityContainerCommands).Build() testPod, err = manifest.NewWindowsPodBuilder(). Namespace("windows-test"). @@ -253,6 +490,17 @@ var _ = Describe("Windows Integration Test", func() { NodeName(nodeName). Build() Expect(err).ToNot(HaveOccurred()) + + testPodLongLiving, err = manifest.NewWindowsPodBuilder(). + Namespace("windows-test"). + Name("windows-pod-long-living"). + Container(testContainerLongLiving). + OS("windows"). + TerminationGracePeriod(0). + RestartPolicy(v1.RestartPolicyNever). + NodeName(nodeName). + Build() + Expect(err).ToNot(HaveOccurred()) }) Context("when prefix delegation is enabled", func() { @@ -271,8 +519,9 @@ var _ = Describe("Windows Integration Test", func() { Context("[CANARY] When enable-windows-prefix-delegation is true", func() { It("pod should be running and assigned ips are from prefix", func() { By("creating pod and waiting for ready") - _, prefixesBefore, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + numIPsBefore, prefixesBefore, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) Expect(err).ToNot(HaveOccurred()) + Expect(len(numIPsBefore)).To(Equal(0)) Expect(len(prefixesBefore)).To(Equal(1)) // verify if ip assigned is coming from a prefix @@ -297,8 +546,9 @@ var _ = Describe("Windows Integration Test", func() { It("two prefixes should be assigned", func() { // allow some time for previous test pod to cool down time.Sleep(bufferForCoolDown) - _, prefixesBefore, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + numIPsBefore, prefixesBefore, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) Expect(err).ToNot(HaveOccurred()) + Expect(len(numIPsBefore)).To(Equal(0)) Expect(len(prefixesBefore)).To(Equal(2)) By("creating pod and waiting for ready should have 1 new prefix assigned") @@ -308,8 +558,9 @@ var _ = Describe("Windows Integration Test", func() { verify.WindowsPodHaveIPv4AddressFromPrefixes(createdPod, prefixesBefore) // number of prefixes should increase by 1 since need 1 more prefix to fulfill warm-prefix-target of 2 - _, prefixesAfter, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + numIPsAfter, prefixesAfter, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) Expect(err).ToNot(HaveOccurred()) + Expect(len(numIPsAfter)).To(Equal(0)) Expect(len(prefixesAfter) - len(prefixesBefore)).To(Equal(1)) err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) @@ -335,7 +586,7 @@ var _ = Describe("Windows Integration Test", func() { By("creating 1 pod and waiting for ready should not create new prefix") // verify if ip assigned is coming from a prefix - createdPod, err = frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod, utils.WindowsPodsCreationTimeout) + createdPod, err = frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPodLongLiving, utils.WindowsPodsCreationTimeout) Expect(err).ToNot(HaveOccurred()) _, prefixesAfterPod1, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) @@ -357,7 +608,7 @@ var _ = Describe("Windows Integration Test", func() { Expect(len(privateIPsBefore)).To(Equal(len(privateIPsAfter))) verify.WindowsPodHaveIPv4AddressFromPrefixes(createdPod, prefixesAfterPod2) - err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPodLongLiving) Expect(err).ToNot(HaveOccurred()) err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod2) Expect(err).ToNot(HaveOccurred()) @@ -430,8 +681,9 @@ var _ = Describe("Windows Integration Test", func() { It("two prefixes should be assigned", func() { // allow some time for previous test pod to cool down time.Sleep(bufferForCoolDown) - _, prefixesBefore, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + numIPsBefore, prefixesBefore, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) Expect(err).ToNot(HaveOccurred()) + Expect(len(numIPsBefore)).To(Equal(0)) Expect(len(prefixesBefore)).To(Equal(2)) By("creating pod and waiting for ready should have 1 new prefix assigned") @@ -441,8 +693,9 @@ var _ = Describe("Windows Integration Test", func() { verify.WindowsPodHaveIPv4AddressFromPrefixes(createdPod, prefixesBefore) // number of prefixes should increase by 1 since need 1 more prefix to fulfill warm-prefix-target of 2 - _, prefixesAfter, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) + numIPsAfter, prefixesAfter, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) Expect(err).ToNot(HaveOccurred()) + Expect(len(numIPsAfter)).To(Equal(0)) Expect(len(prefixesAfter) - len(prefixesBefore)).To(Equal(1)) err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) @@ -467,7 +720,7 @@ var _ = Describe("Windows Integration Test", func() { By("creating 1 pod and waiting for ready should not create new prefix") // verify if ip assigned is coming from a prefix - createdPod, err = frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPod, utils.WindowsPodsCreationTimeout) + createdPod, err = frameWork.PodManager.CreateAndWaitTillPodIsRunning(ctx, testPodLongLiving, utils.WindowsPodsCreationTimeout) Expect(err).ToNot(HaveOccurred()) _, prefixesAfterPod1, err := frameWork.EC2Manager.GetPrivateIPv4AddressAndPrefix(instanceID) @@ -489,7 +742,7 @@ var _ = Describe("Windows Integration Test", func() { Expect(len(privateIPsBefore)).To(Equal(len(privateIPsAfter))) verify.WindowsPodHaveIPv4AddressFromPrefixes(createdPod, prefixesAfterPod2) - err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod) + err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPodLongLiving) Expect(err).ToNot(HaveOccurred()) err = frameWork.PodManager.DeleteAndWaitTillPodIsDeleted(ctx, testPod2) Expect(err).ToNot(HaveOccurred()) @@ -631,7 +884,7 @@ var _ = Describe("Windows Integration Test", func() { CreateAndWaitUntilDeploymentReady(ctx, oldControllerDeployment) Expect(err).ToNot(HaveOccurred()) - By("creating windows pod and waiting for it to timout") + By("creating windows pod and waiting for it to timeout") createdPod, err := frameWork.PodManager. CreateAndWaitTillPodIsRunning(ctx, testPod, utils.WindowsPodsCreationTimeout) Expect(err).To(HaveOccurred()) @@ -685,7 +938,7 @@ var _ = Describe("Windows Integration Test", func() { BeforeEach(func() { jobParallelism = 30 testerContainerCommands = []string{ - GetCommandToTestHostConnectivity(service.Spec.ClusterIP, service.Spec.Ports[0].Port, 10), + GetCommandToTestHostConnectivity(service.Spec.ClusterIP, service.Spec.Ports[0].Port, 10, false), } }) @@ -702,7 +955,7 @@ var _ = Describe("Windows Integration Test", func() { BeforeEach(func() { jobParallelism = 1 testerContainerCommands = []string{ - GetCommandToTestHostConnectivity(service.Spec.ClusterIP, 1, 1), + GetCommandToTestHostConnectivity(service.Spec.ClusterIP, 1, 1, false), } }) @@ -714,7 +967,7 @@ var _ = Describe("Windows Integration Test", func() { Context("when connecting to internet", func() { BeforeEach(func() { testerContainerCommands = []string{ - GetCommandToTestHostConnectivity("www.amazon.com", 80, 2), + GetCommandToTestHostConnectivity("www.amazon.com", 80, 2, false), } }) @@ -727,7 +980,7 @@ var _ = Describe("Windows Integration Test", func() { Context("when connecting to invalid url", func() { BeforeEach(func() { testerContainerCommands = []string{ - GetCommandToTestHostConnectivity("www.amazon.zzz", 80, 1), + GetCommandToTestHostConnectivity("www.amazon.zzz", 80, 1, false), } }) @@ -778,7 +1031,7 @@ var _ = Describe("Windows Integration Test", func() { testerContainer = manifest.NewWindowsContainerBuilder(). Args([]string{ - GetCommandToTestHostConnectivity(service.Spec.ClusterIP, service.Spec.Ports[0].Port, 10)}). + GetCommandToTestHostConnectivity(service.Spec.ClusterIP, service.Spec.Ports[0].Port, 10, false)}). Build() testerJob = manifest.NewWindowsJob(). @@ -835,7 +1088,7 @@ var _ = Describe("Windows Integration Test", func() { Describe("when creating pod with same namespace and name", func() { BeforeEach(func() { testerContainerCommands = []string{ - GetCommandToTestHostConnectivity("www.amazon.com", 80, 2), + GetCommandToTestHostConnectivity("www.amazon.com", 80, 2, false), } }) @@ -883,19 +1136,45 @@ var _ = Describe("Windows Integration Test", func() { }) }) -// GetCommandToTestHostConnectivity tests the DNS Resolution and the tcp connection to the -// host -func GetCommandToTestHostConnectivity(host string, port int32, retries int) string { +func generateTestPodSpec(index int, testerContainer v1.Container, nodeName string) *v1.Pod { + testPod, err := manifest.NewWindowsPodBuilder(). + Namespace("windows-test"). + Name(fmt.Sprintf("windows-secondary-ip-pod-%d", index)). + Container(testerContainer). + OS("windows"). + TerminationGracePeriod(0). + RestartPolicy(v1.RestartPolicyNever). + NodeName(nodeName). + Build() + Expect(err).ToNot(HaveOccurred()) + return testPod +} + +func updateConfigMap(data map[string]string, waitTime time.Duration) { + By("updating the configmap") + builtConfigMap := *manifest.NewConfigMapBuilder().Data(data).Build() + configMapWrapper.UpdateConfigMap(frameWork.ConfigMapManager, ctx, &builtConfigMap) + GinkgoWriter.Printf("Updated amazon-vpc-cni config map data: %v\n", data) + GinkgoWriter.Printf("Waiting %d seconds for pool reconciliation...\n", int(waitTime.Seconds())) + time.Sleep(waitTime) +} + +func GetCommandToTestHostConnectivity(host string, port int32, retries int, sleepForever bool) string { return fmt.Sprintf(` $Server = "%s" $Port = %d $Retries = %d $RetryInterval = 1 + $SleepForever = $%t # If true, sleep forever after the test is complete While (-Not (Test-NetConnection -ComputerName $Server -Port $Port).TcpTestSucceeded) { if ($Retries -le 0) { Write-Warning "maximum number of connection attempts reached, exiting" - exit 1 + if (!$SleepForever) { + exit 1 + } else { + break + } } Write-Warning "failed to connect to server $Server, will retry" Start-Sleep -s $RetryInterval @@ -903,7 +1182,14 @@ func GetCommandToTestHostConnectivity(host string, port int32, retries int) stri # Limit RetryInterval to 20 seconds after it exceeds certain value $RetryInterval = if ($RetryInterval -lt 20) {$RetryInterval*2} else {20} } - Write-Output "connection from $env:COMPUTERNAME to $Server succeeded"`, host, port, retries) + Write-Output "connection from $env:COMPUTERNAME to $Server succeeded" + if ($SleepForever) { + while ($true) { + $SleepSeconds = 3600 + Write-Output "Sleeping forver, will sleep for $SleepSeconds seconds at a time..." + Start-Sleep -Seconds $SleepSeconds; + } + }`, host, port, retries, sleepForever) } // Install and start the dot net web server, it's light weight so starts pretty quick @@ -921,5 +1207,5 @@ func GetCommandToContinuouslyTestHostConnectivity(host string, tries int, interv Start-Sleep -s %d # Sleep for specified interval before testing connection %s # The test connection command $val++ - }`, tries, interval, GetCommandToTestHostConnectivity(host, 80, 10)) + }`, tries, interval, GetCommandToTestHostConnectivity(host, 80, 10, false)) } From f31cb131fccfc2c9a06b6d7b522cd9d6acbb5591 Mon Sep 17 00:00:00 2001 From: Yash Thakkar Date: Wed, 9 Oct 2024 21:35:58 +0000 Subject: [PATCH 11/12] adding context param to function --- test/integration/windows/windows_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/windows/windows_test.go b/test/integration/windows/windows_test.go index aadfaaf8..dd96965b 100644 --- a/test/integration/windows/windows_test.go +++ b/test/integration/windows/windows_test.go @@ -228,7 +228,7 @@ var _ = Describe("Windows Integration Test", func() { } JustBeforeEach(func() { - windowsNodeList = node.GetNodeAndWaitTillCapacityPresent(frameWork.NodeManager, "windows", + windowsNodeList = node.GetNodeAndWaitTillCapacityPresent(frameWork.NodeManager, ctx, "windows", config.ResourceNameIPAddress) instanceID = manager.GetNodeInstanceID(&windowsNodeList.Items[0]) nodeName = windowsNodeList.Items[0].Name From 45dfbde1fb270a916a35473ecf55345715e72752 Mon Sep 17 00:00:00 2001 From: Yash Thakkar Date: Wed, 9 Oct 2024 23:33:09 +0000 Subject: [PATCH 12/12] updating k8s manifest --- .../v1alpha1/zz_generated.deepcopy.go | 1 - .../v1beta1/zz_generated.deepcopy.go | 1 - .../bases/vpcresources.k8s.aws_cninodes.yaml | 25 +++-- ...sources.k8s.aws_securitygrouppolicies.yaml | 96 ++++++++++--------- config/rbac/role.yaml | 27 ++---- config/webhook/manifests.yaml | 16 ++-- 6 files changed, 81 insertions(+), 85 deletions(-) diff --git a/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go b/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go index 3fbda7f0..f30ae4fe 100644 --- a/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go +++ b/apis/vpcresources/v1alpha1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated // Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. // diff --git a/apis/vpcresources/v1beta1/zz_generated.deepcopy.go b/apis/vpcresources/v1beta1/zz_generated.deepcopy.go index d3910ca5..ea8f716e 100644 --- a/apis/vpcresources/v1beta1/zz_generated.deepcopy.go +++ b/apis/vpcresources/v1beta1/zz_generated.deepcopy.go @@ -1,5 +1,4 @@ //go:build !ignore_autogenerated -// +build !ignore_autogenerated // Copyright Amazon.com Inc. or its affiliates. All Rights Reserved. // diff --git a/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml b/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml index 393a50ab..9bb9bb8c 100644 --- a/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml +++ b/config/crd/bases/vpcresources.k8s.aws_cninodes.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.0 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.16.3 name: cninodes.vpcresources.k8s.aws spec: group: vpcresources.k8s.aws @@ -27,20 +26,26 @@ spec: openAPIV3Schema: properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object spec: - description: 'Important: Run "make" to regenerate code after modifying - this file CNINodeSpec defines the desired state of CNINode' + description: |- + Important: Run "make" to regenerate code after modifying this file + CNINodeSpec defines the desired state of CNINode properties: features: items: diff --git a/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml b/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml index 64d4aac0..8346af85 100644 --- a/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml +++ b/config/crd/bases/vpcresources.k8s.aws_securitygrouppolicies.yaml @@ -3,8 +3,7 @@ apiVersion: apiextensions.k8s.io/v1 kind: CustomResourceDefinition metadata: annotations: - controller-gen.kubebuilder.io/version: v0.9.0 - creationTimestamp: null + controller-gen.kubebuilder.io/version: v0.16.3 name: securitygrouppolicies.vpcresources.k8s.aws spec: group: vpcresources.k8s.aws @@ -29,14 +28,19 @@ spec: description: Custom Resource Definition for applying security groups to pods properties: apiVersion: - description: 'APIVersion defines the versioned schema of this representation - of an object. Servers should convert recognized schemas to the latest - internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources' + description: |- + APIVersion defines the versioned schema of this representation of an object. + Servers should convert recognized schemas to the latest internal value, and + may reject unrecognized values. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources type: string kind: - description: 'Kind is a string value representing the REST resource this - object represents. Servers may infer this from the endpoint the client - submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' + description: |- + Kind is a string value representing the REST resource this object represents. + Servers may infer this from the endpoint the client submits requests to. + Cannot be updated. + In CamelCase. + More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds type: string metadata: type: object @@ -44,33 +48,33 @@ spec: description: SecurityGroupPolicySpec defines the desired state of SecurityGroupPolicy properties: podSelector: - description: A label selector is a label query over a set of resources. - The result of matchLabels and matchExpressions are ANDed. An empty - label selector matches all objects. A null label selector matches - no objects. + description: |- + A label selector is a label query over a set of resources. The result of matchLabels and + matchExpressions are ANDed. An empty label selector matches all objects. A null + label selector matches no objects. properties: matchExpressions: description: matchExpressions is a list of label selector requirements. The requirements are ANDed. items: - description: A label selector requirement is a selector that - contains values, a key, and an operator that relates the key - and values. + description: |- + A label selector requirement is a selector that contains values, a key, and an operator that + relates the key and values. properties: key: description: key is the label key that the selector applies to. type: string operator: - description: operator represents a key's relationship to - a set of values. Valid operators are In, NotIn, Exists - and DoesNotExist. + description: |- + operator represents a key's relationship to a set of values. + Valid operators are In, NotIn, Exists and DoesNotExist. type: string values: - description: values is an array of string values. If the - operator is In or NotIn, the values array must be non-empty. - If the operator is Exists or DoesNotExist, the values - array must be empty. This array is replaced during a strategic + description: |- + values is an array of string values. If the operator is In or NotIn, + the values array must be non-empty. If the operator is Exists or DoesNotExist, + the values array must be empty. This array is replaced during a strategic merge patch. items: type: string @@ -83,13 +87,13 @@ spec: matchLabels: additionalProperties: type: string - description: matchLabels is a map of {key,value} pairs. A single - {key,value} in the matchLabels map is equivalent to an element - of matchExpressions, whose key field is "key", the operator - is "In", and the values array contains only "value". The requirements - are ANDed. + description: |- + matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels + map is equivalent to an element of matchExpressions, whose key field is "key", the + operator is "In", and the values array contains only "value". The requirements are ANDed. type: object type: object + x-kubernetes-map-type: atomic securityGroups: description: GroupIds contains the list of security groups that will be applied to the network interface of the pod matching the criteria. @@ -104,33 +108,33 @@ spec: type: array type: object serviceAccountSelector: - description: A label selector is a label query over a set of resources. - The result of matchLabels and matchExpressions are ANDed. An empty - label selector matches all objects. A null label selector matches - no objects. + description: |- + A label selector is a label query over a set of resources. The result of matchLabels and + matchExpressions are ANDed. An empty label selector matches all objects. A null + label selector matches no objects. properties: matchExpressions: description: matchExpressions is a list of label selector requirements. The requirements are ANDed. items: - description: A label selector requirement is a selector that - contains values, a key, and an operator that relates the key - and values. + description: |- + A label selector requirement is a selector that contains values, a key, and an operator that + relates the key and values. properties: key: description: key is the label key that the selector applies to. type: string operator: - description: operator represents a key's relationship to - a set of values. Valid operators are In, NotIn, Exists - and DoesNotExist. + description: |- + operator represents a key's relationship to a set of values. + Valid operators are In, NotIn, Exists and DoesNotExist. type: string values: - description: values is an array of string values. If the - operator is In or NotIn, the values array must be non-empty. - If the operator is Exists or DoesNotExist, the values - array must be empty. This array is replaced during a strategic + description: |- + values is an array of string values. If the operator is In or NotIn, + the values array must be non-empty. If the operator is Exists or DoesNotExist, + the values array must be empty. This array is replaced during a strategic merge patch. items: type: string @@ -143,13 +147,13 @@ spec: matchLabels: additionalProperties: type: string - description: matchLabels is a map of {key,value} pairs. A single - {key,value} in the matchLabels map is equivalent to an element - of matchExpressions, whose key field is "key", the operator - is "In", and the values array contains only "value". The requirements - are ANDed. + description: |- + matchLabels is a map of {key,value} pairs. A single {key,value} in the matchLabels + map is equivalent to an element of matchExpressions, whose key field is "key", the + operator is "In", and the values array contains only "value". The requirements are ANDed. type: object type: object + x-kubernetes-map-type: atomic type: object type: object served: true diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index b292d57b..31948367 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -2,7 +2,6 @@ apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - creationTimestamp: null name: controller-role rules: - apiGroups: @@ -13,19 +12,11 @@ rules: - create - patch - update -- apiGroups: - - "" - resources: - - pods - verbs: - - get - - list - - patch - - watch - apiGroups: - "" resources: - nodes + - serviceaccounts verbs: - get - list @@ -40,10 +31,11 @@ rules: - apiGroups: - "" resources: - - serviceaccounts + - pods verbs: - get - list + - patch - watch - apiGroups: - crd.k8s.amazonaws.com @@ -74,26 +66,25 @@ rules: apiVersion: rbac.authorization.k8s.io/v1 kind: Role metadata: - creationTimestamp: null name: controller-role namespace: kube-system rules: - apiGroups: - - apps + - "" resourceNames: - - vpc-resource-controller + - amazon-vpc-cni resources: - - deployments + - configmaps verbs: - get - list - watch - apiGroups: - - "" + - apps resourceNames: - - amazon-vpc-cni + - vpc-resource-controller resources: - - configmaps + - deployments verbs: - get - list diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 78f57a87..cf8e7b86 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -2,7 +2,6 @@ apiVersion: admissionregistration.k8s.io/v1 kind: MutatingWebhookConfiguration metadata: - creationTimestamp: null name: mutating-webhook-configuration webhooks: - admissionReviewVersions: @@ -29,7 +28,6 @@ webhooks: apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: - creationTimestamp: null name: validating-webhook-configuration webhooks: - admissionReviewVersions: @@ -38,20 +36,19 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-v1-pod + path: /validate-v1-node failurePolicy: Ignore matchPolicy: Equivalent - name: vpod.vpc.k8s.aws + name: vnode.vpc.k8s.aws rules: - apiGroups: - "" apiVersions: - v1 operations: - - CREATE - UPDATE resources: - - pods + - nodes sideEffects: None - admissionReviewVersions: - v1 @@ -59,17 +56,18 @@ webhooks: service: name: webhook-service namespace: system - path: /validate-v1-node + path: /validate-v1-pod failurePolicy: Ignore matchPolicy: Equivalent - name: vnode.vpc.k8s.aws + name: vpod.vpc.k8s.aws rules: - apiGroups: - "" apiVersions: - v1 operations: + - CREATE - UPDATE resources: - - nodes + - pods sideEffects: None