propogate labels from limitador to deployment template #183

laurafitzgerald · 2025-02-06T15:50:05Z

Closes #176

Verification

Checkout locally.
Run make run
kubectl edit deployment
Add label to spec.template.labels something like useradded: label
kubectl edit limitador limitador-sample
Add label to metadata.labels something like limitdatorcradded: label
Verify that the deployment spec.template.labels includes the following labels

app: limitador
limitador-resource: limitador-sample
useradded: label
limitdatorcradded: label

codecov-commenter · 2025-02-06T15:54:27Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 75.00000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 82.23%. Comparing base (48cb256) to head (9b6b9d0).

Files with missing lines	Patch %	Lines
pkg/helpers/helpers.go	22.22%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #183      +/-   ##
==========================================
- Coverage   83.47%   82.23%   -1.24%     
==========================================
  Files          18       18              
  Lines        1210     1233      +23     
==========================================
+ Hits         1010     1014       +4     
- Misses        152      165      +13     
- Partials       48       54       +6

Flag	Coverage Δ
integration	`75.42% <53.57%> (-2.18%)`	⬇️
unit	`64.18% <70.58%> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api/v1alpha1 (u)	`100.00% <ø> (ø)`
pkg/helpers (u)	`71.73% <22.22%> (-12.05%)`	⬇️
pkg/log (u)	`93.18% <ø> (ø)`
pkg/reconcilers (u)	`73.70% <100.00%> (+0.43%)`	⬆️
pkg/limitador (u)	`97.69% <100.00%> (+0.04%)`	⬆️
controllers (i)	`71.31% <100.00%> (-2.99%)`	⬇️
pkg/upgrades	`∅ <ø> (∅)`

Files with missing lines	Coverage Δ
controllers/limitador_controller.go	`68.29% <100.00%> (-4.09%)`	⬇️
pkg/limitador/k8s_objects.go	`96.01% <100.00%> (+0.14%)`	⬆️
pkg/reconcilers/deployment.go	`96.00% <100.00%> (+0.13%)`	⬆️
pkg/helpers/helpers.go	`65.38% <22.22%> (-22.86%)`	⬇️

laurafitzgerald · 2025-02-07T11:01:50Z

pkg/reconcilers/deployment.go

@@ -37,6 +37,16 @@ func DeploymentMutator(opts ...DeploymentMutateFn) MutateFn {
 	}
 }

+func DeploymentTemplateLabelMutator(desired, existing *appsv1.Deployment) bool {


@eguzki @KevFan From (Kuadrant/authorino-operator#236 (comment)) point
Keep any other user added labels already added to the deployment pod template.
is not fully addressed here. This current iteration checks if the labels from limitador are present in the template labels and only if not does a update trigger. However any of the Mutator functions trigger an update, in this current state any manually added labels to the spec.template.labels will get lost.
for example a user that has followed this guide -> https://github.com/Kuadrant/kuadrant-operator/blob/main/doc/install/mtls-configuration.md will lose the configuration. that would be the current state.
Is the intention that that point should be addressed as part of this change or is it sufficient that

Known hardcoded labels take precedence

Merge Authorino CR labels

are now covered?

Let me propose implementation to covert any manually added labels:

In some helper.go (or whatever)

package helper func MergeMapStringString(modified *bool, existing *map[string]string, desired map[string]string) if *existing == nil { *existing = map[string]string{} } for k, v := range desired { if existingVal, ok := (*existing)[k]; !ok || v != existingVal { (*existing)[k] = v *modified = true } } }

here

import ( "github.com/kuadrant/limitador-operator/pkg/helper" (or whatever) ) func DeploymentTemplateLabelMutator(desired, existing *appsv1.Deployment) bool { updated := false helper.MergeMapStringString(&updated, &existing.Spec.Template.Labels, desired.Spec.Template.Labels) return updated }

@eguzki Nice yes I like that. About the question above.
Even with this helper method, we would still overwrite any user added labels to the deployment due to the fact that we don't have the cluster copy of the deployment here -> https://github.com/Kuadrant/limitador-operator/pull/183/files#diff-41181542228742e869e5d34e69e5c72a4bc8f1a5ddc93c6199470479fc3b73fbR55
We build the deployment and then do a create or update using that object. As we wouldn't have access to any existing user added labels (or any other configuration) that would get lost.
My question around that is, are we fine with that? aka do we tell users that everytime they update the limitador cr, any objects owned by that are at risk of losing user added configuration. I think that's the stance now. But labels haven't been a trigger up to now so are we still fine with that?

Personally, I think this doesn’t have to be part of this PR if we want to discuss it further, as I see it as an additional enhancement of behavior. With this PR, labels are hardened, and the Limitador CR becomes a source of truth for labels. The Limitador CR is the only place where a user should add or update labels to ensure they are not removed during a reconciliation event. Previous to this, labels are not reconciled by the controller from the CR, so users would be adding labels via the deployment instead 🤔

One thing to note, after reflecting on this, is the consequence of "keeping user-added labels directly in the deployment pod template." If a user adds labels to the Limitador CR and later removes them from the CR, these labels will still persist in the deployment pod template. This happens because the controller cannot differentiate between labels added by a previous CR event and those added manually to the deployment by the user. As a result, a user must remove the label from both the CR and the deployment pod template manually to fully remove it 🤔

@eguzki Nice yes I like that. About the question above. Even with this helper method, we would still overwrite any user added labels to the deployment due to the fact that we don't have the cluster copy of the deployment here -> https://github.com/Kuadrant/limitador-operator/pull/183/files#diff-41181542228742e869e5d34e69e5c72a4bc8f1a5ddc93c6199470479fc3b73fbR55 We build the deployment and then do a create or update using that object. As we wouldn't have access to any existing user added labels (or any other configuration) that would get lost. My question around that is, are we fine with that? aka do we tell users that everytime they update the limitador cr, any objects owned by that are at risk of losing user added configuration. I think that's the stance now. But labels haven't been a trigger up to now so are we still fine with that?

On this, with Eguzki's suggestion, the update / keeping of the existing labels happens as part of the MergeMapStringString function and DeploymentTemplateLabelMutator(desired, existing *appsv1.Deployment) mutator which has access to the existing and desired deployements also

Good points @KevFan

So, what is what we want to do? My idea was to implement what I explained Kuadrant/authorino-operator#236 (comment)

Basically, labels from Limitador CR are being propagated to deployment and pods. Quoting: If deployment object exist and there are more labels than those existing in the authorino CR, those are accepted and not removed. If authorino CR changes labels, the new labels need to be copied to the deployment labels.

If a user adds labels to the Limitador CR and later removes them from the CR, these labels will still persist in the deployment pod template

Right. I knew about that and I think it is a reasonable downside to have. There are ways to have better management of labels and removing those if those are removed from the Limitador CR. But the complexity to implement those does not payoff IMO. I have implemented those for volumes/volumemounts in other projects successfully (running in prod), so I can give more details.

Allowing manually adding labels to deployment is desirable because users usually end up adding custom labels for reasons we cannot foresee. And usually, adding labels is not harmful. So having the operator removing them is usually frustrating for the user.

If you think the operator should be more strict, whatever the majority decides 🤷

Ok. I think I misunderstood the modify functions. I understood that it only checks whether it should trigger update. But I see the existing and desired are pointers so we can do some modification in there? aka merge the map to include the user added lables and add the expected ones also. am i understanding that correctly now?

Allowing manually adding labels to deployment is desirable because users usually end up adding custom labels for reasons we cannot foresee. And usually, adding labels is not harmful. So having the operator removing them is usually frustrating for the user.

If you think the operator should be more strict, whatever the majority decides 🤷

I don't feel too strongly about making it more strict, but I just wanted to call out whether this is an intended behavior or consequence. Though having some documentation explaining how labels are managed by the operator would be helpful for users to understand this I think (as part of this PR if implemented or as a follow up)

Ok. I think I misunderstood the modify functions. I understood that it only checks whether it should trigger update. But I see the existing and desired are pointers so we can do some modification in there? aka merge the map to include the user added lables and add the expected ones also. am i understanding that correctly now?

Yes, it is so called mutator pattern. You get the desired and existing objects, you basically compute the difference (aka delta) and if delta is not empty, update (existing + delta) is applied to the cluster. I find it very intuitive and very flexible to implement any business logic you want.

pkg/limitador/k8s_objects_test.go

eguzki · 2025-02-10T17:33:01Z

Propagate Limitador CR labels to deployment labels as well here https://github.com/Kuadrant/limitador-operator/blob/main/pkg/limitador/k8s_objects.go#L77

eguzki

Looking very nice

eguzki · 2025-02-10T17:42:32Z

pkg/reconcilers/deployment.go

@@ -37,6 +37,16 @@ func DeploymentMutator(opts ...DeploymentMutateFn) MutateFn {
 	}
 }

+func DeploymentTemplateLabelMutator(desired, existing *appsv1.Deployment) bool {


Let me propose implementation to covert any manually added labels:

In some helper.go (or whatever)

package helper func MergeMapStringString(modified *bool, existing *map[string]string, desired map[string]string) if *existing == nil { *existing = map[string]string{} } for k, v := range desired { if existingVal, ok := (*existing)[k]; !ok || v != existingVal { (*existing)[k] = v *modified = true } } }

here

import ( "github.com/kuadrant/limitador-operator/pkg/helper" (or whatever) ) func DeploymentTemplateLabelMutator(desired, existing *appsv1.Deployment) bool { updated := false helper.MergeMapStringString(&updated, &existing.Spec.Template.Labels, desired.Spec.Template.Labels) return updated }

pkg/limitador/k8s_objects.go

pkg/helpers/helpers.go

pkg/limitador/k8s_objects.go

KevFan · 2025-02-14T09:21:53Z

pkg/helpers/helpers.go

+func MergeMapStringString(modified *bool, existing map[string]string, desired *map[string]string) {
+	if existing == nil {
+		existing = map[string]string{}
+	}
+
+	// for each desired key value set, e.g. labels
+	// check if it's present in existing. if not add it to existing.
+	// e.g. preserving existing labels while adding those that are in the desired set.
+	for desiredKey, desiredValue := range *desired {
+		if existingValue, exists := (existing)[desiredKey]; !exists || existingValue != desiredValue {
+			(*desired)[desiredKey] = existingValue
+			*modified = true
+		}
+	}


nit: Go maps are by default passed by reference, I don't think we need deference desired *map[string]string typically? Would make the function a bit simpler 🤔

Suggested change

func MergeMapStringString(modified *bool, existing map[string]string, desired *map[string]string) {

if existing == nil {

existing = map[string]string{}

}

// for each desired key value set, e.g. labels

// check if it's present in existing. if not add it to existing.

// e.g. preserving existing labels while adding those that are in the desired set.

for desiredKey, desiredValue := range *desired {

if existingValue, exists := (existing)[desiredKey]; !exists || existingValue != desiredValue {

(*desired)[desiredKey] = existingValue

*modified = true

}

}

func MergeMapStringString(modified *bool, existing map[string]string, desired map[string]string) {

if existing == nil {

existing = map[string]string{}

}

// for each desired key value set, e.g. labels

// check if it's present in existing. if not add it to existing.

// e.g. preserving existing labels while adding those that are in the desired set.

for desiredKey, desiredValue := range desired {

if existingValue, exists := existing[desiredKey]; !exists || existingValue != desiredValue {

desired[desiredKey] = existingValue

*modified = true

}

}

If you check my proposal, it is the existing map which is passed as a pointer, because it could be empty and it is being initialized within the function. The desired map is read-only so, no need to be passed as a pointer. a reference is enough.

And it is the existing map which needs to be updated. The desired represents an idea, a willing. The actual thing is the existing.

Thanks @eguzki I though I had addressed it, i updated the comment but not the assignment. Should be good now. 👍

Signed-off-by: Laura Fitzgerald <[email protected]>

laurafitzgerald force-pushed the label-propagation branch from d496720 to 6530652 Compare February 6, 2025 15:51

laurafitzgerald force-pushed the label-propagation branch 4 times, most recently from c77999c to d142de1 Compare February 7, 2025 09:35

laurafitzgerald requested review from eguzki and adam-cattermole February 7, 2025 09:35

laurafitzgerald force-pushed the label-propagation branch 3 times, most recently from bd6f1d8 to 04eaa19 Compare February 7, 2025 10:55

laurafitzgerald requested review from KevFan and removed request for adam-cattermole February 7, 2025 10:55

laurafitzgerald force-pushed the label-propagation branch from 04eaa19 to 39ae954 Compare February 7, 2025 10:57

laurafitzgerald commented Feb 7, 2025

View reviewed changes

laurafitzgerald marked this pull request as ready for review February 7, 2025 11:02

maleck13 reviewed Feb 7, 2025

View reviewed changes

pkg/limitador/k8s_objects_test.go Outdated Show resolved Hide resolved

laurafitzgerald force-pushed the label-propagation branch 2 times, most recently from b792015 to 2f46bc6 Compare February 10, 2025 09:57

eguzki reviewed Feb 10, 2025

View reviewed changes

laurafitzgerald force-pushed the label-propagation branch from 2f46bc6 to c84accc Compare February 11, 2025 11:35

KevFan reviewed Feb 11, 2025

View reviewed changes

pkg/limitador/k8s_objects.go Outdated Show resolved Hide resolved

laurafitzgerald force-pushed the label-propagation branch 2 times, most recently from 18b0c08 to 8f40569 Compare February 12, 2025 11:55

KevFan reviewed Feb 13, 2025

View reviewed changes

pkg/helpers/helpers.go Outdated Show resolved Hide resolved

laurafitzgerald force-pushed the label-propagation branch 2 times, most recently from 97168ec to 519d065 Compare February 13, 2025 14:40

laurafitzgerald commented Feb 13, 2025

View reviewed changes

pkg/limitador/k8s_objects.go Show resolved Hide resolved

laurafitzgerald force-pushed the label-propagation branch from 519d065 to 10cf420 Compare February 13, 2025 15:05

KevFan reviewed Feb 14, 2025

View reviewed changes

propogate labels from limitador to deployment template

9b6b9d0

Signed-off-by: Laura Fitzgerald <[email protected]>

laurafitzgerald force-pushed the label-propagation branch from 10cf420 to 9b6b9d0 Compare February 14, 2025 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

propogate labels from limitador to deployment template #183

propogate labels from limitador to deployment template #183

laurafitzgerald commented Feb 6, 2025 •

edited

Loading

codecov-commenter commented Feb 6, 2025 •

edited

Loading

laurafitzgerald Feb 7, 2025 •

edited

Loading

eguzki Feb 10, 2025

laurafitzgerald Feb 11, 2025

KevFan Feb 11, 2025

KevFan Feb 11, 2025 •

edited

Loading

eguzki Feb 11, 2025

eguzki Feb 11, 2025

laurafitzgerald Feb 11, 2025

KevFan Feb 11, 2025

eguzki Feb 12, 2025

eguzki commented Feb 10, 2025

eguzki left a comment

eguzki Feb 10, 2025

KevFan Feb 14, 2025

eguzki Feb 14, 2025

eguzki Feb 14, 2025

laurafitzgerald Feb 14, 2025

propogate labels from limitador to deployment template #183

Are you sure you want to change the base?

propogate labels from limitador to deployment template #183

Conversation

laurafitzgerald commented Feb 6, 2025 • edited Loading

Verification

codecov-commenter commented Feb 6, 2025 • edited Loading

Codecov Report

laurafitzgerald Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevFan Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eguzki commented Feb 10, 2025

eguzki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurafitzgerald commented Feb 6, 2025 •

edited

Loading

codecov-commenter commented Feb 6, 2025 •

edited

Loading

laurafitzgerald Feb 7, 2025 •

edited

Loading

KevFan Feb 11, 2025 •

edited

Loading