fix(labels): do not remove user added labels from Deployment's spec.template #243

laurafitzgerald · 2025-02-11T12:38:30Z

PR #91 brought the ability to propagate labels defined for Authorino CR down to Deployment. However, more often than not, it's more desired to have those labels propagated to template and therefore pods which are part of the deployment. This can simplify e.g. making Authorino part of service mesh, as injection label can be consistently added to relevant resources.

~~When creating the deployment, instead of having fixed set of labels for Deployment's spec.template and objectmeta.lables this change appends Authorino CR labels as well.~~

EDIT: This change will now allow the ability to add a label via your chosen mechanism to the deployment.spec.template.labels without being overwritten by the operator.

As per discussion at https://groups.google.com/a/redhat.com/g/kuadrant-dev/c/rbsY4kDaSnE

Closes #239
Replaces #236

codecov-commenter · 2025-02-17T10:13:50Z

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

Codecov Report

Attention: Patch coverage is 48.88889% with 23 lines in your changes missing coverage. Please review.

Project coverage is 53.48%. Comparing base (3d5ee08) to head (63bac01).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
controllers/authorino_controller.go	42.85%	13 Missing and 7 partials ⚠️
pkg/resources/k8s_deployment.go	62.50%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
+ Coverage   53.40%   53.48%   +0.07%     
==========================================
  Files          10       10              
  Lines        1380     1408      +28     
==========================================
+ Hits          737      753      +16     
- Misses        587      596       +9     
- Partials       56       59       +3

