Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert resource group name in Azure provider ID to lower cases #74882

Merged
merged 1 commit into from Mar 5, 2019

Conversation

@feiskyer
Copy link
Member

commented Mar 4, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

The resource group name in Azure VM's ID may be in different cases from time to time. This would cause the node's providerID are also in different cases.

While it's still working, this different cases issue may introduce troubles for external components that are dpending on providerID (e.g. cluster autoscaler).

This PR converts the resource group name in Azure provider ID to lower cases, so that all nodes' providerID are still in same cases in such case.

Which issue(s) this PR fixes:

Fixes #71994

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

The resource group name in Azure providerID is not converted to lower cases.

/sig azure
/kind bug
/priority critical-urgent
/milestone v1.14

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer

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

The pull request process is described 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

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

/test pull-kubernetes-e2e-aks-engine-azure

@andyzhangx
Copy link
Member

left a comment

there are other functions like getFrontendIPConfigID, getBackendPoolID, especially getFrontendIPConfigID func since az.ResourceGroup is read from azure.json config file, it's controlled by user config, could be upper case.

func (az *Cloud) getFrontendIPConfigID(lbName, fipConfigName string) string {
	return fmt.Sprintf(
		frontendIPConfigIDTemplate,
		az.SubscriptionID,
		az.ResourceGroup,
		lbName,
		fipConfigName)
}
Show resolved Hide resolved pkg/cloudprovider/providers/azure/azure_standard.go
@@ -185,7 +185,13 @@ func (ss *scaleSet) GetInstanceIDByNodeName(name string) (string, error) {
return "", err
}

return *vm.ID, nil
resourceID := *vm.ID

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Mar 4, 2019

Member
return convertResourceGroupNameToLower(*vm.ID)

This comment has been minimized.

Copy link
@feiskyer

feiskyer Mar 4, 2019

Author Member

add an error log here

This comment has been minimized.

Copy link
@andyzhangx

andyzhangx Mar 4, 2019

Member

then add this log into function convertResourceGroupNameToLower?

Show resolved Hide resolved pkg/cloudprovider/providers/azure/azure_wrap.go Outdated
Show resolved Hide resolved pkg/cloudprovider/providers/azure/azure_standard.go
@feiskyer

This comment has been minimized.

Copy link
Member Author

commented Mar 4, 2019

there are other functions like getFrontendIPConfigID, getBackendPoolID, especially getFrontendIPConfigID func since az.ResourceGroup is read from azure.json config file, it's controlled by user config, could be upper case.

Other IDs are used only internally, hence it's ok of any cases. Only exposed providerID is required for conversion.

@feiskyer feiskyer force-pushed the feiskyer:convert-rg-to-lower branch from 158b193 to 1d61d8d Mar 5, 2019

@feiskyer

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2019

/test pull-kubernetes-e2e-aks-engine-azure

@feiskyer feiskyer added this to Needs Review in SIG Azure Mar 5, 2019

@andyzhangx
Copy link
Member

left a comment

/lgtm

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 5, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit e330c01 into kubernetes:master Mar 5, 2019

18 checks passed

cla/linuxfoundation feiskyer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-aks-engine-azure Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

SIG Azure automation moved this from Needs Review to Done Mar 5, 2019

mgdevstack added a commit to mgdevstack/kubernetes that referenced this pull request Mar 5, 2019

Merge pull request kubernetes#74882 from feiskyer/convert-rg-to-lower
Convert resource group name in Azure provider ID to lower cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.