Skip to content

Commit 124d9be

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #8577 from jackfrancis/providerid-string-literal
🌱 use providerID string as-is
2 parents 3ed5f64 + f62294e commit 124d9be

File tree

13 files changed

+69
-94
lines changed

13 files changed

+69
-94
lines changed

‎api/v1beta1/index/machine.go‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,6 @@ import (
2626
"sigs.k8s.io/controller-runtime/pkg/client"
2727

2828
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
29-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
3029
)
3130

3231
const (
@@ -82,14 +81,11 @@ func machineByProviderID(o client.Object) []string {
8281
panic(fmt.Sprintf("Expected a Machine but got a %T", o))
8382
}
8483

85-
if pointer.StringDeref(machine.Spec.ProviderID, "") == "" {
86-
return nil
87-
}
84+
providerID := pointer.StringDeref(machine.Spec.ProviderID, "")
8885

89-
providerID, err := noderefutil.NewProviderID(*machine.Spec.ProviderID)
90-
if err != nil {
91-
// Failed to create providerID, skipping.
86+
if providerID == "" {
9287
return nil
9388
}
94-
return []string{providerID.IndexKey()}
89+
90+
return []string{providerID}
9591
}

‎api/v1beta1/index/machine_test.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ import (
2525
"sigs.k8s.io/controller-runtime/pkg/client"
2626

2727
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
28-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
2928
)
3029

3130
func TestIndexMachineByNodeName(t *testing.T) {
@@ -62,9 +61,7 @@ func TestIndexMachineByNodeName(t *testing.T) {
6261
}
6362

6463
func TestIndexMachineByProviderID(t *testing.T) {
65-
validProviderID, err := noderefutil.NewProviderID("aws://region/zone/id")
66-
g := NewWithT(t)
67-
g.Expect(err).ToNot(HaveOccurred())
64+
validProviderID := "aws://region/zone/id"
6865

6966
testCases := []struct {
7067
name string
@@ -80,7 +77,7 @@ func TestIndexMachineByProviderID(t *testing.T) {
8077
name: "Machine has invalid providerID",
8178
object: &clusterv1.Machine{
8279
Spec: clusterv1.MachineSpec{
83-
ProviderID: pointer.String("invalid"),
80+
ProviderID: pointer.String(""),
8481
},
8582
},
8683
expected: nil,
@@ -89,10 +86,10 @@ func TestIndexMachineByProviderID(t *testing.T) {
8986
name: "Machine has valid providerID",
9087
object: &clusterv1.Machine{
9188
Spec: clusterv1.MachineSpec{
92-
ProviderID: pointer.String(validProviderID.String()),
89+
ProviderID: pointer.String(validProviderID),
9390
},
9491
},
95-
expected: []string{validProviderID.IndexKey()},
92+
expected: []string{validProviderID},
9693
},
9794
}
9895

‎api/v1beta1/index/machinepool.go‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
ctrl "sigs.k8s.io/controller-runtime"
2525
"sigs.k8s.io/controller-runtime/pkg/client"
2626

27-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
2827
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
2928
)
3029

@@ -93,12 +92,11 @@ func machinePoolByProviderID(o client.Object) []string {
9392

9493
providerIDs := make([]string, 0, len(machinepool.Spec.ProviderIDList))
9594
for _, id := range machinepool.Spec.ProviderIDList {
96-
providerID, err := noderefutil.NewProviderID(id)
97-
if err != nil {
98-
// Failed to create providerID, skipping.
95+
if id == "" {
96+
// Valid providerID not found, skipping.
9997
continue
10098
}
101-
providerIDs = append(providerIDs, providerID.IndexKey())
99+
providerIDs = append(providerIDs, id)
102100
}
103101

104102
return providerIDs

‎api/v1beta1/index/machinepool_test.go‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
corev1 "k8s.io/api/core/v1"
2424
"sigs.k8s.io/controller-runtime/pkg/client"
2525

26-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
2726
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
2827
)
2928

@@ -66,11 +65,8 @@ func TestIndexMachinePoolByNodeName(t *testing.T) {
6665
}
6766

6867
func TestIndexMachinePoolByProviderID(t *testing.T) {
69-
g := NewWithT(t)
70-
validProviderID, err := noderefutil.NewProviderID("aws://region/zone/1")
71-
g.Expect(err).ToNot(HaveOccurred())
72-
otherValidProviderID, err := noderefutil.NewProviderID("aws://region/zone/2")
73-
g.Expect(err).ToNot(HaveOccurred())
68+
validProviderID := "aws://region/zone/1"
69+
otherValidProviderID := "aws://region/zone/2"
7470

7571
testCases := []struct {
7672
name string
@@ -86,7 +82,7 @@ func TestIndexMachinePoolByProviderID(t *testing.T) {
8682
name: "MachinePool has invalid providerID",
8783
object: &expv1.MachinePool{
8884
Spec: expv1.MachinePoolSpec{
89-
ProviderIDList: []string{"invalid"},
85+
ProviderIDList: []string{""},
9086
},
9187
},
9288
expected: []string{},
@@ -95,10 +91,10 @@ func TestIndexMachinePoolByProviderID(t *testing.T) {
9591
name: "MachinePool has valid providerIDs",
9692
object: &expv1.MachinePool{
9793
Spec: expv1.MachinePoolSpec{
98-
ProviderIDList: []string{validProviderID.String(), otherValidProviderID.String()},
94+
ProviderIDList: []string{validProviderID, otherValidProviderID},
9995
},
10096
},
101-
expected: []string{validProviderID.IndexKey(), otherValidProviderID.IndexKey()},
97+
expected: []string{validProviderID, otherValidProviderID},
10298
},
10399
}
104100

‎api/v1beta1/index/node.go‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,8 +21,6 @@ import (
2121

2222
corev1 "k8s.io/api/core/v1"
2323
"sigs.k8s.io/controller-runtime/pkg/client"
24-
25-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
2624
)
2725

2826
const (
@@ -42,10 +40,5 @@ func NodeByProviderID(o client.Object) []string {
4240
return nil
4341
}
4442

45-
providerID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
46-
if err != nil {
47-
// Failed to create providerID, skipping.
48-
return nil
49-
}
50-
return []string{providerID.IndexKey()}
43+
return []string{node.Spec.ProviderID}
5144
}

‎api/v1beta1/index/node_test.go‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,10 @@ import (
2222
. "github.com/onsi/gomega"
2323
corev1 "k8s.io/api/core/v1"
2424
"sigs.k8s.io/controller-runtime/pkg/client"
25-
26-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
2725
)
2826

2927
func TestIndexNodeByProviderID(t *testing.T) {
30-
validProviderID, err := noderefutil.NewProviderID("aws://region/zone/id")
31-
g := NewWithT(t)
32-
g.Expect(err).ToNot(HaveOccurred())
28+
validProviderID := "aws://region/zone/id"
3329

3430
testCases := []struct {
3531
name string
@@ -45,7 +41,7 @@ func TestIndexNodeByProviderID(t *testing.T) {
4541
name: "Node has invalid providerID",
4642
object: &corev1.Node{
4743
Spec: corev1.NodeSpec{
48-
ProviderID: "invalid",
44+
ProviderID: "",
4945
},
5046
},
5147
expected: nil,
@@ -54,10 +50,10 @@ func TestIndexNodeByProviderID(t *testing.T) {
5450
name: "Node has valid providerID",
5551
object: &corev1.Node{
5652
Spec: corev1.NodeSpec{
57-
ProviderID: validProviderID.String(),
53+
ProviderID: validProviderID,
5854
},
5955
},
60-
expected: []string{validProviderID.IndexKey()},
56+
expected: []string{validProviderID},
6157
},
6258
}
6359

‎controllers/noderefutil/providerid.go‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ limitations under the License.
1515
*/
1616

1717
// Package noderefutil implements NodeRef utils.
18+
// The ProviderID type is deprecated and unused by Cluster API internally.
19+
// It will be removed entirely in a future release.
1820
package noderefutil
1921

2022
import (
@@ -25,14 +27,20 @@ import (
2527

2628
var (
2729
// ErrEmptyProviderID means that the provider id is empty.
30+
//
31+
// Deprecated: This var is going to be removed in a future release.
2832
ErrEmptyProviderID = errors.New("providerID is empty")
2933

3034
// ErrInvalidProviderID means that the provider id has an invalid form.
35+
//
36+
// Deprecated: This var is going to be removed in a future release.
3137
ErrInvalidProviderID = errors.New("providerID must be of the form <cloudProvider>://<optional>/<segments>/<provider id>")
3238
)
3339

3440
// ProviderID is a struct representation of a Kubernetes ProviderID.
3541
// Format: cloudProvider://optional/segments/etc/id
42+
//
43+
// Deprecated: This struct is going to be removed in a future release.
3644
type ProviderID struct {
3745
original string
3846
cloudProvider string
@@ -48,6 +56,8 @@ type ProviderID struct {
4856
var providerIDRegex = regexp.MustCompile("^[^:]+://.*[^/]$")
4957

5058
// NewProviderID parses the input string and returns a new ProviderID.
59+
//
60+
// Deprecated: This constructor is going to be removed in a future release.
5161
func NewProviderID(id string) (*ProviderID, error) {
5262
if id == "" {
5363
return nil, ErrEmptyProviderID
@@ -77,32 +87,44 @@ func NewProviderID(id string) (*ProviderID, error) {
7787
}
7888

7989
// CloudProvider returns the cloud provider portion of the ProviderID.
90+
//
91+
// Deprecated: This method is going to be removed in a future release.
8092
func (p *ProviderID) CloudProvider() string {
8193
return p.cloudProvider
8294
}
8395

8496
// ID returns the identifier portion of the ProviderID.
97+
//
98+
// Deprecated: This method is going to be removed in a future release.
8599
func (p *ProviderID) ID() string {
86100
return p.id
87101
}
88102

89103
// Equals returns true if this ProviderID string matches another ProviderID string.
104+
//
105+
// Deprecated: This method is going to be removed in a future release.
90106
func (p *ProviderID) Equals(o *ProviderID) bool {
91107
return p.String() == o.String()
92108
}
93109

94110
// String returns the string representation of this object.
111+
//
112+
// Deprecated: This method is going to be removed in a future release.
95113
func (p ProviderID) String() string {
96114
return p.original
97115
}
98116

99117
// Validate returns true if the provider id is valid.
118+
//
119+
// Deprecated: This method is going to be removed in a future release.
100120
func (p *ProviderID) Validate() bool {
101121
return p.CloudProvider() != "" && p.ID() != ""
102122
}
103123

104124
// IndexKey returns the required level of uniqueness
105125
// to represent and index machines uniquely from their node providerID.
126+
//
127+
// Deprecated: This method is going to be removed in a future release.
106128
func (p *ProviderID) IndexKey() string {
107129
return p.String()
108130
}

‎controllers/noderefutil/providerid_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ See the License for the specific language governing permissions and
1414
limitations under the License.
1515
*/
1616

17+
// The ProviderID type is deprecated and unused by Cluster API internally.
18+
// It will be removed entirely in a future release.
1719
package noderefutil
1820

1921
import (

‎exp/internal/controllers/machinepool_controller.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,6 @@ import (
4141
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
4242
"sigs.k8s.io/cluster-api/api/v1beta1/index"
4343
"sigs.k8s.io/cluster-api/controllers/external"
44-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
4544
"sigs.k8s.io/cluster-api/controllers/remote"
4645
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
4746
"sigs.k8s.io/cluster-api/util"
@@ -352,15 +351,14 @@ func (r *MachinePoolReconciler) nodeToMachinePool(o client.Object) []reconcile.R
352351

353352
// Otherwise let's match by providerID. This is useful when e.g the NodeRef has not been set yet.
354353
// Match by providerID
355-
nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
356-
if err != nil {
354+
if node.Spec.ProviderID == "" {
357355
return nil
358356
}
359357
machinePoolList = &expv1.MachinePoolList{}
360358
if err := r.Client.List(
361359
context.TODO(),
362360
machinePoolList,
363-
append(filters, client.MatchingFields{index.MachinePoolProviderIDField: nodeProviderID.IndexKey()})...); err != nil {
361+
append(filters, client.MatchingFields{index.MachinePoolProviderIDField: node.Spec.ProviderID})...); err != nil {
364362
return nil
365363
}
366364

‎exp/internal/controllers/machinepool_controller_noderef.go‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import (
2727
"sigs.k8s.io/controller-runtime/pkg/client"
2828

2929
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
30-
"sigs.k8s.io/cluster-api/controllers/noderefutil"
3130
"sigs.k8s.io/cluster-api/controllers/remote"
3231
expv1 "sigs.k8s.io/cluster-api/exp/api/v1beta1"
3332
"sigs.k8s.io/cluster-api/internal/util/taints"
@@ -132,21 +131,19 @@ func (r *MachinePoolReconciler) deleteRetiredNodes(ctx context.Context, c client
132131
continue
133132
}
134133

135-
nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
136-
if err != nil {
137-
log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", node.Spec.ProviderID)
134+
if node.Spec.ProviderID == "" {
135+
log.V(2).Info("No ProviderID detected, skipping", "providerID", node.Spec.ProviderID)
138136
continue
139137
}
140138

141-
nodeRefsMap[nodeProviderID.String()] = node
139+
nodeRefsMap[node.Spec.ProviderID] = node
142140
}
143141
for _, providerID := range providerIDList {
144-
pid, err := noderefutil.NewProviderID(providerID)
145-
if err != nil {
146-
log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", providerID)
142+
if providerID == "" {
143+
log.V(2).Info("No ProviderID detected, skipping", "providerID", providerID)
147144
continue
148145
}
149-
delete(nodeRefsMap, pid.String())
146+
delete(nodeRefsMap, providerID)
150147
}
151148
for _, node := range nodeRefsMap {
152149
if err := c.Delete(ctx, node); err != nil {
@@ -168,13 +165,12 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.
168165
}
169166

170167
for _, node := range nodeList.Items {
171-
nodeProviderID, err := noderefutil.NewProviderID(node.Spec.ProviderID)
172-
if err != nil {
173-
log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", node.Spec.ProviderID)
168+
if node.Spec.ProviderID == "" {
169+
log.V(2).Info("No ProviderID detected, skipping", "providerID", node.Spec.ProviderID)
174170
continue
175171
}
176172

177-
nodeRefsMap[nodeProviderID.String()] = node
173+
nodeRefsMap[node.Spec.ProviderID] = node
178174
}
179175

180176
if nodeList.Continue == "" {
@@ -184,12 +180,11 @@ func (r *MachinePoolReconciler) getNodeReferences(ctx context.Context, c client.
184180

185181
var nodeRefs []corev1.ObjectReference
186182
for _, providerID := range providerIDList {
187-
pid, err := noderefutil.NewProviderID(providerID)
188-
if err != nil {
189-
log.V(2).Info("Failed to parse ProviderID, skipping", "err", err, "providerID", providerID)
183+
if providerID == "" {
184+
log.V(2).Info("No ProviderID detected, skipping", "providerID", providerID)
190185
continue
191186
}
192-
if node, ok := nodeRefsMap[pid.String()]; ok {
187+
if node, ok := nodeRefsMap[providerID]; ok {
193188
available++
194189
if nodeIsReady(&node) {
195190
ready++

0 commit comments

Comments
 (0)