grafana / tempo-operator

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

Merge annotations from desired with existing ones in the ServiceAccounts #969

Closed rubenvp8510 closed 2 months ago

rubenvp8510 commented 2 months ago

Fixes https://github.com/grafana/tempo-operator/issues/970

codecov-commenter commented 2 months ago

Codecov Report

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

Project coverage is 73.36%. Comparing base (76bfab6) to head (8b09d44).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #969 +/- ## ========================================== - Coverage 73.36% 73.36% -0.01% ========================================== Files 105 105 Lines 6488 6487 -1 ========================================== - Hits 4760 4759 -1 Misses 1438 1438 Partials 290 290 ``` | [Flag](https://app.codecov.io/gh/grafana/tempo-operator/pull/969/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/969/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=grafana) | `73.36% <ø> (-0.01%)` | :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.

rubenvp8510 commented 2 months ago

@andreasgerstmayr This PR only verifies if the SA exists or not in order to determine if needs to be recreated. Do you think we need a more sophisticated way to validate this? Because currently our SAs are very simple.

rubenvp8510 commented 2 months ago

Tested on OCP 4.16

➜  reconcile git:(sa_fix) ✗ oc get pods -n ruben-test
NAME                                             READY   STATUS    RESTARTS   AGE
minio-687794c4f-6g45v                            1/1     Running   0          7h25m
tempo-simplest-compactor-7b9cd9d4dd-jn7jg        1/1     Running   0          77s
tempo-simplest-distributor-8584d97bc6-wkcnp      1/1     Running   0          77s
tempo-simplest-ingester-0                        1/1     Running   0          77s
tempo-simplest-querier-7fd7d8b76f-nrfcw          1/1     Running   0          77s
tempo-simplest-query-frontend-6c79bbb7d7-96q6c   1/1     Running   0          77s
➜  reconcile git:(sa_fix) ✗ oc get secrets -n ruben-test
NAME                                 TYPE                      DATA   AGE
builder-dockercfg-dcvmn              kubernetes.io/dockercfg   1      7h45m
default-dockercfg-mj4wx              kubernetes.io/dockercfg   1      7h45m
deployer-dockercfg-h5szn             kubernetes.io/dockercfg   1      7h45m
minio-test                           Opaque                    4      7h45m
tempo-simplest-compactor-mtls        kubernetes.io/tls         2      21m
tempo-simplest-distributor-mtls      kubernetes.io/tls         2      21m
tempo-simplest-dockercfg-xdwgm       kubernetes.io/dockercfg   1      21m
tempo-simplest-gateway-mtls          kubernetes.io/tls         2      21m
tempo-simplest-ingester-mtls         kubernetes.io/tls         2      21m
tempo-simplest-querier-mtls          kubernetes.io/tls         2      21m
tempo-simplest-query-frontend-mtls   kubernetes.io/tls         2      21m
tempo-simplest-signing-ca            kubernetes.io/tls         2      21m
test-sa-dockercfg-5cdlk              kubernetes.io/dockercfg   1      3h23m
➜  reconcile git:(sa_fix) ✗ 
andreasgerstmayr commented 2 months ago

@andreasgerstmayr This PR only verifies if the SA exists or not in order to determine if needs to be recreated. Do you think we need a more sophisticated way to validate this? Because currently our SAs are very simple.

I think for this PR we only need to remove this line: https://github.com/grafana/tempo-operator/blob/76bfab6754344a74a261f9f72d2b4e0c8b6a9674/internal/manifests/mutate.go#L184 then the issue will be resolved if I'm not mistaken.

Or something like https://github.com/grafana/tempo-operator/blob/76bfab6754344a74a261f9f72d2b4e0c8b6a9674/internal/manifests/mutate.go#L172 to be more sophisticated :D

rubenvp8510 commented 2 months ago

@andreasgerstmayr This PR only verifies if the SA exists or not in order to determine if needs to be recreated. Do you think we need a more sophisticated way to validate this? Because currently our SAs are very simple.

I think for this PR we only need to remove this line:

https://github.com/grafana/tempo-operator/blob/76bfab6754344a74a261f9f72d2b4e0c8b6a9674/internal/manifests/mutate.go#L184 then the issue will be resolved if I'm not mistaken.

Or something like

https://github.com/grafana/tempo-operator/blob/76bfab6754344a74a261f9f72d2b4e0c8b6a9674/internal/manifests/mutate.go#L172 to be more sophisticated :D

Not sure if that will work, I think other things are added to the SA not just the annotation. Anyway I'm gonna check this in the morning. If this simple approach work I don't see why not using it.

rubenvp8510 commented 2 months ago

Ill switch this solution to use those mutation functions that @andreasgerstmayr is mentioning.

This PR is still rudimentary because it is only for demostrate the problem

rubenvp8510 commented 2 months ago

@andreasgerstmayr This PR only verifies if the SA exists or not in order to determine if needs to be recreated. Do you think we need a more sophisticated way to validate this? Because currently our SAs are very simple.

I think for this PR we only need to remove this line:

https://github.com/grafana/tempo-operator/blob/76bfab6754344a74a261f9f72d2b4e0c8b6a9674/internal/manifests/mutate.go#L184 then the issue will be resolved if I'm not mistaken.

This did the trick. :)

Or something like

https://github.com/grafana/tempo-operator/blob/76bfab6754344a74a261f9f72d2b4e0c8b6a9674/internal/manifests/mutate.go#L172 to be more sophisticated :D