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

Jump to bottom

🐛 Allow rancher cluster to be deleted from UI #87

Merged
merged 3 commits into from
Sep 1, 2023
Merged

🐛 Allow rancher cluster to be deleted from UI #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
08fff7f
Ensure the rancher cluster change actually propagates event to capi c…
Danil-Grigorev Aug 22, 2023
f1410e8
Fix annotation logic to be used automatically on every imported CAPI …
Danil-Grigorev Aug 22, 2023
3a7b43f
Apply capi cluster predicates on mapper methods
Danil-Grigorev Aug 22, 2023
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
61 changes: 33 additions & 28 deletions internal/controllers/import_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,10 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/source"

clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
Expand All @@ -35,6 +37,7 @@ import (

"github.com/rancher-sandbox/rancher-turtles/internal/rancher"
turtlesannotations "github.com/rancher-sandbox/rancher-turtles/util/annotations"
turtelesnaming "github.com/rancher-sandbox/rancher-turtles/util/naming"
turtlespredicates "github.com/rancher-sandbox/rancher-turtles/util/predicates"
)

Expand Down Expand Up @@ -86,7 +89,7 @@ func (r *CAPIImportReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma

err = c.Watch(
source.Kind(mgr.GetCache(), u),
handler.EnqueueRequestsFromMapFunc(r.rancherClusterToCapiCluster(ctx)),
handler.EnqueueRequestsFromMapFunc(r.rancherClusterToCapiCluster(ctx, capiPredicates)),
//&handler.EnqueueRequestForOwner{OwnerType: &clusterv1.Cluster{}},
)
if err != nil {
Expand All @@ -96,7 +99,7 @@ func (r *CAPIImportReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Ma
ns := &corev1.Namespace{}
err = c.Watch(
source.Kind(mgr.GetCache(), ns),
handler.EnqueueRequestsFromMapFunc(r.namespaceToCapiClusters(ctx)),
handler.EnqueueRequestsFromMapFunc(r.namespaceToCapiClusters(ctx, capiPredicates)),
)

if err != nil {
Expand Down Expand Up @@ -173,7 +176,7 @@ func (r *CAPIImportReconciler) Reconcile(ctx context.Context, req ctrl.Request)
func (r *CAPIImportReconciler) reconcile(ctx context.Context, capiCluster *clusterv1.Cluster) (ctrl.Result, error) {
// fetch the rancher clusters
rancherClusterHandler := rancher.NewClusterHandler(ctx, r.Client)
rancherClusterName := rancherClusterNameFromCAPICluster(capiCluster.Name)
rancherClusterName := turtelesnaming.Name(capiCluster.Name).ToRancherName()

rancherCluster, err := rancherClusterHandler.Get(client.ObjectKey{Namespace: capiCluster.Namespace, Name: rancherClusterName})
if err != nil {
Expand All @@ -184,7 +187,7 @@ func (r *CAPIImportReconciler) reconcile(ctx context.Context, capiCluster *clust

if rancherCluster != nil {
if !rancherCluster.ObjectMeta.DeletionTimestamp.IsZero() {
return r.reconcileDelete(ctx, capiCluster, rancherCluster)
return r.reconcileDelete(ctx, capiCluster)
}
}

Expand All @@ -209,7 +212,7 @@ func (r *CAPIImportReconciler) reconcileNormal(ctx context.Context, capiCluster

if err := rancherClusterHandler.Create(&rancher.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: rancherClusterNameFromCAPICluster(capiCluster.Name),
Name: turtelesnaming.Name(capiCluster.Name).ToRancherName(),
Namespace: capiCluster.Namespace,
OwnerReferences: []metav1.OwnerReference{
{
Expand Down Expand Up @@ -303,27 +306,23 @@ func (r *CAPIImportReconciler) shouldAutoImport(ctx context.Context, capiCluster
return autoImport, nil
}

func (r *CAPIImportReconciler) reconcileDelete(ctx context.Context, capiCluster *clusterv1.Cluster,
rancherCluster *rancher.Cluster,
) (ctrl.Result, error) {
func (r *CAPIImportReconciler) reconcileDelete(ctx context.Context, capiCluster *clusterv1.Cluster) (ctrl.Result, error) {
log := log.FromContext(ctx)
log.Info("Reconciling CAPI Cluster Delete")
log.Info("Reconciling rancher cluster deletion")
Copy link
Member

Choose a reason for hiding this comment

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

but we are reconciling CAPI cluster deletion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right now we have to act upon the fact that rancher cluster is getting deleted


// If the Rancher Cluster has ClusterImportedAnnotation annotation that said we imported it,
// then annotate the CAPI cluster so that we don't auto-import again.
if turtlesannotations.HasClusterImportAnnotation(rancherCluster) {
log.Info(fmt.Sprintf("rancher cluster %s has %s annotation, so annotating CAPI cluster %s",
rancherCluster.Name, turtlesannotations.ClusterImportedAnnotation, capiCluster.Name))

annotations := capiCluster.GetAnnotations()
if annotations == nil {
annotations = map[string]string{}
}
// If the Rancher Cluster was already imported, then annotate the CAPI cluster so that we don't auto-import again.
Copy link
Contributor

Choose a reason for hiding this comment

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

where is the check in the code from this comment, I seem to be missing it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/rancher-sandbox/rancher-turtles/pull/87/files#diff-df81a5620bfede50d1c70fd0ebb982e652d28721e15b825c52dddcf97ceda0deR384

Copy link
Contributor

Choose a reason for hiding this comment

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

can we log the message in that case, it was not obvious from first look

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Operator does not reconcile non-imported rancher clusters. Imported rancher cluster follows the <capi-cluster>-capi naming convention. Therefore it is not possible here that any of the reconcile code, including reconcile delete would be executed on non-imported rancher cluster, unless it follows given naming convention. So log message on the https://github.com/rancher-sandbox/rancher-turtles/pull/87/files#diff-df81a5620bfede50d1c70fd0ebb982e652d28721e15b825c52dddcf97ceda0deR311 and https://github.com/rancher-sandbox/rancher-turtles/pull/87/files#diff-df81a5620bfede50d1c70fd0ebb982e652d28721e15b825c52dddcf97ceda0deR314 are both describing what is happening with the cluster. Don’t see a need to make it more vivid than that.

Copy link
Member

Choose a reason for hiding this comment

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

It will also be logged in the predicate itself. My concern is that as far as I know predicates might not always work as expected and we should structure our code like they don't exist, it's nice if they filter out resources for us but don't expect them to always do that. I wouldn't remove the code.

log.Info(fmt.Sprintf("Rancher cluster is being removed, annotating CAPI cluster %s with %s",
capiCluster.Name,
turtlesannotations.ClusterImportedAnnotation))

annotations[turtlesannotations.ClusterImportedAnnotation] = "true"
capiCluster.SetAnnotations(annotations)
annotations := capiCluster.GetAnnotations()
if annotations == nil {
annotations = map[string]string{}
}

annotations[turtlesannotations.ClusterImportedAnnotation] = "true"
capiCluster.SetAnnotations(annotations)

return ctrl.Result{}, nil
}

Expand Down Expand Up @@ -369,11 +368,11 @@ func shouldImport(obj metav1.Object) (hasLabel bool, labelValue bool) {
return true, autoImport
}

func (r *CAPIImportReconciler) rancherClusterToCapiCluster(ctx context.Context) handler.MapFunc {
func (r *CAPIImportReconciler) rancherClusterToCapiCluster(ctx context.Context, clusterPredicate predicate.Funcs) handler.MapFunc {
log := log.FromContext(ctx)

return func(_ context.Context, o client.Object) []ctrl.Request {
key := client.ObjectKey{Name: o.GetName(), Namespace: o.GetNamespace()}
key := client.ObjectKey{Name: turtelesnaming.Name(o.GetName()).ToCapiName(), Namespace: o.GetNamespace()}

capiCluster := &clusterv1.Cluster{}
if err := r.Client.Get(ctx, key, capiCluster); err != nil {
Expand All @@ -384,11 +383,15 @@ func (r *CAPIImportReconciler) rancherClusterToCapiCluster(ctx context.Context)
return nil
}

if !clusterPredicate.Generic(event.GenericEvent{Object: capiCluster}) {
return nil
}

return []ctrl.Request{{NamespacedName: client.ObjectKey{Namespace: capiCluster.Namespace, Name: capiCluster.Name}}}
}
}

func (r *CAPIImportReconciler) namespaceToCapiClusters(ctx context.Context) handler.MapFunc {
func (r *CAPIImportReconciler) namespaceToCapiClusters(ctx context.Context, clusterPredicate predicate.Funcs) handler.MapFunc {
log := log.FromContext(ctx)

return func(_ context.Context, o client.Object) []ctrl.Request {
Expand Down Expand Up @@ -416,7 +419,13 @@ func (r *CAPIImportReconciler) namespaceToCapiClusters(ctx context.Context) hand
}

reqs := []ctrl.Request{}

for _, cluster := range capiClusters.Items {
cluster := cluster
if !clusterPredicate.Generic(event.GenericEvent{Object: &cluster}) {
continue
}

reqs = append(reqs, ctrl.Request{
NamespacedName: client.ObjectKey{
Namespace: cluster.Namespace,
Expand All @@ -429,10 +438,6 @@ func (r *CAPIImportReconciler) namespaceToCapiClusters(ctx context.Context) hand
}
}

func rancherClusterNameFromCAPICluster(capiClusterName string) string {
return fmt.Sprintf("%s-capi", capiClusterName)
}

func (r *CAPIImportReconciler) downloadManifest(url string) (string, error) {
resp, err := http.Get(url) //nolint:gosec,noctx
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion internal/controllers/import_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"github.com/rancher-sandbox/rancher-turtles/internal/controllers/testdata"
"github.com/rancher-sandbox/rancher-turtles/internal/rancher"
"github.com/rancher-sandbox/rancher-turtles/internal/test"
turtelesnaming "github.com/rancher-sandbox/rancher-turtles/util/naming"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
Expand Down Expand Up @@ -57,7 +58,7 @@ var _ = Describe("reconcile CAPI Cluster", func() {

rancherCluster = &rancher.Cluster{
ObjectMeta: metav1.ObjectMeta{
Name: rancherClusterNameFromCAPICluster(capiCluster.Name),
Name: turtelesnaming.Name(capiCluster.Name).ToRancherName(),
Namespace: testNamespace,
},
}
Expand Down
21 changes: 21 additions & 0 deletions util/naming/name_converter.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package naming

import (
"fmt"
"strings"
)

var rancherCAPISuffix = "-capi"

// Name is a wrapper around CAPI/Rancher cluster names to simplify convertation between the two.
type Name string

// ToRancherName converts a CAPI cluster name to Rancher cluster name.
func (n Name) ToRancherName() string {
return fmt.Sprintf("%s%s", n.ToCapiName(), rancherCAPISuffix)
}

// ToCapiName converts a Rancher cluster name to CAPI cluster name.
func (n Name) ToCapiName() string {
return strings.TrimSuffix(string(n), rancherCAPISuffix)
}
38 changes: 38 additions & 0 deletions util/naming/name_converter_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
package naming

import (
"testing"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

var _ = Describe("Cluster name mapping", func() {

It("Should suffix rancher cluster name with -capi", func() {
name := Name("some-cluster").ToRancherName()
Expect(name).To(Equal("some-cluster-capi"))
})

It("Should only add suffix once", func() {
name := Name("some-cluster").ToRancherName()
name = Name(name).ToRancherName()
Expect(string(name)).To(Equal("some-cluster-capi"))
})

It("should remove suffix from rancher cluster", func() {
name := Name("some-cluster").ToRancherName()
name = Name(name).ToCapiName()
Expect(string(name)).To(Equal("some-cluster"))
})

It("should remove suffix from rancher cluster only if it is present", func() {
name := Name("some-cluster").ToCapiName()
Expect(string(name)).To(Equal("some-cluster"))
})
})

func TestNameConverter(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Test naming convention")
}