grafana / tempo-operator

Grafana Tempo Kubernetes operator
https://grafana.com/docs/tempo/latest/setup/operator/
GNU Affero General Public License v3.0
61 stars 30 forks source link

Fix infinite reconciliation when oauth-proxy is used #1020

Closed pavolloffay closed 2 months ago

pavolloffay commented 2 months ago

Resolves #1018

See https://github.com/openshift/oauth-proxy?tab=readme-ov-file#other-configuration-flags

Tested with

kubectl apply -f - <<EOF
apiVersion: tempo.grafana.com/v1alpha1
kind: TempoMonolithic
metadata:
  name: simplestmono
spec:
  jaegerui:
    enabled: true 
    route:
      enabled: true
  storage:
    traces:
      backend: s3 
      size: 3Gi 
      s3: 
        secret: minio-test
EOF

kubectl apply -f - <<EOF
apiVersion: tempo.grafana.com/v1alpha1
kind: TempoStack
metadata:
  name: simplest
spec:
  storage:
    secret:
      name: minio-test
      type: s3
  storageSize: 1Gi
  template:
    queryFrontend:
      jaegerQuery:
        enabled: true
        ingress:
          type: route
EOF
rubenvp8510 commented 2 months ago

oh wait. what if the serviceaccount token changes, then all the cookies will be invalidated because the secret changed?

I'm assuming yes, how frecuent does the token changes?

iblancasa commented 2 months ago

oh wait. what if the serviceaccount token changes, then all the cookies will be invalidated because the secret changed?

I would expect this to happen.

I'm assuming yes, how frecuent does the token changes?

Since we are not setting expirationSeconds... I think the token will not expire. If the token changes is because the service account changed and it will be removed or something. In that case, that is not usual, I think it should be ok to just log out all the users.

What do you think @rubenvp8510 @andreasgerstmayr? If you agree, please, merge the PR. Thanks!

rubenvp8510 commented 2 months ago

oh wait. what if the serviceaccount token changes, then all the cookies will be invalidated because the secret changed?

I would expect this to happen.

I'm assuming yes, how frecuent does the token changes?

Since we are not setting expirationSeconds... I think the token will not expire. If the token changes is because the service account changed and it will be removed or something. In that case, that is not usual, I think it should be ok to just log out all the users.

What do you think @rubenvp8510 @andreasgerstmayr? If you agree, please, merge the PR. Thanks!

If that is the case (the token doesn't expire and change only if the service account is updated), I think is an acceptable behavior. On the other side, if the token is expiring for example each 60 seconds or so, then it will be really annoying for the users to have to login again.

IshwarKanse commented 2 months ago

@pavolloffay I tested the fix and it is working fine. I didn't see any reconciliation loop, the UI is not logging out and there are no significant increase in the metrics controller_runtime_reconcile_time_seconds_count{controller="tempostack"} controller_runtime_reconcile_time_seconds_count{controller="tempomonolithic"} I'll keep an eye during the release testing if I run into any issues.

codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 72.94%. Comparing base (dd28af7) to head (731c4c7).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1020 +/- ## ========================================== - Coverage 72.99% 72.94% -0.05% ========================================== Files 106 106 Lines 6624 6575 -49 ========================================== - Hits 4835 4796 -39 + Misses 1496 1489 -7 + Partials 293 290 -3 ``` | [Flag](https://app.codecov.io/gh/grafana/tempo-operator/pull/1020/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/grafana/tempo-operator/pull/1020/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | `72.94% <100.00%> (-0.05%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.