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

'cannot convert int64 to float64' with runtime.DefaultUnstructuredConverter.FromUnstructured on CustomResourceDefinitions with integer minimum, maximum, and/or multipleOf values #87675

Closed
nrb opened this issue Jan 29, 2020 · 11 comments · Fixed by #93250
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.

Comments

@nrb
Copy link

nrb commented Jan 29, 2020

What happened:

When converting JSON representations of v1beta1 CustomResourceDefintions with Spec.Validation.OpenAPIV3Schema.Properties.(Minimum|Maximum|multipleOf) fields set to a whole number, the runtime.DefaultUnstructuredConverter.FromUnstructured function returns 'cannot convert int64 to float64'.

Making the field decimal numbers converts as expected.

What you expected to happen:
OpenAPI schema should convert from unstructured.Unstructured whether whole or decimal representation.

How to reproduce it (as minimally and precisely as possible):

A Go test with a minimal CRD & failing test is at https://github.com/nrb/unstructured-conversion-bug/blob/master/main_test.go. The repo includes a go.sum with pinned versions of dependencies. Use go test to run it and get a failure.

Anything else we need to know?:
This was identified by someone reviewing a Velero pull request that waited on CRDs to be 'ready' before restoring CRDs from their serialized JSON representations. The reproduced case is based on his sample CRD.

There's also a Helm 2 issue referencing this, with an included fix. helm/helm#5806.

I believe #77665 may be related, but I'm not 100% sure.

Environment:

  • Kubernetes version (use kubectl version): apimachinery 0.17.2
@nrb nrb added the kind/bug Categorizes issue or PR as related to a bug. label Jan 29, 2020
@k8s-ci-robot k8s-ci-robot added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Jan 29, 2020
@nrb
Copy link
Author

nrb commented Jan 29, 2020

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 29, 2020
@liggitt
Copy link
Member

liggitt commented Jan 30, 2020

I don't think number type conversion is intended to be performed by that helper. It's primary purpose is to round-trip between a typed and unstructured object without requiring json serialization/deserialization.

If you have JSON you want to unmarshal, you can do so directly into the CRD go type without first unmarshaling into an Unstructured object.

@fedebongio
Copy link
Contributor

/assign @jennybuckley

@nrb
Copy link
Author

nrb commented Mar 5, 2020

@liggitt Apologies for the delayed response.

In the original problem this was actually with information returned by the Kubernetes API server, which I then tried to change into the CRD Go type. The original goal was for Velero (a backup/restore tool) to check a CRD in the cluster for readiness before restoring any CRs. In Velero's case, because we deal with more or less any object that could be in the API server, we're using a Dynamic client as we can't import every possible Go type, though well-known ones like CRDs are different.

The example I have is the simplest possible reproduction case I could come up with, without involving a full Kubernetes API server.

Given we're constrained to using Unstructured types from the Kubernetes API server, is there an alternative method we could use for translating them into the CRD Go types?

@liggitt
Copy link
Member

liggitt commented Mar 5, 2020

Given we're constrained to using Unstructured types from the Kubernetes API server, is there an alternative method we could use for translating them into the CRD Go types?

I'm not sure I understand. If you're working with go types, you can unmarshal into the typed structs, correct?

@nrb
Copy link
Author

nrb commented Mar 5, 2020

