-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Fixes nil pointer panic on autoscaling types conversion #71744
Fixes nil pointer panic on autoscaling types conversion #71744
Conversation
@@ -171,7 +171,8 @@ func Convert_autoscaling_ExternalMetricStatus_To_v1_ExternalMetricStatus(in *aut | |||
out.CurrentValue = *in.Current.Value | |||
} | |||
if in.Current.AverageValue != nil { | |||
out.CurrentAverageValue = in.Current.AverageValue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is another surprise when i walk thru all the autoscaling conversion. should do a deep copy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conversion is not required to make copies of the input object values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
conversion is not required to make copies of the input object values
@liggitt if we call this conversion directly (not recommended tho), modifying the input obj will propagates to the output. is that okay? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
edit: mmm.. quantity might be special.. we don't actually modify that in-place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we call this conversion directly (not recommended tho), modifying the input obj will propagates to the output. is that okay?
Yes. Generated conversions share memory between the input and output object where possible:
kubernetes/pkg/apis/core/v1/zz_generated.conversion.go
Lines 7457 to 7494 in a0c2788
func autoConvert_v1_VolumeSource_To_core_VolumeSource(in *v1.VolumeSource, out *core.VolumeSource, s conversion.Scope) error { | |
out.HostPath = (*core.HostPathVolumeSource)(unsafe.Pointer(in.HostPath)) | |
out.EmptyDir = (*core.EmptyDirVolumeSource)(unsafe.Pointer(in.EmptyDir)) | |
out.GCEPersistentDisk = (*core.GCEPersistentDiskVolumeSource)(unsafe.Pointer(in.GCEPersistentDisk)) | |
out.AWSElasticBlockStore = (*core.AWSElasticBlockStoreVolumeSource)(unsafe.Pointer(in.AWSElasticBlockStore)) | |
out.GitRepo = (*core.GitRepoVolumeSource)(unsafe.Pointer(in.GitRepo)) | |
out.Secret = (*core.SecretVolumeSource)(unsafe.Pointer(in.Secret)) | |
out.NFS = (*core.NFSVolumeSource)(unsafe.Pointer(in.NFS)) | |
out.ISCSI = (*core.ISCSIVolumeSource)(unsafe.Pointer(in.ISCSI)) | |
out.Glusterfs = (*core.GlusterfsVolumeSource)(unsafe.Pointer(in.Glusterfs)) | |
out.PersistentVolumeClaim = (*core.PersistentVolumeClaimVolumeSource)(unsafe.Pointer(in.PersistentVolumeClaim)) | |
out.RBD = (*core.RBDVolumeSource)(unsafe.Pointer(in.RBD)) | |
out.FlexVolume = (*core.FlexVolumeSource)(unsafe.Pointer(in.FlexVolume)) | |
out.Cinder = (*core.CinderVolumeSource)(unsafe.Pointer(in.Cinder)) | |
out.CephFS = (*core.CephFSVolumeSource)(unsafe.Pointer(in.CephFS)) | |
out.Flocker = (*core.FlockerVolumeSource)(unsafe.Pointer(in.Flocker)) | |
out.DownwardAPI = (*core.DownwardAPIVolumeSource)(unsafe.Pointer(in.DownwardAPI)) | |
out.FC = (*core.FCVolumeSource)(unsafe.Pointer(in.FC)) | |
out.AzureFile = (*core.AzureFileVolumeSource)(unsafe.Pointer(in.AzureFile)) | |
out.ConfigMap = (*core.ConfigMapVolumeSource)(unsafe.Pointer(in.ConfigMap)) | |
out.VsphereVolume = (*core.VsphereVirtualDiskVolumeSource)(unsafe.Pointer(in.VsphereVolume)) | |
out.Quobyte = (*core.QuobyteVolumeSource)(unsafe.Pointer(in.Quobyte)) | |
out.AzureDisk = (*core.AzureDiskVolumeSource)(unsafe.Pointer(in.AzureDisk)) | |
out.PhotonPersistentDisk = (*core.PhotonPersistentDiskVolumeSource)(unsafe.Pointer(in.PhotonPersistentDisk)) | |
if in.Projected != nil { | |
in, out := &in.Projected, &out.Projected | |
*out = new(core.ProjectedVolumeSource) | |
if err := Convert_v1_ProjectedVolumeSource_To_core_ProjectedVolumeSource(*in, *out, s); err != nil { | |
return err | |
} | |
} else { | |
out.Projected = nil | |
} | |
out.PortworxVolume = (*core.PortworxVolumeSource)(unsafe.Pointer(in.PortworxVolume)) | |
out.ScaleIO = (*core.ScaleIOVolumeSource)(unsafe.Pointer(in.ScaleIO)) | |
out.StorageOS = (*core.StorageOSVolumeSource)(unsafe.Pointer(in.StorageOS)) | |
return nil | |
} |
/assign @liggitt |
@@ -247,8 +247,10 @@ func Convert_autoscaling_ObjectMetricStatus_To_v2beta1_ObjectMetricStatus(in *au | |||
} | |||
out.MetricName = in.Metric.Name | |||
out.Selector = in.Metric.Selector | |||
currentAverageValue := *in.Current.AverageValue | |||
out.AverageValue = ¤tAverageValue | |||
if in.Current.AverageValue != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a test demonstrating the bug and that this fixes the issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this have the same issue:
kubernetes/pkg/apis/autoscaling/v2beta1/conversion.go
Lines 188 to 189 in a0c2788
targetAverageValue := *in.Target.AverageValue | |
out.TargetAverageValue = targetAverageValue |
b5ebbba
to
5721e77
Compare
5721e77
to
e85924e
Compare
@liggitt is it ready for cherry-picks? |
/lgtm let's wait for green CI before opening picks |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liggitt, yue9944882 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 |
/retest Review the full test history for this PR. Silence the bot with an |
@yue9944882 can you open the picks for this to release-1.13 and release-1.12? |
@liggitt sure, done (wake up hour here) |
…1744-origin-release-1.12 Automated cherry pick of #71744: fixes autoscaling types conversion
…1744-origin-release-1.13 Automated cherry pick of #71744: fixes autoscaling types conversion
/kind bug
/sig autoscaling
/priority critical-urgent
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #70806
Special notes for your reviewer:
Does this PR introduce a user-facing change?: