From 9fe5c4900e173abec09e89cfed63c09cf8052f13 Mon Sep 17 00:00:00 2001 From: fabriziopandini Date: Thu, 3 Jun 2021 21:56:12 +0200 Subject: [PATCH] Adapt clusterctl to webhook deployed with managers --- cmd/clusterctl/api/v1alpha3/labels.go | 15 - cmd/clusterctl/client/cluster/components.go | 59 ++-- .../client/cluster/components_test.go | 74 ++--- cmd/clusterctl/client/cluster/installer.go | 65 +--- .../client/cluster/installer_test.go | 75 +---- cmd/clusterctl/client/delete.go | 1 - .../client/repository/components.go | 125 ++------ .../repository/components_client_test.go | 8 +- .../client/repository/components_test.go | 279 ------------------ cmd/clusterctl/internal/util/objs.go | 13 - docs/book/src/clusterctl/provider-contract.md | 16 - 11 files changed, 73 insertions(+), 657 deletions(-) diff --git a/cmd/clusterctl/api/v1alpha3/labels.go b/cmd/clusterctl/api/v1alpha3/labels.go index 71e09c0f7563..be962651d978 100644 --- a/cmd/clusterctl/api/v1alpha3/labels.go +++ b/cmd/clusterctl/api/v1alpha3/labels.go @@ -25,12 +25,6 @@ const ( // ClusterctlCoreLabelName is applied to all the core objects managed by clusterctl. ClusterctlCoreLabelName = "clusterctl.cluster.x-k8s.io/core" - // ClusterctlResourceLifecyleLabelName describes the lifecyle for a specific resource. - // - // Example: resources shared between instances of the same provider: CRDs, - // ValidatingWebhookConfiguration, MutatingWebhookConfiguration, and so on. - ClusterctlResourceLifecyleLabelName = "clusterctl.cluster.x-k8s.io/lifecycle" - // ClusterctlMoveLabelName can be set on CRDs that providers wish to move but that are not part of a Cluster. ClusterctlMoveLabelName = "clusterctl.cluster.x-k8s.io/move" @@ -38,15 +32,6 @@ const ( ClusterctlMoveHierarchyLabelName = "clusterctl.cluster.x-k8s.io/move-hierarchy" ) -// ResourceLifecycle configures the lifecycle of a resource. -type ResourceLifecycle string - -const ( - // ResourceLifecycleShared is used to indicate that a resource is shared between - // multiple instances of a provider. - ResourceLifecycleShared = ResourceLifecycle("shared") -) - // ManifestLabel returns the cluster.x-k8s.io/provider label value for a provider/type. // // Note: the label uniquely describes the provider type and its kind (e.g. bootstrap-kubeadm); diff --git a/cmd/clusterctl/client/cluster/components.go b/cmd/clusterctl/client/cluster/components.go index 6b89de337e23..ce4788691372 100644 --- a/cmd/clusterctl/client/cluster/components.go +++ b/cmd/clusterctl/client/cluster/components.go @@ -29,13 +29,19 @@ import ( "k8s.io/apimachinery/pkg/util/sets" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" - "sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository" "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/util" logf "sigs.k8s.io/cluster-api/cmd/clusterctl/log" "sigs.k8s.io/controller-runtime/pkg/client" ) -// DeleteOptions defines delete options. +const ( + namespaceKind = "Namespace" + validatingWebhookConfigurationKind = "ValidatingWebhookConfiguration" + mutatingWebhookConfigurationKind = "MutatingWebhookConfiguration" + customResourceDefinitionKind = "CustomResourceDefinition" +) + +// DeleteOptions holds options for ComponentsClient.Delete func. type DeleteOptions struct { Provider clusterctlv1.Provider IncludeNamespace bool @@ -123,22 +129,13 @@ func (p *providerComponents) Delete(options DeleteOptions) error { log.Info("Deleting", "Provider", options.Provider.Name, "Version", options.Provider.Version, "TargetNamespace", options.Provider.Namespace) // Fetch all the components belonging to a provider. - // We want that the delete operation is able to clean-up everything in a the most common use case that is - // single-tenant management clusters. However, the downside of this is that this operation might be destructive - // in multi-tenant scenario, because a single operation could delete both instance specific and shared CRDs/web-hook components. - // This is considered acceptable because we are considering the multi-tenant scenario an advanced use case, and the assumption - // is that user in this case understand the potential impacts of this operation. - // TODO: in future we can eventually block delete --IncludeCRDs in case more than one instance of a provider exists + // We want that the delete operation is able to clean-up everything. labels := map[string]string{ clusterctlv1.ClusterctlLabelName: "", clusterv1.ProviderLabelName: options.Provider.ManifestLabel(), } namespaces := []string{options.Provider.Namespace} - if options.IncludeCRDs { - namespaces = append(namespaces, repository.WebhookNamespaceName) - } - resources, err := p.proxy.ListResources(labels, namespaces...) if err != nil { return err @@ -149,15 +146,15 @@ func (p *providerComponents) Delete(options DeleteOptions) error { namespacesToDelete := sets.NewString() instanceNamespacePrefix := fmt.Sprintf("%s-", options.Provider.Namespace) for _, obj := range resources { - // If the CRDs (and by extensions, all the shared resources) should NOT be deleted, skip it; + // If the CRDs should NOT be deleted, skip it; // NB. Skipping CRDs deletion ensures that also the objects of Kind defined in the CRDs Kind are not deleted. - isSharedResource := util.IsSharedResource(obj) - if !options.IncludeCRDs && isSharedResource { + isCRD := obj.GroupVersionKind().Kind == customResourceDefinitionKind + if !options.IncludeCRDs && isCRD { continue } // If the resource is a namespace - isNamespace := obj.GroupVersionKind().Kind == "Namespace" + isNamespace := obj.GroupVersionKind().Kind == namespaceKind if isNamespace { // Skip all the namespaces not related to the provider instance being processed. if obj.GetName() != options.Provider.Namespace { @@ -171,17 +168,17 @@ func (p *providerComponents) Delete(options DeleteOptions) error { namespacesToDelete.Insert(obj.GetName()) } - // If not a shared resource or not a namespace - if !isSharedResource && !isNamespace { - // If the resource is a cluster resource, skip it if the resource name does not start with the instance prefix. - // This is required because there are cluster resources like e.g. ClusterRoles and ClusterRoleBinding, which are instance specific; - // During the installation, clusterctl adds the instance namespace prefix to such resources (see fixRBAC), and so we can rely - // on that for deleting only the global resources belonging the the instance we are processing. - if util.IsClusterResource(obj.GetKind()) { - if !strings.HasPrefix(obj.GetName(), instanceNamespacePrefix) { - continue - } - } + // If the resource is a cluster resource, skip it if the resource name does not start with the instance prefix. + // This is required because there are cluster resources like e.g. ClusterRoles and ClusterRoleBinding, which are instance specific; + // During the installation, clusterctl adds the instance namespace prefix to such resources (see fixRBAC), and so we can rely + // on that for deleting only the global resources belonging the the instance we are processing. + // NOTE: namespace and CRD are special case managed above; webhook instead goes hand by hand with the controller they + // should always be deleted. + isWebhook := obj.GroupVersionKind().Kind == validatingWebhookConfigurationKind || obj.GroupVersionKind().Kind == mutatingWebhookConfigurationKind + if util.IsClusterResource(obj.GetKind()) && + !isNamespace && !isCRD && !isWebhook && + !strings.HasPrefix(obj.GetName(), instanceNamespacePrefix) { + continue } resourcesToDelete = append(resourcesToDelete, obj) @@ -219,20 +216,22 @@ func (p *providerComponents) Delete(options DeleteOptions) error { } func (p *providerComponents) DeleteWebhookNamespace() error { + const webhookNamespaceName = "capi-webhook-system" + log := logf.Log - log.V(5).Info("Deleting", "namespace", repository.WebhookNamespaceName) + log.V(5).Info("Deleting", "namespace", webhookNamespaceName) c, err := p.proxy.NewClient() if err != nil { return err } - coreProviderWebhookNs := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: repository.WebhookNamespaceName}} + coreProviderWebhookNs := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: webhookNamespaceName}} if err := c.Delete(ctx, coreProviderWebhookNs); err != nil { if apierrors.IsNotFound(err) { return nil } - return errors.Wrapf(err, "failed to delete namespace %s", repository.WebhookNamespaceName) + return errors.Wrapf(err, "failed to delete namespace %s", webhookNamespaceName) } return nil diff --git a/cmd/clusterctl/client/cluster/components_test.go b/cmd/clusterctl/client/cluster/components_test.go index c2b7ed85fe2d..cda8af22c25a 100644 --- a/cmd/clusterctl/client/cluster/components_test.go +++ b/cmd/clusterctl/client/cluster/components_test.go @@ -28,7 +28,6 @@ import ( "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha4" clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" - "sigs.k8s.io/cluster-api/cmd/clusterctl/client/repository" "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/test" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -37,22 +36,18 @@ func Test_providerComponents_Delete(t *testing.T) { labels := map[string]string{ clusterv1.ProviderLabelName: "infrastructure-infra", } - sharedLabels := map[string]string{ - clusterv1.ProviderLabelName: "infrastructure-infra", - clusterctlv1.ClusterctlResourceLifecyleLabelName: string(clusterctlv1.ResourceLifecycleShared), - } crd := unstructured.Unstructured{} crd.SetAPIVersion("apiextensions.k8s.io/v1beta1") crd.SetKind("CustomResourceDefinition") crd.SetName("crd1") - crd.SetLabels(sharedLabels) + crd.SetLabels(labels) mutatingWebhook := unstructured.Unstructured{} mutatingWebhook.SetAPIVersion("admissionregistration.k8s.io/v1beta1") mutatingWebhook.SetKind("MutatingWebhookConfiguration") mutatingWebhook.SetName("mwh1") - mutatingWebhook.SetLabels(sharedLabels) + mutatingWebhook.SetLabels(labels) initObjs := []client.Object{ // Namespace (should be deleted only if includeNamespace) @@ -89,28 +84,6 @@ func Test_providerComponents_Delete(t *testing.T) { // CRDs (should be deleted only if includeCRD) &crd, &mutatingWebhook, - &corev1.Namespace{ - TypeMeta: metav1.TypeMeta{ - Kind: "Namespace", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: repository.WebhookNamespaceName, - Labels: map[string]string{ - clusterctlv1.ClusterctlResourceLifecyleLabelName: string(clusterctlv1.ResourceLifecycleShared), - // NB. the capi-webhook-system namespace doe not have a provider label (see fixSharedLabels) - }, - }, - }, - &corev1.Pod{ - TypeMeta: metav1.TypeMeta{ - Kind: "Pod", - }, - ObjectMeta: metav1.ObjectMeta{ - Namespace: repository.WebhookNamespaceName, - Name: "podx", - Labels: sharedLabels, - }, - }, // A cluster-wide provider component (should always be deleted) &rbacv1.ClusterRole{ TypeMeta: metav1.TypeMeta{ @@ -127,8 +100,7 @@ func Test_providerComponents_Delete(t *testing.T) { Kind: "ClusterRole", }, ObjectMeta: metav1.ObjectMeta{ - Name: "some-cluster-role", - Labels: labels, + Name: "some-cluster-role", }, }, // Another object out of the provider namespace (should never be deleted) @@ -168,16 +140,14 @@ func Test_providerComponents_Delete(t *testing.T) { includeCRD: false, }, wantDiff: []wantDiff{ - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be preserved - {object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: false}, // crd should be preserved - {object: corev1.ObjectReference{APIVersion: "admissionregistration.k8s.io/v1beta1", Kind: "MutatingWebhookConfiguration", Name: "mwh1"}, deleted: false}, // MutatingWebhookConfiguration should be preserved - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: repository.WebhookNamespaceName}, deleted: false}, // capi-webhook-system namespace should never be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: repository.WebhookNamespaceName, Name: "podx"}, deleted: false}, // provider objects in the capi-webhook-system namespace should be preserved - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod1"}, deleted: true}, // provider components should be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod2"}, deleted: false}, // other objects in the namespace should not be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete - {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted - {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be preserved + {object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: false}, // crd should be preserved + {object: corev1.ObjectReference{APIVersion: "admissionregistration.k8s.io/v1beta1", Kind: "MutatingWebhookConfiguration", Name: "mwh1"}, deleted: true}, // MutatingWebhookConfiguration goes away with the controller + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod1"}, deleted: true}, // provider components should be deleted + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod2"}, deleted: false}, // other objects in the namespace should not be deleted + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete + {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted + {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved }, wantErr: false, }, @@ -189,16 +159,14 @@ func Test_providerComponents_Delete(t *testing.T) { includeCRD: false, }, wantDiff: []wantDiff{ - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: true}, // namespace should be deleted - {object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: false}, // crd should be preserved - {object: corev1.ObjectReference{APIVersion: "admissionregistration.k8s.io/v1beta1", Kind: "MutatingWebhookConfiguration", Name: "mwh1"}, deleted: false}, // MutatingWebhookConfiguration should be preserved - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: repository.WebhookNamespaceName}, deleted: false}, // capi-webhook-system namespace should never be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: repository.WebhookNamespaceName, Name: "podx"}, deleted: false}, // provider objects in the capi-webhook-system namespace should be preserved - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod1"}, deleted: true}, // provider components should be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod2"}, deleted: true}, // other objects in the namespace goes away when deleting the namespace - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete - {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted - {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: true}, // namespace should be deleted + {object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: false}, // crd should be preserved + {object: corev1.ObjectReference{APIVersion: "admissionregistration.k8s.io/v1beta1", Kind: "MutatingWebhookConfiguration", Name: "mwh1"}, deleted: true}, // MutatingWebhookConfiguration goes away with the controller + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod1"}, deleted: true}, // provider components should be deleted + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod2"}, deleted: true}, // other objects in the namespace goes away when deleting the namespace + {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete + {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "ns1-cluster-role"}, deleted: true}, // cluster-wide provider components should be deleted + {object: corev1.ObjectReference{APIVersion: "rbac.authorization.k8s.io/v1", Kind: "ClusterRole", Name: "some-cluster-role"}, deleted: false}, // other cluster-wide objects should be preserved }, wantErr: false, }, @@ -213,8 +181,6 @@ func Test_providerComponents_Delete(t *testing.T) { {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be preserved {object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: true}, // crd should be deleted {object: corev1.ObjectReference{APIVersion: "admissionregistration.k8s.io/v1beta1", Kind: "MutatingWebhookConfiguration", Name: "mwh1"}, deleted: true}, // MutatingWebhookConfiguration should be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: repository.WebhookNamespaceName}, deleted: false}, // capi-webhook-system namespace should never be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: repository.WebhookNamespaceName, Name: "podx"}, deleted: true}, // provider objects in the capi-webhook-system namespace should be deleted {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod1"}, deleted: true}, // provider components should be deleted {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod2"}, deleted: false}, // other objects in the namespace should not be deleted {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete @@ -234,8 +200,6 @@ func Test_providerComponents_Delete(t *testing.T) { {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: true}, // namespace should be deleted {object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: true}, // crd should be deleted {object: corev1.ObjectReference{APIVersion: "admissionregistration.k8s.io/v1beta1", Kind: "MutatingWebhookConfiguration", Name: "mwh1"}, deleted: true}, // MutatingWebhookConfiguration should be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: repository.WebhookNamespaceName}, deleted: false}, // capi-webhook-namespace should never be deleted - {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: repository.WebhookNamespaceName, Name: "podx"}, deleted: true}, // provider objects in the capi-webhook-namespace should be deleted {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod1"}, deleted: true}, // provider components should be deleted {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns1", Name: "pod2"}, deleted: true}, // other objects in the namespace goes away when deleting the namespace {object: corev1.ObjectReference{APIVersion: "v1", Kind: "Pod", Namespace: "ns2", Name: "pod3"}, deleted: false}, // this object is in another namespace, and should never be touched by delete diff --git a/cmd/clusterctl/client/cluster/installer.go b/cmd/clusterctl/client/cluster/installer.go index d1a04c0ce4d9..5f2dd0ea9817 100644 --- a/cmd/clusterctl/client/cluster/installer.go +++ b/cmd/clusterctl/client/cluster/installer.go @@ -81,35 +81,8 @@ func installComponentsAndUpdateInventory(components repository.Components, provi inventoryObject := components.InventoryObject() - // Check the list of providers currently in the cluster and decide if to install shared components (CRDs, web-hooks) or not. - // We are required to install shared components in two cases: - // - when this is the first instance of the provider being installed. - // - when the version of the provider being installed is newer than the max version already installed in the cluster. - // Nb. this assumes the newer version of shared components are fully retro-compatible. - providerList, err := providerInventory.List() - if err != nil { - return err - } - - installSharedComponents, err := shouldInstallSharedComponents(providerList, inventoryObject) - if err != nil { - return err - } - if installSharedComponents { - log.V(1).Info("Creating shared objects", "Provider", components.ManifestLabel(), "Version", components.Version()) - // TODO: currently shared components overrides existing shared components. As a future improvement we should - // consider if to delete (preserving CRDs) before installing so there will be no left-overs in case the list of resources changes - if err := providerComponents.Create(components.SharedObjs()); err != nil { - return err - } - } else { - log.V(1).Info("Shared objects already up to date", "Provider", components.ManifestLabel()) - } - - // Then always install the instance specific objects and the then inventory item for the provider - - log.V(1).Info("Creating instance objects", "Provider", components.ManifestLabel(), "Version", components.Version(), "TargetNamespace", components.TargetNamespace()) - if err := providerComponents.Create(components.InstanceObjs()); err != nil { + log.V(1).Info("Creating objects", "Provider", components.ManifestLabel(), "Version", components.Version(), "TargetNamespace", components.TargetNamespace()) + if err := providerComponents.Create(components.Objs()); err != nil { return err } @@ -117,40 +90,6 @@ func installComponentsAndUpdateInventory(components repository.Components, provi return providerInventory.Create(inventoryObject) } -// shouldInstallSharedComponents checks if it is required to install shared components for a provider. -func shouldInstallSharedComponents(providerList *clusterctlv1.ProviderList, provider clusterctlv1.Provider) (bool, error) { - // Get the max version of the provider already installed in the cluster. - var maxVersion *version.Version - for _, other := range providerList.FilterByProviderNameAndType(provider.ProviderName, provider.GetProviderType()) { - otherVersion, err := version.ParseSemantic(other.Version) - if err != nil { - return false, errors.Wrapf(err, "failed to parse version for the %s provider", other.InstanceName()) - } - if maxVersion == nil || otherVersion.AtLeast(maxVersion) { - maxVersion = otherVersion - } - } - // If there is no max version, this is the first instance of the provider being installed, so it is required - // to install the shared components. - if maxVersion == nil { - return true, nil - } - - // If the installed version is newer or equal than than the version of the provider being installed, - // return false because we should not down grade the shared components. - providerVersion, err := version.ParseSemantic(provider.Version) - if err != nil { - return false, errors.Wrapf(err, "failed to parse version for the %s provider", provider.InstanceName()) - } - if maxVersion.AtLeast(providerVersion) { - return false, nil - } - - // Otherwise, the version of the provider being installed is newer that the current max version, so it is - // required to install also the new version of shared components. - return true, nil -} - func (i *providerInstaller) Validate() error { // Get the list of providers currently in the cluster. providerList, err := i.providerInventory.List() diff --git a/cmd/clusterctl/client/cluster/installer_test.go b/cmd/clusterctl/client/cluster/installer_test.go index f48a54084dba..7ee159e2060c 100644 --- a/cmd/clusterctl/client/cluster/installer_test.go +++ b/cmd/clusterctl/client/cluster/installer_test.go @@ -282,11 +282,7 @@ func (c *fakeComponents) InventoryObject() clusterctlv1.Provider { return c.inventoryObject } -func (c *fakeComponents) InstanceObjs() []unstructured.Unstructured { - panic("not implemented") -} - -func (c *fakeComponents) SharedObjs() []unstructured.Unstructured { +func (c *fakeComponents) Objs() []unstructured.Unstructured { panic("not implemented") } @@ -301,72 +297,3 @@ func newFakeComponents(name string, providerType clusterctlv1.ProviderType, vers inventoryObject: inventoryObject, } } - -func Test_shouldInstallSharedComponents(t *testing.T) { - type args struct { - providerList *clusterctlv1.ProviderList - provider clusterctlv1.Provider - } - tests := []struct { - name string - args args - want bool - wantErr bool - }{ - { - name: "First instance of the provider, must install shared components", - args: args{ - providerList: &clusterctlv1.ProviderList{Items: []clusterctlv1.Provider{}}, // no core provider installed - provider: fakeProvider("core", clusterctlv1.CoreProviderType, "v2.0.0", ""), - }, - want: true, - wantErr: false, - }, - { - name: "Second instance of the provider, same version, must NOT install shared components", - args: args{ - providerList: &clusterctlv1.ProviderList{Items: []clusterctlv1.Provider{ - fakeProvider("core", clusterctlv1.CoreProviderType, "v2.0.0", ""), - }}, - provider: fakeProvider("core", clusterctlv1.CoreProviderType, "v2.0.0", ""), - }, - want: false, - wantErr: false, - }, - { - name: "Second instance of the provider, older version, must NOT install shared components", - args: args{ - providerList: &clusterctlv1.ProviderList{Items: []clusterctlv1.Provider{ - fakeProvider("core", clusterctlv1.CoreProviderType, "v2.0.0", ""), - }}, - provider: fakeProvider("core", clusterctlv1.CoreProviderType, "v1.0.0", ""), - }, - want: false, - wantErr: false, - }, - { - name: "Second instance of the provider, newer version, must install shared components", - args: args{ - providerList: &clusterctlv1.ProviderList{Items: []clusterctlv1.Provider{ - fakeProvider("core", clusterctlv1.CoreProviderType, "v2.0.0", ""), - }}, - provider: fakeProvider("core", clusterctlv1.CoreProviderType, "v3.0.0", ""), - }, - want: true, - wantErr: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - got, err := shouldInstallSharedComponents(tt.args.providerList, tt.args.provider) - if tt.wantErr { - g.Expect(err).To(HaveOccurred()) - return - } - g.Expect(err).NotTo(HaveOccurred()) - g.Expect(got).To(Equal(tt.want)) - }) - } -} diff --git a/cmd/clusterctl/client/delete.go b/cmd/clusterctl/client/delete.go index 1a4a3bbd256e..73067f7560e2 100644 --- a/cmd/clusterctl/client/delete.go +++ b/cmd/clusterctl/client/delete.go @@ -52,7 +52,6 @@ type DeleteOptions struct { IncludeNamespace bool // IncludeCRDs forces the deletion of the provider's CRDs (and of all the related objects). - // By Extension, this forces the deletion of all the resources shared among provider instances, like e.g. web-hooks. IncludeCRDs bool } diff --git a/cmd/clusterctl/client/repository/components.go b/cmd/clusterctl/client/repository/components.go index 4cfd81077f90..0c725247dca0 100644 --- a/cmd/clusterctl/client/repository/components.go +++ b/cmd/clusterctl/client/repository/components.go @@ -33,13 +33,10 @@ import ( ) const ( - namespaceKind = "Namespace" - clusterRoleKind = "ClusterRole" - clusterRoleBindingKind = "ClusterRoleBinding" - roleBindingKind = "RoleBinding" - validatingWebhookConfigurationKind = "ValidatingWebhookConfiguration" - mutatingWebhookConfigurationKind = "MutatingWebhookConfiguration" - customResourceDefinitionKind = "CustomResourceDefinition" + namespaceKind = "Namespace" + clusterRoleKind = "ClusterRole" + clusterRoleBindingKind = "ClusterRoleBinding" + roleBindingKind = "RoleBinding" ) const ( @@ -82,11 +79,8 @@ type Components interface { // Yaml return the provider components in the form of a YAML file. Yaml() ([]byte, error) - // InstanceObjs return the instance specific components in the form of a list of Unstructured objects. - InstanceObjs() []unstructured.Unstructured - - // SharedObjs returns CRDs, web-hooks and all the components shared across instances in the form of a list of Unstructured objects. - SharedObjs() []unstructured.Unstructured + // Objs return the components in the form of a list of Unstructured objects. + Objs() []unstructured.Unstructured } // components implement Components. @@ -96,8 +90,7 @@ type components struct { variables []string images []string targetNamespace string - instanceObjs []unstructured.Unstructured - sharedObjs []unstructured.Unstructured + objs []unstructured.Unstructured } // ensure components implement Components. @@ -139,20 +132,12 @@ func (c *components) InventoryObject() clusterctlv1.Provider { } } -func (c *components) InstanceObjs() []unstructured.Unstructured { - return c.instanceObjs -} - -func (c *components) SharedObjs() []unstructured.Unstructured { - return c.sharedObjs +func (c *components) Objs() []unstructured.Unstructured { + return c.objs } func (c *components) Yaml() ([]byte, error) { - objs := []unstructured.Unstructured{} - objs = append(objs, c.sharedObjs...) - objs = append(objs, c.instanceObjs...) - - return utilyaml.FromUnstructured(objs) + return utilyaml.FromUnstructured(c.objs) } // ComponentsOptions represents specific inputs that are passed in to @@ -216,17 +201,9 @@ func NewComponents(input ComponentsInput) (Components, error) { return nil, errors.Wrap(err, "failed to detect required images") } - // splits the component resources from the shared resources. - // This is required because a component yaml is designed for allowing users to create a single instance of a provider - // by running kubectl apply, while multi-tenant installations requires manual modifications to the yaml file. - // clusterctl does such modification for the user, and in order to do so, it is required to split objects in two sets; - // components resources are processed in order to make instance specific modifications, while identifying labels - // are applied to shared resources. - instanceObjs, sharedObjs := splitInstanceAndSharedResources(objs) - // inspect the list of objects for the default target namespace // the default target namespace is the namespace object defined in the component yaml read from the repository, if any - defaultTargetNamespace, err := inspectTargetNamespace(instanceObjs) + defaultTargetNamespace, err := inspectTargetNamespace(objs) if err != nil { return nil, errors.Wrap(err, "failed to detect default target namespace") } @@ -244,27 +221,21 @@ func NewComponents(input ComponentsInput) (Components, error) { } // add a Namespace object if missing (ensure the targetNamespace will be created) - instanceObjs = addNamespaceIfMissing(instanceObjs, input.Options.TargetNamespace) + objs = addNamespaceIfMissing(objs, input.Options.TargetNamespace) // fix Namespace name in all the objects - instanceObjs = fixTargetNamespace(instanceObjs, input.Options.TargetNamespace) + objs = fixTargetNamespace(objs, input.Options.TargetNamespace) // ensures all the ClusterRole and ClusterRoleBinding have the name prefixed with the namespace name and that // all the clusterRole/clusterRoleBinding namespaced subjects refers to targetNamespace // Nb. Making all the RBAC rules "namespaced" is required for supporting multi-tenancy - instanceObjs, err = fixRBAC(instanceObjs, input.Options.TargetNamespace) + objs, err = fixRBAC(objs, input.Options.TargetNamespace) if err != nil { return nil, errors.Wrap(err, "failed to fix ClusterRoleBinding names") } - // Add common labels to both the obj groups. - instanceObjs = addCommonLabels(instanceObjs, input.Provider) - sharedObjs = addCommonLabels(sharedObjs, input.Provider) - - // Add an identifying label to shared components so next invocation of init, clusterctl delete and clusterctl upgrade can act accordingly. - // Additionally, the capi-webhook-system namespace gets detached from any provider, so we prevent that deleting - // a provider can delete all the web-hooks. - sharedObjs = fixSharedLabels(sharedObjs) + // Add common labels. + objs = addCommonLabels(objs, input.Provider) return &components{ Provider: input.Provider, @@ -272,43 +243,10 @@ func NewComponents(input ComponentsInput) (Components, error) { variables: variables, images: images, targetNamespace: input.Options.TargetNamespace, - instanceObjs: instanceObjs, - sharedObjs: sharedObjs, + objs: objs, }, nil } -// splitInstanceAndSharedResources divides the objects contained in the component yaml into two sets, instance specific objects -// and objects shared across many instances. -func splitInstanceAndSharedResources(objs []unstructured.Unstructured) (instanceObjs []unstructured.Unstructured, sharedObjs []unstructured.Unstructured) { - for _, o := range objs { - // CRDs, and web-hook objects are shared among instances. - if o.GetKind() == customResourceDefinitionKind || - o.GetKind() == mutatingWebhookConfigurationKind || - o.GetKind() == validatingWebhookConfigurationKind { - sharedObjs = append(sharedObjs, o) - continue - } - - // Web-hook objects are backed by a controller handling the web-hook calls; byt definition this - // controller and everything releted to it (eg. services, certificates) it is expected to be deployed in well - // know namespace named capi-webhook-system. - // So this namespace and all the objected belonging to it are considered shared resources. - if o.GetKind() == namespaceKind && o.GetName() == WebhookNamespaceName { - sharedObjs = append(sharedObjs, o) - continue - } - - if util.IsResourceNamespaced(o.GetKind()) && o.GetNamespace() == WebhookNamespaceName { - sharedObjs = append(sharedObjs, o) - continue - } - - // Everything else is considered an instance specific object. - instanceObjs = append(instanceObjs, o) - } - return -} - // inspectTargetNamespace identifies the name of the namespace object contained in the components YAML, if any. // In case more than one Namespace object is identified, an error is returned. func inspectTargetNamespace(objs []unstructured.Unstructured) (string, error) { @@ -397,11 +335,9 @@ func fixRBAC(objs []unstructured.Unstructured, targetNamespace string) ([]unstru // assign a namespaced name b.Name = fmt.Sprintf("%s-%s", targetNamespace, b.Name) - // ensure that namespaced subjects refers to targetNamespace; the only exception - // for this rule are the namespaced subjects located in the capi-webhook-system, which are - // not affected by the targetNamespace value + // ensure that namespaced subjects refers to targetNamespace for k := range b.Subjects { - if b.Subjects[k].Namespace != "" && b.Subjects[k].Namespace != WebhookNamespaceName { + if b.Subjects[k].Namespace != "" { b.Subjects[k].Namespace = targetNamespace } } @@ -424,11 +360,9 @@ func fixRBAC(objs []unstructured.Unstructured, targetNamespace string) ([]unstru return nil, err } - // ensure that namespaced subjects refers to targetNamespace; the only exception - // for this rule are the namespaced subjects located in the capi-webhook-system, which are - // not affected by the targetNamespace value + // ensure that namespaced subjects refers to targetNamespace for k := range b.Subjects { - if b.Subjects[k].Namespace != "" && b.Subjects[k].Namespace != WebhookNamespaceName { + if b.Subjects[k].Namespace != "" { b.Subjects[k].Namespace = targetNamespace } } @@ -466,20 +400,3 @@ func getCommonLabels(provider config.Provider) map[string]string { clusterv1.ProviderLabelName: provider.ManifestLabel(), } } - -// fixSharedLabels ensures all the shared components have an identifying label so next invocation of init, clusterctl delete -// and clusterctl upgrade can act accordingly. -func fixSharedLabels(objs []unstructured.Unstructured) []unstructured.Unstructured { - for _, o := range objs { - labels := o.GetLabels() - labels[clusterctlv1.ClusterctlResourceLifecyleLabelName] = string(clusterctlv1.ResourceLifecycleShared) - - // the capi-webhook-system namespace is shared among many providers, so removing the ProviderLabelName label. - if o.GetKind() == namespaceKind && o.GetName() == WebhookNamespaceName { - delete(labels, clusterv1.ProviderLabelName) - } - o.SetLabels(labels) - } - - return objs -} diff --git a/cmd/clusterctl/client/repository/components_client_test.go b/cmd/clusterctl/client/repository/components_client_test.go index 1b83ab9dfa87..f78a92c0cb19 100644 --- a/cmd/clusterctl/client/repository/components_client_test.go +++ b/cmd/clusterctl/client/repository/components_client_test.go @@ -299,17 +299,11 @@ func Test_componentsClient_Get(t *testing.T) { } } - for _, o := range got.InstanceObjs() { + for _, o := range got.Objs() { for _, v := range []string{clusterctlv1.ClusterctlLabelName, clusterv1.ProviderLabelName} { gs.Expect(o.GetLabels()).To(HaveKey(v)) } } - - for _, o := range got.SharedObjs() { - for _, v := range []string{clusterctlv1.ClusterctlLabelName, clusterv1.ProviderLabelName, clusterctlv1.ClusterctlResourceLifecyleLabelName} { - gs.Expect(o.GetLabels()).To(HaveKey(v)) - } - } }) } } diff --git a/cmd/clusterctl/client/repository/components_test.go b/cmd/clusterctl/client/repository/components_test.go index 3427beb31d11..5c08414a83cb 100644 --- a/cmd/clusterctl/client/repository/components_test.go +++ b/cmd/clusterctl/client/repository/components_test.go @@ -485,134 +485,6 @@ func Test_fixRBAC(t *testing.T) { }, wantErr: false, }, - { - name: "ClusterRoleBinding with subjects IN capi-webhook-system get fixed (without changing the subject namespace)", - args: args{ - objs: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": map[string]interface{}{ - "name": "foo", - }, - "roleRef": map[string]interface{}{ - "apiGroup": "", - "kind": "", - "name": "bar", - }, - "subjects": []interface{}{ - map[string]interface{}{ - "kind": "ServiceAccount", - "name": "baz", - "namespace": "capi-webhook-system", - }, - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "ClusterRole", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": map[string]interface{}{ - "name": "bar", - }, - }, - }, - }, - targetNamespace: "target", - }, - want: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "kind": "ClusterRoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": map[string]interface{}{ - "name": "target-foo", // ClusterRoleBinding name fixed! - "creationTimestamp": nil, - }, - "roleRef": map[string]interface{}{ - "apiGroup": "", - "kind": "", - "name": "target-bar", // ClusterRole name fixed! - }, - "subjects": []interface{}{ - map[string]interface{}{ - "kind": "ServiceAccount", - "name": "baz", - "namespace": "capi-webhook-system", // Subjects namespace get preserved! - }, - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "ClusterRole", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": map[string]interface{}{ - "name": "target-bar", // ClusterRole fixed! - }, - }, - }, - }, - wantErr: false, - }, - { - name: "RoleBinding with subjects IN capi-webhook-system get fixed (without changing the subject namespace)", - args: args{ - objs: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "kind": "RoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "target", - }, - "roleRef": map[string]interface{}{ - "apiGroup": "", - "kind": "", - "name": "bar", - }, - "subjects": []interface{}{ - map[string]interface{}{ - "kind": "ServiceAccount", - "name": "baz", - "namespace": "capi-webhook-system", - }, - }, - }, - }, - }, - targetNamespace: "target", - }, - want: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "kind": "RoleBinding", - "apiVersion": "rbac.authorization.k8s.io/v1", - "metadata": map[string]interface{}{ - "name": "foo", - "namespace": "target", - "creationTimestamp": nil, - }, - "roleRef": map[string]interface{}{ - "apiGroup": "", - "kind": "", - "name": "bar", - }, - "subjects": []interface{}{ - map[string]interface{}{ - "kind": "ServiceAccount", - "name": "baz", - "namespace": "capi-webhook-system", // Subjects namespace get preserved! - }, - }, - }, - }, - }, - wantErr: false, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -678,154 +550,3 @@ func Test_addCommonLabels(t *testing.T) { }) } } - -func Test_splitInstanceAndSharedResources(t *testing.T) { - type args struct { - objs []unstructured.Unstructured - } - tests := []struct { - name string - args args - wantInstanceObjs []unstructured.Unstructured - wantSharedObjs []unstructured.Unstructured - }{ - { - name: "objects are split in two sets", - args: args{ - objs: []unstructured.Unstructured{ - // Instance objs - { - Object: map[string]interface{}{ - "kind": "Namespace", - "metadata": map[string]interface{}{ - "name": "capi-system", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "Deployment", - "metadata": map[string]interface{}{ - "name": "capi-controller-manager", - "namespace": "capi-system", - }, - }, - }, - // Shared objs - { - Object: map[string]interface{}{ - "kind": "Namespace", - "metadata": map[string]interface{}{ - "name": "capi-webhook-system", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "Deployment", - "metadata": map[string]interface{}{ - "name": "capi-controller-manager", - "namespace": "capi-webhook-system", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "CustomResourceDefinition", - "metadata": map[string]interface{}{ - "name": "clusters.cluster.x-k8s.io", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "MutatingWebhookConfiguration", - "metadata": map[string]interface{}{ - "name": "capi-mutating-webhook-configuration", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "ValidatingWebhookConfiguration", - "metadata": map[string]interface{}{ - "name": "capi-validating-webhook-configuration", - }, - }, - }, - }, - }, - wantInstanceObjs: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "kind": "Namespace", - "metadata": map[string]interface{}{ - "name": "capi-system", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "Deployment", - "metadata": map[string]interface{}{ - "name": "capi-controller-manager", - "namespace": "capi-system", - }, - }, - }, - }, - wantSharedObjs: []unstructured.Unstructured{ - { - Object: map[string]interface{}{ - "kind": "Namespace", - "metadata": map[string]interface{}{ - "name": "capi-webhook-system", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "Deployment", - "metadata": map[string]interface{}{ - "name": "capi-controller-manager", - "namespace": "capi-webhook-system", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "CustomResourceDefinition", - "metadata": map[string]interface{}{ - "name": "clusters.cluster.x-k8s.io", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "MutatingWebhookConfiguration", - "metadata": map[string]interface{}{ - "name": "capi-mutating-webhook-configuration", - }, - }, - }, - { - Object: map[string]interface{}{ - "kind": "ValidatingWebhookConfiguration", - "metadata": map[string]interface{}{ - "name": "capi-validating-webhook-configuration", - }, - }, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - g := NewWithT(t) - - gotInstanceObjs, gotWebHookObjs := splitInstanceAndSharedResources(tt.args.objs) - g.Expect(gotInstanceObjs).To(ConsistOf(tt.wantInstanceObjs)) - g.Expect(gotWebHookObjs).To(ConsistOf(tt.wantSharedObjs)) - }) - } -} diff --git a/cmd/clusterctl/internal/util/objs.go b/cmd/clusterctl/internal/util/objs.go index 431448d027f4..da8d5e72f7d8 100644 --- a/cmd/clusterctl/internal/util/objs.go +++ b/cmd/clusterctl/internal/util/objs.go @@ -21,7 +21,6 @@ import ( appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - clusterctlv1 "sigs.k8s.io/cluster-api/cmd/clusterctl/api/v1alpha3" "sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme" ) @@ -101,18 +100,6 @@ func IsResourceNamespaced(kind string) bool { } } -// IsSharedResource returns true if the resource lifecycle is shared. -func IsSharedResource(o unstructured.Unstructured) bool { - lifecycle, ok := o.GetLabels()[clusterctlv1.ClusterctlResourceLifecyleLabelName] - if !ok { - return false - } - if lifecycle == string(clusterctlv1.ResourceLifecycleShared) { - return true - } - return false -} - // FixImages alters images using the give alter func // NB. The implemented approach is specific for the provider components YAML & for the cert-manager manifest; it is not // intended to cover all the possible objects used to deploy containers existing in Kubernetes. diff --git a/docs/book/src/clusterctl/provider-contract.md b/docs/book/src/clusterctl/provider-contract.md index 8f7b189dcdb1..9d4a8503e8c1 100644 --- a/docs/book/src/clusterctl/provider-contract.md +++ b/docs/book/src/clusterctl/provider-contract.md @@ -107,22 +107,6 @@ It is strongly recommended that: * Bootstrap providers release a file called ` bootstrap-components.yaml` * Control plane providers release a file called `control-plane-components.yaml` -#### Shared and instance components - -The objects contained in a component YAML file can be divided in two sets: - -* Instance specific objects, like the Deployment for the controller, the ServiceAccount used for running the controller - and the related RBAC rules. -* The objects that are shared among all the provider instances, like e.g. CRDs, ValidatingWebhookConfiguration or the - Deployment implementing the web-hook servers and related Service and Certificates. - -As per the Cluster API contract, all the shared objects are expected to be deployed in a namespace named `capi-webhook-system` -(if applicable). - -clusterctl implements a different lifecycle for shared resources e.g. -* Ensuring that the version of the shared objects for each provider matches the latest version installed in the cluster. -* Ensuring that deleting an instance of a provider does not destroy shared resources unless explicitly requested by the user. - #### Target namespace The instance components should contain one Namespace object, which will be used as the default target namespace