Not always, because in some instances, we hit this error. We got another report of this today (vmware-tanzu/velero#2319), which I traced back to my code at https://github.com/vmware-tanzu/velero/blob/master/pkg/backup/remap_crd_version_action.go#L65, the runtime.DefaultUnstructuredConverter.FromUnstructured call to take the Unstructured data to the Go type.

I should clarify - Velero's not working with JSON directly at this point. It's working with unstructured.Unstructured.

@fejta-bot
Copy link

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 k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 3, 2020
@nrb
Copy link
Author

nrb commented Jun 17, 2020

/remove-lifecycle-stale

This is still affecting Velero, though we've worked around it by not using FromUnstructured on CRDs returned directly from the Kube API server as Unstructured through a dynamic client. We instead marshal to JSON and back for now.

I think I may have not explained my case clearly - is there anything I can do or provide to help resolve this?

@fejta-bot
Copy link

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

@k8s-ci-robot 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 Jul 17, 2020
@liggitt
Copy link
Member

liggitt commented Jul 20, 2020

apologies for the delay, I dug into this and opened #93250 as a fix

@liggitt liggitt added priority/backlog Higher priority than priority/awaiting-more-evidence. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Jul 20, 2020
@liggitt liggitt assigned liggitt and unassigned jennybuckley Jul 20, 2020
@nrb
Copy link
Author

nrb commented Jul 20, 2020

@liggitt Thanks a ton!

reasonerjt added a commit to reasonerjt/velero that referenced this issue Aug 25, 2021
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `IsV1CRDReady` to check the readiness of CRD.
A testcase is added to verify this func is also applicable to v1beta1
CRDs.

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Aug 31, 2021
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Aug 31, 2021
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
reasonerjt added a commit to reasonerjt/velero that referenced this issue Sep 1, 2021
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
jenting pushed a commit to vmware-tanzu/velero that referenced this issue Sep 1, 2021
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
danfengliu pushed a commit to danfengliu/velero that referenced this issue Jan 25, 2022
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
derekbit added a commit to derekbit/longhorn that referenced this issue Apr 1, 2022
Fix the helm upgrade failure on kuberenetes v1.18.
Root casue: kubernetes/kubernetes#87675

Longhorn 3631

Signed-off-by: Derek Su <derek.su@suse.com>
derekbit added a commit to derekbit/longhorn-manager that referenced this issue Apr 1, 2022
Fix the helm upgrade failure on kuberenetes v1.18.
Root casue: kubernetes/kubernetes#87675

Longhorn 3631

Signed-off-by: Derek Su <derek.su@suse.com>
derekbit added a commit to derekbit/longhorn-manager that referenced this issue Apr 1, 2022
Fix the helm upgrade failure on kuberenetes v1.18.
Root cause: kubernetes/kubernetes#87675

Longhorn 3631

Signed-off-by: Derek Su <derek.su@suse.com>
derekbit added a commit to derekbit/longhorn that referenced this issue Apr 1, 2022
Fix the helm upgrade failure on kuberenetes v1.18.
Root cause: kubernetes/kubernetes#87675

Longhorn 3631

Signed-off-by: Derek Su <derek.su@suse.com>
innobead pushed a commit to longhorn/longhorn that referenced this issue Apr 1, 2022
Fix the helm upgrade failure on kuberenetes v1.18.
Root cause: kubernetes/kubernetes#87675

Longhorn 3631

Signed-off-by: Derek Su <derek.su@suse.com>
derekbit added a commit to derekbit/longhorn-manager that referenced this issue Apr 1, 2022
Fix the helm upgrade failure on kuberenetes v1.18.
Root cause: kubernetes/kubernetes#87675

Longhorn 3631

Signed-off-by: Derek Su <derek.su@suse.com>
innobead pushed a commit to longhorn/longhorn-manager that referenced this issue Apr 1, 2022
Fix the helm upgrade failure on kuberenetes v1.18.
Root cause: kubernetes/kubernetes#87675

Longhorn 3631

Signed-off-by: Derek Su <derek.su@suse.com>
gyaozhou pushed a commit to gyaozhou/velero-read that referenced this issue May 14, 2022
This commit removes `IsUnstructuredCRDReady` since
kubernetes/kubernetes#87675 is fixed.
Is uses `Is1CRDReady` to check the readiness of CRD.

After v1.7 we may consider merge the funcx `IsV1Beta1CRDReady` and
`IsV1CRDReady`

Signed-off-by: Daniel Jiang <jiangd@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/backlog Higher priority than priority/awaiting-more-evidence. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants