Modified providerID to unified format #81859

bells17 · 2019-08-23T18:37:55Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Modified providerID to unified format.
I think instances.InstanceExistsByProviderID's providerID is needed <provider name>://<instance id> format.
But currently 2 patterns exists like below:

<provider name>://<instance id>
<instance id>

e.g.:

Which issue(s) this PR fixes:

Special notes for your reviewer:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

k8s-ci-robot · 2019-08-23T18:38:03Z

Hi @bells17. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2019-08-23T18:38:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bells17
To complete the pull request process, please assign wlan0
You can assign the PR to them by writing /assign @wlan0 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

neolit123 · 2019-08-24T12:35:00Z

/cc @andrewsykim

bells17 · 2019-08-25T04:34:44Z

Sorry before a review, I fixed typo I found.

andrewsykim · 2019-08-26T17:26:32Z

pkg/controller/cloud/node_controller.go

 	providerID := node.Spec.ProviderID
 	if providerID == "" {
 		var err error
-		providerID, err = instances.InstanceID(context.TODO(), types.NodeName(node.Name))
+		providerID, err = cloudprovider.GetInstanceProviderID(context.TODO(), cloud, types.NodeName(node.Name))


This is originally calling InstanceID for legacy reasons. It only applies if a provider ID isn't already set which shouldn't be the case.

@andrewsykim Thank you for commenting.

This is originally calling InstanceID for legacy reasons. It only applies if a provider ID isn't already set which shouldn't be the case.

I understand. But I think if the situation that InstanceID can get an instance id, cloud.ProviderName can get a provider name.
because of cloud.ProviderName is a very simple method what return a provider name string only.
So if the situation that a provider id can use <provider name>://<instance id>, I think a provider id should be a unified format.

Sorry I don't quite follow the reasoning yet.

The main reason for actually calling InstanceID here is because (for legacy reasons) it can actually return the cloudprovider.InstanceNotFound error which we check for below. Some providers still depend on that behavior IIRC

@andrewsykim Ah...I see. Sorry, I mistake.
I fixed my code 😅

This reverts commit 899e293.

This reverts commit f20c488.

andrewsykim · 2019-08-28T19:54:56Z

pkg/controller/cloud/node_controller.go

 	providerID := node.Spec.ProviderID
 	if providerID == "" {
 		var err error
-		providerID, err = instances.InstanceID(context.TODO(), types.NodeName(node.Name))


Oddly I think using the raw instance type is expected here. See how the AWS provider parses this for example in https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/legacy-cloud-providers/aws/instances.go#L50-L91. I agree it's confusing and not ideal, but I think trying to force the format <provider>://<instance-id> could break some implementations.

@andrewsykim Um...Sorry, I don't understand.

It looks support <provider>://<instance-id> format because bellow:

kubernetes/staging/src/k8s.io/legacy-cloud-providers/aws/instances.go

Lines 61 to 72 in 41049fd

if !strings.HasPrefix(s, "aws://") {

// Assume a bare aws volume id (vol-1234...)

// Build a URL with an empty host (AZ)

s = "aws://" + "/" + "/" + s

}

url, err := url.Parse(s)

if err != nil {

return "", fmt.Errorf("Invalid instance name (%s): %v", name, err)

}

if url.Scheme != "aws" {

return "", fmt.Errorf("Invalid scheme for AWS instance (%s)", name)

}

kubernetes/staging/src/k8s.io/legacy-cloud-providers/aws/instances.go

Lines 52 to 53 in 41049fd

// * aws:///<zone>/<awsInstanceId>

// * aws:////<awsInstanceId>

And other legacy cloud providers looks support <provider>://<instance-id> too:

azure

kubernetes/staging/src/k8s.io/legacy-cloud-providers/azure/azure_wrap.go

Line 43 in 41049fd

azureNodeProviderIDRE = regexp.MustCompile(`^azure:///subscriptions/(?:.*)/resourceGroups/(?:.*)/providers/Microsoft.Compute/(?:.*)`)

kubernetes/staging/src/k8s.io/legacy-cloud-providers/azure/azure_wrap.go

Lines 326 to 330 in 41049fd

// IsNodeUnmanagedByProviderID returns true if the node is not managed by Azure cloud provider.

// All managed node's providerIDs are in format 'azure:///subscriptions/<id>/resourceGroups/<rg>/providers/Microsoft.Compute/.*'

func (az *Cloud) IsNodeUnmanagedByProviderID(providerID string) bool {

return !azureNodeProviderIDRE.Match([]byte(providerID))

}

gcp

kubernetes/staging/src/k8s.io/legacy-cloud-providers/gce/gce_util.go

Lines 206 to 215 in 41049fd

// splitProviderID splits a provider's id into core components.

// A providerID is build out of '${ProviderName}://${project-id}/${zone}/${instance-name}'

// See cloudprovider.GetInstanceProviderID.

func splitProviderID(providerID string) (project, zone, instance string, err error) {

matches := providerIDRE.FindStringSubmatch(providerID)

if len(matches) != 4 {

return "", "", "", errors.New("error splitting providerID")

}

return matches[1], matches[2], matches[3], nil

}

vsphere

kubernetes/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_util.go

Lines 615 to 619 in 41049fd

// ProviderID is UUID for nodes v1.9.3+

if node.VMUUID == GetUUIDFromProviderID(providerID) || node.NodeName == providerID {

nodeName = node.NodeName

break

}

kubernetes/staging/src/k8s.io/legacy-cloud-providers/vsphere/vsphere_util.go

Lines 628 to 630 in 41049fd

func GetUUIDFromProviderID(providerID string) string {

return strings.TrimPrefix(providerID, ProviderPrefix)

}

@andrewsykim Hi, as wrote above that looks like InstanceExistsByProviderID method in AWS and other providers support <provider>://<instance-id> format.
So maybe provider ID format could be unified.
But, shouldn't provider ID format be unified?

Ideally it should, but there are some contraints here because we have to support legacy formats of the provider ID.

Thank you. I understand.

fejta-bot · 2020-01-23T00:51:59Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2020-01-23T00:52:06Z

@bells17: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

fejta-bot · 2020-03-13T22:43:59Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

Modified to unified providerID format

f20c488

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 23, 2019

k8s-ci-robot requested review from derekwaynecarr and thockin August 23, 2019 18:38

k8s-ci-robot added area/cloudprovider sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 23, 2019

bells17 changed the title ~~Modified to unified providerID format~~ Modified providerID to unified format Aug 24, 2019

k8s-ci-robot requested a review from andrewsykim August 24, 2019 12:35

Fix typo

899e293

andrewsykim reviewed Aug 26, 2019

View reviewed changes

bells17 added 3 commits August 28, 2019 23:40

Revert "Fix typo"

14aef1f

This reverts commit 899e293.

Revert "Modified to unified providerID format"

343bd9f

This reverts commit f20c488.

Modified providerID to unified format

f8b0f6e

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 28, 2019

andrewsykim reviewed Aug 28, 2019

View reviewed changes

bells17 requested a review from andrewsykim October 25, 2019 00:46

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 23, 2020

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 23, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Mar 13, 2020

bells17 closed this Apr 8, 2020

bells17 deleted the modified-to-unified-providerid-format branch April 8, 2020 10:07

shyamradhakrishnan mentioned this pull request Apr 13, 2023

Allow provider id regex matching to support more provider formats kubernetes-sigs/cluster-api#8485

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified providerID to unified format #81859

Modified providerID to unified format #81859

bells17 commented Aug 23, 2019 •

edited

Loading

k8s-ci-robot commented Aug 23, 2019

k8s-ci-robot commented Aug 23, 2019

neolit123 commented Aug 24, 2019

bells17 commented Aug 25, 2019

andrewsykim Aug 26, 2019

bells17 Aug 26, 2019 •

edited

Loading

andrewsykim Aug 27, 2019

bells17 Aug 28, 2019

andrewsykim Aug 28, 2019

bells17 Aug 28, 2019

bells17 Sep 10, 2019 •

edited

Loading

andrewsykim Feb 12, 2020

bells17 Apr 8, 2020

fejta-bot commented Jan 23, 2020

k8s-ci-robot commented Jan 23, 2020

fejta-bot commented Mar 13, 2020

	if !strings.HasPrefix(s, "aws://") {
	// Assume a bare aws volume id (vol-1234...)
	// Build a URL with an empty host (AZ)
	s = "aws://" + "/" + "/" + s
	}
	url, err := url.Parse(s)
	if err != nil {
	return "", fmt.Errorf("Invalid instance name (%s): %v", name, err)
	}
	if url.Scheme != "aws" {
	return "", fmt.Errorf("Invalid scheme for AWS instance (%s)", name)
	}

	// * aws:///<zone>/<awsInstanceId>
	// * aws:////<awsInstanceId>

	// IsNodeUnmanagedByProviderID returns true if the node is not managed by Azure cloud provider.
	// All managed node's providerIDs are in format 'azure:///subscriptions/<id>/resourceGroups/<rg>/providers/Microsoft.Compute/.*'
	func (az *Cloud) IsNodeUnmanagedByProviderID(providerID string) bool {
	return !azureNodeProviderIDRE.Match([]byte(providerID))
	}

	// splitProviderID splits a provider's id into core components.
	// A providerID is build out of '${ProviderName}://${project-id}/${zone}/${instance-name}'
	// See cloudprovider.GetInstanceProviderID.
	func splitProviderID(providerID string) (project, zone, instance string, err error) {
	matches := providerIDRE.FindStringSubmatch(providerID)
	if len(matches) != 4 {
	return "", "", "", errors.New("error splitting providerID")
	}
	return matches[1], matches[2], matches[3], nil
	}

	// ProviderID is UUID for nodes v1.9.3+
	if node.VMUUID == GetUUIDFromProviderID(providerID) \|\| node.NodeName == providerID {
	nodeName = node.NodeName
	break
	}

	func GetUUIDFromProviderID(providerID string) string {
	return strings.TrimPrefix(providerID, ProviderPrefix)
	}

Modified providerID to unified format #81859

Modified providerID to unified format #81859

Conversation

bells17 commented Aug 23, 2019 • edited Loading

k8s-ci-robot commented Aug 23, 2019

k8s-ci-robot commented Aug 23, 2019

neolit123 commented Aug 24, 2019

bells17 commented Aug 25, 2019

andrewsykim Aug 26, 2019

Choose a reason for hiding this comment

bells17 Aug 26, 2019 • edited Loading

Choose a reason for hiding this comment

andrewsykim Aug 27, 2019

Choose a reason for hiding this comment

bells17 Aug 28, 2019

Choose a reason for hiding this comment

andrewsykim Aug 28, 2019

Choose a reason for hiding this comment

bells17 Aug 28, 2019

Choose a reason for hiding this comment

bells17 Sep 10, 2019 • edited Loading

Choose a reason for hiding this comment

andrewsykim Feb 12, 2020

Choose a reason for hiding this comment

bells17 Apr 8, 2020

Choose a reason for hiding this comment

fejta-bot commented Jan 23, 2020

k8s-ci-robot commented Jan 23, 2020

fejta-bot commented Mar 13, 2020

bells17 commented Aug 23, 2019 •

edited

Loading

bells17 Aug 26, 2019 •

edited

Loading

bells17 Sep 10, 2019 •

edited

Loading