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
+265 −60
Merged

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

Show file tree
Hide file tree
Changes from all 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
17 changes: 17 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,17 @@
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
kibanaRef:
name: eck-kibana
http:
service:
spec:
type: ClusterIP
serviceAccount:
name: fleet-server
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 }}

{{- $statefulSet := (or (hasKey (.Values.spec) "statefulSet") (hasKey .Values "statefulSet")) }}
{{- $deployment := (or (hasKey (.Values.spec) "deployment") (hasKey .Values "deployment")) }}
{{- if and (not $statefulSet) (not $deployment) }}
{{ fail "At least one of statefulSet or deployment is required" }}
{{- end }}
{{- if $statefulSet }}
{{- $ss := or ((.Values.spec).statefulSet) (.Values.statefulSet) }}
statefulSet:
{{- /* This is required to render the empty statefulSet ( {} ) properly */}}
{{- $ss | 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:
deployment: {}
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
deployment: {}
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:
deployment: {}
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
deployment: {}
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 statefulset or deployment should fail
asserts:
- failedTemplate:
errorMessage: "At least one of statefulSet or deployment is required"
100 changes: 64 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,74 @@ 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:

# Deployment or StatefulSet specification for Fleet Server.
# At least one is required of [deployment, statefulSet].
# 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:
# replicas: 1
# podTemplate:
# spec:
# serviceAccountName: fleet-server
# automountServiceAccountToken: true
#
# statefulSet:
# podTemplate:
# spec:
# serviceAccountName: fleet-server
# automountServiceAccountToken: true

# 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