From d98c65f69e2cd75a0763c22de9cce93aa8d401f0 Mon Sep 17 00:00:00 2001 From: Bridget McErlean Date: Thu, 5 Aug 2021 15:17:14 -0400 Subject: [PATCH] Use appropriate CRD API during readiness check (#4015) * Use appropriate CRD API during readiness check The readiness check for the Velero CRDs was still using the v1beta1 API. This would cause the readiness check to fail on 1.22 clusters as the v1beta1 API is no longer available. Previously, this error would be ignored and the installation would proceed, however with #4002, we are no longer ignoring errors from this check. This change modifies the CRD readiness check to check the CRDs using the same API version that was used when submitting the CRDs to the cluster. It also introduces a new CRD builder using the V1 API for testing. This change also fixes a bug that was identified in the polling code where if the CRDs were not ready on the first polling iteration, they would be added again to the list of CRDs to check resulting in duplicates. This would cause the length check to fail on all subsequent polls and the timeout would always be reached. Signed-off-by: Bridget McErlean * Remove duplicate V1 CRD builder and update comment Signed-off-by: Bridget McErlean --- changelogs/unreleased/4015-zubron | 1 + pkg/backup/backup_test.go | 38 +++--- pkg/backup/remap_crd_version_action_test.go | 4 +- .../customresourcedefinition_builder.go | 91 ------------- ...ustomresourcedefinition_v1beta1_builder.go | 91 +++++++++++++ pkg/cmd/cli/install/install.go | 12 +- pkg/install/install.go | 120 ++++++++++++------ pkg/util/kube/utils.go | 27 +++- pkg/util/kube/utils_test.go | 78 +++++++++--- test/e2e/velero_utils.go | 2 +- 10 files changed, 284 insertions(+), 180 deletions(-) create mode 100644 changelogs/unreleased/4015-zubron delete mode 100644 pkg/builder/customresourcedefinition_builder.go create mode 100644 pkg/builder/customresourcedefinition_v1beta1_builder.go diff --git a/changelogs/unreleased/4015-zubron b/changelogs/unreleased/4015-zubron new file mode 100644 index 000000000..937ba33da --- /dev/null +++ b/changelogs/unreleased/4015-zubron @@ -0,0 +1 @@ +Use the cluster preferred CRD API version when polling for Velero CRD readiness. \ No newline at end of file diff --git a/pkg/backup/backup_test.go b/pkg/backup/backup_test.go index 63d334a18..351c60429 100644 --- a/pkg/backup/backup_test.go +++ b/pkg/backup/backup_test.go @@ -1,5 +1,5 @@ /* -Copyright 2020 the Velero contributors. +Copyright the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -767,9 +767,9 @@ func TestCRDInclusion(t *testing.T) { Result(), apiResources: []*test.APIResource{ test.CRDs( - builder.ForCustomResourceDefinition("backups.velero.io").Result(), - builder.ForCustomResourceDefinition("volumesnapshotlocations.velero.io").Result(), - builder.ForCustomResourceDefinition("test.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("backups.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("volumesnapshotlocations.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("test.velero.io").Result(), ), test.VSLs( builder.ForVolumeSnapshotLocation("foo", "vsl-1").Result(), @@ -793,9 +793,9 @@ func TestCRDInclusion(t *testing.T) { Result(), apiResources: []*test.APIResource{ test.CRDs( - builder.ForCustomResourceDefinition("backups.velero.io").Result(), - builder.ForCustomResourceDefinition("volumesnapshotlocations.velero.io").Result(), - builder.ForCustomResourceDefinition("test.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("backups.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("volumesnapshotlocations.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("test.velero.io").Result(), ), test.VSLs( builder.ForVolumeSnapshotLocation("foo", "vsl-1").Result(), @@ -813,9 +813,9 @@ func TestCRDInclusion(t *testing.T) { Result(), apiResources: []*test.APIResource{ test.CRDs( - builder.ForCustomResourceDefinition("backups.velero.io").Result(), - builder.ForCustomResourceDefinition("volumesnapshotlocations.velero.io").Result(), - builder.ForCustomResourceDefinition("test.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("backups.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("volumesnapshotlocations.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("test.velero.io").Result(), ), test.VSLs( builder.ForVolumeSnapshotLocation("foo", "vsl-1").Result(), @@ -839,9 +839,9 @@ func TestCRDInclusion(t *testing.T) { Result(), apiResources: []*test.APIResource{ test.CRDs( - builder.ForCustomResourceDefinition("backups.velero.io").Result(), - builder.ForCustomResourceDefinition("volumesnapshotlocations.velero.io").Result(), - builder.ForCustomResourceDefinition("test.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("backups.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("volumesnapshotlocations.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("test.velero.io").Result(), ), test.VSLs( builder.ForVolumeSnapshotLocation("foo", "vsl-1").Result(), @@ -862,9 +862,9 @@ func TestCRDInclusion(t *testing.T) { Result(), apiResources: []*test.APIResource{ test.CRDs( - builder.ForCustomResourceDefinition("backups.velero.io").Result(), - builder.ForCustomResourceDefinition("volumesnapshotlocations.velero.io").Result(), - builder.ForCustomResourceDefinition("test.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("backups.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("volumesnapshotlocations.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("test.velero.io").Result(), ), test.VSLs( builder.ForVolumeSnapshotLocation("foo", "vsl-1").Result(), @@ -883,9 +883,9 @@ func TestCRDInclusion(t *testing.T) { Result(), apiResources: []*test.APIResource{ test.CRDs( - builder.ForCustomResourceDefinition("backups.velero.io").Result(), - builder.ForCustomResourceDefinition("volumesnapshotlocations.velero.io").Result(), - builder.ForCustomResourceDefinition("test.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("backups.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("volumesnapshotlocations.velero.io").Result(), + builder.ForCustomResourceDefinitionV1Beta1("test.velero.io").Result(), ), test.VSLs( builder.ForVolumeSnapshotLocation("foo", "vsl-1").Result(), diff --git a/pkg/backup/remap_crd_version_action_test.go b/pkg/backup/remap_crd_version_action_test.go index 0a58fb08a..9308ea049 100644 --- a/pkg/backup/remap_crd_version_action_test.go +++ b/pkg/backup/remap_crd_version_action_test.go @@ -1,5 +1,5 @@ /* -Copyright 2020 the Velero contributors. +Copyright the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -44,7 +44,7 @@ func TestRemapCRDVersionAction(t *testing.T) { // build a v1beta1 CRD with the same name and add it to the fake client that the plugin is going to call. // keep the same one for all 3 tests, since there's little value in recreating it - b := builder.ForCustomResourceDefinition("test.velero.io") + b := builder.ForCustomResourceDefinitionV1Beta1("test.velero.io") c := b.Result() _, err := betaClient.Create(context.TODO(), c, metav1.CreateOptions{}) require.NoError(t, err) diff --git a/pkg/builder/customresourcedefinition_builder.go b/pkg/builder/customresourcedefinition_builder.go deleted file mode 100644 index 847492884..000000000 --- a/pkg/builder/customresourcedefinition_builder.go +++ /dev/null @@ -1,91 +0,0 @@ -/* -Copyright 2019 the Velero contributors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package builder - -import ( - apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -// CustomResourceDefinitionBuilder builds CustomResourceDefinition objects. -type CustomResourceDefinitionBuilder struct { - object *apiextv1beta1.CustomResourceDefinition -} - -// ForCustomResourceDefinition is the constructor for a CustomResourceDefinitionBuilder. -func ForCustomResourceDefinition(name string) *CustomResourceDefinitionBuilder { - return &CustomResourceDefinitionBuilder{ - object: &apiextv1beta1.CustomResourceDefinition{ - TypeMeta: metav1.TypeMeta{ - APIVersion: apiextv1beta1.SchemeGroupVersion.String(), - Kind: "CustomResourceDefinition", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: name, - }, - }, - } -} - -// Condition adds a CustomResourceDefinitionCondition objects to a CustomResourceDefinitionBuilder. -func (c *CustomResourceDefinitionBuilder) Condition(cond apiextv1beta1.CustomResourceDefinitionCondition) *CustomResourceDefinitionBuilder { - c.object.Status.Conditions = append(c.object.Status.Conditions, cond) - return c -} - -// Result returns the built CustomResourceDefinition. -func (b *CustomResourceDefinitionBuilder) Result() *apiextv1beta1.CustomResourceDefinition { - return b.object -} - -// ObjectMeta applies functional options to the CustomResourceDefinition's ObjectMeta. -func (b *CustomResourceDefinitionBuilder) ObjectMeta(opts ...ObjectMetaOpt) *CustomResourceDefinitionBuilder { - for _, opt := range opts { - opt(b.object) - } - - return b -} - -// CustomResourceDefinitionConditionBuilder builds CustomResourceDefinitionCondition objects. -type CustomResourceDefinitionConditionBuilder struct { - object apiextv1beta1.CustomResourceDefinitionCondition -} - -// ForCustomResourceDefinitionConditionBuilder is the construction for a CustomResourceDefinitionConditionBuilder. -func ForCustomResourceDefinitionCondition() *CustomResourceDefinitionConditionBuilder { - return &CustomResourceDefinitionConditionBuilder{ - object: apiextv1beta1.CustomResourceDefinitionCondition{}, - } -} - -// Type sets the Condition's type. -func (c *CustomResourceDefinitionConditionBuilder) Type(t apiextv1beta1.CustomResourceDefinitionConditionType) *CustomResourceDefinitionConditionBuilder { - c.object.Type = t - return c -} - -// Status sets the Condition's status. -func (c *CustomResourceDefinitionConditionBuilder) Status(cs apiextv1beta1.ConditionStatus) *CustomResourceDefinitionConditionBuilder { - c.object.Status = cs - return c -} - -// Results returns the built CustomResourceDefinitionCondition. -func (b *CustomResourceDefinitionConditionBuilder) Result() apiextv1beta1.CustomResourceDefinitionCondition { - return b.object -} diff --git a/pkg/builder/customresourcedefinition_v1beta1_builder.go b/pkg/builder/customresourcedefinition_v1beta1_builder.go new file mode 100644 index 000000000..d5ab9b248 --- /dev/null +++ b/pkg/builder/customresourcedefinition_v1beta1_builder.go @@ -0,0 +1,91 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package builder + +import ( + apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// CustomResourceDefinitionV1Beta1Builder builds v1beta1 CustomResourceDefinition objects. +type CustomResourceDefinitionV1Beta1Builder struct { + object *apiextv1beta1.CustomResourceDefinition +} + +// ForCustomResourceDefinitionV1Beta1 is the constructor for a CustomResourceDefinitionV1Beta1Builder. +func ForCustomResourceDefinitionV1Beta1(name string) *CustomResourceDefinitionV1Beta1Builder { + return &CustomResourceDefinitionV1Beta1Builder{ + object: &apiextv1beta1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: apiextv1beta1.SchemeGroupVersion.String(), + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + }, + } +} + +// Condition adds a CustomResourceDefinitionCondition objects to a CustomResourceDefinitionV1Beta1Builder. +func (c *CustomResourceDefinitionV1Beta1Builder) Condition(cond apiextv1beta1.CustomResourceDefinitionCondition) *CustomResourceDefinitionV1Beta1Builder { + c.object.Status.Conditions = append(c.object.Status.Conditions, cond) + return c +} + +// Result returns the built CustomResourceDefinition. +func (b *CustomResourceDefinitionV1Beta1Builder) Result() *apiextv1beta1.CustomResourceDefinition { + return b.object +} + +// ObjectMeta applies functional options to the CustomResourceDefinition's ObjectMeta. +func (b *CustomResourceDefinitionV1Beta1Builder) ObjectMeta(opts ...ObjectMetaOpt) *CustomResourceDefinitionV1Beta1Builder { + for _, opt := range opts { + opt(b.object) + } + + return b +} + +// CustomResourceDefinitionV1Beta1ConditionBuilder builds CustomResourceDefinitionV1Beta1Condition objects. +type CustomResourceDefinitionV1Beta1ConditionBuilder struct { + object apiextv1beta1.CustomResourceDefinitionCondition +} + +// ForCustomResourceDefinitionV1Beta1Condition is the construction for a CustomResourceDefinitionV1Beta1ConditionBuilder. +func ForCustomResourceDefinitionV1Beta1Condition() *CustomResourceDefinitionV1Beta1ConditionBuilder { + return &CustomResourceDefinitionV1Beta1ConditionBuilder{ + object: apiextv1beta1.CustomResourceDefinitionCondition{}, + } +} + +// Type sets the Condition's type. +func (c *CustomResourceDefinitionV1Beta1ConditionBuilder) Type(t apiextv1beta1.CustomResourceDefinitionConditionType) *CustomResourceDefinitionV1Beta1ConditionBuilder { + c.object.Type = t + return c +} + +// Status sets the Condition's status. +func (c *CustomResourceDefinitionV1Beta1ConditionBuilder) Status(cs apiextv1beta1.ConditionStatus) *CustomResourceDefinitionV1Beta1ConditionBuilder { + c.object.Status = cs + return c +} + +// Result returns the built v1beta1 CustomResourceDefinitionCondition. +func (c *CustomResourceDefinitionV1Beta1ConditionBuilder) Result() apiextv1beta1.CustomResourceDefinitionCondition { + return c.object +} diff --git a/pkg/cmd/cli/install/install.go b/pkg/cmd/cli/install/install.go index d07051f5e..f14c8725c 100644 --- a/pkg/cmd/cli/install/install.go +++ b/pkg/cmd/cli/install/install.go @@ -298,24 +298,28 @@ func (o *InstallOptions) Run(c *cobra.Command, f client.Factory) error { if err != nil { return err } - factory := client.NewDynamicFactory(dynamicClient) + dynamicFactory := client.NewDynamicFactory(dynamicClient) + kbClient, err := f.KubebuilderClient() + if err != nil { + return err + } errorMsg := fmt.Sprintf("\n\nError installing Velero. Use `kubectl logs deploy/velero -n %s` to check the deploy logs", o.Namespace) - err = install.Install(factory, resources, os.Stdout) + err = install.Install(dynamicFactory, kbClient, resources, os.Stdout) if err != nil { return errors.Wrap(err, errorMsg) } if o.Wait { fmt.Println("Waiting for Velero deployment to be ready.") - if _, err = install.DeploymentIsReady(factory, o.Namespace); err != nil { + if _, err = install.DeploymentIsReady(dynamicFactory, o.Namespace); err != nil { return errors.Wrap(err, errorMsg) } if o.UseRestic { fmt.Println("Waiting for Velero restic daemonset to be ready.") - if _, err = install.DaemonSetIsReady(factory, o.Namespace); err != nil { + if _, err = install.DaemonSetIsReady(dynamicFactory, o.Namespace); err != nil { return errors.Wrap(err, errorMsg) } } diff --git a/pkg/install/install.go b/pkg/install/install.go index 5a9996303..85257f4ad 100644 --- a/pkg/install/install.go +++ b/pkg/install/install.go @@ -1,5 +1,5 @@ /* -Copyright 2019 the Velero contributors. +Copyright the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -17,6 +17,7 @@ limitations under the License. package install import ( + "context" "fmt" "io" "strings" @@ -25,6 +26,7 @@ import ( "github.com/pkg/errors" appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -32,6 +34,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/wait" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" "github.com/vmware-tanzu/velero/pkg/client" "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -57,56 +60,95 @@ type ResourceGroup struct { OtherResources []*unstructured.Unstructured } -// crdsAreReady polls the API server to see if the BackupStorageLocation and VolumeSnapshotLocation CRDs are ready to create objects. -func crdsAreReady(factory client.DynamicFactory, crdKinds []string) (bool, error) { - gvk := schema.FromAPIVersionAndKind(apiextv1beta1.SchemeGroupVersion.String(), "CustomResourceDefinition") - apiResource := metav1.APIResource{ - Name: kindToResource["CustomResourceDefinition"], - Namespaced: false, - } - c, err := factory.ClientForGroupVersionResource(gvk.GroupVersion(), apiResource, "") - if err != nil { - return false, errors.Wrapf(err, "Error creating client for CustomResourceDefinition polling") - } - // Track all the CRDs that have been found and successfully marshalled. - // len should be equal to len(crdKinds) in the happy path. - foundCRDs := make([]*apiextv1beta1.CustomResourceDefinition, 0) - var areReady bool - err = wait.PollImmediate(time.Second, time.Minute, func() (bool, error) { - for _, k := range crdKinds { - unstruct, err := c.Get(k, metav1.GetOptions{}) +// crdV1Beta1ReadinessFn returns a function that can be used for polling to check +// if the provided unstructured v1beta1 CRDs are ready for use in the cluster. +func crdV1Beta1ReadinessFn(kbClient kbclient.Client, unstructuredCrds []*unstructured.Unstructured) func() (bool, error) { + // Track all the CRDs that have been found and in ready state. + // len should be equal to len(unstructuredCrds) in the happy path. + return func() (bool, error) { + foundCRDs := make([]*apiextv1beta1.CustomResourceDefinition, 0) + for _, unstructuredCrd := range unstructuredCrds { + crd := &apiextv1beta1.CustomResourceDefinition{} + key := kbclient.ObjectKey{Name: unstructuredCrd.GetName()} + err := kbClient.Get(context.Background(), key, crd) if apierrors.IsNotFound(err) { return false, nil } else if err != nil { - return false, errors.Wrapf(err, "error waiting for %s to be ready", k) + return false, errors.Wrapf(err, "error waiting for %s to be ready", crd.GetName()) } - - crd := new(apiextv1beta1.CustomResourceDefinition) - if err := runtime.DefaultUnstructuredConverter.FromUnstructured(unstruct.Object, crd); err != nil { - return false, errors.Wrapf(err, "error converting %s from unstructured", k) - } - foundCRDs = append(foundCRDs, crd) } - if len(foundCRDs) != len(crdKinds) { + if len(foundCRDs) != len(unstructuredCrds) { return false, nil } for _, crd := range foundCRDs { - if !kube.IsCRDReady(crd) { + ready := kube.IsV1Beta1CRDReady(crd) + if !ready { return false, nil } - } - areReady = true - return true, nil - }) - if err != nil { - return false, errors.Wrap(err, "Error polling for CRD") } - return areReady, nil +} + +// crdV1ReadinessFn returns a function that can be used for polling to check +// if the provided unstructured v1 CRDs are ready for use in the cluster. +func crdV1ReadinessFn(kbClient kbclient.Client, unstructuredCrds []*unstructured.Unstructured) func() (bool, error) { + return func() (bool, error) { + foundCRDs := make([]*apiextv1.CustomResourceDefinition, 0) + for _, unstructuredCrd := range unstructuredCrds { + crd := &apiextv1.CustomResourceDefinition{} + key := kbclient.ObjectKey{Name: unstructuredCrd.GetName()} + err := kbClient.Get(context.Background(), key, crd) + if apierrors.IsNotFound(err) { + return false, nil + } else if err != nil { + return false, errors.Wrapf(err, "error waiting for %s to be ready", crd.GetName()) + } + foundCRDs = append(foundCRDs, crd) + } + + if len(foundCRDs) != len(unstructuredCrds) { + return false, nil + } + + for _, crd := range foundCRDs { + ready := kube.IsV1CRDReady(crd) + if !ready { + return false, nil + } + } + return true, nil + } +} + +// crdsAreReady polls the API server to see if the Velero CRDs are ready to create objects. +func crdsAreReady(kbClient kbclient.Client, crds []*unstructured.Unstructured) (bool, error) { + if len(crds) == 0 { + // no CRDs to check so return + return true, nil + } + + // We assume that all Velero CRDs have the same GVK so we can use the GVK of the + // first CRD to determine whether to use the v1beta1 or v1 API during polling. + gvk := crds[0].GroupVersionKind() + + var crdReadinessFn func() (bool, error) + if gvk.Version == "v1beta1" { + crdReadinessFn = crdV1Beta1ReadinessFn(kbClient, crds) + } else if gvk.Version == "v1" { + crdReadinessFn = crdV1ReadinessFn(kbClient, crds) + } else { + return false, fmt.Errorf("unsupported CRD version %q", gvk.Version) + } + + err := wait.PollImmediate(time.Second, time.Minute, crdReadinessFn) + if err != nil { + return false, errors.Wrap(err, "Error polling for CRDs") + } + return true, nil } func isAvailable(c appsv1.DeploymentCondition) bool { @@ -285,19 +327,19 @@ func CreateClient(r *unstructured.Unstructured, factory client.DynamicFactory, w // Resources will be sorted into CustomResourceDefinitions and any other resource type, and the function will wait up to 1 minute // for CRDs to be ready before proceeding. // An io.Writer can be used to output to a log or the console. -func Install(factory client.DynamicFactory, resources *unstructured.UnstructuredList, w io.Writer) error { +func Install(dynamicFactory client.DynamicFactory, kbClient kbclient.Client, resources *unstructured.UnstructuredList, w io.Writer) error { rg := GroupResources(resources) //Install CRDs first for _, r := range rg.CRDResources { - if err := createResource(r, factory, w); err != nil { + if err := createResource(r, dynamicFactory, w); err != nil { return err } } // Wait for CRDs to be ready before proceeding fmt.Fprint(w, "Waiting for resources to be ready in cluster...\n") - _, err := crdsAreReady(factory, []string{"backupstoragelocations.velero.io", "volumesnapshotlocations.velero.io"}) + _, err := crdsAreReady(kbClient, rg.CRDResources) if err == wait.ErrWaitTimeout { return errors.Errorf("timeout reached, CRDs not ready") } else if err != nil { @@ -306,7 +348,7 @@ func Install(factory client.DynamicFactory, resources *unstructured.Unstructured // Install all other resources for _, r := range rg.OtherResources { - if err = createResource(r, factory, w); err != nil { + if err = createResource(r, dynamicFactory, w); err != nil { return err } } diff --git a/pkg/util/kube/utils.go b/pkg/util/kube/utils.go index 994e8f370..a80f864de 100644 --- a/pkg/util/kube/utils.go +++ b/pkg/util/kube/utils.go @@ -1,5 +1,5 @@ /* -Copyright 2017, 2019 the Velero contributors. +Copyright the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -23,6 +23,7 @@ import ( "github.com/pkg/errors" corev1api "k8s.io/api/core/v1" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -145,8 +146,23 @@ func GetVolumeDirectory(pod *corev1api.Pod, volumeName string, pvcLister corev1l return pvc.Spec.VolumeName, nil } -// IsCRDReady checks a CRD to see if it's ready, with both the Established and NamesAccepted conditions. -func IsCRDReady(crd *apiextv1beta1.CustomResourceDefinition) bool { +// IsV1CRDReady checks a v1 CRD to see if it's ready, with both the Established and NamesAccepted conditions. +func IsV1CRDReady(crd *apiextv1.CustomResourceDefinition) bool { + var isEstablished, namesAccepted bool + for _, cond := range crd.Status.Conditions { + if cond.Type == apiextv1.Established && cond.Status == apiextv1.ConditionTrue { + isEstablished = true + } + if cond.Type == apiextv1.NamesAccepted && cond.Status == apiextv1.ConditionTrue { + namesAccepted = true + } + } + + return (isEstablished && namesAccepted) +} + +// IsV1Beta1CRDReady checks a v1beta1 CRD to see if it's ready, with both the Established and NamesAccepted conditions. +func IsV1Beta1CRDReady(crd *apiextv1beta1.CustomResourceDefinition) bool { var isEstablished, namesAccepted bool for _, cond := range crd.Status.Conditions { if cond.Type == apiextv1beta1.Established && cond.Status == apiextv1beta1.ConditionTrue { @@ -161,8 +177,8 @@ func IsCRDReady(crd *apiextv1beta1.CustomResourceDefinition) bool { } // IsUnstructuredCRDReady checks an unstructured CRD to see if it's ready, with both the Established and NamesAccepted conditions. -// TODO: Delete this function and use IsCRDReady when the upstream runtime.FromUnstructured function properly handles int64 field conversions. -// Duplicated function because the velero install package uses IsCRDReady with the beta types. +// 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 @@ -207,6 +223,7 @@ func IsUnstructuredCRDReady(crd *unstructured.Unstructured) (bool, error) { // 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 } diff --git a/pkg/util/kube/utils_test.go b/pkg/util/kube/utils_test.go index d72f99404..cb9d2c79c 100644 --- a/pkg/util/kube/utils_test.go +++ b/pkg/util/kube/utils_test.go @@ -1,5 +1,5 @@ /* -Copyright 2017 the Velero contributors. +Copyright the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -25,6 +25,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apiextv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -210,7 +211,7 @@ func TestGetVolumeDirectorySuccess(t *testing.T) { } } -func TestIsCRDReady(t *testing.T) { +func TestIsV1Beta1CRDReady(t *testing.T) { tests := []struct { name string crd *apiextv1beta1.CustomResourceDefinition @@ -218,33 +219,72 @@ func TestIsCRDReady(t *testing.T) { }{ { name: "CRD is not established & not accepting names - not ready", - crd: builder.ForCustomResourceDefinition("MyCRD").Result(), + crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD").Result(), want: false, }, { name: "CRD is established & not accepting names - not ready", - crd: builder.ForCustomResourceDefinition("MyCRD"). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()).Result(), + 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", - crd: builder.ForCustomResourceDefinition("MyCRD"). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()).Result(), + crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD"). + Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()).Result(), want: false, }, { name: "CRD is established & accepting names - ready", - crd: builder.ForCustomResourceDefinition("MyCRD"). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()). + crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD"). + Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()). + Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()). Result(), want: true, }, } for _, tc := range tests { - result := IsCRDReady(tc.crd) + result := IsV1Beta1CRDReady(tc.crd) + assert.Equal(t, tc.want, result) + } +} + +func TestIsV1CRDReady(t *testing.T) { + tests := []struct { + name string + crd *apiextv1.CustomResourceDefinition + want bool + }{ + { + name: "CRD is not established & not accepting names - not ready", + crd: builder.ForV1CustomResourceDefinition("MyCRD").Result(), + want: false, + }, + { + name: "CRD 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: "CRD 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: "CRD 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 tests { + result := IsV1CRDReady(tc.crd) assert.Equal(t, tc.want, result) } } @@ -257,26 +297,26 @@ func TestIsUnstructuredCRDReady(t *testing.T) { }{ { name: "CRD is not established & not accepting names - not ready", - crd: builder.ForCustomResourceDefinition("MyCRD").Result(), + crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD").Result(), want: false, }, { name: "CRD is established & not accepting names - not ready", - crd: builder.ForCustomResourceDefinition("MyCRD"). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()).Result(), + 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", - crd: builder.ForCustomResourceDefinition("MyCRD"). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()).Result(), + crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD"). + Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()).Result(), want: false, }, { name: "CRD is established & accepting names - ready", - crd: builder.ForCustomResourceDefinition("MyCRD"). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()). - Condition(builder.ForCustomResourceDefinitionCondition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()). + crd: builder.ForCustomResourceDefinitionV1Beta1("MyCRD"). + Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.Established).Status(apiextv1beta1.ConditionTrue).Result()). + Condition(builder.ForCustomResourceDefinitionV1Beta1Condition().Type(apiextv1beta1.NamesAccepted).Status(apiextv1beta1.ConditionTrue).Result()). Result(), want: true, }, diff --git a/test/e2e/velero_utils.go b/test/e2e/velero_utils.go index e8e753b8c..dabfedfc3 100644 --- a/test/e2e/velero_utils.go +++ b/test/e2e/velero_utils.go @@ -121,7 +121,7 @@ func installVeleroServer(io *cliinstall.InstallOptions, registryCredentialFile s } } - err = install.Install(client.dynamicFactory, resources, os.Stdout) + err = install.Install(client.dynamicFactory, client.kubebuilder, resources, os.Stdout) if err != nil { return errors.Wrap(err, errorMsg) }