From f42026254345e919dc51eff1925c3bc708b3e6b8 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Wed, 13 Oct 2021 10:22:21 +0200 Subject: [PATCH] update --- cmd/clusterctl/client/cluster/components.go | 10 +- cmd/clusterctl/client/cluster/proxy.go | 103 +++++--------------- cmd/clusterctl/internal/test/fake_proxy.go | 9 -- test/e2e/clusterctl_upgrade.go | 7 ++ test/framework/clusterctl/client.go | 26 +++++ 5 files changed, 67 insertions(+), 88 deletions(-) diff --git a/cmd/clusterctl/client/cluster/components.go b/cmd/clusterctl/client/cluster/components.go index 1f840b78f55d..5ff96d5686e2 100644 --- a/cmd/clusterctl/client/cluster/components.go +++ b/cmd/clusterctl/client/cluster/components.go @@ -17,7 +17,6 @@ limitations under the License. package cluster import ( - "context" "fmt" "strings" @@ -28,6 +27,7 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" + clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/util" logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log" @@ -132,7 +132,13 @@ func (p *providerComponents) Delete(options DeleteOptions) error { // Fetch all the components belonging to a provider. // We want that the delete operation is able to clean-up everything. - resources, err := p.proxy.ListProviderResources(context.TODO(), options.Provider.ManifestLabel(), options.Provider.Namespace) + labels := map[string]string{ + clusterctlv1.ClusterctlLabelName: "", + clusterv1.ProviderLabelName: options.Provider.ManifestLabel(), + } + + namespaces := []string{options.Provider.Namespace} + resources, err := p.proxy.ListResources(labels, namespaces...) if err != nil { return err } diff --git a/cmd/clusterctl/client/cluster/proxy.go b/cmd/clusterctl/client/cluster/proxy.go index 99a33cfd95a0..eec7495ec959 100644 --- a/cmd/clusterctl/client/cluster/proxy.go +++ b/cmd/clusterctl/client/cluster/proxy.go @@ -17,7 +17,6 @@ limitations under the License. package cluster import ( - "context" "fmt" "strings" "time" @@ -35,7 +34,6 @@ import ( "k8s.io/client-go/rest" "k8s.io/client-go/tools/clientcmd" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" - clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme" "sigs.k8s.io/cluster-api/version" "sigs.k8s.io/controller-runtime/pkg/client" @@ -62,12 +60,12 @@ type Proxy interface { // CheckClusterAvailable checks if a a cluster is available and reachable. CheckClusterAvailable() error - // ListResources returns all the Kubernetes objects with the given labels existing the listed namespaces. + // ListResources lists namespaced and cluster-wide resources matching the labels. Namespaced resources are only listed + // in the given namespaces. + // If labels contains the ProviderLabelName label, CRDs of other providers are excluded. + // This is done to avoid errors when listing resources of providers which have already been deleted. ListResources(labels map[string]string, namespaces ...string) ([]unstructured.Unstructured, error) - // ListProviderResources lists Kubernetes objects of a specific provider. - ListProviderResources(ctx context.Context, provider, namespace string) ([]unstructured.Unstructured, error) - // GetContexts returns the list of contexts in kubeconfig which begin with prefix. GetContexts(prefix string) ([]string, error) @@ -207,69 +205,18 @@ func (k *proxy) CheckClusterAvailable() error { return nil } -func (k *proxy) ListResources(labelss map[string]string, namespaces ...string) ([]unstructured.Unstructured, error) { - return k.list(context.TODO(), &listOptions{Labels: labelss, Namespaces: namespaces}) -} - -func (k *proxy) ListProviderResources(ctx context.Context, provider, namespace string) ([]unstructured.Unstructured, error) { - return k.list(ctx, &listOptions{ - Labels: map[string]string{ - clusterctlv1.ClusterctlLabelName: "", - clusterv1.ProviderLabelName: provider, - }, - Namespaces: []string{namespace}, - ExcludeCRD: func(crd apiextensionsv1.CustomResourceDefinition) bool { - if v, ok := crd.Labels[clusterv1.ProviderLabelName]; ok && v != provider { - return true - } - return false - }, - }) -} - -// listOption is some configuration that modifies options for a list request. -type listOption interface { - // ApplyToList applies this configuration to the given list options. - ApplyToList(*listOptions) -} - -type listOptions struct { - // Labels filters resources by labels. - Labels map[string]string - - // Namespaces represents the namespaces to list for. - Namespaces []string - - // ExcludeCRD excludes resources of CRDs if the func returns true for them. - ExcludeCRD func(definition apiextensionsv1.CustomResourceDefinition) bool -} - -// ApplyToList implements listOption for listOptions. -func (o *listOptions) ApplyToList(lo *listOptions) { - if o.Labels != nil { - lo.Labels = o.Labels - } - if o.Namespaces != nil { - lo.Namespaces = o.Namespaces - } - if o.ExcludeCRD != nil { - lo.ExcludeCRD = o.ExcludeCRD - } -} - -// ApplyOptions applies the given list options on these options, -// and then returns itself (for convenient chaining). -func (o *listOptions) ApplyOptions(opts []listOption) *listOptions { - for _, opt := range opts { - opt.ApplyToList(o) - } - return o -} - -func (k *proxy) list(ctx context.Context, opts ...listOption) ([]unstructured.Unstructured, error) { - listOpts := listOptions{} - listOpts.ApplyOptions(opts) - +// ListResources lists namespaced and cluster-wide resources matching the labels. Namespaced resources are only listed +// in the given namespaces. +// If labels contains the ProviderLabelName label, CRDs of other providers are excluded. +// This is done to avoid errors when listing resources of providers which have already been deleted. +// For example: +// * The AWS provider has already been deleted, but there are still cluster-wide resources of AWSClusterControllerIdentity. +// * The AWSClusterControllerIdentity resources are still stored in an older version (e.g. v1alpha4, when the preferred +// version is v1beta1) +// * If we now want to delete e.g. the kubeadm bootstrap provider, we cannot list AWSClusterControllerIdentity resources +// as the conversion would fail, because the AWS controller hosting the conversion webhook has already been deleted. +// * Thus we exclude resources of other providers if we detect that ListResources is called to list resources of a provider. +func (k *proxy) ListResources(labels map[string]string, namespaces ...string) ([]unstructured.Unstructured, error) { cs, err := k.newClientSet() if err != nil { return nil, err @@ -290,19 +237,20 @@ func (k *proxy) list(ctx context.Context, opts ...listOption) ([]unstructured.Un return nil, errors.Wrap(err, "failed to list api resources") } + // If labels indicates that resources of a specific provider should be listed, exclude CRDs of other providers. crdsToExclude := sets.String{} - if listOpts.ExcludeCRD != nil { - // Get all CRDs in the cluster. - crdListBackoff := newReadBackoff() + if providerName, ok := labels[clusterv1.ProviderLabelName]; ok { + // List all CRDs in the cluster. crdList := &apiextensionsv1.CustomResourceDefinitionList{} - if err := retryWithExponentialBackoff(crdListBackoff, func() error { + if err := retryWithExponentialBackoff(newReadBackoff(), func() error { return c.List(ctx, crdList) }); err != nil { return nil, errors.Wrap(err, "failed to list CRDs") } + // Exclude CRDs of other providers. for _, crd := range crdList.Items { - if listOpts.ExcludeCRD(crd) { + if v, ok := crd.Labels[clusterv1.ProviderLabelName]; ok && v != providerName { for _, version := range crd.Spec.Versions { crdsToExclude.Insert(metav1.GroupVersionKind{ Group: crd.Spec.Group, @@ -326,6 +274,7 @@ func (k *proxy) list(ctx context.Context, opts ...listOption) ([]unstructured.Un continue } + // Continue if the resource is an excluded CRD. gv, err := schema.ParseGroupVersion(resourceGroup.GroupVersion) if err != nil { return nil, errors.Wrapf(err, "failed to parse GroupVersion") @@ -340,15 +289,15 @@ func (k *proxy) list(ctx context.Context, opts ...listOption) ([]unstructured.Un // List all the object instances of this resourceKind with the given labels if resourceKind.Namespaced { - for _, namespace := range listOpts.Namespaces { - objList, err := listObjByGVK(c, resourceGroup.GroupVersion, resourceKind.Kind, []client.ListOption{client.MatchingLabels(listOpts.Labels), client.InNamespace(namespace)}) + for _, namespace := range namespaces { + objList, err := listObjByGVK(c, resourceGroup.GroupVersion, resourceKind.Kind, []client.ListOption{client.MatchingLabels(labels), client.InNamespace(namespace)}) if err != nil { return nil, err } ret = append(ret, objList.Items...) } } else { - objList, err := listObjByGVK(c, resourceGroup.GroupVersion, resourceKind.Kind, []client.ListOption{client.MatchingLabels(listOpts.Labels)}) + objList, err := listObjByGVK(c, resourceGroup.GroupVersion, resourceKind.Kind, []client.ListOption{client.MatchingLabels(labels)}) if err != nil { return nil, err } diff --git a/cmd/clusterctl/internal/test/fake_proxy.go b/cmd/clusterctl/internal/test/fake_proxy.go index ae207153ee87..e9aaedf8cfb4 100644 --- a/cmd/clusterctl/internal/test/fake_proxy.go +++ b/cmd/clusterctl/internal/test/fake_proxy.go @@ -17,7 +17,6 @@ limitations under the License. package test import ( - "context" "errors" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -136,14 +135,6 @@ func (f *FakeProxy) ListResources(labels map[string]string, namespaces ...string return ret, nil } -func (f *FakeProxy) ListProviderResources(_ context.Context, provider, namespace string) ([]unstructured.Unstructured, error) { - // TODO: this func should filter out objects of CRDs of other providers - return f.ListResources(map[string]string{ - clusterctlv1.ClusterctlLabelName: "", - clusterv1.ProviderLabelName: provider, - }, namespace) -} - func (f *FakeProxy) GetContexts(prefix string) ([]string, error) { return nil, nil } diff --git a/test/e2e/clusterctl_upgrade.go b/test/e2e/clusterctl_upgrade.go index 19e7f9f12729..b9952d3330bb 100644 --- a/test/e2e/clusterctl_upgrade.go +++ b/test/e2e/clusterctl_upgrade.go @@ -319,6 +319,13 @@ func ClusterctlUpgradeSpec(ctx context.Context, inputGetter func() ClusterctlUpg By("THE UPGRADED MANAGEMENT CLUSTER WORKS!") + By("Deleting providers") + clusterctl.Delete(ctx, clusterctl.DeleteInput{ + LogFolder: filepath.Join(input.ArtifactFolder, "clusters", cluster.Name), + ClusterctlConfigPath: input.ClusterctlConfigPath, + KubeconfigPath: managementClusterProxy.GetKubeconfigPath(), + }) + By("PASSED!") }) diff --git a/test/framework/clusterctl/client.go b/test/framework/clusterctl/client.go index 80fa7ce036a3..f306a551f48b 100644 --- a/test/framework/clusterctl/client.go +++ b/test/framework/clusterctl/client.go @@ -137,6 +137,32 @@ func Upgrade(ctx context.Context, input UpgradeInput) { Expect(err).ToNot(HaveOccurred(), "failed to run clusterctl upgrade") } +// DeleteInput is the input for Delete. +type DeleteInput struct { + LogFolder string + ClusterctlConfigPath string + KubeconfigPath string +} + +// Delete calls clusterctl delete --all. +func Delete(ctx context.Context, input DeleteInput) { + log.Logf("clusterctl delete --all") + + deleteOpts := clusterctlclient.DeleteOptions{ + Kubeconfig: clusterctlclient.Kubeconfig{ + Path: input.KubeconfigPath, + Context: "", + }, + DeleteAll: true, + } + + clusterctlClient, log := getClusterctlClientWithLogger(input.ClusterctlConfigPath, "clusterctl-delete.log", input.LogFolder) + defer log.Close() + + err := clusterctlClient.Delete(deleteOpts) + Expect(err).ToNot(HaveOccurred(), "failed to run clusterctl upgrade") +} + // ConfigClusterInput is the input for ConfigCluster. type ConfigClusterInput struct { LogFolder string