Make helm charts consistent with how fields in spec are handled. (fleet-server only)

[naemono]

related: #8192
related: #8264
related: #8248

Details of suggested change

This is the follow-up to the above PRs, where we are making the eck-fleet-server helm chart consistent in how it handles values that end up within spec, and making it backwards compatible.

Implementation Note

1. The way this is implemented makes the previous behavior (having .spec.field in the values take precedence over just field in the values.
2. Mixing these 2 forms of values (some in spec in the values file, some not in spec) likely has unpredictable behavior. (for instance setting spec.http.something, and also setting http.somethingElse)

[pebrc]

Suggested change

	serviceAccount:
	name: elastic-fleet-server
	serviceAccountName: elastic-fleet-server

for consistency with the other charts?

[naemono]

All of the other charts expect serviceAccount.name, but render in the template serviceAccountName.

[pebrc]

find . -name 'values.yaml' -exec grep -H -A 1 '^# serviceAccoun' \{\} \;
./eck-stack/charts/eck-kibana/values.yaml:# serviceAccountName: ""
./eck-stack/charts/eck-kibana/values.yaml-
./eck-stack/charts/eck-elasticsearch/values.yaml:# serviceAccountName: ""
./eck-stack/charts/eck-elasticsearch/values.yaml-
./eck-stack/charts/eck-apm-server/values.yaml:# serviceAccountName: ""
./eck-stack/charts/eck-apm-server/values.yaml-
./eck-stack/charts/eck-logstash/values.yaml:# serviceAccountName: ""
./eck-stack/charts/eck-logstash/values.yaml-
./eck-stack/charts/eck-enterprise-search/values.yaml:# serviceAccountName: ""
./eck-stack/charts/eck-enterprise-search/values.yaml-

[naemono]

Well, I'll make fleet consistent here, and then open an additional pr for beats/agent that's backwards compatible to make them also consistent.

So I went back and looked at the original PR for Agent+Fleet Helm charts to better understand this functionality, and we set the serviceAccount as a map for specific reasons:

• This is controlling (as you mentioned below) a specific SA that has it's own RBAC rules associated with it.
• We are supporting the SA being in a separate namespace than the installed agent/fleet (why are we doing this? It's not supported iiuc)
• We are supporting custom labels on the SA object
• We are supporting custom annotations on the SA object

[pebrc]

I will try to take another look to understand better why we did that.

[pebrc]

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.

[pebrc]

There is a disconnect here between the name of this service account and the service account that is being generated.

We are also setting the generated service account name as the .spec.serviceAccountName which is distinct from the service account in the pod spec. The latter is required for agent to work correctly the former is only relevant for the cross-namespace RBAC feature we have built into ECK ("Am I allowed to associate with an Elasticsearch in namespace x") . I am not sure if it is a problem to combine the two into one service account. Curious to get @barkbay 's perspective.

[pebrc]

At the very least the two service account names in this example have to be the same for the example to work.

[naemono]

I've updated this to make them consistent. What I tested was this: https://github.com/naemono/cloud-on-k8s/blob/helm-chart-image-fix-fleet/deploy/eck-stack/examples/agent/fleet-agents.yaml, which worked without issues, which only includes the sa name in the podTemplate.

[barkbay]

I'm not sure that's a problem either, that being said we made the choice to let the user specify a SA which can be different from the one used by the Pods (https://www.elastic.co/guide/en/cloud-on-k8s/master/k8s-restrict-cross-namespace-associations.html), and I'm not sure it is a reflected in the chart. This comment suggests that serviceAccount.name is used by the Pods while it is actually used by the cross-namespace restriction mechanism?

[naemono]

💚 All backports created successfully

Status	Branch	Result
✅	2.16

Questions ?

Please refer to the Backport tool documentation