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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Dec 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3f310ed
wip
naemono Dec 2, 2024
9e0095a
wip
naemono Dec 2, 2024
e739f7d
Adding tests
naemono Dec 3, 2024
8217c26
adding fleet-server example.
naemono Dec 3, 2024
1261b81
adjust values file
naemono Dec 3, 2024
b27130a
Remove daemonset as an option.
naemono Dec 3, 2024
f07d51d
make serviceaccount consistent
naemono Dec 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions deploy/eck-stack/charts/eck-fleet-server/examples/fleet-server.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
version: 8.17.0-SNAPSHOT
deployment:
replicas: 1
podTemplate:
spec:
serviceAccountName: fleet-server
Copy link
Collaborator

@pebrc pebrc Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@pebrc pebrc Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

automountServiceAccountToken: true
elasticsearchRefs:
- name: eck-elasticsearch
namespace: default
naemono marked this conversation as resolved.
Show resolved Hide resolved
kibanaRef:
name: eck-kibana
namespace: default
naemono marked this conversation as resolved.
Show resolved Hide resolved
http:
service:
spec:
type: ClusterIP
serviceAccount:
name: elastic-fleet-server
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

for consistency with the other charts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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-

Copy link
Contributor Author

@naemono naemono Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@pebrc pebrc Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

53 changes: 47 additions & 6 deletions deploy/eck-stack/charts/eck-fleet-server/templates/fleet-server.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,54 @@ metadata:
{{- toYaml . | nindent 4 }}
{{- end }}
spec:
version: {{ required "A Fleet Server version is required" .Values.version }}
version: {{ required "A Fleet Server version is required" (or ((.Values.spec).version) (.Values.version)) }}
mode: fleet
fleetServerEnabled: true
{{- if (hasKey .Values.spec "mode") }}
{{- fail "spec.mode cannot be changed" }}
{{- if (or (hasKey (.Values.spec) "mode") (hasKey .Values "mode")) }}
{{- fail "mode cannot be changed" }}
{{- end }}
{{- if (hasKey .Values.spec "fleetServerEnabled") }}
{{- fail "spec.fleetServerEnabled cannot be changed" }}
{{- if (or (hasKey (.Values.spec) "fleetServerEnabled") (hasKey .Values "fleetServerEnabled"))}}
{{- fail "fleetServerEnabled cannot be changed" }}
{{- end }}

