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.

Sign up for GitHub

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

Jump to bottom

fix: GetLabelsForVolume panic issue for azure disk PV #92166

Merged
merged 1 commit into from Jun 18, 2020
Merged

fix: GetLabelsForVolume panic issue for azure disk PV #92166

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 9 additions & 5 deletions staging/src/k8s.io/legacy-cloud-providers/azure/azure_managedDiskController.go
View file
Open in desktop
Expand Up @@ -346,6 +346,13 @@ func (c *Cloud) GetAzureDiskLabels(diskURI string) (map[string]string, error) {
return nil, err
}

labels := map[string]string{
v1.LabelZoneRegion: c.Location,
}
// no azure credential is set, return nil
if c.DisksClient == nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also skip when c==nil?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if c==nil, then c.GetAzureDiskLabels() already crashed ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why can DisksClient be nil? Is that even a valid situation for Cloud?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sttts that's because azure identity is not set in azure.json for kube-apiserver, so DisksClient is not initialized, and it does not make sense to initialize DisksClient if azure identity is not set:

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

Lines 462 to 466 in b224790

// No credentials provided, InstanceMetadataService would be used for getting Azure resources.
// Note that this only applies to Kubelet, controller-manager should configure credentials for managing Azure resources.
if servicePrincipalToken == nil {
return nil
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sttts, email Bz1847185 . API server fix might be needed too.

Copy link
Member

@enxebre enxebre Jun 16, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's because azure identity is not set in azure.json for kube-apiserver,...

It'd be useful to share some context/bug ref in the commit message i.e git commit -m"" -m"here"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to include a unit test for this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shall we go with this PR first? One of my colleagues are adding unit test for the azure_managedDiskController.go
cc @weijiehu

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my PR for azure_managedDiskController.go unittest is in progress, I will send it tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyzhangx I new unit test PR here #92216

return labels, nil
}
// Get information of the disk.
ctx, cancel := getContextWithCancel()
defer cancel()
Expand All @@ -358,7 +365,7 @@ func (c *Cloud) GetAzureDiskLabels(diskURI string) (map[string]string, error) {
// Check whether availability zone is specified.
if disk.Zones == nil || len(*disk.Zones) == 0 {
klog.V(4).Infof("Azure disk %q is not zoned", diskName)
return nil, nil
return labels, nil
}

zones := *disk.Zones
Expand All @@ -369,9 +376,6 @@ func (c *Cloud) GetAzureDiskLabels(diskURI string) (map[string]string, error) {

zone := c.makeZone(c.Location, zoneID)
klog.V(4).Infof("Got zone %q for Azure disk %q", zone, diskName)
labels := map[string]string{
v1.LabelZoneRegion: c.Location,
v1.LabelZoneFailureDomain: zone,
}
labels[v1.LabelZoneFailureDomain] = zone
return labels, nil
}