Skip to content

🔨 Fix pod template labels mutator 🔨 #256

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 15, 2025
Merged

Conversation

eguzki
Copy link
Contributor

@eguzki eguzki commented Apr 14, 2025

What

When the existing deployment template label has few extra labels, the pod template label mutator was returning true to update the resource even when there was no update to be done. The extra few labels are preserved by the mutator. That apparently little issue of reporting "update" when it should not had secondary effects. The status controller would never set the status as Ready because it is being told that there is an update still going on.

Verifications steps

  • Checkout this branch

  • Run cluster

kind create cluster --name authorino-operator-system
  • Install CRDs
make install
  • Run operator locally
make run
  • Create authorino object.
k apply -f - <<EOF
apiVersion: operator.authorino.kuadrant.io/v1beta1
kind: Authorino
metadata:
  name: authorino
spec:
  listener:
    tls:
      enabled: false
  oidcServer:
    tls:
      enabled: false
EOF
  • Wait for authorino instance to be ready
kubectl wait --timeout=300s --for=condition=Ready authorino authorino
  • Add manually few extra pod template labels
kubectl patch deployment authorino --type=merge --patch '{"spec": {"template": {"metadata": {"labels": {"foo": "bar", "foo2": "bar2"}}}}}'
  • Wait for authorino instance to be ready
kubectl wait --timeout=300s --for=condition=Ready authorino authorino

If authorino CR gets the Ready condition in status, the fix worked. If trying out the same verification steps with current HEAD of main, the authorino status looks like this:

❯ k get authorino authorino -o jsonpath="{.status}" | yq e -P
conditions:
  - lastTransitionTime: "2025-04-14T17:19:18Z"
    message: Authorino Deployment resource updated
    reason: Updated
    status: "False"
    type: Ready

Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
@codecov-commenter
Copy link

codecov-commenter commented Apr 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.84%. Comparing base (a5502a9) to head (86bdf7f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #256      +/-   ##
==========================================
+ Coverage   63.74%   63.84%   +0.09%     
==========================================
  Files          14       15       +1     
  Lines        1553     1557       +4     
==========================================
+ Hits          990      994       +4     
  Misses        482      482              
  Partials       81       81              
Flag Coverage Δ
unit 63.84% <100.00%> (+0.09%) ⬆️

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@eguzki eguzki marked this pull request as ready for review April 14, 2025 17:22
@eguzki eguzki self-assigned this Apr 14, 2025
@eguzki eguzki added the kind/bug Something isn't working label Apr 14, 2025
@@ -0,0 +1,22 @@
package resources

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: might be nice to add a quick comment so its known the bool returned is to let you know if the map was modified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Signed-off-by: Eguzki Astiz Lezaun <eastizle@redhat.com>
@eguzki eguzki added this pull request to the merge queue Apr 15, 2025
Merged via the queue into main with commit a336830 Apr 15, 2025
13 checks passed
@github-project-automation github-project-automation bot moved this to Done in Kuadrant Apr 15, 2025
@eguzki eguzki deleted the fix-pod-template-mutator branch April 15, 2025 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants