Skip to content

Commit e7e2c6a

Browse files
yash97yash97
authored
Release 1.7.2 (#554)
* cluster eni cleaner will use cluster tag filter of vpc-cni and vpc-rc (#550) * keeping cluster tag same as vpc-cni * adding OR filter which will use vpc-rc and vpc-cni tag * adding log level to noisy log (#551) * skipping node cleaner if node id is not present (#553) * skipping node cleaner if node id is not present * changing var name
1 parent 23a2153 commit e7e2c6a

File tree

15 files changed

+244
-94
lines changed

15 files changed

+244
-94
lines changed

‎controllers/crds/cninode_controller.go‎

Lines changed: 29 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -67,14 +67,15 @@ func prometheusRegister() {
6767
// CNINodeReconciler reconciles a CNINode object
6868
type CNINodeReconciler struct {
6969
client.Client
70-
scheme *runtime.Scheme
71-
context context.Context
72-
log logr.Logger
73-
eC2Wrapper ec2API.EC2Wrapper
74-
k8sAPI k8s.K8sWrapper
75-
clusterName string
76-
vpcId string
77-
finalizerManager k8s.FinalizerManager
70+
scheme *runtime.Scheme
71+
context context.Context
72+
log logr.Logger
73+
eC2Wrapper ec2API.EC2Wrapper
74+
k8sAPI k8s.K8sWrapper
75+
clusterName string
76+
vpcId string
77+
finalizerManager k8s.FinalizerManager
78+
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner
7879
}
7980

8081
func NewCNINodeReconciler(
@@ -87,17 +88,19 @@ func NewCNINodeReconciler(
8788
clusterName string,
8889
vpcId string,
8990
finalizerManager k8s.FinalizerManager,
91+
newResourceCleaner func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner,
9092
) *CNINodeReconciler {
9193
return &CNINodeReconciler{
92-
Client: client,
93-
scheme: scheme,
94-
context: ctx,
95-
log: logger,
96-
eC2Wrapper: ec2Wrapper,
97-
k8sAPI: k8sWrapper,
98-
clusterName: clusterName,
99-
vpcId: vpcId,
100-
finalizerManager: finalizerManager,
94+
Client: client,
95+
scheme: scheme,
96+
context: ctx,
97+
log: logger,
98+
eC2Wrapper: ec2Wrapper,
99+
k8sAPI: k8sWrapper,
100+
clusterName: clusterName,
101+
vpcId: vpcId,
102+
finalizerManager: finalizerManager,
103+
newResourceCleaner: newResourceCleaner,
101104
}
102105
}
103106

@@ -128,13 +131,13 @@ func (r *CNINodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
128131
shouldPatch := false
129132
cniNodeCopy := cniNode.DeepCopy()
130133
// Add cluster name tag if it does not exist
131-
val, ok := cniNode.Spec.Tags[config.CNINodeClusterNameKey]
134+
val, ok := cniNode.Spec.Tags[config.VPCCNIClusterNameKey]
132135
if !ok || val != r.clusterName {
133136
if len(cniNodeCopy.Spec.Tags) != 0 {
134-
cniNodeCopy.Spec.Tags[config.CNINodeClusterNameKey] = r.clusterName
137+
cniNodeCopy.Spec.Tags[config.VPCCNIClusterNameKey] = r.clusterName
135138
} else {
136139
cniNodeCopy.Spec.Tags = map[string]string{
137-
config.CNINodeClusterNameKey: r.clusterName,
140+
config.VPCCNIClusterNameKey: r.clusterName,
138141
}
139142
}
140143
shouldPatch = true
@@ -174,19 +177,12 @@ func (r *CNINodeReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct
174177
// run cleanup for Linux nodes only
175178
if val, ok := cniNode.ObjectMeta.Labels[config.NodeLabelOS]; ok && val == config.OSLinux {
176179
r.log.Info("running the finalizer routine on cniNode", "cniNode", cniNode.Name)
177-
cleaner := &cleanup.NodeTerminationCleaner{
178-
NodeID: cniNode.Spec.Tags[config.NetworkInterfaceNodeIDKey],
179-
}
180-
cleaner.ENICleaner = &cleanup.ENICleaner{
181-
EC2Wrapper: r.eC2Wrapper,
182-
Manager: cleaner,
183-
VpcId: r.vpcId,
184-
Log: ctrl.Log.WithName("eniCleaner").WithName("node"),
185-
}
186-
187-
if err := cleaner.DeleteLeakedResources(); err != nil {
188-
r.log.Error(err, "failed to cleanup resources during node termination")
189-
ec2API.NodeTerminationENICleanupFailure.Inc()
180+
// run cleanup when node id is present
181+
if nodeID, ok := cniNode.Spec.Tags[config.NetworkInterfaceNodeIDKey]; ok && nodeID != "" {
182+
if err := r.newResourceCleaner(nodeID, r.eC2Wrapper, r.vpcId).DeleteLeakedResources(); err != nil {
183+
r.log.Error(err, "failed to cleanup resources during node termination")
184+
ec2API.NodeTerminationENICleanupFailure.Inc()
185+
}
190186
}
191187
}
192188

‎controllers/crds/cninode_controller_test.go‎

Lines changed: 83 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,11 @@ import (
55
"testing"
66

77
"github.com/aws/amazon-vpc-resource-controller-k8s/apis/vpcresources/v1alpha1"
8+
mock_api "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api"
89
mock_cleanup "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
910
mock_k8s "github.com/aws/amazon-vpc-resource-controller-k8s/mocks/amazon-vcp-resource-controller-k8s/pkg/k8s"
11+
ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api"
12+
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
1013
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
1114
"github.com/golang/mock/gomock"
1215
"github.com/stretchr/testify/assert"
@@ -42,12 +45,12 @@ var (
4245
}
4346
)
4447

45-
func NewCNINodeMock(ctrl *gomock.Controller, mockObjects ...client.Object) CNINodeMock {
48+
func NewCNINodeMock(ctrl *gomock.Controller, mockObjects ...client.Object) *CNINodeMock {
4649
scheme := runtime.NewScheme()
4750
_ = corev1.AddToScheme(scheme)
4851
_ = v1alpha1.AddToScheme(scheme)
4952
client := fakeClient.NewClientBuilder().WithScheme(scheme).WithObjects(mockObjects...).Build()
50-
return CNINodeMock{
53+
return &CNINodeMock{
5154
Reconciler: CNINodeReconciler{
5255
Client: client,
5356
scheme: scheme,
@@ -67,6 +70,8 @@ func TestCNINodeReconcile(t *testing.T) {
6770
mockResourceCleaner *mock_cleanup.MockResourceCleaner
6871
mockK8sApi *mock_k8s.MockK8sWrapper
6972
mockFinalizerManager *mock_k8s.MockFinalizerManager
73+
mockEC2API *mock_api.MockEC2APIHelper
74+
mockCNINode *CNINodeMock
7075
}
7176
tests := []struct {
7277
name string
@@ -89,21 +94,94 @@ func TestCNINodeReconcile(t *testing.T) {
8994
assert.NoError(t, err)
9095
assert.Equal(t, res, reconcile.Result{})
9196
assert.Equal(t, cniNode.Labels, map[string]string{config.NodeLabelOS: "linux"})
92-
assert.Equal(t, cniNode.Spec.Tags, map[string]string{config.CNINodeClusterNameKey: mockClusterName})
97+
assert.Equal(t, cniNode.Spec.Tags, map[string]string{config.VPCCNIClusterNameKey: mockClusterName})
98+
},
99+
},
100+
{
101+
name: "verify cleaner was not called if node id is not present given that node is being finalized",
102+
args: args{
103+
mockNode: nil,
104+
mockCNINode: &v1alpha1.CNINode{
105+
ObjectMeta: metav1.ObjectMeta{
106+
Name: mockName,
107+
Labels: map[string]string{
108+
config.NodeLabelOS: config.OSLinux,
109+
},
110+
Finalizers: []string{config.NodeTerminationFinalizer},
111+
DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time},
112+
},
113+
},
114+
},
115+
prepare: func(f *fields) {
116+
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner {
117+
return f.mockResourceCleaner
118+
}
119+
f.mockResourceCleaner.EXPECT().DeleteLeakedResources().Times(0)
120+
121+
f.mockFinalizerManager.EXPECT().
122+
RemoveFinalizers(gomock.Any(), gomock.Any(), config.NodeTerminationFinalizer).
123+
Return(nil)
124+
},
125+
asserts: func(res reconcile.Result, err error, cniNode *v1alpha1.CNINode) {
126+
assert.NoError(t, err)
127+
assert.Equal(t, res, reconcile.Result{})
128+
},
129+
},
130+
{
131+
name: "verify cleaner was called if node id is not empty when node is being finalized",
132+
args: args{
133+
mockNode: nil,
134+
mockCNINode: &v1alpha1.CNINode{
135+
ObjectMeta: metav1.ObjectMeta{
136+
Name: mockName,
137+
Labels: map[string]string{
138+
config.NodeLabelOS: config.OSLinux,
139+
},
140+
Finalizers: []string{config.NodeTerminationFinalizer},
141+
DeletionTimestamp: &metav1.Time{Time: metav1.Now().Time},
142+
},
143+
Spec: v1alpha1.CNINodeSpec{
144+
Tags: map[string]string{
145+
config.NetworkInterfaceNodeIDKey: "i-1234567890",
146+
},
147+
},
148+
},
149+
},
150+
prepare: func(f *fields) {
151+
f.mockCNINode.Reconciler.newResourceCleaner = func(nodeID string, eC2Wrapper ec2API.EC2Wrapper, vpcID string) cleanup.ResourceCleaner {
152+
assert.Equal(t, "i-1234567890", nodeID)
153+
return f.mockResourceCleaner
154+
}
155+
f.mockResourceCleaner.EXPECT().DeleteLeakedResources().Times(1).Return(nil)
156+
f.mockFinalizerManager.EXPECT().
157+
RemoveFinalizers(gomock.Any(), gomock.Any(), config.NodeTerminationFinalizer).
158+
Return(nil)
159+
160+
},
161+
asserts: func(res reconcile.Result, err error, cniNode *v1alpha1.CNINode) {
162+
assert.NoError(t, err)
163+
assert.Equal(t, res, reconcile.Result{})
93164
},
94165
},
95166
}
96167
for _, tt := range tests {
97168
t.Run(tt.name, func(t *testing.T) {
98169
ctrl := gomock.NewController(t)
99170
defer ctrl.Finish()
100-
101-
mock := NewCNINodeMock(ctrl, tt.args.mockNode, tt.args.mockCNINode)
171+
objs := []client.Object{tt.args.mockCNINode}
172+
if tt.args.mockNode != nil {
173+
objs = append(objs, tt.args.mockNode)
174+
}
175+
mock := NewCNINodeMock(ctrl, objs...)
102176
f := fields{
103177
mockResourceCleaner: mock_cleanup.NewMockResourceCleaner(ctrl),
104178
mockK8sApi: mock_k8s.NewMockK8sWrapper(ctrl),
105179
mockFinalizerManager: mock_k8s.NewMockFinalizerManager(ctrl),
180+
mockEC2API: mock_api.NewMockEC2APIHelper(ctrl),
181+
mockCNINode: mock,
106182
}
183+
mock.Reconciler.finalizerManager = f.mockFinalizerManager
184+
mock.Reconciler.k8sAPI = f.mockK8sApi
107185
if tt.prepare != nil {
108186
tt.prepare(&f)
109187
}
@@ -118,5 +196,4 @@ func TestCNINodeReconcile(t *testing.T) {
118196
}
119197
})
120198
}
121-
122199
}

‎main.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ import (
2929
crdcontroller "github.com/aws/amazon-vpc-resource-controller-k8s/controllers/crds"
3030
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/api"
3131
ec2API "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api"
32+
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
3233
eniCleaner "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/ec2/api/cleanup"
3334
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/condition"
3435
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
@@ -438,6 +439,7 @@ func main() {
438439
clusterName,
439440
vpcID,
440441
finalizerManager,
442+
cleanup.NewNodeResourceCleaner,
441443
).SetupWithManager(mgr, maxNodeConcurrentReconciles)); err != nil {
442444
setupLog.Error(err, "unable to create controller", "controller", "CNINode")
443445
os.Exit(1)

‎pkg/aws/ec2/api/cleanup/eni_cleanup.go‎

Lines changed: 40 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/config"
2424
rcHealthz "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/healthz"
2525
"github.com/aws/amazon-vpc-resource-controller-k8s/pkg/utils"
26-
"github.com/samber/lo"
2726

2827
ec2Errors "github.com/aws/amazon-vpc-resource-controller-k8s/pkg/aws/errors"
2928
"github.com/aws/aws-sdk-go-v2/aws"
@@ -38,6 +37,7 @@ import (
3837
// NetworkInterfaceManager interface allows to define the ENI filters and checks if ENI should be deleted for different callers like in the periodic cleanup routine or
3938
// during node termination
4039
type NetworkInterfaceManager interface {
40+
// If there are multiple filters then we will OR them.
4141
GetENITagFilters() []ec2types.Filter
4242
ShouldDeleteENI(eniID *string) bool
4343
UpdateAvailableENIsIfNeeded(eniMap *map[string]struct{})
@@ -126,18 +126,36 @@ func (e *ENICleaner) DeleteLeakedResources() error {
126126
}...)
127127

128128
// only apply extra filters when the controller is enabled which provides cninode resources
129+
var OrFilters []ec2types.Filter
130+
var err error
131+
var networkInterfaces []*ec2types.NetworkInterface
129132
if !e.ControllerDisabled {
130133
// get cleaner specific filters
131-
filters = append(filters, e.Manager.GetENITagFilters()...)
132-
}
133-
describeNetworkInterfaceIp := &ec2.DescribeNetworkInterfacesInput{
134-
Filters: filters,
135-
}
134+
OrFilters = e.Manager.GetENITagFilters()
135+
for _, OrFilter := range OrFilters {
136+
filterCopy := append([]ec2types.Filter{}, filters...)
137+
filterCopy = append(filterCopy, OrFilter)
138+
139+
describeNetworkInterfaceIp := &ec2.DescribeNetworkInterfacesInput{
140+
Filters: filterCopy,
141+
}
136142

137-
networkInterfaces, err := e.EC2Wrapper.DescribeNetworkInterfacesPagesWithRetry(describeNetworkInterfaceIp)
138-
if err != nil {
139-
e.Log.Error(err, "failed to describe network interfaces, cleanup will be retried in next cycle")
140-
return err
143+
tempNetworkInterfaces, err := e.EC2Wrapper.DescribeNetworkInterfacesPagesWithRetry(describeNetworkInterfaceIp)
144+
if err != nil {
145+
e.Log.Error(err, "failed to describe network interfaces, cleanup will be retried in next cycle")
146+
return err
147+
}
148+
networkInterfaces = append(networkInterfaces, tempNetworkInterfaces...)
149+
}
150+
} else {
151+
describeNetworkInterfaceIp := &ec2.DescribeNetworkInterfacesInput{
152+
Filters: filters,
153+
}
154+
networkInterfaces, err = e.EC2Wrapper.DescribeNetworkInterfacesPagesWithRetry(describeNetworkInterfaceIp)
155+
if err != nil {
156+
e.Log.Error(err, "failed to describe network interfaces, cleanup will be retried in next cycle")
157+
return err
158+
}
141159
}
142160

143161
for _, nwInterface := range networkInterfaces {
@@ -169,9 +187,12 @@ func (e *ENICleaner) DeleteLeakedResources() error {
169187
}
170188
continue
171189
}
172-
e.Log.Info("deleted leaked ENI successfully",
173-
"eniID", nwInterface.NetworkInterfaceId,
174-
"instanceID", lo.TernaryF(nwInterface.Attachment == nil, func() *string { return lo.ToPtr("") }, func() *string { return nwInterface.Attachment.InstanceId }))
190+
// It is possible for eni attachment to be nil, if it was never attached to instance
191+
instanceID := ""
192+
if nwInterface.Attachment != nil && nwInterface.Attachment.InstanceId != nil {
193+
instanceID = aws.ToString(nwInterface.Attachment.InstanceId)
194+
}
195+
e.Log.Info("deleted leaked ENI successfully", "eni id", *nwInterface.NetworkInterfaceId, "instance id", instanceID)
175196
} else {
176197
// Seeing the ENI for the first time, add it to the new list of available network interfaces
177198
availableENIs[*nwInterface.NetworkInterfaceId] = struct{}{}
@@ -184,11 +205,14 @@ func (e *ENICleaner) DeleteLeakedResources() error {
184205
}
185206

186207
func (e *ClusterENICleaner) GetENITagFilters() []ec2types.Filter {
187-
clusterNameTagKey := fmt.Sprintf(config.ClusterNameTagKeyFormat, e.ClusterName)
188208
return []ec2types.Filter{
189209
{
190-
Name: aws.String("tag:" + clusterNameTagKey),
191-
Values: []string{config.ClusterNameTagValue},
210+
Name: aws.String("tag:" + config.VPCCNIClusterNameKey),
211+
Values: []string{e.ClusterName},
212+
},
213+
{
214+
Name: aws.String("tag:" + fmt.Sprintf(config.VPCRCClusterNameTagKeyFormat, e.ClusterName)),
215+
Values: []string{config.VPCRCClusterNameTagValue},
192216
},
193217
}
194218
}

0 commit comments

Comments
 (0)