Skip to content

Commit 9719e4f

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #4521 from enxebre/get-node-index
🌱 Add providerID index to get nodes
2 parents 604a465 + 3c9e61a commit 9719e4f

11 files changed

+273
-102
lines changed

‎controllers/machine_controller_noderef.go

+31-20
Original file line numberDiff line numberDiff line change
@@ -20,16 +20,15 @@ import (
2020
"context"
2121
"fmt"
2222

23-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24-
"sigs.k8s.io/cluster-api/util/annotations"
25-
"sigs.k8s.io/cluster-api/util/patch"
26-
2723
"github.com/pkg/errors"
2824
corev1 "k8s.io/api/core/v1"
25+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2926
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
3027
"sigs.k8s.io/cluster-api/controllers/noderefutil"
3128
"sigs.k8s.io/cluster-api/util"
29+
"sigs.k8s.io/cluster-api/util/annotations"
3230
"sigs.k8s.io/cluster-api/util/conditions"
31+
"sigs.k8s.io/cluster-api/util/patch"
3332
ctrl "sigs.k8s.io/controller-runtime"
3433
"sigs.k8s.io/controller-runtime/pkg/client"
3534
)
@@ -170,29 +169,41 @@ func summarizeNodeConditions(node *corev1.Node) (corev1.ConditionStatus, string)
170169