{{- $daemonSet := (or (hasKey (.Values.spec) "daemonSet") (hasKey .Values "daemonSet")) }}
{{- $deployment := (or (hasKey (.Values.spec) "deployment") (hasKey .Values "deployment")) }}
{{- if and (not $daemonSet) (not $deployment) }}
{{ fail "At least one of daemonSet or deployment is required" }}
{{- end }}
{{- if $daemonSet }}
{{- $ds := or ((.Values.spec).daemonSet) (.Values.daemonSet) }}
daemonSet:
{{- /* This is required to render the empty daemonset ( {} ) properly */}}
{{- $ds | default dict | toYaml | nindent 4 }}
{{- end }}
{{- if $deployment }}
{{- $deploy := or ((.Values.spec).deployment) (.Values.deployment) }}
deployment:
{{- /* This is required to render the empty deployment ( {} ) properly */}}
{{- $deploy | default dict | toYaml | nindent 4 }}
{{- end }}
{{- with or ((.Values.spec).image) (.Values.image) }}
image: {{ . }}
{{- end }}
{{- with or ((.Values.spec).elasticsearchRefs) (.Values.elasticsearchRefs) }}
elasticsearchRefs:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- with or ((.Values.spec).kibanaRef) (.Values.kibanaRef) }}
kibanaRef:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- with or ((.Values.spec).policyID) (.Values.policyID) }}
policyID: {{ . }}
{{- end }}
{{- with or ((.Values.spec).http) (.Values.http) }}
http:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- with or ((.Values.spec).revisionHistoryLimit) (.Values.revisionHistoryLimit) }}
revisionHistoryLimit: {{ . }}
{{- end }}
{{- with or (((.Values.spec).serviceAccount).name) ((.Values.serviceAccount).name) }}
serviceAccountName: {{ . }}
{{- end }}
{{- toYaml .Values.spec | nindent 2 }}
135 changes: 124 additions & 11 deletions deploy/eck-stack/charts/eck-fleet-server/templates/tests/fleet-server_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,15 @@ templates:
- templates/fleet-server.yaml
tests:
- it: should render default fleet server properly
set:
deployment:
replicas: 1
podTemplate:
spec:
serviceAccountName: fleet-server
automountServiceAccountToken: true
kibanaRef:
name: eck-kibana
release:
name: quickstart
asserts:
Expand All @@ -18,23 +27,20 @@ tests:
path: spec.kibanaRef.name
value: eck-kibana
- equal:
path: spec.deployment.replicas
value: 1
- equal:
path: spec.deployment.podTemplate.spec.serviceAccountName
value: fleet-server
- equal:
path: spec.deployment.podTemplate.spec.automountServiceAccountToken
value: true
- equal:
path: spec.deployment.podTemplate.spec.securityContext.runAsUser
value: 0
path: spec.deployment
value:
replicas: 1
podTemplate:
spec:
serviceAccountName: fleet-server
automountServiceAccountToken: true
- it: should render custom labels and annotations properly.
set:
labels:
test: label
annotations:
test: annotation
deployment: {}
release:
name: quickstart
asserts:
Expand All @@ -53,3 +59,110 @@ tests:
value:
eck.k8s.elastic.co/license: basic
test: annotation
- it: should render properly when using spec fields
set:
spec:
daemonSet: {}
elasticsearchRefs:
- name: eck-elasticsearch
namespace: default
kibanaRef:
name: eck-kibana
namespace: default
http:
service:
spec:
type: ClusterIP
revisionHistoryLimit: 4
serviceAccount:
name: elastic-fleet-server
release:
name: quickstart
asserts:
- isKind:
of: Agent
- equal:
path: metadata.labels
value:
app.kubernetes.io/instance: quickstart
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/name: eck-fleet-server
helm.sh/chart: eck-fleet-server-0.14.0-SNAPSHOT
- equal:
path: spec
value:
fleetServerEnabled: true
mode: fleet
policyID: eck-fleet-server
version: 8.17.0-SNAPSHOT
daemonSet: {}
elasticsearchRefs:
- name: eck-elasticsearch
namespace: default
kibanaRef:
name: eck-kibana
namespace: default
http:
service:
spec:
type: ClusterIP
revisionHistoryLimit: 4
serviceAccountName: elastic-fleet-server
- it: should render properly when not using spec fields
set:
daemonSet: {}
elasticsearchRefs:
- name: eck-elasticsearch
namespace: default
kibanaRef:
name: eck-kibana
namespace: default
http:
service:
spec:
type: ClusterIP
revisionHistoryLimit: 4
serviceAccount:
name: elastic-fleet-server
release:
name: quickstart
asserts:
- isKind:
of: Agent
- equal:
path: metadata.labels
value:
app.kubernetes.io/instance: quickstart
app.kubernetes.io/managed-by: Helm
app.kubernetes.io/name: eck-fleet-server
helm.sh/chart: eck-fleet-server-0.14.0-SNAPSHOT
- equal:
path: spec
value:
fleetServerEnabled: true
mode: fleet
policyID: eck-fleet-server
version: 8.17.0-SNAPSHOT
daemonSet: {}
elasticsearchRefs:
- name: eck-elasticsearch
namespace: default
kibanaRef:
name: eck-kibana
namespace: default
http:
service:
spec:
type: ClusterIP
revisionHistoryLimit: 4
serviceAccountName: elastic-fleet-server
- it: not setting version should fail
set:
version: ""
asserts:
- failedTemplate:
errorMessage: "A Fleet Server version is required"
- it: not setting a daemonset or deployment should fail
asserts:
- failedTemplate:
errorMessage: "At least one of daemonSet or deployment is required"
102 changes: 66 additions & 36 deletions deploy/eck-stack/charts/eck-fleet-server/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,46 +28,76 @@ labels: {}
#
annotations: {}

spec:
# policyID determines into which Agent Policy this Fleet Server will be enrolled.
policyID: eck-fleet-server
# Elastic Fleet Server Agent image to deploy.
#
# image: docker.elastic.co/beats/elastic-agent:8.17.0-SNAPSHOT

