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

🐛 Change delete behaviour to respect inventory #5044

Merged
merged 1 commit into from
Sep 22, 2021
+227 −11
Merged

🐛 Change delete behaviour to respect inventory #5044

Show file tree
Hide file tree
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
13 changes: 11 additions & 2 deletions cmd/clusterctl/client/cluster/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,15 @@ const (
validatingWebhookConfigurationKind = "ValidatingWebhookConfiguration"
mutatingWebhookConfigurationKind = "MutatingWebhookConfiguration"
customResourceDefinitionKind = "CustomResourceDefinition"
providerGroupKind = "Provider.clusterctl.cluster.x-k8s.io"
)

// DeleteOptions holds options for ComponentsClient.Delete func.
type DeleteOptions struct {
Provider clusterctlv1.Provider
IncludeNamespace bool
IncludeCRDs bool
SkipInventory bool
}

// ComponentsClient has methods to work with provider components in the cluster.
Expand Down Expand Up @@ -152,7 +154,6 @@ func (p *providerComponents) Delete(options DeleteOptions) error {
if !options.IncludeCRDs && isCRD {
continue
}

// If the resource is a namespace
isNamespace := obj.GroupVersionKind().Kind == namespaceKind
if isNamespace {
Expand All @@ -168,19 +169,27 @@ func (p *providerComponents) Delete(options DeleteOptions) error {
namespacesToDelete.Insert(obj.GetName())
}

// If the resource is part of the inventory for clusterctl don't delete it at this point as losing this information makes the
// upgrade function non-reentrant. Instead keep the inventory objects around until the upgrade is finished and working and
// delete them at the end of the upgrade flow.
isInventory := obj.GroupVersionKind().GroupKind().String() == providerGroupKind
fabriziopandini marked this conversation as resolved.
Show resolved Hide resolved
if isInventory && options.SkipInventory {
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)
}

Expand Down
212 changes: 206 additions & 6 deletions cmd/clusterctl/client/cluster/components_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,12 @@ limitations under the License.
package cluster

import (
"fmt"
"testing"

"github.com/google/go-cmp/cmp"
"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme"

. "github.com/onsi/gomega"

corev1 "k8s.io/api/core/v1"
Expand All @@ -42,7 +46,11 @@ func Test_providerComponents_Delete(t *testing.T) {
crd.SetKind("CustomResourceDefinition")
crd.SetName("crd1")
crd.SetLabels(labels)

providerInventory := unstructured.Unstructured{}
fabriziopandini marked this conversation as resolved.
Show resolved Hide resolved
providerInventory.SetAPIVersion(clusterctlv1.GroupVersion.String())
providerInventory.SetKind("Provider")
providerInventory.SetName("providerOne")
providerInventory.SetLabels(labels)
mutatingWebhook := unstructured.Unstructured{}
mutatingWebhook.SetAPIVersion("admissionregistration.k8s.io/v1beta1")
mutatingWebhook.SetKind("MutatingWebhookConfiguration")
Expand Down Expand Up @@ -83,6 +91,8 @@ func Test_providerComponents_Delete(t *testing.T) {
},
// CRDs (should be deleted only if includeCRD)
&crd,
// Inventory should be deleted ony if skipInventory
&providerInventory,
&mutatingWebhook,
// A cluster-wide provider component (should always be deleted)
&rbacv1.ClusterRole{
Expand Down Expand Up @@ -120,7 +130,9 @@ func Test_providerComponents_Delete(t *testing.T) {
provider clusterctlv1.Provider
includeNamespace bool
includeCRD bool
skipInventory bool
}

type wantDiff struct {
object corev1.ObjectReference
deleted bool
Expand All @@ -133,11 +145,12 @@ func Test_providerComponents_Delete(t *testing.T) {
wantErr bool
}{
{
name: "Delete provider while preserving Namespace and CRDs",
name: "Delete provider while preserving Namespace and CRDs and providerInventory",
args: args{
provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)},
includeNamespace: false,
includeCRD: false,
skipInventory: true,
},
wantDiff: []wantDiff{
{object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be preserved
Expand All @@ -148,15 +161,17 @@ func Test_providerComponents_Delete(t *testing.T) {
{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: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: false}, // providerInventory should be preserved
},
wantErr: false,
},
{
name: "Delete provider and provider namespace, while preserving CRDs",
name: "Delete provider and provider namespace, while preserving CRDs and providerInventory",
args: args{
provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)},
includeNamespace: true,
includeCRD: false,
skipInventory: true,
},
wantDiff: []wantDiff{
{object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: true}, // namespace should be deleted
Expand All @@ -167,15 +182,17 @@ func Test_providerComponents_Delete(t *testing.T) {
{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: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: false}, // providerInventory should be preserved
},
wantErr: false,
},
{
name: "Delete provider and provider CRDs, while preserving the provider namespace",
name: "Delete provider and provider CRDs, while preserving the provider namespace and the providerInventory",
args: args{
provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)},
includeNamespace: false,
includeCRD: true,
skipInventory: true,
},
wantDiff: []wantDiff{
{object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be preserved
Expand All @@ -186,15 +203,38 @@ func Test_providerComponents_Delete(t *testing.T) {
{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: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: false}, // providerInventory should be preserved
},
wantErr: false,
},
{
name: "Delete providerInventory and provider while preserving provider CRDs and provider namespace",
args: args{
provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)},
includeNamespace: false,
includeCRD: false,
skipInventory: false,
},
wantDiff: []wantDiff{
{object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: false}, // namespace should be deleted
{object: corev1.ObjectReference{APIVersion: "apiextensions.k8s.io/v1beta1", Kind: "CustomResourceDefinition", Name: "crd1"}, deleted: false}, // crd should not 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: "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: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: true}, // providerInventory should be deleted
},
wantErr: false,
},
{
name: "Delete provider, provider namespace and provider CRDs",
name: "Delete provider, provider namespace and provider CRDs and the providerInventory",
args: args{
provider: clusterctlv1.Provider{ObjectMeta: metav1.ObjectMeta{Name: "infrastructure-infra", Namespace: "ns1"}, ProviderName: "infra", Type: string(clusterctlv1.InfrastructureProviderType)},
includeNamespace: true,
includeCRD: true,
skipInventory: false,
},
wantDiff: []wantDiff{
{object: corev1.ObjectReference{APIVersion: "v1", Kind: "Namespace", Name: "ns1"}, deleted: true}, // namespace should be deleted
Expand All @@ -205,20 +245,23 @@ func Test_providerComponents_Delete(t *testing.T) {
{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: clusterctlv1.GroupVersion.String(), Kind: "Provider", Name: "providerOne"}, deleted: true}, // providerInventory should be deleted
},
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

proxy := test.NewFakeProxy().WithObjs(initObjs...)

c := newComponentsClient(proxy)

err := c.Delete(DeleteOptions{
Provider: tt.args.provider,
IncludeNamespace: tt.args.includeNamespace,
IncludeCRDs: tt.args.includeCRD,
SkipInventory: tt.args.skipInventory,
})
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
Expand Down Expand Up @@ -295,3 +338,160 @@ func Test_providerComponents_DeleteCoreProviderWebhookNamespace(t *testing.T) {
g.Expect(len(nsList.Items)).Should(Equal(0))
})
}

func Test_providerComponents_Create(t *testing.T) {
labelsOne := map[string]string{
clusterv1.ProviderLabelName: "infrastructure-infra",
}
commonObjects := []client.Object{
// Namespace for the provider
&corev1.Namespace{
TypeMeta: metav1.TypeMeta{
Kind: "Namespace",
APIVersion: "v1",
},
ObjectMeta: metav1.ObjectMeta{
Name: "ns1",
Labels: labelsOne,
},
},
// A cluster-wide provider component.
&rbacv1.ClusterRole{
TypeMeta: metav1.TypeMeta{
Kind: "ClusterRole",
APIVersion: rbacv1.SchemeGroupVersion.WithKind("ClusterRole").GroupVersion().String(),
},
ObjectMeta: metav1.ObjectMeta{
Name: "ns1-cluster-role", // global objects belonging to the provider have a namespace prefix.
Labels: labelsOne,
},
},
}
// A namespaced provider component as a pod.
podOne := &corev1.Pod{
TypeMeta: metav1.TypeMeta{
Kind: "Pod",
APIVersion: "v1",
},
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns1",
Name: "pod1",
Labels: labelsOne,
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Image: "pod1-v1",
},
},
},
}
// podTwo is the same as podTwo but has an image titled pod1-v2.
podTwo := &corev1.Pod{
TypeMeta: metav1.TypeMeta{
Kind: "Pod",
APIVersion: "v1",
},
ObjectMeta: metav1.ObjectMeta{
Namespace: "ns1",
Name: "pod1",
Labels: labelsOne,
},
Spec: corev1.PodSpec{
Containers: []corev1.Container{
{
Image: "pod1-v2",
},
},
},
}
type args struct {
objectsToCreate []client.Object
initObjects []client.Object
}

