Skip to content

Commit 0ba1ee6

Browse files
ValyaBValyaB
committed
refactor
1 parent 810d81e commit 0ba1ee6

File tree

4 files changed

+84
-29
lines changed

4 files changed

+84
-29
lines changed

‎internal/actions/check_node_status.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,14 @@ import (
77
"reflect"
88
"time"
99

10+
"github.com/samber/lo"
1011
"github.com/sirupsen/logrus"
1112
corev1 "k8s.io/api/core/v1"
1213
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1314
"k8s.io/client-go/kubernetes"
1415

1516
"github.com/castai/cluster-controller/internal/castai"
1617
"github.com/castai/cluster-controller/internal/waitext"
17-
"github.com/samber/lo"
1818
)
1919

2020
var _ ActionHandler = &CheckNodeStatusHandler{}

‎internal/actions/check_node_status_test.go‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,18 @@ import (
77
"time"
88

99
"github.com/google/uuid"
10+
"github.com/samber/lo"
1011
"github.com/sirupsen/logrus"
1112
"github.com/stretchr/testify/require"
1213
v1 "k8s.io/api/core/v1"
1314
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
15+
"k8s.io/apimachinery/pkg/runtime"
16+
"k8s.io/apimachinery/pkg/types"
1417
"k8s.io/apimachinery/pkg/watch"
1518
"k8s.io/client-go/kubernetes/fake"
1619
k8stest "k8s.io/client-go/testing"
1720

1821
"github.com/castai/cluster-controller/internal/castai"
19-
"github.com/samber/lo"
20-
"k8s.io/apimachinery/pkg/runtime"
21-
"k8s.io/apimachinery/pkg/types"
2222
)
2323

2424
func TestCheckNodeStatusHandler_Handle_Deleted(t *testing.T) {
@@ -426,5 +426,4 @@ func TestCheckStatus_Ready(t *testing.T) {
426426

427427
r.NoError(err)
428428
})
429-
430429
}

