From 76b864b74557066c1cef3cd1ad491a69292a2de8 Mon Sep 17 00:00:00 2001 From: Daniel Jiang Date: Tue, 31 Aug 2021 23:56:52 +0800 Subject: [PATCH] remove IsUnstructuredCRDReady 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 --- pkg/restore/restore.go | 7 +- pkg/util/kube/utils.go | 77 ++++++--------------- pkg/util/kube/utils_test.go | 134 ++++++++++++++++++++++-------------- 3 files changed, 105 insertions(+), 113 deletions(-) diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index bde76777371..c874dd7c709 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -842,12 +842,7 @@ func (ctx *restoreContext) crdAvailable(name string, crdClient client.Dynamic) ( if err != nil { return true, err } - - // TODO: Due to upstream conversion issues in runtime.FromUnstructured, - // we use the unstructured object here. Once the upstream conversion - // functions are fixed, we should convert to the CRD types and use - // IsCRDReady. - available, err = kube.IsUnstructuredCRDReady(unstructuredCRD) + available, err = kube.IsCRDReady(unstructuredCRD) if err != nil { return true, err } diff --git a/pkg/util/kube/utils.go b/pkg/util/kube/utils.go index a80f864deac..ce2b924db5b 100644 --- a/pkg/util/kube/utils.go +++ b/pkg/util/kube/utils.go @@ -28,6 +28,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/wait" corev1client "k8s.io/client-go/kubernetes/typed/core/v1" corev1listers "k8s.io/client-go/listers/core/v1" @@ -176,61 +177,25 @@ func IsV1Beta1CRDReady(crd *apiextv1beta1.CustomResourceDefinition) bool { return (isEstablished && namesAccepted) } -// IsUnstructuredCRDReady checks an unstructured CRD to see if it's ready, with both the Established and NamesAccepted conditions. -// TODO: Delete this function and use IsV1CRDReady/IsV1Beta1CRDReady when the upstream runtime.FromUnstructured function properly handles int64 field conversions. -// Duplicated function because the velero install package uses IsV1CRDReady/IsV1Beta1CRDReady with instances of v1/v1beta1 types. -// See https://github.com/kubernetes/kubernetes/issues/87675 -// This is different from the fix for https://github.com/vmware-tanzu/velero/issues/2319 because here, -// we need to account for *both* v1beta1 and v1 CRDs, so doing marshalling into JSON to convert to a Go type may not be as useful here, unless we do -// type switching. -func IsUnstructuredCRDReady(crd *unstructured.Unstructured) (bool, error) { - var isEstablished, namesAccepted bool - - conditions, ok, err := unstructured.NestedSlice(crd.UnstructuredContent(), "status", "conditions") - if !ok { - return false, nil - } - if err != nil { - return false, errors.Wrap(err, "unable to access CRD's conditions") - } - - for _, c := range conditions { - // Unlike the typed version of this function, we need to cast the Condition since it's an interface{} here, - // then we fetch the type and status of the Condition before inspecting them for relevant values - cond, ok := c.(map[string]interface{}) - if !ok { - return false, errors.New("unable to convert condition to map[string]interface{}") - } - conditionType, ok, err := unstructured.NestedString(cond, "type") - if !ok { - // This should never happen unless someone manually edits the serialized data. - return false, errors.New("condition missing a type") - } - - if err != nil { - return false, errors.Wrap(err, "unable to access condition's type") - } - - status, ok, err := unstructured.NestedString(cond, "status") - if !ok { - // This should never happen unless someone manually edits the serialized data. - return false, errors.New("condition missing a status") - } - - if err != nil { - return false, errors.Wrap(err, "unable to access condition's status") - } - - // Here is the actual logic of the function - // Cast the API's types into strings since we're pulling strings out of the unstructured data. - // We are using the v1beta1 constants here but they are the same as the v1 constants. - if conditionType == string(apiextv1beta1.Established) && status == string(apiextv1beta1.ConditionTrue) { - isEstablished = true - } - if conditionType == string(apiextv1beta1.NamesAccepted) && status == string(apiextv1beta1.ConditionTrue) { - namesAccepted = true - } +// IsCRDReady triggers IsV1Beta1CRDReady/IsV1CRDReady according to the version of the input parm +func IsCRDReady(crd *unstructured.Unstructured)(bool, error) { + ver := crd.GroupVersionKind().Version + switch ver { + case "v1beta1": + v1beta1crd := &apiextv1beta1.CustomResourceDefinition{} + err := runtime.DefaultUnstructuredConverter.FromUnstructured(crd.Object, v1beta1crd) + if err != nil{ + return false, err + } + return IsV1Beta1CRDReady(v1beta1crd), nil + case "v1": + v1crd := &apiextv1.CustomResourceDefinition{} + err := runtime.DefaultUnstructuredConverter.FromUnstructured(crd.Object, v1crd) + if err != nil{ + return false, err + } + return IsV1CRDReady(v1crd), nil + default: + return false, fmt.Errorf("unable to handle CRD with version %s", ver) } - - return (isEstablished && namesAccepted), nil } diff --git a/pkg/util/kube/utils_test.go b/pkg/util/kube/utils_test.go index d5966256dad..c04756ac9a9 100644 --- a/pkg/util/kube/utils_test.go +++ b/pkg/util/kube/utils_test.go @@ -17,6 +17,7 @@ limitations under the License. package kube import ( + "encoding/json" "testing" "time" @@ -288,31 +289,31 @@ func TestIsV1CRDReady(t *testing.T) { } } -func TestIsUnstructuredCRDReady(t *testing.T) { - tests := []struct { +func TestIsCRDReady(t *testing.T) { + v1beta1tests := []struct { name string crd *apiextv1beta1.CustomResourceDefinition want bool }{ { - name: "CRD is not established & not accepting names - not ready", + name: "v1beta1CRD is not established & not accepting names - not ready", crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD").Result(), want: false, }, { - name: "CRD is established & not accepting names - not ready", + name: "v1beta1CRD is established & not accepting names - not ready", crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD"). Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()).Result(), want: false, }, { - name: "CRD is not established & accepting names - not ready", + name: "v1beta1CRD is not established & accepting names - not ready", crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD"). Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()).Result(), want: false, }, { - name: "CRD is established & accepting names - ready", + name: "v1beta1CRD is established & accepting names - ready", crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD"). Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()). Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()). @@ -321,66 +322,97 @@ func TestIsUnstructuredCRDReady(t *testing.T) { }, } - for _, tc := range tests { + for _, tc := range v1beta1tests { m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.crd) require.NoError(t, err) - result, err := IsUnstructuredCRDReady(&unstructured.Unstructured{Object: m}) + result, err := IsCRDReady(&unstructured.Unstructured{Object: m}) require.NoError(t, err) assert.Equal(t, tc.want, result) } -} -// TestFromUnstructuredIntToFloatBug tests for a bug where runtime.DefaultUnstructuredConverter.FromUnstructured can't take a whole number into a float. -// This test should fail when https://github.com/kubernetes/kubernetes/issues/87675 is fixed upstream, letting us know we can remove the IsUnstructuredCRDReady function. -/* -func TestFromUnstructuredIntToFloatBug(t *testing.T) { - b := []byte(` + v1tests := []struct { + name string + crd *apiextv1.CustomResourceDefinition + want bool + }{ + { + name: "v1CRD is not established & not accepting names - not ready", + crd: builder.ForV1CustomResourceDefinition("MyCRD").Result(), + want: false, + }, + { + name: "v1CRD is established & not accepting names - not ready", + crd: builder.ForV1CustomResourceDefinition("MyCRD"). + Condition(builder.ForV1CustomResourceDefinitionCondition().Type(apiextv1.Established).Status(apiextv1.ConditionTrue).Result()).Result(), + want: false, + }, + { + name: "v1CRD is not established & accepting names - not ready", + crd: builder.ForV1CustomResourceDefinition("MyCRD"). + Condition(builder.ForV1CustomResourceDefinitionCondition().Type(apiextv1.NamesAccepted).Status(apiextv1.ConditionTrue).Result()).Result(), + want: false, + }, + { + name: "v1CRD is established & accepting names - ready", + crd: builder.ForV1CustomResourceDefinition("MyCRD"). + Condition(builder.ForV1CustomResourceDefinitionCondition().Type(apiextv1.Established).Status(apiextv1.ConditionTrue).Result()). + Condition(builder.ForV1CustomResourceDefinitionCondition().Type(apiextv1.NamesAccepted).Status(apiextv1.ConditionTrue).Result()). + Result(), + want: true, + }, + } + + for _, tc := range v1tests { + m, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.crd) + require.NoError(t, err) + result, err := IsCRDReady(&unstructured.Unstructured{Object: m}) + require.NoError(t, err) + assert.Equal(t, tc.want, result) + } + + // input parm is unrecognized + resBytes := []byte(` { - "apiVersion": "apiextensions.k8s.io/v1beta1", + "apiVersion": "apiextensions.k8s.io/v9", "kind": "CustomResourceDefinition", "metadata": { - "name": "foos.example.foo.com" + "name": "foos.example.foo.com" }, "spec": { - "group": "example.foo.com", - "version": "v1alpha1", - "scope": "Namespaced", - "names": { - "plural": "foos", - "singular": "foo", - "kind": "Foo" - }, - "validation": { - "openAPIV3Schema": { - "required": [ - "spec" - ], - "properties": { - "spec": { - "required": [ - "bar" - ], - "properties": { - "bar": { - "type": "integer", - "minimum": 1 + "group": "example.foo.com", + "version": "v1alpha1", + "scope": "Namespaced", + "names": { + "plural": "foos", + "singular": "foo", + "kind": "Foo" + }, + "validation": { + "openAPIV3Schema": { + "required": [ + "spec" + ], + "properties": { + "spec": { + "required": [ + "bar" + ], + "properties": { + "bar": { + "type": "integer", + "minimum": 1 + } + } + } } - } } - } } - } } - } +} `) - - var obj unstructured.Unstructured - err := json.Unmarshal(b, &obj) + obj := &unstructured.Unstructured{} + err := json.Unmarshal(resBytes, obj) require.NoError(t, err) - - var newCRD apiextv1beta1.CustomResourceDefinition - err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &newCRD) - // If there's no error, then the upstream issue is fixed, and we need to remove our workarounds. - require.Error(t, err) + _, err = IsCRDReady(obj) + assert.NotNil(t, err) } -*/