fix: don't delete coredns unless previously managed by kamaji

[Marlinc]

This is still a draft

[netlify]

✅ Deploy Preview for kamaji-documentation canceled.

Name	Link
🔨 Latest commit	2492801
🔍 Latest deploy log	https://app.netlify.com/sites/kamaji-documentation/deploys/66c727341cc94d00089fbef7

[prometherion]

Several adopters reported this, and it's a neat feature/bug.

I'll be happy to have a different approach, tho: the Resource Handler is an interface with the following functions.

kamaji/internal/resources/resource.go

Lines 23 to 31 in d8dfc62

	type Resource interface {
	Define(context.Context, *kamajiv1alpha1.TenantControlPlane) error
	ShouldCleanup(*kamajiv1alpha1.TenantControlPlane) bool
	CleanUp(context.Context, *kamajiv1alpha1.TenantControlPlane) (bool, error)
	CreateOrUpdate(context.Context, *kamajiv1alpha1.TenantControlPlane) (controllerutil.OperationResult, error)
	GetName() string
	ShouldStatusBeUpdated(context.Context, *kamajiv1alpha1.TenantControlPlane) bool
	UpdateTenantControlPlaneStatus(context.Context, *kamajiv1alpha1.TenantControlPlane) error
	}

The function ShouldCleanup is the one responsible for deleting the CoreDNS, as well as other add-ons.

kamaji/internal/resources/addons/coredns.go

Lines 77 to 79 in d8dfc62

	func (c *CoreDNS) ShouldCleanup(tcp *kamajiv1alpha1.TenantControlPlane) bool {
	return tcp.Spec.Addons.CoreDNS == nil
	}

For the given CoreDNS, we're just checking if the Specification addons are set to nil, but this is not efficient and not idempotent.

Since the TenantControlPlane API is storing the status, we could rely on a different condition here, such as:

  return tcp.Spec.Addons.CoreDNS == nil && tcp.Status.Addons.CoreDNS.Enabled

The said condition should be true when CoreDNS is disabled in the spec, but the status still reports an enabled CoreDNS deployment.

Upon completion of the deletion here with an Updated resource, a status update is performed, meaning we're ending up with the correct reporting

kamaji/internal/resources/addons/coredns.go

Lines 186 to 191 in d8dfc62

	func (c *CoreDNS) UpdateTenantControlPlaneStatus(_ context.Context, tcp *kamajiv1alpha1.TenantControlPlane) error {
	tcp.Status.Addons.CoreDNS.Enabled = tcp.Spec.Addons.CoreDNS != nil
	tcp.Status.Addons.CoreDNS.LastUpdate = metav1.Now()
	return nil
	}

The same logic could be used for the other addons, maybe we could have some trouble with the konnectivity one since it is much more complicated.

[prometherion]

About the maps, you can take advantage of the utilities.MergeMaps function:

e.g.:

d.SetLabels(utilities.MergeMaps(d.GetLabels(), map[string]string{"constants.ProjectNameLabelKey": constants.ProjectNameLabelValue}))

It must be applied to all the objects, but it's going to be a one-liner and much easier to read, IMO.

[prometherion]

LGTM, feel free to convert it to Ready for review so we can get this merged