171170
func (r *MachineReconciler) getNode(ctx context.Context, c client.Reader, providerID *noderefutil.ProviderID) (*corev1.Node, error) {
172171
log := ctrl.LoggerFrom(ctx, "providerID", providerID)
173-
174172
nodeList := corev1.NodeList{}
175-
for {
176-
if err := c.List(ctx, &nodeList, client.Continue(nodeList.Continue)); err != nil {
177-
return nil, err
178-
}
173+
if err := c.List(ctx, &nodeList, client.MatchingFields{noderefutil.NodeProviderIDIndex: providerID.IndexKey()}); err != nil {
174+
return nil, err
175+
}
176+
if len(nodeList.Items) == 0 {
177+
// If for whatever reason the index isn't registered or available, we fallback to loop over the whole list.
178+
nl := corev1.NodeList{}
179+
for {
180+
if err := c.List(ctx, &nl, client.Continue(nl.Continue)); err != nil {
181+
return nil, err
182+
}
183+
184+
for key, node := range nl.Items {
185+
nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
186+
if err != nil {
187+
log.Error(err, "Failed to parse ProviderID", "node", client.ObjectKeyFromObject(&nl.Items[key]).String())
188+
continue
189+
}
179190

180-
for _, node := range nodeList.Items {
181-
nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
182-
if err != nil {
183-
log.Error(err, "Failed to parse ProviderID", "node", node.Name)
184-
continue
191+
if providerID.Equals(nodeProviderID) {
192+
return &node, nil
193+
}
185194
}
186195

187-
if providerID.Equals(nodeProviderID) {
188-
return &node, nil
196+
if nl.Continue == "" {
197+
break
189198
}
190199
}
191200

192-
if nodeList.Continue == "" {
193-
break
194-
}
201+
return nil, ErrNodeNotFound
202+
}
203+
204+
if len(nodeList.Items) != 1 {
205+
return nil, fmt.Errorf("unexpectedly found more than one Node matching the providerID %s", providerID.String())
195206
}
196207

197-
return nil, ErrNodeNotFound
208+
return &nodeList.Items[0], nil
198209
}

‎controllers/machine_controller_noderef_test.go

+117-67
Original file line numberDiff line numberDiff line change
@@ -20,101 +20,151 @@ import (
2020
"testing"
2121

2222
. "github.com/onsi/gomega"
23-
2423
corev1 "k8s.io/api/core/v1"
2524
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
26-
"k8s.io/client-go/tools/record"
25+
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
2726
"sigs.k8s.io/cluster-api/controllers/noderefutil"
27+
"sigs.k8s.io/cluster-api/controllers/remote"
28+
"sigs.k8s.io/cluster-api/util"
29+
ctrl "sigs.k8s.io/controller-runtime"
2830
"sigs.k8s.io/controller-runtime/pkg/client"
29-
"sigs.k8s.io/controller-runtime/pkg/client/fake"
31+
"sigs.k8s.io/controller-runtime/pkg/handler"
32+
"sigs.k8s.io/controller-runtime/pkg/reconcile"
3033
)
3134

32-
func TestGetNodeReference(t *testing.T) {
35+
func TestGetNode(t *testing.T) {
3336
g := NewWithT(t)
3437

35-
r := &MachineReconciler{
36-
Client: fake.NewClientBuilder().Build(),
37-
recorder: record.NewFakeRecorder(32),
38-
}
38+
ns, err := env.CreateNamespace(ctx, "test-get-node")
39+
g.Expect(err).ToNot(HaveOccurred())
3940

40-
nodeList := []client.Object{
41-
&corev1.Node{
42-
ObjectMeta: metav1.ObjectMeta{
43-
Name: "node-1",
44-
},
45-
Spec: corev1.NodeSpec{
46-
ProviderID: "aws://us-east-1/id-node-1",
47-
},
48-
},
49-
&corev1.Node{
50-
ObjectMeta: metav1.ObjectMeta{
51-
Name: "node-2",
52-
},
53-
Spec: corev1.NodeSpec{
54-
ProviderID: "aws://us-west-2/id-node-2",
55-
},
56-
},
57-
&corev1.Node{
58-
ObjectMeta: metav1.ObjectMeta{
59-
Name: "gce-node-2",
60-
},
61-
Spec: corev1.NodeSpec{
62-
ProviderID: "gce://us-central1/id-node-2",
63-
},
41+
// Set up cluster to test against.
42+
testCluster := &clusterv1.Cluster{
43+
ObjectMeta: metav1.ObjectMeta{
44+
GenerateName: "test-get-node-",
45+
Namespace: ns.Name,
6446
},
6547
}
6648

67-
client := fake.NewClientBuilder().WithObjects(nodeList...).Build()
49+
g.Expect(env.Create(ctx, testCluster)).To(BeNil())
50+
g.Expect(env.CreateKubeconfigSecret(ctx, testCluster)).To(Succeed())
51+
defer func(do ...client.Object) {
52+
g.Expect(env.Cleanup(ctx, do...)).To(Succeed())
53+
}(ns, testCluster)
6854

6955
testCases := []struct {
70-
name string
71-
providerID string
72-
expected *corev1.ObjectReference
73-
err error
56+
name string
57+
node *corev1.Node
58+
providerIDInput string
59+
error error
7460
}{
7561
{
76-
name: "valid provider id, valid aws node",
77-
providerID: "aws:///id-node-1",
78-
expected: &corev1.ObjectReference{Name: "node-1"},
62+
name: "full providerID matches",
63+
node: &corev1.Node{
64+
ObjectMeta: metav1.ObjectMeta{
65+
Name: "test-get-node-node-1",
66+
},
67+
Spec: corev1.NodeSpec{
68+
ProviderID: "aws://us-east-1/test-get-node-1",
69+
},
70+
},
71+
providerIDInput: "aws://us-east-1/test-get-node-1",
7972
},
8073
{
81-
name: "valid provider id, valid aws node",
82-
providerID: "aws:///id-node-2",
83-
expected: &corev1.ObjectReference{Name: "node-2"},
74+
name: "aws prefix: cloudProvider and ID matches",
75+
node: &corev1.Node{
76+
ObjectMeta: metav1.ObjectMeta{
77+
Name: "test-get-node-node-2",
78+
},
79+
Spec: corev1.NodeSpec{
80+
ProviderID: "aws://us-west-2/test-get-node-2",
81+
},
82+
},
83+
providerIDInput: "aws:///test-get-node-2",
8484
},
8585
{
86-
name: "valid provider id, valid gce node",
87-
providerID: "gce:///id-node-2",
88-
expected: &corev1.ObjectReference{Name: "gce-node-2"},
86+
name: "gce prefix, cloudProvider and ID matches",
87+
node: &corev1.Node{
88+
ObjectMeta: metav1.ObjectMeta{
89+
Name: "test-get-node-gce-node-2",
90+
},
91+
Spec: corev1.NodeSpec{
92+
ProviderID: "gce://us-central1/test-get-node-2",
93+
},
94+
},
95+
providerIDInput: "gce:///test-get-node-2",
8996
},
9097
{
91-
name: "valid provider id, no node found",
92-
providerID: "aws:///id-node-100",
93-
expected: nil,
94-
err: ErrNodeNotFound,
98+
name: "Node is not found",
99+
node: &corev1.Node{
100+
ObjectMeta: metav1.ObjectMeta{
101+
Name: "test-get-node-not-found",
102+
},
103+
Spec: corev1.NodeSpec{
104+
ProviderID: "gce://us-central1/anything",
105+
},
106+
},
107+
providerIDInput: "gce://not-found",
108+
error: ErrNodeNotFound,
95109
},
96110
}
97111

98-
for _, test := range testCases {
99-
t.Run(test.name, func(t *testing.T) {
100-
gt := NewWithT(t)
101-
providerID, err := noderefutil.NewProviderID(test.providerID)
102-
gt.Expect(err).NotTo(HaveOccurred(), "Expected no error parsing provider id %q, got %v", test.providerID, err)
103-
104-
node, err := r.getNode(ctx, client, providerID)
105-
if test.err == nil {
106-
g.Expect(err).To(BeNil())
107-
} else {
108-
gt.Expect(err).NotTo(BeNil())
109-
gt.Expect(err).To(Equal(test.err), "Expected error %v, got %v", test.err, err)
110-
}
112+
nodesToCleanup := make([]client.Object, 0, len(testCases))
113+
for _, tc := range testCases {
114+
g.Expect(env.Create(ctx, tc.node)).To(BeNil())
115+
nodesToCleanup = append(nodesToCleanup, tc.node)
116+
}
117+
defer func(do ...client.Object) {
118+
g.Expect(env.Cleanup(ctx, do...)).To(Succeed())
119+
}(nodesToCleanup...)
120+
121+
tracker, err := remote.NewClusterCacheTracker(
122+
env.Manager, remote.ClusterCacheTrackerOptions{
123+
Indexes: []remote.Index{
124+
{
125+
Object: &corev1.Node{},
126+
Field: noderefutil.NodeProviderIDIndex,
127+
ExtractValue: noderefutil.IndexNodeByProviderID,
128+
},
129+
},
130+
},
131+
)
132+
g.Expect(err).ToNot(HaveOccurred())
133+
134+
r := &MachineReconciler{
135+
Tracker: tracker,
136+
Client: env,
137+
}
138+
139+
w, err := ctrl.NewControllerManagedBy(env.Manager).For(&corev1.Node{}).Build(r)
140+
g.Expect(err).ToNot(HaveOccurred())
141+
142+
g.Expect(tracker.Watch(ctx, remote.WatchInput{
143+
Name: "TestGetNode",
144+
Cluster: util.ObjectKey(testCluster),
145+
Watcher: w,
146+
Kind: &corev1.Node{},
147+
EventHandler: handler.EnqueueRequestsFromMapFunc(func(client.Object) []reconcile.Request {
148+
return nil
149+
}),
150+
})).To(Succeed())
151+
152+
for _, tc := range testCases {
153+
t.Run(tc.name, func(t *testing.T) {
154+
g := NewWithT(t)
155+
remoteClient, err := r.Tracker.GetClient(ctx, util.ObjectKey(testCluster))
156+
g.Expect(err).ToNot(HaveOccurred())
111157

112-
if test.expected == nil && node == nil {
158+
providerID, err := noderefutil.NewProviderID(tc.providerIDInput)
159+
g.Expect(err).ToNot(HaveOccurred())
160+
161+
node, err := r.getNode(ctx, remoteClient, providerID)
162+
if tc.error != nil {
163+
g.Expect(err).To(Equal(tc.error))
113164
return
114165
}
115-
116-
gt.Expect(node.Name).To(Equal(test.expected.Name), "Expected NodeRef's name to be %v, got %v", node.Name, test.expected.Name)
117-
gt.Expect(node.Namespace).To(Equal(test.expected.Namespace), "Expected NodeRef's namespace to be %v, got %v", node.Namespace, test.expected.Namespace)
166+
g.Expect(err).ToNot(HaveOccurred())
167+
g.Expect(node.Name).To(Equal(tc.node.Name))
118168
})
119169
}
120170
}

‎controllers/noderefutil/indexer.go

+26
Original file line numberDiff line numberDiff line change
@@ -22,11 +22,18 @@ import (
2222
"fmt"
2323

2424
"github.com/pkg/errors"
25+
corev1 "k8s.io/api/core/v1"
2526
clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4"
2627
ctrl "sigs.k8s.io/controller-runtime"
2728
"sigs.k8s.io/controller-runtime/pkg/client"
2829
)
2930

31+
const (
32+
// NodeProviderIDIndex is used to index Nodes by ProviderID. Remote caches use this to find Nodes in a workload cluster
33+
// out of management cluster machine.spec.providerID.
34+
NodeProviderIDIndex = "spec.providerID"
35+
)
36+
3037
// AddMachineNodeIndex adds the machine node name index to the
3138
// managers cache.
3239
func AddMachineNodeIndex(ctx context.Context, mgr ctrl.Manager) error {
@@ -50,3 +57,22 @@ func indexMachineByNodeName(o client.Object) []string {
5057
}
5158
return nil
5259
}
60+
61+
// IndexNodeByProviderID contains the logic to index Nodes by ProviderID.
62+
func IndexNodeByProviderID(o client.Object) []string {
63+
node, ok := o.(*corev1.Node)
64+
if !ok {
65+
panic(fmt.Sprintf("Expected a Node but got a %T", o))
66+
}
67+
68+
if node.Spec.ProviderID != "" {
69+
providerID, err := NewProviderID(node.Spec.ProviderID)
70+
if err != nil {
71+
// Failed to create providerID, skipping.
72+
return nil
73+
}
74+
return []string{providerID.IndexKey()}
75+
}
76+
77+
return nil
78+
}

‎controllers/noderefutil/providerid.go

+9
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package noderefutil
1919

2020
import (
2121
"errors"
22+
"fmt"
2223
"regexp"
2324
"strings"
2425
)
@@ -100,3 +101,11 @@ func (p *ProviderID) String() string {
100101
func (p *ProviderID) Validate() bool {
101102
return p.CloudProvider() != "" && p.ID() != ""
102103
}
104+
105+
// IndexKey returns a string concatenating the cloudProvider and the ID parts of the providerID.
106+
// E.g Format: cloudProvider://optional/segments/etc/id. IndexKey: cloudProvider/id
107+
// This is useful to use the providerID as a reliable index between nodes and machines
108+
// as it guarantees the infra Providers contract.
109+
func (p *ProviderID) IndexKey() string {
110+
return fmt.Sprintf("%s/%s", p.CloudProvider(), p.ID())
111+
}

0 commit comments

Comments
 (0)