tests := []struct {
name string
args args
want []client.Object
wantErr bool
}{
{
name: "Create Provider Pod, Namespace and ClusterRole",
args: args{
objectsToCreate: append(commonObjects, podOne),
initObjects: []client.Object{},
},
want: append(commonObjects, podOne),
wantErr: false,
},
{
name: "Upgrade Provider Pod, Namespace and ClusterRole",
args: args{
objectsToCreate: append(commonObjects, podTwo),
initObjects: append(commonObjects, podOne),
},
want: append(commonObjects, podTwo),
wantErr: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
g := NewWithT(t)

proxy := test.NewFakeProxy().WithObjs(tt.args.initObjects...)
c := newComponentsClient(proxy)
var unstructuredObjectsToCreate []unstructured.Unstructured
for _, obj := range tt.args.objectsToCreate {
uns := &unstructured.Unstructured{}
if err := scheme.Scheme.Convert(obj, uns, nil); err != nil {
g.Expect(fmt.Errorf("%v %v could not be converted to unstructured", err.Error(), obj)).NotTo(HaveOccurred())
}
unstructuredObjectsToCreate = append(unstructuredObjectsToCreate, *uns)
}
err := c.Create(unstructuredObjectsToCreate)
if tt.wantErr {
g.Expect(err).To(HaveOccurred())
return
}

g.Expect(err).NotTo(HaveOccurred())

cs, err := proxy.NewClient()
g.Expect(err).NotTo(HaveOccurred())

for _, item := range tt.want {
obj := &unstructured.Unstructured{}
obj.SetKind(item.GetObjectKind().GroupVersionKind().Kind)
obj.SetAPIVersion(item.GetObjectKind().GroupVersionKind().GroupVersion().String())
key := client.ObjectKey{
Namespace: item.GetNamespace(),
Name: item.GetName(),
}

err := cs.Get(ctx, key, obj)

if err != nil && !apierrors.IsNotFound(err) {
t.Fatalf("Failed to get %v from the cluster: %v", key, err)
}
g.Expect(obj.GetNamespace()).To(Equal(item.GetNamespace()), cmp.Diff(obj.GetNamespace(), item.GetNamespace()))
g.Expect(obj.GetName()).To(Equal(item.GetName()), cmp.Diff(obj.GetName(), item.GetName()))
g.Expect(obj.GetAPIVersion()).To(Equal(item.GetObjectKind().GroupVersionKind().GroupVersion().String()), cmp.Diff(obj.GetAPIVersion(), item.GetObjectKind().GroupVersionKind().GroupVersion().String()))
if item.GetObjectKind().GroupVersionKind().Kind == "Pod" {
p1, okp1 := item.(*corev1.Pod)
if !(okp1) {
g.Expect(fmt.Errorf("%v %v could retrieve pod", err.Error(), obj)).NotTo(HaveOccurred())
}
p2 := &corev1.Pod{}
if err := scheme.Scheme.Convert(obj, p2, nil); err != nil {
g.Expect(fmt.Errorf("%v %v could not be converted to unstructured", err.Error(), obj)).NotTo(HaveOccurred())
}
if len(p1.Spec.Containers) == 0 || len(p2.Spec.Containers) == 0 {
g.Expect(fmt.Errorf("%v %v could not be converted to unstructured", err.Error(), obj)).NotTo(HaveOccurred())
}
g.Expect(p1.Spec.Containers[0].Image).To(Equal(p2.Spec.Containers[0].Image), cmp.Diff(obj.GetNamespace(), item.GetNamespace()))
}
}
})
}
}
3 changes: 2 additions & 1 deletion cmd/clusterctl/client/cluster/upgrader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,12 @@ func (u *providerUpgrader) doUpgrade(upgradePlan *UpgradePlan) error {
return err
}

// Delete the provider, preserving CRD and namespace.
// Delete the provider, preserving CRD, namespace and the inventory.
if err := u.providerComponents.Delete(DeleteOptions{
Provider: upgradeItem.Provider,
IncludeNamespace: false,
IncludeCRDs: false,
SkipInventory: true,
}); err != nil {
return err
}
Expand Down
Loading