Must override target allocator name when app/name is overridden

[jaronoff97]

Component(s)

operator, target allocator

What happened?

Description

Right now if a user sets the app name label for the collector, we don't propagate down to the target allocator which results in the following:

  Warning  Error   59s (x2553 over 10m)    opentelemetry-operator  failed to create objects for kube-otel-stack-metrics: Deployment.apps "kube-otel-stack-metrics-targetallocator" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"app":"kube-otel-stack-metrics", "app.kubernetes.io/component":"opentelemetry-targetallocator", "app.kubernetes.io/instance":"staging.kube-otel-stack-metrics", "app.kubernetes.io/managed-by":"opentelemetry-operator", "app.kubernetes.io/name":"kube-otel-stack-metrics", "app.kubernetes.io/part-of":"opentelemetry", "app.kubernetes.io/version":"0.90.0", "chart":"kube-otel-stack-0.2.12", "cost_environment":"staging", "cost_function":"saas", "cost_service":"kube-otel-stack-metrics", "environment":"staging", "heritage":"Helm", "release":"kube-otel-stack"}: `selector` does not match template `labels`

Steps to Reproduce

override app.kubernetes.io/name in a collector CRD with TA enabled.

Expected Result

TA rolls out fine

Actual Result

operator errors.

Kubernetes Version

1.29.0

Operator version

0.93.0

Collector version

0.93.0

Environment information

Environment

OS: (e.g., "Ubuntu 20.04")
Compiler(if manually compiled): (e.g., "go 14.2")

Log output

No response

Additional context

No response

[changexd]

Hi @jaronoff97, if this issue hasn't been assigned yet, I would like to help this out :)

[pavolloffay]

Please take it @changexd

[changexd]

Hi @pavolloffay @jaronoff97 , I was able to make app name label be propagated to the target allocator with additional checks and mutation of labels, but I would like to know two things before creating a PR:

1. this will cause the difference between app.kubernetes.io/name and instance name of the TA, is this expected?

2. since there are app.kubernetes.io/instance and app.kubernetes.io/component to make sure we select the TA managed objects, is there a reason behind adding app.kubernetes.io/name specifically for the TA? since I found this comment and I am not sure what painful breaking change will happen though I tried to figure it out.

	// TargetAllocator uses the name label as well for selection
	// This is inconsistent with the Collector, but changing is a somewhat painful breaking change

[jaronoff97]

IMO what we have today is actually broken – by not propagating the name for the selector we make it possible for the operator to error when making the TA's deployment. Either we should change the selector or change the labels to match the selector. cc @swiatekm-sumo, we talked about this sometime ago...

[swiatekm]

The reason I marked that as a painful breaking change is that the field is immutable, and changing it would require the controller to recreate the Deployment. See https://github.com/open-telemetry/opentelemetry-operator/pull/2455/files#r1433035505 for the original discussion.

If it's a problem, we might want to do it anyway, or at least not overwrite the name label if it already exists. The fact that it does this is a bug introduced in #2455, so at the very least we should have a test checking for it.

[changexd]

thanks for clearing this out, in this case I'll change the selector to propagate name label? since it seems like not overwriting the name label if already exists is our primary goal here.

[swiatekm]

That sounds like a fix we can easily do without breaking anything, yes.