# Referenced resources are below and both elasticsearchRefs and kibanaRef are required for a functional Fleet Server.
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-beat-configuration.html#k8s-beat-connect-es
#
# Reference to ECK-managed Kibana resource.
#
kibanaRef:
name: eck-kibana
# Optional namespace reference to Kibana resource.
# If not specified, then the namespace of the Fleet Server resource
# will be assumed.
#
# namespace: default
# ** Deprecation Notice **
# The previous versions of this Helm Chart simply used the `spec` field here
# and allowed the user to specify any fields below `spec` that were templated directly
# into the final Agent/Fleet Server manifest. This is no longer the preferred way to specify these
# fields and each field that is supported underneath `spec` is now directly specified
# in this values file. Currently both patterns are supported for backwards compatibility
# but we plan to remove the `spec` field in the future.
# spec: {}

# References to ECK-managed Elasticsearch resource.
# This is required for fleet server.
# Referenced resources are below and both elasticsearchRefs and kibanaRef are required for a functional Fleet Server.
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-elastic-agent-fleet-configuration.html#k8s-elastic-agent-fleet-configuration-setting-referenced-resources
#
# Reference to ECK-managed Kibana instance.
# This is required for Fleet Server.
#
# kibanaRef:
# name: quickstart
# Optional namespace reference to Kibana instance.
# If not specified, then the namespace of the Fleet Server resource
# will be assumed.
#
elasticsearchRefs:
- name: eck-elasticsearch
# Optional namespace reference to Elasticsearch resource.
# If not specified, then the namespace of the Fleet Server resource
# will be assumed.
#
# namespace: default

# Daemonset, or Deployment specification for the type of Fleet Server specified.
# At least one is required of [daemonSet, deployment].
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-beat-configuration.html#k8s-beat-chose-the-deployment-model
# namespace: default

# Reference to ECK-managed Elasticsearch instance.
# This is required for Fleet Server.
#
elasticsearchRefs: []
# - name: eck-elasticsearch
# Optional namespace reference to Elasticsearch instance.
# If not specified, then the namespace of the Fleet Server resource
# will be assumed.
#
deployment:
replicas: 1
podTemplate:
spec:
serviceAccountName: fleet-server
automountServiceAccountToken: true
securityContext:
runAsUser: 0
# namespace: default

# policyID determines into which Agent Policy this Fleet Server will be enrolled.
policyID: eck-fleet-server

# The HTTP layer configuration for the Fleet Server Service.
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-elastic-agent-fleet-configuration.html#k8s-elastic-agent-fleet-configuration-customize-fleet-server-service
#
# http:

# DaemonSet or Deployment specification for Fleet Server.
naemono marked this conversation as resolved.
Show resolved Hide resolved
# At least one is required of [daemonSet, deployment].
# No default is currently set, refer to https://github.com/elastic/cloud-on-k8s/issues/7429.
# ref: https://www.elastic.co/guide/en/cloud-on-k8s/current/k8s-elastic-agent-fleet-configuration.html#k8s-elastic-agent-fleet-configuration-chose-the-deployment-model
#
# deployment:
# podTemplate:
# spec:
# containers:
# - name: agent
# securityContext:
# runAsUser: 0
# daemonSet:
# podTemplate:
# spec:
# containers:
# - name: agent
# securityContext:
# runAsUser: 0

# Number of revisions to retain to allow rollback in the underlying Deployment.
# If not set Kubernetes sets this to 10 by default.
#
# revisionHistoryLimit: 2

# ServiceAccount to be used by Elastic Fleet Server. Some Fleet Server features (such as autodiscover or Kubernetes module metricsets)
# require that Fleet Server Pods interact with Kubernetes APIs. This functionality requires specific permissions
Expand Down
20 changes: 13 additions & 7 deletions deploy/eck-stack/examples/agent/fleet-agents.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,16 @@ eck-fleet-server:

fullnameOverride: "fleet-server"

spec:
# Agent policy to be used.
policyID: eck-fleet-server
kibanaRef:
name: kibana
elasticsearchRefs:
- name: elasticsearch
deployment:
replicas: 1
podTemplate:
spec:
serviceAccountName: fleet-server
automountServiceAccountToken: true

# Agent policy to be used.
policyID: eck-fleet-server
kibanaRef:
name: kibana
elasticsearchRefs:
- name: elasticsearch