From 580e6b0090e5e78f8a629bf10a4ce95ea02692f8 Mon Sep 17 00:00:00 2001 From: Hao Zhou Date: Fri, 5 May 2023 01:23:49 +0000 Subject: [PATCH] enable node events when instance type is not supported --- .../pkg/node/mock_node.go | 25 ++++---- pkg/aws/ec2/instance.go | 2 +- pkg/aws/ec2/instance_test.go | 5 +- pkg/node/manager/manager.go | 10 ++-- pkg/node/manager/manager_test.go | 12 ++-- pkg/node/node.go | 34 +++++++---- pkg/node/node_test.go | 57 +++++++++++++++---- pkg/provider/branch/provider.go | 10 +++- pkg/provider/ip/eni/eni.go | 3 +- pkg/provider/ip/eni/eni_test.go | 22 ++++++- pkg/provider/ip/provider.go | 8 +++ pkg/utils/errors.go | 7 +++ pkg/utils/events.go | 21 +++++++ 13 files changed, 165 insertions(+), 51 deletions(-) create mode 100644 pkg/utils/errors.go create mode 100644 pkg/utils/events.go diff --git a/mocks/amazon-vcp-resource-controller-k8s/pkg/node/mock_node.go b/mocks/amazon-vcp-resource-controller-k8s/pkg/node/mock_node.go index 49c2f56e..476e145a 100644 --- a/mocks/amazon-vcp-resource-controller-k8s/pkg/node/mock_node.go +++ b/mocks/amazon-vcp-resource-controller-k8s/pkg/node/mock_node.go @@ -20,7 +20,6 @@ package mock_node import ( reflect "reflect" - api "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api" resource "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/resource" gomock "github.com/golang/mock/gomock" ) @@ -49,17 +48,17 @@ func (m *MockNode) EXPECT() *MockNodeMockRecorder { } // DeleteResources mocks base method. -func (m *MockNode) DeleteResources(arg0 resource.ResourceManager, arg1 api.EC2APIHelper) error { +func (m *MockNode) DeleteResources(arg0 resource.ResourceManager) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteResources", arg0, arg1) + ret := m.ctrl.Call(m, "DeleteResources", arg0) ret0, _ := ret[0].(error) return ret0 } // DeleteResources indicates an expected call of DeleteResources. -func (mr *MockNodeMockRecorder) DeleteResources(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockNodeMockRecorder) DeleteResources(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteResources", reflect.TypeOf((*MockNode)(nil).DeleteResources), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteResources", reflect.TypeOf((*MockNode)(nil).DeleteResources), arg0) } // GetNodeInstanceID mocks base method. @@ -91,17 +90,17 @@ func (mr *MockNodeMockRecorder) HasInstance() *gomock.Call { } // InitResources mocks base method. -func (m *MockNode) InitResources(arg0 resource.ResourceManager, arg1 api.EC2APIHelper) error { +func (m *MockNode) InitResources(arg0 resource.ResourceManager) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "InitResources", arg0, arg1) + ret := m.ctrl.Call(m, "InitResources", arg0) ret0, _ := ret[0].(error) return ret0 } // InitResources indicates an expected call of InitResources. -func (mr *MockNodeMockRecorder) InitResources(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockNodeMockRecorder) InitResources(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InitResources", reflect.TypeOf((*MockNode)(nil).InitResources), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "InitResources", reflect.TypeOf((*MockNode)(nil).InitResources), arg0) } // IsManaged mocks base method. @@ -145,15 +144,15 @@ func (mr *MockNodeMockRecorder) UpdateCustomNetworkingSpecs(arg0, arg1 interface } // UpdateResources mocks base method. -func (m *MockNode) UpdateResources(arg0 resource.ResourceManager, arg1 api.EC2APIHelper) error { +func (m *MockNode) UpdateResources(arg0 resource.ResourceManager) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateResources", arg0, arg1) + ret := m.ctrl.Call(m, "UpdateResources", arg0) ret0, _ := ret[0].(error) return ret0 } // UpdateResources indicates an expected call of UpdateResources. -func (mr *MockNodeMockRecorder) UpdateResources(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockNodeMockRecorder) UpdateResources(arg0 interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateResources", reflect.TypeOf((*MockNode)(nil).UpdateResources), arg0, arg1) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateResources", reflect.TypeOf((*MockNode)(nil).UpdateResources), arg0) } diff --git a/pkg/aws/ec2/instance.go b/pkg/aws/ec2/instance.go index 6edde892..edd3b013 100644 --- a/pkg/aws/ec2/instance.go +++ b/pkg/aws/ec2/instance.go @@ -115,7 +115,7 @@ func (i *ec2Instance) LoadDetails(ec2APIHelper api.EC2APIHelper) error { i.instanceType = *instance.InstanceType limits, ok := vpc.Limits[i.instanceType] if !ok { - return fmt.Errorf("unsupported instance type, couldn't find ENI Limit for instance %s", i.instanceType) + return fmt.Errorf("unsupported instance type, couldn't find ENI Limit for instance %s, error: %w", i.instanceType, utils.ErrNotFound) } defaultCardIdx := limits.DefaultNetworkCardIndex diff --git a/pkg/aws/ec2/instance_test.go b/pkg/aws/ec2/instance_test.go index f705d0fe..864a3e63 100644 --- a/pkg/aws/ec2/instance_test.go +++ b/pkg/aws/ec2/instance_test.go @@ -17,7 +17,8 @@ import ( "fmt" "testing" - "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api" + mock_api "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -258,6 +259,8 @@ func TestEc2Instance_LoadDetails_InstanceENILimitNotFound(t *testing.T) { err := ec2Instance.LoadDetails(mockEC2ApiHelper) assert.NotNil(t, err) + // ensure the expected error is returned to trigger a node event + assert.ErrorIs(t, err, utils.ErrNotFound) // Clean up nwInterfaces.InstanceType = &instanceType diff --git a/pkg/node/manager/manager.go b/pkg/node/manager/manager.go index 75db777b..62ab2768 100644 --- a/pkg/node/manager/manager.go +++ b/pkg/node/manager/manager.go @@ -143,7 +143,7 @@ func (m *manager) AddNode(nodeName string) error { if shouldManage { newNode = node.NewManagedNode(m.Log, k8sNode.Name, GetNodeInstanceID(k8sNode), - GetNodeOS(k8sNode)) + GetNodeOS(k8sNode), m.wrapper.K8sAPI, m.wrapper.EC2API) err := m.updateSubnetIfUsingENIConfig(newNode, k8sNode) if err != nil { return err @@ -195,7 +195,7 @@ func (m *manager) UpdateNode(nodeName string) error { case UnManagedToManaged: log.Info("node was previously un-managed, will be added as managed node now") cachedNode = node.NewManagedNode(m.Log, k8sNode.Name, - GetNodeInstanceID(k8sNode), GetNodeOS(k8sNode)) + GetNodeInstanceID(k8sNode), GetNodeOS(k8sNode), m.wrapper.K8sAPI, m.wrapper.EC2API) // Update the Subnet if the node has custom networking configured err = m.updateSubnetIfUsingENIConfig(cachedNode, k8sNode) if err != nil { @@ -313,7 +313,7 @@ func (m *manager) performAsyncOperation(job interface{}) (ctrl.Result, error) { var err error switch asyncJob.op { case Init: - err = asyncJob.node.InitResources(m.resourceManager, m.wrapper.EC2API) + err = asyncJob.node.InitResources(m.resourceManager) if err != nil { log.Error(err, "removing the node from cache as it failed to initialize") m.removeNodeSafe(asyncJob.nodeName) @@ -328,9 +328,9 @@ func (m *manager) performAsyncOperation(job interface{}) (ctrl.Result, error) { asyncJob.op = Update return m.performAsyncOperation(asyncJob) case Update: - err = asyncJob.node.UpdateResources(m.resourceManager, m.wrapper.EC2API) + err = asyncJob.node.UpdateResources(m.resourceManager) case Delete: - err = asyncJob.node.DeleteResources(m.resourceManager, m.wrapper.EC2API) + err = asyncJob.node.DeleteResources(m.resourceManager) default: m.Log.V(1).Info("no operation operation requested", "node", asyncJob.nodeName) diff --git a/pkg/node/manager/manager_test.go b/pkg/node/manager/manager_test.go index f9ce2aad..1f0e2ac3 100644 --- a/pkg/node/manager/manager_test.go +++ b/pkg/node/manager/manager_test.go @@ -71,7 +71,7 @@ var ( mockError = fmt.Errorf("mock error") unManagedNode = node.NewUnManagedNode(zap.New(), nodeName, instanceID, config.OSLinux) - managedNode = node.NewManagedNode(zap.New(), nodeName, instanceID, config.OSLinux) + managedNode = node.NewManagedNode(zap.New(), nodeName, instanceID, config.OSLinux, nil, nil) healthzHandler = healthz.NewHealthzHandler(5) ) @@ -432,19 +432,19 @@ func Test_performAsyncOperation(t *testing.T) { job.op = Init mock.MockK8sAPI.EXPECT().AddLabelToManageNode(v1Node, config.HasTrunkAttachedLabel, config.BooleanTrue).Return(true, nil).AnyTimes() - mock.MockNode.EXPECT().InitResources(mock.MockResourceManager, mock.MockEC2API).Return(nil) - mock.MockNode.EXPECT().UpdateResources(mock.MockResourceManager, mock.MockEC2API).Return(nil) + mock.MockNode.EXPECT().InitResources(mock.MockResourceManager).Return(nil) + mock.MockNode.EXPECT().UpdateResources(mock.MockResourceManager).Return(nil) _, err := mock.Manager.performAsyncOperation(job) assert.Contains(t, mock.Manager.dataStore, nodeName) assert.NoError(t, err) job.op = Update - mock.MockNode.EXPECT().UpdateResources(mock.MockResourceManager, mock.MockEC2API).Return(nil) + mock.MockNode.EXPECT().UpdateResources(mock.MockResourceManager).Return(nil) _, err = mock.Manager.performAsyncOperation(job) assert.NoError(t, err) job.op = Delete - mock.MockNode.EXPECT().DeleteResources(mock.MockResourceManager, mock.MockEC2API).Return(nil) + mock.MockNode.EXPECT().DeleteResources(mock.MockResourceManager).Return(nil) _, err = mock.Manager.performAsyncOperation(job) assert.NoError(t, err) @@ -465,7 +465,7 @@ func Test_performAsyncOperation_fail(t *testing.T) { op: Init, } - mock.MockNode.EXPECT().InitResources(mock.MockResourceManager, mock.MockEC2API).Return(&node.ErrInitResources{}) + mock.MockNode.EXPECT().InitResources(mock.MockResourceManager).Return(&node.ErrInitResources{}) _, err := mock.Manager.performAsyncOperation(job) assert.NotContains(t, mock.Manager.dataStore, nodeName) // It should be cleared from cache diff --git a/pkg/node/node.go b/pkg/node/node.go index d95b1ad1..09d35dff 100644 --- a/pkg/node/node.go +++ b/pkg/node/node.go @@ -14,13 +14,17 @@ package node import ( + "errors" "fmt" "sync" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/resource" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" + v1 "k8s.io/api/core/v1" "github.com/go-logr/logr" ) @@ -36,6 +40,10 @@ type node struct { managed bool // instance stores the ec2 instance details that is shared by all the providers instance ec2.EC2Instance + // node has reference to k8s APIs + k8sAPI k8s.K8sWrapper + // node has reference to EC2 APIs + ec2API api.EC2APIHelper } // ErrInitResources to wrap error messages for all errors encountered @@ -50,9 +58,9 @@ func (e *ErrInitResources) Error() string { } type Node interface { - InitResources(resourceManager resource.ResourceManager, helper api.EC2APIHelper) error - DeleteResources(resourceManager resource.ResourceManager, helper api.EC2APIHelper) error - UpdateResources(resourceManager resource.ResourceManager, helper api.EC2APIHelper) error + InitResources(resourceManager resource.ResourceManager) error + DeleteResources(resourceManager resource.ResourceManager) error + UpdateResources(resourceManager resource.ResourceManager) error UpdateCustomNetworkingSpecs(subnetID string, securityGroup []string) IsReady() bool @@ -63,12 +71,14 @@ type Node interface { } // NewManagedNode returns node managed by the controller -func NewManagedNode(log logr.Logger, nodeName string, instanceID string, os string) Node { +func NewManagedNode(log logr.Logger, nodeName string, instanceID string, os string, k8sAPI k8s.K8sWrapper, ec2API api.EC2APIHelper) Node { return &node{ managed: true, log: log.WithName("node resource handler"). WithValues("node name", nodeName), instance: ec2.NewEC2Instance(nodeName, instanceID, os), + k8sAPI: k8sAPI, + ec2API: ec2API, } } @@ -86,7 +96,7 @@ func NewUnManagedNode(log logr.Logger, nodeName, instanceID, os string) Node { } // UpdateNode refreshes the capacity if it's reset to 0 -func (n *node) UpdateResources(resourceManager resource.ResourceManager, helper api.EC2APIHelper) error { +func (n *node) UpdateResources(resourceManager resource.ResourceManager) error { n.lock.Lock() defer n.lock.Unlock() @@ -112,7 +122,7 @@ func (n *node) UpdateResources(resourceManager resource.ResourceManager, helper return fmt.Errorf("failed to update one or more resources %v", errUpdates) } - err := n.instance.UpdateCurrentSubnetAndCidrBlock(helper) + err := n.instance.UpdateCurrentSubnetAndCidrBlock(n.ec2API) if err != nil { n.log.Error(err, "failed to update cidr block", "instance", n.instance.Name()) } @@ -121,12 +131,16 @@ func (n *node) UpdateResources(resourceManager resource.ResourceManager, helper } // InitResources initializes the resource pool and provider of all supported resources -func (n *node) InitResources(resourceManager resource.ResourceManager, helper api.EC2APIHelper) error { +func (n *node) InitResources(resourceManager resource.ResourceManager) error { n.lock.Lock() defer n.lock.Unlock() - - err := n.instance.LoadDetails(helper) + err := n.instance.LoadDetails(n.ec2API) if err != nil { + if errors.Is(err, utils.ErrNotFound) { + // Send a node event for users' visibility + msg := fmt.Sprintf("The instance type %s is not supported yet by the vpc resource controller", n.instance.Type()) + utils.SendNodeEvent(n.k8sAPI, n.instance.Name(), "Unsupported", msg, v1.EventTypeWarning, n.log) + } return &ErrInitResources{ Message: "failed to load instance details", Err: err, @@ -167,7 +181,7 @@ func (n *node) InitResources(resourceManager resource.ResourceManager, helper ap } // DeleteResources performs clean up of all the resource pools and provider of the nodes -func (n *node) DeleteResources(resourceManager resource.ResourceManager, _ api.EC2APIHelper) error { +func (n *node) DeleteResources(resourceManager resource.ResourceManager) error { n.lock.Lock() defer n.lock.Unlock() diff --git a/pkg/node/node_test.go b/pkg/node/node_test.go index 84c45cd8..552bd6c6 100644 --- a/pkg/node/node_test.go +++ b/pkg/node/node_test.go @@ -20,14 +20,17 @@ 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_provider "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/provider" mock_resource "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/resource" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/provider" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/golang/mock/gomock" "github.com/stretchr/testify/assert" v1 "k8s.io/api/core/v1" metaV1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/log/zap" ) @@ -49,6 +52,7 @@ type Mocks struct { MockResourceManager *mock_resource.MockResourceManager MockInstance *mock_ec2.MockEC2Instance MockEC2API *mock_api.MockEC2APIHelper + MockK8sAPI *mock_k8s.MockK8sWrapper NodeWithMock node } @@ -67,17 +71,22 @@ func NewMock(ctrl *gomock.Controller, mockProviderCount int) Mocks { ResourceProvider: convertedProvider, MockResourceManager: mock_resource.NewMockResourceManager(ctrl), MockEC2API: mock_api.NewMockEC2APIHelper(ctrl), + MockK8sAPI: mock_k8s.NewMockK8sWrapper(ctrl), MockInstance: mockInstance, NodeWithMock: node{ log: zap.New(zap.UseDevMode(true)).WithName("branch provider"), instance: mockInstance, + ec2API: mock_api.NewMockEC2APIHelper(ctrl), }, } } // TestNewManagedNode tests the new node is not nil and node is managed but not ready func TestNewManagedNode(t *testing.T) { - node := NewManagedNode(zap.New(), nodeName, instanceID, linux) + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + node := NewManagedNode(zap.New(), nodeName, instanceID, linux, mock_k8s.NewMockK8sWrapper(ctrl), mock_api.NewMockEC2APIHelper(ctrl)) assert.NotNil(t, node) assert.True(t, node.GetNodeInstanceID() == instanceID) @@ -108,12 +117,12 @@ func TestNode_InitResources(t *testing.T) { mock.MockProviders["0"].EXPECT().IsInstanceSupported(mock.MockInstance).Return(true) mock.MockProviders["0"].EXPECT().InitResource(mock.MockInstance).Return(nil) - err := mock.NodeWithMock.InitResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.InitResources(mock.MockResourceManager) assert.NoError(t, err) assert.True(t, mock.NodeWithMock.IsReady()) } -func TestNode_InitResources_InstanceNotSupported(t *testing.T) { +func TestNode_InitResources_InstanceNotTrunkSupported(t *testing.T) { ctrl := gomock.NewController(t) defer ctrl.Finish() @@ -124,11 +133,37 @@ func TestNode_InitResources_InstanceNotSupported(t *testing.T) { mock.MockProviders["0"].EXPECT().IsInstanceSupported(mock.MockInstance).Return(false) - err := mock.NodeWithMock.InitResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.InitResources(mock.MockResourceManager) assert.NoError(t, err) assert.True(t, mock.NodeWithMock.IsReady()) } +func TestNode_InitResources_InstanceNotListed(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mock := NewMock(ctrl, 1) + + testInstanceType := "dummy.large" + nodeName = "testInstance" + node := &v1.Node{ + ObjectMeta: metaV1.ObjectMeta{Name: nodeName, UID: types.UID(nodeName)}, + } + + msg := "The instance type dummy.large is not supported yet by the vpc resource controller" + + mock.MockInstance.EXPECT().Type().Return(testInstanceType).Times(1) + mock.MockInstance.EXPECT().Name().Return(nodeName).Times(1) + mock.MockK8sAPI.EXPECT().GetNode(nodeName).Return(node, nil).Times(1) + mock.MockK8sAPI.EXPECT().BroadcastEvent(node, "Unsupported", msg, v1.EventTypeWarning).Times(1) + mock.MockInstance.EXPECT().LoadDetails(mock.MockEC2API).Return(fmt.Errorf("unsupported instance type, couldn't find ENI Limit for instance %s, error: %w", testInstanceType, utils.ErrNotFound)) + + mock.NodeWithMock.k8sAPI = mock.MockK8sAPI + err := mock.NodeWithMock.InitResources(mock.MockResourceManager) + assert.Error(t, err) + assert.False(t, mock.NodeWithMock.IsReady()) +} + // TestNode_InitResources_LoadInstanceDetails_Error tests that error is propagated when load instance details throws an error func TestNode_InitResources_LoadInstanceDetails_Error(t *testing.T) { ctrl := gomock.NewController(t) @@ -138,7 +173,7 @@ func TestNode_InitResources_LoadInstanceDetails_Error(t *testing.T) { mock.MockInstance.EXPECT().LoadDetails(mock.MockEC2API).Return(mockError) - err := mock.NodeWithMock.InitResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.InitResources(mock.MockResourceManager) assert.Error(t, &ErrInitResources{Err: mockError}, err) } @@ -162,7 +197,7 @@ func TestNode_InitResources_SecondProviderInitFails(t *testing.T) { // Expect first provider to be de initialized mock.MockProviders["0"].EXPECT().DeInitResource(mock.MockInstance).Return(nil).AnyTimes() - err := mock.NodeWithMock.InitResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.InitResources(mock.MockResourceManager) assert.NotNil(t, err) } @@ -181,7 +216,7 @@ func TestNode_DeleteResources(t *testing.T) { mock.MockProviders["1"].EXPECT().IsInstanceSupported(mock.MockInstance).Return(true) mock.MockProviders["1"].EXPECT().DeInitResource(mock.MockInstance).Return(nil) - err := mock.NodeWithMock.DeleteResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.DeleteResources(mock.MockResourceManager) assert.NoError(t, err) } @@ -203,7 +238,7 @@ func TestNode_DeleteResources_SomeFail(t *testing.T) { mock.MockProviders["2"].EXPECT().IsInstanceSupported(mock.MockInstance).Return(true) mock.MockProviders["2"].EXPECT().DeInitResource(mock.MockInstance).Return(nil) - err := mock.NodeWithMock.DeleteResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.DeleteResources(mock.MockResourceManager) assert.NotNil(t, err) } @@ -224,7 +259,7 @@ func TestNode_UpdateResources(t *testing.T) { mock.MockProviders["1"].EXPECT().IsInstanceSupported(mock.MockInstance).Return(false) - err := mock.NodeWithMock.UpdateResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.UpdateResources(mock.MockResourceManager) assert.NoError(t, err) } @@ -247,7 +282,7 @@ func TestNode_UpdateResources_SomeFail(t *testing.T) { mock.MockProviders["2"].EXPECT().IsInstanceSupported(mock.MockInstance).Return(true) mock.MockProviders["2"].EXPECT().UpdateResourceCapacity(mock.MockInstance).Return(nil) - err := mock.NodeWithMock.UpdateResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.UpdateResources(mock.MockResourceManager) assert.NotNil(t, err) } @@ -259,6 +294,6 @@ func TestNode_UpdateResources_NodeNotReady(t *testing.T) { mock := NewMock(ctrl, 1) - err := mock.NodeWithMock.UpdateResources(mock.MockResourceManager, mock.MockEC2API) + err := mock.NodeWithMock.UpdateResources(mock.MockResourceManager) assert.Nil(t, err) } diff --git a/pkg/provider/branch/provider.go b/pkg/provider/branch/provider.go index 27833e62..cf6f5de0 100644 --- a/pkg/provider/branch/provider.go +++ b/pkg/provider/branch/provider.go @@ -30,6 +30,7 @@ import ( "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/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" @@ -466,8 +467,15 @@ func (b *branchENIProvider) GetPool(_ string) (pool.Pool, bool) { // IsInstanceSupported returns true for linux node as pod eni is only supported for linux worker node func (b *branchENIProvider) IsInstanceSupported(instance ec2.EC2Instance) bool { limits, found := vpc.Limits[instance.Type()] - return found && instance.Os() == config.OSLinux && limits.IsTrunkingCompatible + supported := found && instance.Os() == config.OSLinux && limits.IsTrunkingCompatible + if !supported { + // Send a node event for users' visibility + msg := fmt.Sprintf("The instance type %s is not supported for trunk interface (Security Group for Pods)", instance.Type()) + utils.SendNodeEvent(b.apiWrapper.K8sAPI, instance.Name(), "Unsupported", msg, v1.EventTypeWarning, b.log) + } + + return supported } func (b *branchENIProvider) Introspect() interface{} { diff --git a/pkg/provider/ip/eni/eni.go b/pkg/provider/ip/eni/eni.go index cfcfe50e..e5ac29eb 100644 --- a/pkg/provider/ip/eni/eni.go +++ b/pkg/provider/ip/eni/eni.go @@ -21,6 +21,7 @@ import ( "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/vpc" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/aws-sdk-go/aws" "github.com/go-logr/logr" @@ -71,7 +72,7 @@ func (e *eniManager) InitResources(ec2APIHelper api.EC2APIHelper) ([]string, err limits, found := vpc.Limits[e.instance.Type()] if !found { - return nil, fmt.Errorf("unsupported instance type") + return nil, fmt.Errorf("unsupported instance type, error: %w", utils.ErrNotFound) } ipLimit := limits.IPv4PerInterface diff --git a/pkg/provider/ip/eni/eni_test.go b/pkg/provider/ip/eni/eni_test.go index 17401728..c082dde0 100644 --- a/pkg/provider/ip/eni/eni_test.go +++ b/pkg/provider/ip/eni/eni_test.go @@ -18,8 +18,9 @@ import ( "reflect" "testing" - "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2" - "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api" + 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" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" "github.com/golang/mock/gomock" @@ -158,6 +159,23 @@ func TestEni_InitResources_Error(t *testing.T) { assert.Error(t, mockError, err) } +// TestEni_InitResources_Unsupported_Type_Error tests that error is returned if the instance type is not supported +func TestEni_InitResources_Unsupported_Type_Error(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + manager, mockInstance, mockEc2APIHelper := getMockManager(ctrl) + dummyType := "dummy.large" + mockInstance.EXPECT().InstanceID().Return(instanceID) + mockInstance.EXPECT().Type().Return(dummyType) + mockEc2APIHelper.EXPECT().GetInstanceNetworkInterface(&instanceID).Return(nwInterfaces, nil) + + _, err := manager.InitResources(mockEc2APIHelper) + + assert.Error(t, mockError, err) + assert.ErrorIs(t, err, utils.ErrNotFound) +} + // TestEniManager_CreateIPV4Address_FromSingleENI tests IP are created using a single ENI when it has the desired // capacity func TestEniManager_CreateIPV4Address_FromSingleENI(t *testing.T) { diff --git a/pkg/provider/ip/provider.go b/pkg/provider/ip/provider.go index 95351e5e..fc09abd0 100644 --- a/pkg/provider/ip/provider.go +++ b/pkg/provider/ip/provider.go @@ -14,6 +14,7 @@ package ip import ( + "errors" "fmt" "net/http" "sync" @@ -26,7 +27,9 @@ import ( "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/ip/eni" + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils" "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/worker" + v1 "k8s.io/api/core/v1" "github.com/go-logr/logr" ctrl "sigs.k8s.io/controller-runtime" @@ -75,6 +78,11 @@ func (p *ipv4Provider) InitResource(instance ec2.EC2Instance) error { eniManager := eni.NewENIManager(instance) presentIPs, err := eniManager.InitResources(p.apiWrapper.EC2API) if err != nil { + if errors.Is(err, utils.ErrNotFound) { + msg := fmt.Sprintf("The instance type %s is not supported for Windows", instance.Type()) + utils.SendNodeEvent(p.apiWrapper.K8sAPI, instance.Name(), "Unsupported", msg, v1.EventTypeWarning, p.log) + } + return err } diff --git a/pkg/utils/errors.go b/pkg/utils/errors.go new file mode 100644 index 00000000..5c816913 --- /dev/null +++ b/pkg/utils/errors.go @@ -0,0 +1,7 @@ +package utils + +import "errors" + +var ( + ErrNotFound = errors.New("resource was not found") +) diff --git a/pkg/utils/events.go b/pkg/utils/events.go new file mode 100644 index 00000000..01771090 --- /dev/null +++ b/pkg/utils/events.go @@ -0,0 +1,21 @@ +package utils + +import ( + "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/k8s" + "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/types" +) + +var ( + unsupportedInstanceTypeReason = "Unsupported" +) + +func SendNodeEvent(client k8s.K8sWrapper, nodeName, reason, msg, eventType string, logger logr.Logger) { + if node, err := client.GetNode(nodeName); err == nil { + // set UID to node name for kubelet filter the event to node description + node.SetUID(types.UID(nodeName)) + client.BroadcastEvent(node, unsupportedInstanceTypeReason, msg, eventType) + } else { + logger.Error(err, "had an error to get the node for sending unsupported event", "Node", nodeName) + } +}