Skip to content
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

eck-[agent|fleet-server|beats] Reconsider serviceAccount.name usage for cross-ns RBAC feature #8291

Open
pebrc opened this issue Dec 4, 2024 · 0 comments
Labels
>enhancement Enhancement of existing functionality :helm-charts

Comments

@pebrc
Copy link
Collaborator

pebrc commented Dec 4, 2024

After looking throught the git history I could not find a good argument why we structured serviceAccount property in the Agent and Beats charts in the way we did.

In all the other charts serviceAccountName serves only the cross-namespace RBAC feature but does not actually create the referenced service acount.

In the Agent/Fleet Server and Beats chart we actually need to generate a custom service account bound to a role with the use-case-dependent RBAC permissions for the application pods, so that the integrations the user wants to run work and can access the k8s API as needed.

There are three problems with the approach taken:

  1. we coupled it with the cross-ns RBAC feature
  2. we allow to specify a namespace (does not makes sense as you pointed out) and undocumented also custom labels/annotations.
  3. we have inconsistent approaches across eck-stack sub-charts

Given that we already released the all three affected charts I don't see how we can now stop supporting these attributes without potentially breaking customer installations.

  1. the pointless namespace: its an ugly wart but does not hurt
  2. the undocumented annotations/labels overrides. Let's keep them, they don't hurt either
  3. the cross-ns coupled with the SA. Violates the principle of least privilege as users have to give the application pods an addtional get permission on Elasticsearch/Kibana CRD to use the cross-ns RBAC feature that is not needed for the correct operation of the Beats/Agent pods. Again, the paramount concern here is backwards compatibility though. So I would keep it as is for now. Potentially we could introduce serviceAccountName in the future that would set the cross-ns SA if specified and otherwise use the .serviceAccount.name of the application pod. I don't like that solution very much as it would be very confusing even with extensive documentation. There is another wart that the SA that is being created by the Helm chart is not actually automatically used in the pod template, so the user has to be extra vigilant here and align all the ducks in one row.

tl;dr
My suggestion is as follows:

  • accept that we have a different approach with .serviceAccount.name for Beats, Agent and Fleet Server
  • make sure that in the examples the names line up (you have already done that)
  • create follow up issue to untangle the SA/cross-ns RBAC thing potentially in the future and document our decisions.

Originally posted by @pebrc in #8285 (comment)

@botelastic botelastic bot added the triage label Dec 4, 2024
@barkbay barkbay added the >enhancement Enhancement of existing functionality label Dec 16, 2024
@botelastic botelastic bot removed the triage label Dec 16, 2024
@barkbay barkbay added :helm-charts and removed >enhancement Enhancement of existing functionality labels Dec 16, 2024
@botelastic botelastic bot added the triage label Dec 16, 2024
@barkbay barkbay added the >enhancement Enhancement of existing functionality label Dec 16, 2024
@botelastic botelastic bot removed the triage label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement Enhancement of existing functionality :helm-charts
Projects
None yet
Development

No branches or pull requests

2 participants