‎internal/actions/kubernetes_helpers.go‎

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -116,30 +116,35 @@ func isNodeIDProviderIDValid(node *v1.Node, nodeID, providerID string, log logru
116116
if nodeID == "" && providerID == "" {
117117
return fmt.Errorf("node and provider IDs are empty %w", errAction)
118118
}
119-
119+
emptyProviderID := providerID == "" || node.Spec.ProviderID == ""
120120
validProviderID := false
121121

122-
// validate provider id only if non-empty in request
123-
if providerID == "" || node.Spec.ProviderID == providerID {
122+
// validate provider id only if non-empty in request and in Node spec
123+
// Azure provider: provider id can be empty even if node is Ready
124+
if !emptyProviderID && node.Spec.ProviderID == providerID {
124125
validProviderID = true
125-
} else {
126+
} else if !emptyProviderID && node.Spec.ProviderID != providerID {
126127
log.Errorf("node %v has provider ID %s, but requested provider ID is %s", node.Name, node.Spec.ProviderID, providerID)
127128
}
128129

129-
if nodeID == "" && validProviderID {
130-
return nil
131-
}
132-
133-
var currentNodeID string
134-
if nodeID != "" {
135-
if currentNodeID, ok := node.Labels[castai.LabelNodeID]; ok {
136-
if currentNodeID == nodeID && validProviderID {
130+
currentNodeID, ok := node.Labels[castai.LabelNodeID]
131+
if ok && currentNodeID != "" {
132+
if currentNodeID == nodeID {
133+
if validProviderID {
134+
return nil
135+
}
136+
if emptyProviderID {
137137
return nil
138138
}
139139
}
140140
}
141+
if (!ok || currentNodeID == "") && validProviderID {
142+
// if node ID is not set in labels, but provider ID is valid, we can still proceed
143+
return nil
144+
}
141145

142-
return fmt.Errorf("node %v has ID %s and provider ID %s: %w", node.Name, currentNodeID, node.Spec.ProviderID, errNodeDoesNotMatch)
146+
return fmt.Errorf("node %v has ID %s and provider ID %s: %w",
147+
node.Name, currentNodeID, node.Spec.ProviderID, errNodeDoesNotMatch)
143148
}
144149

145150
// executeBatchPodActions executes the action for each pod in the list.

‎internal/actions/kubernetes_helpers_test.go‎

Lines changed: 62 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,25 @@
11
package actions
22

33
import (
4+
"context"
5+
"fmt"
6+
"reflect"
47
"testing"
58

9+
"github.com/golang/mock/gomock"
10+
"github.com/sirupsen/logrus"
611
"github.com/stretchr/testify/require"
712
v1 "k8s.io/api/core/v1"
13+
k8serrors "k8s.io/apimachinery/pkg/api/errors"
814
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
915

10-
"context"
11-
"fmt"
1216
mock_actions "github.com/castai/cluster-controller/internal/actions/mock"
1317
"github.com/castai/cluster-controller/internal/castai"
14-
"github.com/golang/mock/gomock"
15-
"github.com/sirupsen/logrus"
16-
k8serrors "k8s.io/apimachinery/pkg/api/errors"
17-
"reflect"
1818
)
1919

2020
func Test_isNodeIDProviderIDValid(t *testing.T) {
21+
t.Parallel()
22+
2123
type args struct {
2224
node *v1.Node
2325
nodeID string
@@ -29,7 +31,7 @@ func Test_isNodeIDProviderIDValid(t *testing.T) {
2931
wantErr error
3032
}{
3133
{
32-
name: "empty node ID and provider id",
34+
name: "empty node ID and provider id in request",
3335
args: args{
3436
node: &v1.Node{},
3537
providerID: "",
@@ -38,12 +40,29 @@ func Test_isNodeIDProviderIDValid(t *testing.T) {
3840
wantErr: errAction,
3941
},
4042
{
41-
name: "node ID is empty but provider ID matches",
43+
name: "request node ID is empty but node id exists in node labels",
4244
args: args{
4345
node: &v1.Node{
4446
ObjectMeta: metav1.ObjectMeta{
4547
Labels: map[string]string{
46-
castai.LabelNodeID: "node-id-123-not-matching",
48+
castai.LabelNodeID: "node-id-123-existed",
49+
},
50+
},
51+
Spec: v1.NodeSpec{
52+
ProviderID: "provider-id-456",
53+
},
54+
},
55+
providerID: "provider-id-456",
56+
},
57+
wantErr: errNodeDoesNotMatch,
58+
},
59+
{
60+
name: "request and labels node ID are empty but provider ID matches",
61+
args: args{
62+
node: &v1.Node{
63+
ObjectMeta: metav1.ObjectMeta{
64+
Labels: map[string]string{
65+
castai.LabelNodeID: "",
4766
},
4867
},
4968
Spec: v1.NodeSpec{
@@ -53,6 +72,20 @@ func Test_isNodeIDProviderIDValid(t *testing.T) {
5372
providerID: "provider-id-456",
5473
},
5574
},
75+
{
76+
name: "request node ID is empty and no labels but provider ID matches",
77+
args: args{
78+
node: &v1.Node{
79+
ObjectMeta: metav1.ObjectMeta{
80+
Labels: map[string]string{},
81+
},
82+
Spec: v1.NodeSpec{
83+
ProviderID: "provider-id-456",
84+
},
85+
},
86+
providerID: "provider-id-456",
87+
},
88+
},
5689
{
5790
name: "node ID and provider ID matches",
5891
args: args{
@@ -88,7 +121,6 @@ func Test_isNodeIDProviderIDValid(t *testing.T) {
88121
},
89122
wantErr: errNodeDoesNotMatch,
90123
},
91-
92124
{
93125
name: "node ID does not match label, provider ID empty",
94126
args: args{
@@ -126,7 +158,7 @@ func Test_isNodeIDProviderIDValid(t *testing.T) {
126158
wantErr: errNodeDoesNotMatch,
127159
},
128160
{
129-
name: "node ID is match and provider ID is empty",
161+
name: "node ID is match and request provider ID is empty",
130162
args: args{
131163
node: &v1.Node{
132164
ObjectMeta: metav1.ObjectMeta{
@@ -142,9 +174,28 @@ func Test_isNodeIDProviderIDValid(t *testing.T) {
142174
providerID: "",
143175
},
144176
},
177+
{
178+
name: "node ID is match and provider ID is empty in Node spec",
179+
args: args{
180+
node: &v1.Node{
181+
ObjectMeta: metav1.ObjectMeta{
182+
Labels: map[string]string{
183+
castai.LabelNodeID: "node-id-123",
184+
},
185+
},
186+
Spec: v1.NodeSpec{
187+
ProviderID: "",
188+
},
189+
},
190+
nodeID: "node-id-123",
191+
providerID: "provider-id-456",
192+
},
193+
},
145194
}
146195
for _, tt := range tests {
196+
tt := tt // capture range variable
147197
t.Run(tt.name, func(t *testing.T) {
198+
t.Parallel()
148199
got := isNodeIDProviderIDValid(tt.args.node, tt.args.nodeID, tt.args.providerID, logrus.New())
149200
require.Equal(t, tt.wantErr != nil, got != nil, "error mismatch", got)
150201
require.ErrorIs(t, got, tt.wantErr)

0 commit comments

Comments
 (0)