feat!: Remove Crossplane standard tags

[MisterMX]

Description of your changes

This is a proposal to remove the tagger initializer that always adds the "standard" Crossplane tags to every managed resource:

• crossplane-kind
• crossplane-name
• crossplane-providerconfig

There have been numerous issues with tags that were added by default, like #1930, #1419, #985 and many more.

The behaviour of setting the tags is also not standardized: Some controllers always overwrite existing tags others are only setting it if the value does not exist. Arrays of tags don't play very nice with compositions as well.

Since there is no technical requirement for this feature, one way to overcome the issues with it, is to remove it completely.

Fixes #1498

I have:

• Read and followed Crossplane's contribution process.
• Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

[chlunde]

FYI; we use theese tags to know which resources are managed by crossplane, which are orphan etc.
We should get some feedback before removing these as this is a breaking change.
It might need to have a conditional flag if many are using this.

I think we could have a composition function which adds (similar) tags instead.

I intended to write a function that just sorted and added back the default tags to avoid extra reconciles when the order changes or default tags are lost.

One reason why we have some of these issues is that tags are not defined as maps. Compositions work much better with patching those than the keyed arrays we have now.

[MisterMX]

A few more thoughts on this:

Existing resources don't get their tags removed so it is not technically breaking. However, if some external scripts (like a resource cleanup) depend on the tags being present it requires some changes on the platform operator site.

If resources are deployed using compositions, the above mentioned tags can be added to the base manifest to have them stick around. I don't know about functions but it should be easy as well to just add the tags there, too.

I am not really a fan to hide this behavior behind a feature flag, since it only makes the code more complicated and adds another edge case to test. Also, there is still no standard way of adding default tags a resource.

[tkaesserfm]

Even though it feels harsh to me to remove the tags completely, looking at the code i think it might be the best trade-off.
Adding a feature flag would make sense if there would be some centralized way of handling this. But the fact that this isn't possible in a single point just highlights what @MisterMX already said, that there is no standard which naturally creates a mess here.
In an ideal world i would love to see this handled globally and without much need to interfere from the user. But to make this happen there would need to exist a standard to begin with and a unified implementation. (in the best case across crossplane providers)
However as of today i think even if it hurts it makes sense to offload the management of these tags to the user as the issues with constant reconciliation are real and can quickly mess especially with large scale setups.

[chlunde]

longer rant + code example using reflection

I don't think we could have a unified implementation with generics (which I was really hoping for), but using reflection is still an option:

package main

import (
	"reflect"

	"github.com/davecgh/go-spew/spew"
)

func main() {
	current := []Tag{
		Tag{Name: "k1", Value: "v1"},
		Tag{Name: "k2", Value: "v2"},
	}
	result = updateOrAddTags(current, map[string]string{
		"k2": "new",
		"k3": "v3",
	})
	spew.Dump(current)
}

type Tag struct {
	Name  string
	Value string
}

func newTag(k, v string, typ reflect.Type) any {
	tag := reflect.Indirect(reflect.New(typ))

	tag.FieldByName("Name").SetString(k)
	tag.FieldByName("Value").SetString(v)

	return tag.Interface()
}

func updateOrAddTags[T any](current []T, add map[string]string) []T {
	updated := make(map[string]bool)
	items := reflect.ValueOf(current)
	if items.Kind() == reflect.Slice {
		for i := 0; i < items.Len(); i++ {
			item := items.Index(i)
			k := item.FieldByName("Name").String()
			// v := item.FieldByName("Value").String()
			if v2, ok := add[k]; ok {
				updated[k] = true

				item.FieldByName("Value").SetString(v2)
			}
		}
	}

	for k, v := range add {
		if !updated[k] {
			items = reflect.Append(items, reflect.ValueOf(newTag(k, v, items.Type().Elem())))
		}
	}

	return items.Interface().([]T)
}

But it still leaves the flapping issue. I think that's a problem which would require fixes in crossplane (or requiring using composition functions), or changing to map[string]string which would be breaking.

If we would want a user toggle, we could wrap managed.GetExternalTags I think

So yeah, in the end I guess it would be easier to suggest that everyone manually tag all resources they want tagged.

[chlunde]

I think it's important to flag this in the release notes

[MisterMX]

I think a good replacement for default tags would be a place in ProviderConfig to define a set of "global" tags that should be added to every MR that is reconciled with it. That has been discussed once in #1306 but never gained traction.

It does not fix the problem of a generic tagging solution in Go but it would give users a better control over the tags to be set. #1378 could be a base for that, but it requires some reworking.