grafana / helm-charts

Apache License 2.0
1.67k stars 2.28k forks source link

[grafana] fix: don't automount default serviceAccount #3302

Open cwrau opened 2 months ago

cwrau commented 2 months ago

this is best-practice

jkroepke commented 2 months ago

Hi, please create a distinct property for it.

https://github.com/grafana/helm-charts/blob/bc8f4a11b3928d8590fd3e0fdc68e3cf2747d393/charts/grafana/templates/_pod.tpl#L8

automount default

The default service account should never used. Could you create a serviceAccount for imageRender the same way at it's done at grafana?

cwrau commented 2 months ago

Hi, please create a distinct property for it.

https://github.com/grafana/helm-charts/blob/bc8f4a11b3928d8590fd3e0fdc68e3cf2747d393/charts/grafana/templates/_pod.tpl#L8

automount default

I was thinking about that, but why? If there is no serviceAccount defined, automount is not needed and default shouldn't be used. So the only situation in which automount even makes sense to be enabled is when a serviceAccount is defined. So why make it configurable just so every user has to touch this field, especially when set to true by default?

Could you create a serviceAccount for imageRender the same way at it's done at grafana?

Now that I think about it, is the imageRenderer even able to use a serviceAccount? Maybe we can remove this whole stuff completely?

jkroepke commented 2 months ago

It also best practice to not reference the default serviceAccount anywhere. Such stupid scanners may still complain that the default serviceAccount is in used.

cwrau commented 2 months ago

It also best practice to not reference the default serviceAccount anywhere.

Yes, that's why I don't think it's useful to have the automountServiceAccountToken to be configurable 👍

Such stupid scanners may still complain that the default serviceAccount is in used.

True, but they can't really detect the opposite, automountServiceAccountToken is true by default and the used serviceAccount is default by default, so the pod does indeed have, by default, access to the default serviceAccount 😅

jkroepke commented 2 months ago

True, but they can't really detect the opposite,

They complain if the default service account is used. I appreciate that you found that issue and I guess its an good time to introduce a dedicate for the image. It's common practice.

cwrau commented 2 months ago

True, but they can't really detect the opposite,

They complain if the default service account is used. I appreciate that you found that issue and I guess its an good time to introduce a dedicate for the image. It's common practice.

Is the imageRenderer even able to use a serviceAccount? Why create one, if it won't be used?

jkroepke commented 2 months ago

Compliance/Security scanners doesn't follow logical rules as well.

cwrau commented 2 months ago

Compliance/Security scanners doesn't follow logical rules as well.

Ok? What does that have to do with anything? 😅

Do you mean we should create an unused serviceAccount / add a flag to be able to set automountServiceAccountToken even though it makes no sense just because some random tools that don't have anything to do with this aren't good?

jkroepke commented 2 months ago

Yes

cwrau commented 2 months ago

Yes

Donso

zanhsieh commented 3 weeks ago

@jkroepke Can you review again please?