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.

Sign up for GitHub

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

🐛clusterctl discovery should ignore provider's resources #5684

Merged
merged 1 commit into from
Nov 19, 2021
Merged

🐛clusterctl discovery should ignore provider's resources #5684

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 29 additions & 25 deletions cmd/clusterctl/client/cluster/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ 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"
Expand All @@ -60,10 +61,12 @@ type Proxy interface {
// CheckClusterAvailable checks if a a cluster is available and reachable.
CheckClusterAvailable() error

// ListResources lists namespaced and cluster-wide resources matching the labels. Namespaced resources are only listed
// ListResources lists namespaced and cluster-wide resources for a component 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.
// Please note that we are not returning resources for the component's CRD (e.g. we are not returning
// Certificates for cert-manager, Clusters for CAPI, AWSCluster for CAPA and so on).
// This is done to avoid errors when listing resources of providers which have already been deleted/scaled down to 0 replicas/with
// malfunctioning webhooks.
ListResources(labels map[string]string, namespaces ...string) ([]unstructured.Unstructured, error)

// GetContexts returns the list of contexts in kubeconfig which begin with prefix.
Expand Down Expand Up @@ -205,10 +208,12 @@ func (k *proxy) CheckClusterAvailable() error {
return nil
}

// ListResources lists namespaced and cluster-wide resources matching the labels. Namespaced resources are only listed
// ListResources lists namespaced and cluster-wide resources for a component 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.
// Please note that we are not returning resources for the component's CRD (e.g. we are not returning
// Certificates for cert-manager, Clusters for CAPI, AWSCluster for CAPA and so on).
// This is done to avoid errors when listing resources of providers which have already been deleted/scaled down to 0 replicas/with
// malfunctioning webhooks.
// 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
Expand Down Expand Up @@ -237,27 +242,26 @@ func (k *proxy) ListResources(labels map[string]string, namespaces ...string) ([
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.
// Exclude from discovery the objects from the cert-manager/provider's CRDs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To confirm my understanding.

We're calling this func in two cases:

  • certManagerClient.EnsureLatestVersion
    • in this case we're not listing any CRDs
  • providerComponents.Delete
    • in this case we're not listing any provider CRDs, i.e. we still list Certificate and CertificateRequest (which is what we want)

If I'm correct, maybe we should adjust the godocs of the func slightly, as they could also be interpreted differently (i.e. that the func is only not returning resources of the CRDs of the current component)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

~ right
In both cases we are not listing resources for any CRDs installed by clusterctl (including cert-manager CRDs and providers CRDs).

In other words, we are only concerned about provider components (deployments, RBAC rules, CRD definitions) but not in the resource created on top of that (Clusters, Machines, AWSCluster etc) because the latter are not relevant for upgrade/delete.

// Those objects are not part of the components, and they will eventually be removed when removing the CRD definition.
crdsToExclude := sets.String{}
if providerName, ok := labels[clusterv1.ProviderLabelName]; ok {
// List all CRDs in the cluster.
crdList := &apiextensionsv1.CustomResourceDefinitionList{}
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 v, ok := crd.Labels[clusterv1.ProviderLabelName]; ok && v != providerName {
for _, version := range crd.Spec.Versions {
crdsToExclude.Insert(metav1.GroupVersionKind{
Group: crd.Spec.Group,
Version: version.Name,
Kind: crd.Spec.Names.Kind,
}.String())
}
crdList := &apiextensionsv1.CustomResourceDefinitionList{}
if err := retryWithExponentialBackoff(newReadBackoff(), func() error {
return c.List(ctx, crdList)
}); err != nil {
return nil, errors.Wrap(err, "failed to list CRDs")
}
for _, crd := range crdList.Items {
component, isCoreComponent := labels[clusterctlv1.ClusterctlCoreLabelName]
_, isProviderResource := crd.Labels[clusterv1.ProviderLabelName]
if (isCoreComponent && component == clusterctlv1.ClusterctlCoreLabelCertManagerValue) || isProviderResource {
for _, version := range crd.Spec.Versions {
crdsToExclude.Insert(metav1.GroupVersionKind{
Group: crd.Spec.Group,
Version: version.Name,
Kind: crd.Spec.Names.Kind,
}.String())
}
}
}
Expand Down