Flag	Coverage Δ
unit	`53.48% <48.88%> (+0.07%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bundle/manifests/authorino.kuadrant.io_authconfigs.yaml

eguzki

Looks good. I left some comments to discuss

controllers/authorino_controller.go

pkg/resources/k8s_deployment.go

controllers/authorino_controller.go

guicassolato · 2025-02-26T15:44:56Z

controllers/authorino_controller.go

+		logger.V(1).Info("existingDeployment", "deployment", existingDeployment)
+		// merge existing key/value pairs back into the desiredDeployment Spec Template Labels so as not to overwrite those.
+		for existingKey, existingValue := range existingDeployment.Spec.Template.Labels {
+			if desiredValue, exists := (desiredDeployment.Spec.Template.Labels)[existingKey]; !exists || desiredValue != existingValue {


Do we need this condition? Can't we just iterate over existing and set them all into desired?

yes, the idea was to not overwrite the desired hardcode values, but due to the fact that k8s api rejects a change to those hard coded ones because they wouldn't match the unmuatable selector.MatchLabels which are all set at creation time, i think it's safe to remove this.

eguzki

Found an issue that needs to be fixed

eguzki · 2025-02-26T15:59:39Z

pkg/resources/k8s_deployment.go

-	objMeta := getObjectMeta(namespace, name, labels)
-	authorinoLabels := labelsForAuthorino(name)
+func GetDeployment(name, namespace, saName string, replicas *int32, containers []k8score.Container, vol []k8score.Volume) *k8sapps.Deployment {
+	authorinoLabels := defaultAuthorinoLabels(name)


There is an issue that was there before this PR, and your PR brought it to the surface. You cannot use the same map to populate metadata.labels, spec.selector and template.metadata.labels. Because maps are references, all those would share (are sharing now actually) the same map, so you change one of them and "magically" all of them are changed at the same time. And spec.selector cannot be changed, hence k8s API server complained.

Build custom maps for each field so they can be safely be updated independently.

Nice catch. I'll add some additional testing as well to cover this.

Nice catch, @eguzki! Also, if the selector cannot change, we need to make sure they override the labels for the template after the modification made by the user. I.e., users can add other labels, they can even change existing ones, as long as those labels are not used in the selector. Right?

if they try to edit the existing values, which have a matching key in the selector, the k8s api will reject that change. i saw it happen in a test where i tried to update that to a bad value. the api rejected the call with error type filevalueinvalid selector does not match template labels on field spec.template.metadata.labels

eguzki · 2025-02-26T16:27:22Z

Verification Steps

Run local cluster

kind create cluster --name authorino-local

Checkout to this PR branch and deploy the operator

make docker-build OPERATOR_IMAGE=quay.io/kuadrant/authorino-operator:eguzki-custom-build-01
make docker-push OPERATOR_IMAGE=quay.io/kuadrant/authorino-operator:eguzki-custom-build-01
make install OPERATOR_IMAGE=quay.io/kuadrant/authorino-operator:eguzki-custom-build-01
make deploy OPERATOR_IMAGE=quay.io/kuadrant/authorino-operator:eguzki-custom-build-01

Deploy authorino instance

k create namespace authorino

k apply -n authorino -f - <<EOF
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
spec:
  listener:
    tls:
      enabled: false
  oidcServer:
    tls:
      enabled: false
EOF

Check labels

k get pods -n authorino -l authorino-resource=authorino -o yaml | yq e '.items[0].metadata.labels'

The result should be the hardcoded ones

authorino-resource: authorino
control-plane: controller-manager
pod-template-hash: 5b6f45cccb

Add user defined labels to deployment pod template

kubectl patch deployment -n authorino authorino --type merge --patch '{"spec": {"template": {"metadata": {"labels": {"sidecar.istio.io/inject": "true"}}}}}'

Check authorino pod labels

k get pods -n authorino -l authorino-resource=authorino -o yaml | yq e '.items[0].metadata.labels'

It should return

authorino-resource: authorino
control-plane: controller-manager
pod-template-hash: 5d7c6567fc
sidecar.istio.io/inject: "true"

Update authorino image using the RELATED_IMAGE_AUTHORINO env var to some different image tag. The env var update triggers rolling out first the operator pod.
Then, the new operator pod should reconcile the deployment of authorino, hence rolling out a new authorino deployment.
The new authorino pod should keep the user defined labels

kubectl set env deployment/authorino-operator -n authorino-operator RELATED_IMAGE_AUTHORINO=quay.io/kuadrant/authorino:cbc71830174cd997d6c6b881ba5d9d94ba72a77f

Check that authorino operator env var has been updated:

k get deployment authorino-operator -n authorino-operator -o jsonpath='{.spec.template.spec.containers[0].env}'

The new image should be there:

[{"name":"RELATED_IMAGE_AUTHORINO","value":"quay.io/kuadrant/authorino:cbc71830174cd997d6c6b881ba5d9d94ba72a77f"}]

Check that authorino instance is running from the new image:

k get deployment authorino -n authorino -o jsonpath='{.spec.template.spec.containers[0].image}'

The new image should be:

quay.io/kuadrant/authorino:cbc71830174cd997d6c6b881ba5d9d94ba72a77f

Check that the new authorino instance still has the user defined labels

k get pods -n authorino -l authorino-resource=authorino -o yaml | yq e '.items[0].metadata.labels'

It should return

authorino-resource: authorino
control-plane: controller-manager
pod-template-hash: 5d7c6567fc
sidecar.istio.io/inject: "true"

Clean up

kind delete cluster --name authorino-local

lphiri · 2025-02-26T16:52:47Z

@laurafitzgerald - I don't have full context, but am I understanding correctly that this does not propagate labels set on the Authorino CR as was proposed in the original #236?

eguzki · 2025-02-27T08:56:01Z

@laurafitzgerald - I don't have full context, but am I understanding correctly that this does not propagate labels set on the Authorino CR as was proposed in the original #236?

You are correct. The user needs to add the labels to deployment pod template.

I would also have done the label propagation. 1) Because the user interface is the CR and 2) the deployment of authorino is owned by the operator and should not be updated externally. On the community call, it was decided to do the way this PR implements because if done in the CR, it needs to be decided where to propagate the labels: i.e. to the deployment labels?, pod template labels? services? configmaps? I will keep pushing to have labels propagated from the authorino CR ;)

laurafitzgerald · 2025-02-27T09:08:44Z

@laurafitzgerald - I don't have full context, but am I understanding correctly that this does not propagate labels set on the Authorino CR as was proposed in the original #236?

Correct. This change allows the user to add the label themselves manually or via some automation. We have spoken with some users (including @bartoszmajsak who opened 236) who are looking for this change who are fine with that. As @eguzki mentioned, we decided to not propagate the labels everywhere for now because the label may have unintended consequences on some of the other k8s resources. This may happen in the future or alternatively there may be some option added in the spec of the cr to do that. For now this change allows the users to add the required label and authorino-operator wont remove it. Please let us know if this does not satisfy your needs.

laurafitzgerald · 2025-02-27T09:24:36Z

Clean up

kind delete cluster --name authorino-local

@eguzki
Just confirming the verification works on my side. 👍
I've also tried editing the value of the existing hardcoded labels and the api rejects it with the error selector does not match template labels

zdtsw · 2025-02-27T11:05:20Z

@laurafitzgerald - I don't have full context, but am I understanding correctly that this does not propagate labels set on the Authorino CR as was proposed in the original #236?

Correct. This change allows the user to add the label themselves manually or via some automation. We have spoken with some users (including @bartoszmajsak who opened 236) who are looking for this change who are fine with that. As @eguzki mentioned, we decided to not propagate the labels everywhere for now because the label may have unintended consequences on some of the other k8s resources. This may happen in the future or alternatively there may be some option added in the spec of the cr to do that. For now this change allows the users to add the required label and authorino-operator wont remove it. Please let us know if this does not satisfy your needs.

Please help me walk thro some scenarios:
if we (another operator) would like to use OSSM envoyproxy, we will need to

create Authorino CR
with or without label on Authorino CR wont have any difference
wait for the deployment of authorino gets created by Authornio Operator, then we need to manually add sidecar.istio.io/inject: "true" to .spec.template.metadata.labels
this label will eventually populated to Authorino pod
Next time when end user upgrade to a newer Authorino release. deployment of authorino retains with this label, so as the pod.
This change of "label" applies to any operations , e.g
manual add new one,
update existin one,
remove old one.
which means, accidently modify label to sidecar.istio.io/inject: "false" wont get automatically rectified unless someone ( we as another operator or end user) correct it on authorino deployment.
same to the case, if deployment gets crashloop or deleted, someone will need to re-apply label again.

KevFan

This looks good to me anyway 👍 I'll leave the approval to @eguzki

When we do the next release of authorino operator containing this change, it's probably a good idea to put in the release notes that this change also removes propagating Authorino CR labels to the authorino deployment. While this feature was not advertised in our docs, it's a behaviour change for existing users, so I think it would be good to note this 📝

laurafitzgerald · 2025-02-27T11:43:34Z

@laurafitzgerald - I don't have full context, but am I understanding correctly that this does not propagate labels set on the Authorino CR as was proposed in the original #236?

Correct. This change allows the user to add the label themselves manually or via some automation. We have spoken with some users (including @bartoszmajsak who opened 236) who are looking for this change who are fine with that. As @eguzki mentioned, we decided to not propagate the labels everywhere for now because the label may have unintended consequences on some of the other k8s resources. This may happen in the future or alternatively there may be some option added in the spec of the cr to do that. For now this change allows the users to add the required label and authorino-operator wont remove it. Please let us know if this does not satisfy your needs.

Please help me walk thro some scenarios: if we (another operator) would like to use OSSM envoyproxy, we will need to

create Authorino CR

with or without label on Authorino CR wont have any difference

wait for the deployment of authorino gets created by Authornio Operator, then we need to manually add sidecar.istio.io/inject: "true" to .spec.template.metadata.labels

this label will eventually populated to Authorino pod

Next time when end user upgrade to a newer Authorino release. deployment of authorino retains with this label, so as the pod.

Yes, correct to this point. I believe you were attempting this via https://github.com/opendatahub-io/opendatahub-operator/pull/1490/files
As the label has been added by something external to authorino-operator if any of the below changes are made authorino won't do anything. It's basically unopinionated about any label added to spec.template.labels by something other than itself. So it would be down to something external to ensure that it is maintained as expected/needed. that would be the case if you were adding the label to the cr. you would need to ensure that is in a state that you want. so no difference in expectation there due to the fact that we are doing it this way.

This change of "label" applies to any operations , e.g

manual add new one,

update existin one,

remove old one.
which means, accidently modify label to sidecar.istio.io/inject: "false" wont get automatically rectified unless someone ( we as another operator or end user) correct it on authorino deployment.
same to the case, if deployment gets crashloop or deleted, someone will need to re-apply label again.

lphiri · 2025-02-27T14:00:37Z

@laurafitzgerald @eguzki @guicassolato in terms of (longer term) requirements, I would say our real need is a declarative API to indicate that we want the Authorino deployment to be part of a service mesh (with options to specify which mesh: OSSM, vanilla Istio etc) , so making that an explicit (optional) config in the Authorino CR would be ideal, instead of the indirect label based approach. IMHO its reasonable to expect this as part of the API given Authorino is going be commonly used with envoy based service meshes.

guicassolato · 2025-02-27T14:30:22Z

@laurafitzgerald @eguzki @guicassolato in terms of (longer term) requirements, I would say our real need is a declarative API to indicate that we want the Authorino deployment to be part of a service mesh (with options to specify which mesh: OSSM, vanilla Istio etc) , so making that an explicit (optional) config in the Authorino CR would be ideal, instead of the indirect label based approach. IMHO its reasonable to expect this as part of the API given Authorino is going be commonly used with envoy based service meshes.

@lphiri, I see it differently actually. Authorino does not know anything about the client that connects to it. It can be integrated from an Envoy-based proxy, but also from pretty much anything else. It doesn't know it's a gateway, a mesh proxy, Kube's API server (we also have written use cases for this), a client app directly, or anything at all. And I think it should remain ignorant about it.

If we want to support propagating a label from the Authorino CR, it should be a generic label, but nothing specific about mesh or sidecar injection.

I think the reason why this propagation was kept out of the initial design was because we want to be very surgical about which exact internal objects we propagate the labels to (Service, Deployment, Pod, etc). I hope we can incorporate this second part in a future PR.

lphiri · 2025-02-27T14:57:07Z

@guicassolato I see. That perspective also makes sense.
Will wait for the future PR - setting labels in the Authorino CR would certainly be better than what we are having to do now.

eguzki · 2025-02-27T15:08:04Z

I agree with @guicassolato. IMHO the implementation details of adding one service (like authorino) to an specific service mesh provider do not belong to the responsibilities of the Authorino Operator. That being said, I have seen worse things, so I would not be against adding some integration between authorino and Istio specifically, something like spec.inject_into_istio: true that ends up adding "sidecar.istio.io/inject": "true" label to the workload.

eguzki

Changes looks good to me. Before approving, I would like to clarify that we are all ok with removing the label propagation from the Authorino CR to the deployment object (not the deployment pod template field) as pointed out correctly by @KevFan

If we are ok with that, LGTM

Verification steps worked 🟢

KevFan

Verified changes and verified Authorino CR labels -> Deployment labels propagation still works. Left minor comment regarding removed lines in tests that I think we should keep, but otherwise looks good 👍

controllers/authorino_controller_test.go

…c.template PR #91 brought the ability to propagate labels defined for Authorino CR down to Deployment. There was some discussion to have those labels propagated to template and therefore pods which are which are part of the deployment. However, for now we decided to add this change which adds the ability for a user added label to not be overwritten by the authorino-operator Signed-off-by: Laura Fitzgerald <[email protected]> Co-authored-by: bartoszmajsak <[email protected]>

KevFan

Nice! Looks good to me 👍

laurafitzgerald requested a review from KevFan February 11, 2025 12:38

laurafitzgerald mentioned this pull request Feb 11, 2025

fix(labels): propagates labels to Deployment's spec.template #236

Closed

laurafitzgerald force-pushed the label-propagation-fix branch from beccb69 to dce2d8f Compare February 11, 2025 12:44

laurafitzgerald requested a review from eguzki February 11, 2025 12:44

laurafitzgerald force-pushed the label-propagation-fix branch from dce2d8f to a3ebcd3 Compare February 17, 2025 10:10

KevFan reviewed Feb 17, 2025

View reviewed changes

bundle/manifests/authorino.kuadrant.io_authconfigs.yaml Outdated Show resolved Hide resolved

KevFan assigned laurafitzgerald Feb 17, 2025

zdtsw mentioned this pull request Feb 17, 2025

update: remove Authorino patch for ossm opendatahub-io/opendatahub-operator#1513

Closed

5 tasks

laurafitzgerald changed the title ~~fix(labels): propagates labels to Deployment's spec.template~~ fix(labels): do not remove user added labels from Deployment's spec.template Feb 25, 2025

laurafitzgerald force-pushed the label-propagation-fix branch 5 times, most recently from f003c58 to 37ae902 Compare February 25, 2025 15:07

guicassolato reviewed Feb 25, 2025

View reviewed changes

bundle/manifests/authorino.kuadrant.io_authconfigs.yaml Outdated Show resolved Hide resolved

laurafitzgerald force-pushed the label-propagation-fix branch from 37ae902 to 6420a79 Compare February 25, 2025 15:15

eguzki reviewed Feb 25, 2025

View reviewed changes

laurafitzgerald force-pushed the label-propagation-fix branch 2 times, most recently from eb778eb to 388da4c Compare February 26, 2025 14:19

guicassolato reviewed Feb 26, 2025

View reviewed changes

eguzki requested changes Feb 26, 2025

View reviewed changes

laurafitzgerald force-pushed the label-propagation-fix branch from 388da4c to 1c70554 Compare February 26, 2025 16:25

KevFan reviewed Feb 27, 2025

View reviewed changes

eguzki reviewed Feb 27, 2025

View reviewed changes

laurafitzgerald force-pushed the label-propagation-fix branch from 1c70554 to 4ef94c9 Compare March 3, 2025 13:24

laurafitzgerald requested review from eguzki and KevFan March 3, 2025 13:27

KevFan reviewed Mar 3, 2025

View reviewed changes

controllers/authorino_controller_test.go Outdated Show resolved Hide resolved

controllers/authorino_controller_test.go Show resolved Hide resolved

laurafitzgerald force-pushed the label-propagation-fix branch from 4ef94c9 to 63bac01 Compare March 3, 2025 15:13

eguzki approved these changes Mar 3, 2025

View reviewed changes

KevFan approved these changes Mar 3, 2025

View reviewed changes

laurafitzgerald merged commit f0012e3 into main Mar 3, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(labels): do not remove user added labels from Deployment's spec.template #243

fix(labels): do not remove user added labels from Deployment's spec.template #243

laurafitzgerald commented Feb 11, 2025 •

edited

Loading

codecov-commenter commented Feb 17, 2025 •

edited

Loading

eguzki left a comment

guicassolato Feb 26, 2025

laurafitzgerald Feb 26, 2025

eguzki left a comment

eguzki Feb 26, 2025

laurafitzgerald Feb 26, 2025

guicassolato Feb 26, 2025

laurafitzgerald Feb 26, 2025

eguzki commented Feb 26, 2025

lphiri commented Feb 26, 2025

eguzki commented Feb 27, 2025

laurafitzgerald commented Feb 27, 2025 •

edited

Loading

laurafitzgerald commented Feb 27, 2025

zdtsw commented Feb 27, 2025 •

edited

Loading

KevFan left a comment •

edited

Loading

laurafitzgerald commented Feb 27, 2025 •

edited

Loading

lphiri commented Feb 27, 2025

guicassolato commented Feb 27, 2025

lphiri commented Feb 27, 2025

eguzki commented Feb 27, 2025 •

edited

Loading

eguzki left a comment •

edited

Loading

KevFan left a comment •

edited

Loading

KevFan left a comment

fix(labels): do not remove user added labels from Deployment's spec.template #243

fix(labels): do not remove user added labels from Deployment's spec.template #243

Conversation

laurafitzgerald commented Feb 11, 2025 • edited Loading

codecov-commenter commented Feb 17, 2025 • edited Loading

Codecov Report

eguzki left a comment

Choose a reason for hiding this comment

guicassolato Feb 26, 2025

Choose a reason for hiding this comment

laurafitzgerald Feb 26, 2025

Choose a reason for hiding this comment

eguzki left a comment

Choose a reason for hiding this comment

eguzki Feb 26, 2025

Choose a reason for hiding this comment

laurafitzgerald Feb 26, 2025

Choose a reason for hiding this comment

guicassolato Feb 26, 2025

Choose a reason for hiding this comment

laurafitzgerald Feb 26, 2025

Choose a reason for hiding this comment

eguzki commented Feb 26, 2025

Verification Steps

lphiri commented Feb 26, 2025

eguzki commented Feb 27, 2025

laurafitzgerald commented Feb 27, 2025 • edited Loading

laurafitzgerald commented Feb 27, 2025

zdtsw commented Feb 27, 2025 • edited Loading

KevFan left a comment • edited Loading

Choose a reason for hiding this comment

laurafitzgerald commented Feb 27, 2025 • edited Loading

lphiri commented Feb 27, 2025

guicassolato commented Feb 27, 2025

lphiri commented Feb 27, 2025

eguzki commented Feb 27, 2025 • edited Loading

eguzki left a comment • edited Loading

Choose a reason for hiding this comment

KevFan left a comment • edited Loading

Choose a reason for hiding this comment

KevFan left a comment

Choose a reason for hiding this comment

laurafitzgerald commented Feb 11, 2025 •

edited

Loading

codecov-commenter commented Feb 17, 2025 •

edited

Loading

laurafitzgerald commented Feb 27, 2025 •

edited

Loading

zdtsw commented Feb 27, 2025 •

edited

Loading

KevFan left a comment •

edited

Loading

laurafitzgerald commented Feb 27, 2025 •

edited

Loading

eguzki commented Feb 27, 2025 •

edited

Loading

eguzki left a comment •

edited

Loading

KevFan left a comment •

edited

Loading