[CCM] Replace stackit:///<server-id in favor of stackit://<server-id> (#132)

* Strictly follow providerID spec

Signed-off-by: Niclas Schad <[email protected]>

* clean up some todos

Signed-off-by: Niclas Schad <[email protected]>

---------

Signed-off-by: Niclas Schad <[email protected]>

Author: nschad

pkg/ccm/instances.go

@@ -38,10 +38,10 @@ const (
 	instanceStopping = "STOPPING"
 )
 
-// If Instances.InstanceID or cloudprovider.GetInstanceProviderID is changed, the regexp should be changed too.
-var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]*)/([^/]+)$`)
+// If makeInstanceID is changed, the regexp should be changed too.
+var providerIDRegexp = regexp.MustCompile(`^` + ProviderName + `://([^/]+)$`)
 
-// TODO: remove old provider after migration
+// TODO(migration): remove old provider support after migration
 var oldProviderIDRegexp = regexp.MustCompile(`^` + oldProviderName + `://([^/]*)/([^/]+)$`)
 
 // Instances encapsulates an implementation of Instances for OpenStack.
@@ -114,7 +114,6 @@ func (i *Instances) InstanceMetadata(ctx context.Context, node *corev1.Node) (*c
 				})
 		}
 
-		// TODO: where to find IPv6SupportDisabled
 		if ipv6, ok := nic.GetIpv6Ok(); ok {
 			addToNodeAddresses(&addresses,
 				corev1.NodeAddress{
@@ -150,7 +149,7 @@ func (i *Instances) InstanceMetadata(ctx context.Context, node *corev1.Node) (*c
 }
 
 func (i *Instances) makeInstanceID(server *iaas.Server) string {
-	return fmt.Sprintf("%s:///%s", ProviderName, server.GetId())
+	return fmt.Sprintf("%s://%s", ProviderName, server.GetId())
 }
 
 // addToNodeAddresses appends the NodeAddresses to the passed-by-pointer slice,
@@ -174,20 +173,31 @@ func addToNodeAddresses(addresses *[]corev1.NodeAddress, addAddresses ...corev1.
 // A providerID is build out of '${ProviderName}:///${instance-id}' which contains ':///'.
 // or '${ProviderName}://${region}/${instance-id}' which contains '://'.
 // See cloudprovider.GetInstanceProviderID and Instances.InstanceID.
+// TODO(migration): rework function once openstack:/// is no longer used
 func instanceIDFromProviderID(providerID string) (instanceID, region string, err error) {
 	// https://github.com/kubernetes/kubernetes/issues/85731
 	if providerID != "" && !strings.Contains(providerID, "://") {
 		providerID = ProviderName + "://" + providerID
 	}
 
-	matches := providerIDRegexp.FindStringSubmatch(providerID)
-	if len(matches) != 3 {
-		matches = oldProviderIDRegexp.FindStringSubmatch(providerID)
+	switch {
+	// TODO(migration): remove old provider support after migration
+	case strings.HasPrefix(providerID, "openstack://"):
+		matches := oldProviderIDRegexp.FindStringSubmatch(providerID)
 		if len(matches) != 3 {
-			return "", "", fmt.Errorf("ProviderID \"%s\" didn't match expected format \"%s://region/InstanceID\"", ProviderName, providerID)
+			return "", "", fmt.Errorf("ProviderID \"%s\" didn't match expected format \"%s://region/InstanceID\"", oldProviderName, providerID)
 		}
+		return matches[2], matches[1], nil
+	case strings.HasPrefix(providerID, "stackit://"):
+		matches := providerIDRegexp.FindStringSubmatch(providerID)
+		if len(matches) != 2 {
+			return "", "", fmt.Errorf("ProviderID \"%s\" didn't match expected format \"%s://InstanceID\"", ProviderName, providerID)
+		}
+		// The new stackit:// doesn't use the old regional providerID anymore and strictly follows the spec
+		return matches[1], "", nil
+	default:
+		return "", "", fmt.Errorf("unknown ProviderName")
 	}
-	return matches[2], matches[1], nil
 }
 
 func getServerByName(ctx context.Context, client stackit.NodeClient, name, projectID, region string) (*iaas.Server, error) {

pkg/ccm/instances_test.go

@@ -92,7 +92,41 @@ var _ = Describe("Node Controller", func() {
 			node := &corev1.Node{
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: corev1.NodeSpec{
-					ProviderID: fmt.Sprintf("stackit:///%s", serverID),
+					ProviderID: fmt.Sprintf("stackit://%s", serverID),
+				},
+			}
+
+			exist, err := instance.InstanceExists(context.Background(), node)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(exist).To(BeTrue())
+		})
+
+		It("successfully get the instance when old provider ID is there", func() {
+			nodeMockClient.EXPECT().GetServer(gomock.Any(), projectID, region, serverID).Return(&iaas.Server{
+				Name: ptr.To("foo"),
+			}, nil)
+
+			node := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: corev1.NodeSpec{
+					ProviderID: fmt.Sprintf("openstack:///%s", serverID),
+				},
+			}
+
+			exist, err := instance.InstanceExists(context.Background(), node)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(exist).To(BeTrue())
+		})
+
+		It("successfully get the instance when old regional provider ID is there", func() {
+			nodeMockClient.EXPECT().GetServer(gomock.Any(), projectID, region, serverID).Return(&iaas.Server{
+				Name: ptr.To("foo"),
+			}, nil)
+
+			node := &corev1.Node{
+				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
+				Spec: corev1.NodeSpec{
+					ProviderID: fmt.Sprintf("openstack://%s/%s", region, serverID),
 				},
 			}
 
@@ -118,7 +152,7 @@ var _ = Describe("Node Controller", func() {
 			node := &corev1.Node{
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: corev1.NodeSpec{
-					ProviderID: fmt.Sprintf("stackit:///%s", serverID),
+					ProviderID: fmt.Sprintf("stackit://%s", serverID),
 				},
 			}
 
@@ -155,7 +189,7 @@ var _ = Describe("Node Controller", func() {
 			node := &corev1.Node{
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: corev1.NodeSpec{
-					ProviderID: fmt.Sprintf("stackit:///%s", serverID),
+					ProviderID: fmt.Sprintf("stackit://%s", serverID),
 				},
 			}
 
@@ -210,7 +244,7 @@ var _ = Describe("Node Controller", func() {
 
 			metadata, err := instance.InstanceMetadata(context.Background(), node)
 			Expect(err).NotTo(HaveOccurred())
-			Expect(metadata.ProviderID).To(Equal(fmt.Sprintf("stackit:///%s", serverID)))
+			Expect(metadata.ProviderID).To(Equal(fmt.Sprintf("stackit://%s", serverID)))
 			Expect(metadata.InstanceType).To(Equal("flatcar"))
 			Expect(metadata.NodeAddresses).To(Equal([]corev1.NodeAddress{
 				{