From 90024bb85ddde79626f29a126c59bab6d3415164 Mon Sep 17 00:00:00 2001 From: Sushmitha Ravikumar <58063229+sushrk@users.noreply.github.com> Date: Thu, 7 Dec 2023 08:32:03 +0530 Subject: [PATCH] Add support to configure branch ENI cooldown period via configmap (#342) * Add support to configure branch ENI cooldown period via configmap * support configurable branch ENI cooldown period * moving error check out from CM update * Fix logs and remove mutex lock in Get function * Update to go1.21.5 --------- Co-authored-by: Hao Zhou --- .github/workflows/presubmit.yaml | 2 +- Makefile | 2 +- controllers/core/configmap_controller.go | 21 ++++ controllers/core/configmap_controller_test.go | 27 +++++ main.go | 4 + .../provider/branch/cooldown/mock_cooldown.go | 74 ++++++++++++ pkg/config/type.go | 1 + pkg/provider/branch/cooldown/cooldown.go | 87 ++++++++++++++ pkg/provider/branch/cooldown/cooldown_test.go | 110 ++++++++++++++++++ pkg/provider/branch/provider.go | 4 +- pkg/provider/branch/trunk/trunk.go | 5 +- pkg/provider/branch/trunk/trunk_test.go | 44 ++++++- pkg/utils/events.go | 11 ++ scripts/gen_mocks.sh | 1 + 14 files changed, 381 insertions(+), 12 deletions(-) create mode 100644 mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/cooldown/mock_cooldown.go create mode 100644 pkg/provider/branch/cooldown/cooldown.go create mode 100644 pkg/provider/branch/cooldown/cooldown_test.go diff --git a/.github/workflows/presubmit.yaml b/.github/workflows/presubmit.yaml index 8def3211..198ef181 100644 --- a/.github/workflows/presubmit.yaml +++ b/.github/workflows/presubmit.yaml @@ -45,7 +45,7 @@ jobs: uses: actions/checkout@v3 - uses: actions/setup-go@v4 with: - go-version: '1.21.4' + go-version: '1.21.5' cache-dependency-path: "**/go.sum" - name: Install `govulncheck` run: go install golang.org/x/vuln/cmd/govulncheck@latest diff --git a/Makefile b/Makefile index a9bc2c1f..e66943e8 100644 --- a/Makefile +++ b/Makefile @@ -12,7 +12,7 @@ MAKEFILE_PATH = $(dir $(realpath -s $(firstword $(MAKEFILE_LIST)))) VERSION ?= $(GIT_VERSION) IMAGE ?= $(REPO):$(VERSION) BASE_IMAGE ?= public.ecr.aws/eks-distro-build-tooling/eks-distro-minimal-base-nonroot:latest.2 -BUILD_IMAGE ?= public.ecr.aws/bitnami/golang:1.21.4 +BUILD_IMAGE ?= public.ecr.aws/bitnami/golang:1.21.5 GOARCH ?= amd64 PLATFORM ?= linux/amd64 diff --git a/controllers/core/configmap_controller.go b/controllers/core/configmap_controller.go index 5391526d..e56e3847 100644 --- a/controllers/core/configmap_controller.go +++ b/controllers/core/configmap_controller.go @@ -22,9 +22,12 @@ import ( rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/node/manager" + cooldown "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/go-logr/logr" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" ctrl "sigs.k8s.io/controller-runtime" @@ -73,6 +76,24 @@ func (r *ConfigMapReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } + // Check if branch ENI cooldown period is updated + curCoolDownPeriod := cooldown.GetCoolDown().GetCoolDownPeriod() + if newCoolDownPeriod, err := cooldown.GetVpcCniConfigMapCoolDownPeriodOrDefault(r.K8sAPI, r.Log); err == nil { + if curCoolDownPeriod != newCoolDownPeriod { + r.Log.Info("Branch ENI cool down period has been updated", "newCoolDownPeriod", newCoolDownPeriod, "OldCoolDownPeriod", curCoolDownPeriod) + cooldown.GetCoolDown().SetCoolDownPeriod(newCoolDownPeriod) + utils.SendBroadcastNodeEvent( + r.K8sAPI, + utils.BranchENICoolDownUpdateReason, + fmt.Sprintf("Branch ENI cool down period has been updated to %s", cooldown.GetCoolDown().GetCoolDownPeriod()), + v1.EventTypeNormal, + r.Log, + ) + } + } else { + r.Log.Error(err, "failed to retrieve branch ENI cool down period from amazon-vpc-cni configmap, will retain the current cooldown period", "cool down period", curCoolDownPeriod) + } + // Check if the Windows IPAM flag has changed newWinIPAMEnabledCond := r.Condition.IsWindowsIPAMEnabled() diff --git a/controllers/core/configmap_controller_test.go b/controllers/core/configmap_controller_test.go index a4bd66a7..34635b3c 100644 --- a/controllers/core/configmap_controller_test.go +++ b/controllers/core/configmap_controller_test.go @@ -23,9 +23,11 @@ import ( 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" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -112,6 +114,9 @@ func Test_Reconcile_ConfigMap_Updated(t *testing.T) { 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{}) @@ -125,6 +130,9 @@ func Test_Reconcile_ConfigMap_PD_Disabled_If_IPAM_Disabled(t *testing.T) { mock := NewConfigMapMock(ctrl, mockConfigMapPD) 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() + + cooldown.InitCoolDownPeriod(mock.MockK8sAPI, zap.New(zap.UseDevMode(true)).WithName("cooldown")) res, err := mock.ConfigMapReconciler.Reconcile(context.TODO(), mockConfigMapReq) assert.NoError(t, err) @@ -142,6 +150,9 @@ func Test_Reconcile_ConfigMap_NoData(t *testing.T) { 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() + + 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{}) @@ -154,7 +165,9 @@ func Test_Reconcile_ConfigMap_Deleted(t *testing.T) { mock := NewConfigMapMock(ctrl) 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() + 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{}) @@ -170,9 +183,23 @@ func Test_Reconcile_UpdateNode_Error(t *testing.T) { mock.MockK8sAPI.EXPECT().ListNodes().Return(nodeList, nil) mock.MockNodeManager.EXPECT().GetNode(mockNodeName).Return(mock.MockNode, true) mock.MockNodeManager.EXPECT().UpdateNode(mockNodeName).Return(errMock) + 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.Error(t, err) assert.Equal(t, res, reconcile.Result{}) } + +func createCoolDownMockCM(cooldownTime string) *v1.ConfigMap { + return &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.VpcCniConfigMapName, + Namespace: config.KubeSystemNamespace, + }, + Data: map[string]string{ + config.BranchENICooldownPeriodKey: cooldownTime, + }, + } +} diff --git a/main.go b/main.go index fb4366b5..f7bc7137 100644 --- a/main.go +++ b/main.go @@ -34,6 +34,7 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s/pod" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/node/manager" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/resource" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/version" @@ -290,6 +291,9 @@ func main() { controllerConditions := condition.NewControllerConditions( ctrl.Log.WithName("controller conditions"), k8sApi, enableWindowsPrefixDelegation) + // initialize the branch ENI cool down period + cooldown.InitCoolDownPeriod(k8sApi, ctrl.Log) + // when Windows PD feature flag is OFF, do not initialize resource for prefix IPs var supportedResources []string if enableWindowsPrefixDelegation { diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/cooldown/mock_cooldown.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/cooldown/mock_cooldown.go new file mode 100644 index 00000000..ba1f1427 --- /dev/null +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/cooldown/mock_cooldown.go @@ -0,0 +1,74 @@ +// 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. + +// Code generated by MockGen. DO NOT EDIT. +// Source: github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown (interfaces: CoolDown) + +// Package mock_cooldown is a generated GoMock package. +package mock_cooldown + +import ( + reflect "reflect" + time "time" + + gomock "github.com/golang/mock/gomock" +) + +// MockCoolDown is a mock of CoolDown interface. +type MockCoolDown struct { + ctrl *gomock.Controller + recorder *MockCoolDownMockRecorder +} + +// MockCoolDownMockRecorder is the mock recorder for MockCoolDown. +type MockCoolDownMockRecorder struct { + mock *MockCoolDown +} + +// NewMockCoolDown creates a new mock instance. +func NewMockCoolDown(ctrl *gomock.Controller) *MockCoolDown { + mock := &MockCoolDown{ctrl: ctrl} + mock.recorder = &MockCoolDownMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockCoolDown) EXPECT() *MockCoolDownMockRecorder { + return m.recorder +} + +// GetCoolDownPeriod mocks base method. +func (m *MockCoolDown) GetCoolDownPeriod() time.Duration { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetCoolDownPeriod") + ret0, _ := ret[0].(time.Duration) + return ret0 +} + +// GetCoolDownPeriod indicates an expected call of GetCoolDownPeriod. +func (mr *MockCoolDownMockRecorder) GetCoolDownPeriod() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCoolDownPeriod", reflect.TypeOf((*MockCoolDown)(nil).GetCoolDownPeriod)) +} + +// SetCoolDownPeriod mocks base method. +func (m *MockCoolDown) SetCoolDownPeriod(arg0 time.Duration) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetCoolDownPeriod", arg0) +} + +// SetCoolDownPeriod indicates an expected call of SetCoolDownPeriod. +func (mr *MockCoolDownMockRecorder) SetCoolDownPeriod(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetCoolDownPeriod", reflect.TypeOf((*MockCoolDown)(nil).SetCoolDownPeriod), arg0) +} diff --git a/pkg/config/type.go b/pkg/config/type.go index 6b6a3553..f46f2621 100644 --- a/pkg/config/type.go +++ b/pkg/config/type.go @@ -80,6 +80,7 @@ const ( KubeSystemNamespace = "kube-system" VpcCNIDaemonSetName = "aws-node" OldVPCControllerDeploymentName = "vpc-resource-controller" + BranchENICooldownPeriodKey = "branch-eni-cooldown" ) type ResourceType string diff --git a/pkg/provider/branch/cooldown/cooldown.go b/pkg/provider/branch/cooldown/cooldown.go new file mode 100644 index 00000000..631770df --- /dev/null +++ b/pkg/provider/branch/cooldown/cooldown.go @@ -0,0 +1,87 @@ +// 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 cooldown + +import ( + "fmt" + "strconv" + "sync" + "time" + + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" + "github.com/go-logr/logr" +) + +// Global variable for CoolDownPeriod allows packages to Get and Set the coolDown period +var coolDown *cooldown + +type cooldown struct { + mu sync.RWMutex + // CoolDownPeriod is the period to wait before deleting the branch ENI for propagation of ip tables rule for deleted pod + coolDownPeriod time.Duration +} + +type CoolDown interface { + GetCoolDownPeriod() time.Duration + SetCoolDownPeriod(time.Duration) +} + +const ( + DefaultCoolDownPeriod = time.Second * 60 + MinimalCoolDownPeriod = time.Second * 30 +) + +// Initialize coolDown period by setting the value in configmap or to default +func InitCoolDownPeriod(k8sApi k8s.K8sWrapper, log logr.Logger) { + coolDown = &cooldown{} + coolDownPeriod, err := GetVpcCniConfigMapCoolDownPeriodOrDefault(k8sApi, log) + if err != nil { + log.Info("setting coolDown period to default", "cool down period", coolDownPeriod) + } + coolDown.SetCoolDownPeriod(coolDownPeriod) +} + +func GetCoolDown() CoolDown { + return coolDown +} + +func GetVpcCniConfigMapCoolDownPeriodOrDefault(k8sApi k8s.K8sWrapper, log logr.Logger) (time.Duration, error) { + vpcCniConfigMap, err := k8sApi.GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace) + if err == nil && vpcCniConfigMap.Data != nil { + if val, ok := vpcCniConfigMap.Data[config.BranchENICooldownPeriodKey]; ok { + coolDownPeriodInt, err := strconv.Atoi(val) + if err != nil { + log.Error(err, "failed to parse branch ENI coolDown period", "cool down period", val) + } else { + return time.Second * time.Duration(coolDownPeriodInt), nil + } + } + } + // If configmap not found, or configmap data not found, or error in parsing coolDown period, return default coolDown period and error + return DefaultCoolDownPeriod, fmt.Errorf("failed to get cool down period:%v", err) +} + +func (c *cooldown) GetCoolDownPeriod() time.Duration { + if c.coolDownPeriod < 30*time.Second { + return MinimalCoolDownPeriod + } + return c.coolDownPeriod +} + +func (c *cooldown) SetCoolDownPeriod(newCoolDownPeriod time.Duration) { + c.mu.Lock() + defer c.mu.Unlock() + c.coolDownPeriod = newCoolDownPeriod +} diff --git a/pkg/provider/branch/cooldown/cooldown_test.go b/pkg/provider/branch/cooldown/cooldown_test.go new file mode 100644 index 00000000..ef891a47 --- /dev/null +++ b/pkg/provider/branch/cooldown/cooldown_test.go @@ -0,0 +1,110 @@ +// 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 cooldown + +import ( + "fmt" + "testing" + "time" + + 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/config" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/log/zap" +) + +var log = zap.New(zap.UseDevMode(true)).WithName("cooldown test") +var ( + mockConfigMap30s = &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{Name: config.VpcCniConfigMapName, Namespace: config.KubeSystemNamespace}, + Data: map[string]string{config.BranchENICooldownPeriodKey: "30"}, + } + mockConfigMap29s = &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{Name: config.VpcCniConfigMapName, Namespace: config.KubeSystemNamespace}, + Data: map[string]string{config.BranchENICooldownPeriodKey: "29"}, + } + mockConfigMapNilData = &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{Name: config.VpcCniConfigMapName, Namespace: config.KubeSystemNamespace}, + Data: map[string]string{}, + } + mockConfigMapErrData = &corev1.ConfigMap{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{Name: config.VpcCniConfigMapName, Namespace: config.KubeSystemNamespace}, + Data: map[string]string{config.BranchENICooldownPeriodKey: "aaa"}, + } +) + +func TestCoolDown_InitCoolDownPeriod(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + type args struct { + vpcCniConfigMap *corev1.ConfigMap + } + tests := []struct { + name string + args args + expectedCoolDown time.Duration + err error + }{ + { + name: "VpcCniConfigMap_Exists, verifies cooldown period is set to configmap value when exists", + args: args{vpcCniConfigMap: mockConfigMap30s}, + expectedCoolDown: time.Second * 30, + err: nil, + }, + { + name: "VpcCniConfigMap_NotExists, verifies cool down period is set to default when configmap does not exist", + args: args{}, + expectedCoolDown: time.Second * 60, + err: fmt.Errorf("mock error"), + }, + { + name: "VpcCniConfigMap_Exists_NilData, verifies cool period is set to default when configmap data does not exist", + args: args{vpcCniConfigMap: mockConfigMapNilData}, + expectedCoolDown: time.Second * 60, + err: nil, + }, + { + name: "VpcCniConfigMap_Exists_ErrData, verifies cool period is set to default when configmap data is incorrect", + args: args{vpcCniConfigMap: mockConfigMapErrData}, + expectedCoolDown: time.Second * 60, + err: nil, + }, + { + // critical check to safeguard the cooldown window. at this moment we don't use any time window less than 30 seconds. + name: "VpcCniConfigMap_Force30s_If_Set_Smaller, verifies cooldown period is set to 30 seconds when setting to less than 30", + args: args{vpcCniConfigMap: mockConfigMap29s}, + expectedCoolDown: time.Second * 30, + err: nil, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + ctrl = gomock.NewController(t) + defer ctrl.Finish() + }) + mockK8sApi := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sApi.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(test.args.vpcCniConfigMap, test.err) + InitCoolDownPeriod(mockK8sApi, log) + assert.Equal(t, test.expectedCoolDown, coolDown.GetCoolDownPeriod()) + } +} diff --git a/pkg/provider/branch/provider.go b/pkg/provider/branch/provider.go index a7e72469..0b525a33 100644 --- a/pkg/provider/branch/provider.go +++ b/pkg/provider/branch/provider.go @@ -29,6 +29,7 @@ import ( rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/pool" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" "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" @@ -71,7 +72,6 @@ var ( ReasonTrunkENICreationFailed = "TrunkENICreationFailed" - reconcileRequeueRequest = ctrl.Result{RequeueAfter: time.Minute * 30, Requeue: true} deleteQueueRequeueRequest = ctrl.Result{RequeueAfter: time.Second * 30, Requeue: true} // NodeDeleteRequeueRequestDelay represents the time after which the resources belonging to a node will be cleaned @@ -358,7 +358,7 @@ func (b *branchENIProvider) CreateAndAnnotateResources(podNamespace string, podN branchENIs, err := trunkENI.CreateAndAssociateBranchENIs(pod, securityGroups, resourceCount) if err != nil { if err == trunk.ErrCurrentlyAtMaxCapacity { - return ctrl.Result{RequeueAfter: config.CoolDownPeriod, Requeue: true}, nil + return ctrl.Result{RequeueAfter: cooldown.GetCoolDown().GetCoolDownPeriod(), Requeue: true}, nil } b.apiWrapper.K8sAPI.BroadcastEvent(pod, ReasonBranchAllocationFailed, fmt.Sprintf("failed to allocate branch ENI to pod: %v", err), v1.EventTypeWarning) diff --git a/pkg/provider/branch/trunk/trunk.go b/pkg/provider/branch/trunk/trunk.go index 7b656fa5..6a2eb5dc 100644 --- a/pkg/provider/branch/trunk/trunk.go +++ b/pkg/provider/branch/trunk/trunk.go @@ -26,6 +26,7 @@ import ( ec2Errors "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/errors" "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/aws/aws-sdk-go/aws" awsEC2 "github.com/aws/aws-sdk-go/service/ec2" @@ -38,8 +39,6 @@ import ( const ( // MaxAllocatableVlanIds is the maximum number of Vlan Ids that can be allocated per trunk. MaxAllocatableVlanIds = 121 - // CoolDownPeriod is the period to wait before deleting the branch ENI for propagation of ip tables rule for deleted pod - CoolDownPeriod = time.Second * 30 // MaxDeleteRetries is the maximum number of times the ENI will be retried before being removed from the delete queue MaxDeleteRetries = 3 ) @@ -475,7 +474,7 @@ func (t *trunkENI) PushBranchENIsToCoolDownQueue(UID string) { func (t *trunkENI) DeleteCooledDownENIs() { for eni, hasENI := t.popENIFromDeleteQueue(); hasENI; eni, hasENI = t.popENIFromDeleteQueue() { if eni.deletionTimeStamp.IsZero() || - time.Now().After(eni.deletionTimeStamp.Add(CoolDownPeriod)) { + time.Now().After(eni.deletionTimeStamp.Add(cooldown.GetCoolDown().GetCoolDownPeriod())) { err := t.deleteENI(eni) if err != nil { eni.deleteRetryCount++ diff --git a/pkg/provider/branch/trunk/trunk_test.go b/pkg/provider/branch/trunk/trunk_test.go index 021dd0b0..cb766cad 100644 --- a/pkg/provider/branch/trunk/trunk_test.go +++ b/pkg/provider/branch/trunk/trunk_test.go @@ -21,8 +21,12 @@ import ( mock_ec2 "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2" mock_api "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api" + mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s" + mock_cooldown "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/cooldown" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown" "github.com/aws/aws-sdk-go/aws" awsEc2 "github.com/aws/aws-sdk-go/service/ec2" @@ -216,6 +220,7 @@ func getMockHelperInstanceAndTrunkObject(ctrl *gomock.Controller) (*trunkENI, *m EniDetails2.deleteRetryCount = 0 return &trunkENI, mockHelper, mockInstance + } func getMockTrunk() trunkENI { @@ -436,6 +441,10 @@ func TestTrunkENI_DeleteCooledDownENIs_NotCooledDown(t *testing.T) { EniDetails2.deletionTimeStamp = time.Now() trunkENI.deleteQueue = append(trunkENI.deleteQueue, EniDetails1, EniDetails2) + mockK8sAPI := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sAPI.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(createCoolDownMockCM("30"), nil) + cooldown.InitCoolDownPeriod(mockK8sAPI, zap.New(zap.UseDevMode(true)).WithName("cooldown")) + trunkENI.DeleteCooledDownENIs() assert.Equal(t, 2, len(trunkENI.deleteQueue)) } @@ -448,7 +457,7 @@ func TestTrunkENI_DeleteCooledDownENIs_NoDeletionTimeStamp(t *testing.T) { trunkENI, ec2APIHelper, _ := getMockHelperInstanceAndTrunkObject(ctrl) EniDetails1.deletionTimeStamp = time.Time{} - EniDetails2.deletionTimeStamp = time.Now().Add(-time.Second * 34) + EniDetails2.deletionTimeStamp = time.Now().Add(-(time.Second * 62)) trunkENI.usedVlanIds[VlanId1] = true trunkENI.usedVlanIds[VlanId2] = true @@ -457,6 +466,10 @@ func TestTrunkENI_DeleteCooledDownENIs_NoDeletionTimeStamp(t *testing.T) { ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails1.ID).Return(nil) ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails2.ID).Return(nil) + mockK8sAPI := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sAPI.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(createCoolDownMockCM("30"), nil) + cooldown.InitCoolDownPeriod(mockK8sAPI, zap.New(zap.UseDevMode(true)).WithName("cooldown")) + trunkENI.DeleteCooledDownENIs() assert.Equal(t, 0, len(trunkENI.deleteQueue)) } @@ -467,7 +480,7 @@ func TestTrunkENI_DeleteCooledDownENIs_CooledDownResource(t *testing.T) { defer ctrl.Finish() trunkENI, ec2APIHelper, _ := getMockHelperInstanceAndTrunkObject(ctrl) - EniDetails1.deletionTimeStamp = time.Now().Add(-time.Second * 30) + EniDetails1.deletionTimeStamp = time.Now().Add(-time.Second * 60) EniDetails2.deletionTimeStamp = time.Now().Add(-time.Second * 24) trunkENI.usedVlanIds[VlanId1] = true trunkENI.usedVlanIds[VlanId2] = true @@ -476,6 +489,10 @@ func TestTrunkENI_DeleteCooledDownENIs_CooledDownResource(t *testing.T) { ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails1.ID).Return(nil) + mockK8sAPI := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sAPI.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(createCoolDownMockCM("30"), nil) + cooldown.InitCoolDownPeriod(mockK8sAPI, zap.New(zap.UseDevMode(true)).WithName("cooldown")) + trunkENI.DeleteCooledDownENIs() assert.Equal(t, 1, len(trunkENI.deleteQueue)) assert.Equal(t, EniDetails2, trunkENI.deleteQueue[0]) @@ -488,18 +505,23 @@ func TestTrunkENI_DeleteCooledDownENIs_DeleteFailed(t *testing.T) { defer ctrl.Finish() trunkENI, ec2APIHelper, _ := getMockHelperInstanceAndTrunkObject(ctrl) - EniDetails1.deletionTimeStamp = time.Now().Add(-time.Second * 31) - EniDetails2.deletionTimeStamp = time.Now().Add(-time.Second * 32) + coolDown := mock_cooldown.NewMockCoolDown(ctrl) + EniDetails1.deletionTimeStamp = time.Now().Add(-time.Second * 61) + EniDetails2.deletionTimeStamp = time.Now().Add(-time.Second * 62) trunkENI.usedVlanIds[VlanId1] = true trunkENI.usedVlanIds[VlanId2] = true trunkENI.deleteQueue = append(trunkENI.deleteQueue, EniDetails1, EniDetails2) - gomock.InOrder( + coolDown.EXPECT().GetCoolDownPeriod().Return(time.Second*60).AnyTimes(), ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails1.ID).Return(MockError).Times(MaxDeleteRetries), ec2APIHelper.EXPECT().DeleteNetworkInterface(&EniDetails2.ID).Return(nil), ) + mockK8sAPI := mock_k8s.NewMockK8sWrapper(ctrl) + mockK8sAPI.EXPECT().GetConfigMap(config.VpcCniConfigMapName, config.KubeSystemNamespace).Return(createCoolDownMockCM("60"), nil) + cooldown.InitCoolDownPeriod(mockK8sAPI, zap.New(zap.UseDevMode(true)).WithName("cooldown")) + trunkENI.DeleteCooledDownENIs() assert.Zero(t, len(trunkENI.deleteQueue)) } @@ -860,3 +882,15 @@ func TestTrunkENI_Introspect(t *testing.T) { PodToBranchENI: map[string][]ENIDetails{PodUID: {*EniDetails1}}}, ) } + +func createCoolDownMockCM(cooldownTime string) *v1.ConfigMap { + return &v1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: config.VpcCniConfigMapName, + Namespace: config.KubeSystemNamespace, + }, + Data: map[string]string{ + config.BranchENICooldownPeriodKey: cooldownTime, + }, + } +} diff --git a/pkg/utils/events.go b/pkg/utils/events.go index 5a1f27c5..54efd250 100644 --- a/pkg/utils/events.go +++ b/pkg/utils/events.go @@ -28,6 +28,7 @@ const ( NodeTrunkFailedInitializationReason = "NodeTrunkFailedInit" EniConfigNameNotFoundReason = "EniConfigNameNotFound" VersionNotice = "ControllerVersionNotice" + BranchENICoolDownUpdateReason = "BranchENICoolDownPeriodUpdated" ) func SendNodeEventWithNodeName(client k8s.K8sWrapper, nodeName, reason, msg, eventType string, logger logr.Logger) { @@ -43,3 +44,13 @@ func SendNodeEventWithNodeName(client k8s.K8sWrapper, nodeName, reason, msg, eve func SendNodeEventWithNodeObject(client k8s.K8sWrapper, node *v1.Node, reason, msg, eventType string, logger logr.Logger) { client.BroadcastEvent(node, reason, msg, eventType) } + +func SendBroadcastNodeEvent(client k8s.K8sWrapper, reason, msg, eventType string, logger logr.Logger) { + if nodeList, err := client.ListNodes(); err == nil { + for _, node := range nodeList.Items { + client.BroadcastEvent(&node, reason, msg, eventType) + } + } else { + logger.Info("failed to list nodes when broadcasting node event", "Reason", reason, "Message", msg) + } +} diff --git a/scripts/gen_mocks.sh b/scripts/gen_mocks.sh index 550c2215..c0b65e9d 100755 --- a/scripts/gen_mocks.sh +++ b/scripts/gen_mocks.sh @@ -13,6 +13,7 @@ mockgen -destination=../mocks/amazon-vcp-resource-controller-k8s/pkg/handler/moc # package provider mocks mockgen -destination=../mocks/amazon-vcp-resource-controller-k8s/pkg/provider/mock_provider.go github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider ResourceProvider mockgen -destination=../mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/trunk/mock_trunk.go github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/trunk TrunkENI +mockgen -destination=../mocks/amazon-vcp-resource-controller-k8s/pkg/provider/branch/cooldown/mock_cooldown.go github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/branch/cooldown CoolDown mockgen -destination=../mocks/amazon-vcp-resource-controller-k8s/pkg/provider/ip/eni/mock_eni.go github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider/ip/eni ENIManager # package node mocks mockgen -destination=../mocks/amazon-vcp-resource-controller-k8s/pkg/node/manager/mock_manager.go github.com/aws/amazon-vpc-resource-controller-k8s/pkg/node/manager Manager