ECK resources Helm chart - Beats #5899

naemono · 2022-07-28T16:46:00Z

related to #5505

This PR contains the 3rd version of the ECK-managed resources helm charts including

eck-beats chart for installing the different types of Beats
minor updates to the eck-stack chart.

…tly. adjust apiversion in template. adjust beat labels in template. add type of beat to the default values, along with a description.

Adjust examples to override the default values for those values.

renaming beats templates, as they're singular now. adding beats helm unit tests to eck-stack chart. adding beats to eck-stack values file. adding beats to eck-stack chart.yaml

barkbay

Disclaimer: I was feeling a bit out of sync with the recent effort on the Helm Charts, so I wanted to catch up by reviewing this PR 😄 I still however feel like a beginner when it comes to Helm, so apologies if some comments are irrelevant or have been already discussed.

deploy/eck-stack/Chart.yaml

deploy/eck-beats/examples/heartbeat_es_kb_health.yaml

deploy/eck-beats/Chart.yaml

deploy/eck-stack/templates/tests/beats_test.yaml

deploy/eck-beats/examples/auditbeat_hosts.yaml

barkbay · 2022-08-10T13:54:34Z

deploy/eck-beats/examples/auditbeat_hosts.yaml

@@ -0,0 +1,113 @@
+name: auditbeat
+type: auditbeat
+version: 8.2.3


There are a lot of references to what, I guess, should be a recent stack version. Should we try to use hack/update-stack-version.sh to maintain them ?

I've updated this script, and made the appropriate changes to the versions in this PR, but am hesitant to update the other charts in this PR, as we'd need to bump the chart versions when this happens.

barkbay · 2022-08-10T13:59:18Z

deploy/eck-beats/Chart.yaml

+apiVersion: v2
+name: eck-beats
+description: A Helm chart to deploy Elastic Beats managed by the ECK Operator.
+kubeVersion: ">= 1.20.0-0"


Same question here, how do maintain this version? (It is also mentioned in deploy/eck-stack/README.md , I think it feels a bit redundant with what's in the main README.md)

I updated this to include the reasoning behind this requirement, as I think it's important. This would need to be updated during the normal release process for the eck-operator, when the requirement changes.

If a user is using a version of the operator that still supports a previous k8s version (for example ECK 2.2 which still supports K8S 1.19), but the user needs to update the Helm Chart: does it prevent the user to use that new version of the Chart on 1.19? I'm just trying to understand how the min. k8s version supported by the operator and the min. k8s version supported by a Chart are tied.

If a user was running eck 2.2 on K8S 1.19 and attempted to use these Helm Charts, they would fail (helm would throw a failure) because of this K8S version requirement in the Chart.yaml.

deploy/eck-stack/examples/metricbeat_hosts.yaml

barkbay · 2022-08-10T14:06:57Z

deploy/eck-beats/examples/auditbeat_hosts.yaml

+  kibanaRef:
+    name: kibana
+  config:
+    # Since filebeat is used in the default values, this needs to be removed with an empty list. 


This is mentioned in several places, does it not feel a bit odd that the user must explicitly disable a Beat to deploy another one?

Over the course of the previous Helm Chart PRs, we've tried to maintain that installing with the default values gives some sort of useful "default" installation. Though just recently, in the agent Helm Charts PR, the config section in the default values was removed, so maybe this makes sense here as well, but the default will not be useful, but will make the user experience for overrides much better. Thoughts?

Sorry for the very late answer. I need to have a look at what is proposed for Agent. Defaulting to Filebeat feels a bit "opinionated" to me. It means we assume that the "most useful" default is Filebeat, not Metricbeat, and I'm not sure I get the reason why.

I see why defaulting to Filebeat may not make the most sense here. I'll update this to useMetricbeat by default.

actually @barkbay, I remember why I defaulted to filebeat once I went back and looked at this. By defaulting to filebeat, we add the minimum amount to the default values and still have a useful "default" Beat install. If we move this to metricbeat, many more default values would be needed to be added to make a useful default Beat install. Let me know your thoughts here.

barkbay · 2022-08-10T14:13:33Z

deploy/eck-beats/values.yaml

+        - name: filebeat
+          securityContext:
+            runAsUser: 0
+            # If using Red Hat OpenShift uncomment this:


nit: this comment about OpenShift is not preserved in the resulting Beat resource, not sure if there is a way to keep comments.

There is no way to keep comments from values.yaml file after helm renders them. Comments can be placed in templates/*.yaml and are preserved, for example:

I updated templates/beats.yaml to include this:

--- apiVersion: beat.k8s.elastic.co/v1beta1 kind: Beat metadata: # Name of the beat name: {{ include "beat.fullname" . }} labels: {{- include "beat.labels" . | nindent 4 }} ---cut---

And ran

helm install eck-beats . --dry-run --debug

And can see in the output

--- # Source: eck-beats/templates/beats.yaml apiVersion: beat.k8s.elastic.co/v1beta1 kind: Beat metadata: # Name of the beat name: eck-beats

Update update-stack-version.sh to support updating helm chart stack versions as well Add notes about where the k8s version requirement comes from.

ensure cluster role binding ref exists before templating. Tests for clusterrolebinding/clusterrole/serviceaccount

naemono · 2022-10-27T14:16:22Z

We had some discussion around this, and will remove any default beat from the values, and require the user to set a value for type of beat to use to move this PR forward.

newlines at end of all files remove plural from all beat names

naemono · 2022-10-31T15:05:01Z

This PR updated:

No beat type, or config set in default values
Will fail if no beat type is set.
Added examples, and tests for all currently supported beat types (IIUC journalbeat is deprecated)

deploy/eck-beats/examples/filebeat_no_autodiscover.yaml

pebrc · 2022-11-03T19:19:59Z

deploy/eck-beats/examples/auditbeat_hosts.yaml

+spec:
+  type: auditbeat
+  elasticsearchRef:
+    name: elasticsearch


Should we use the default names Elasticsearch and Kibana get when using the corresponding Helm charts?

I've updated this per most comments, but want to check into this further tomorrow, as these names are dependent on the name of the chart itself, unless doing a full name override, so I'll want to ensure this is consistent with the other charts.

@pebrc I've updated all values and all examples across all charts to be consistent for naming and updated all versions to latest stack 8.5.0, along with updating all tests to match/pass.

I've also bumped the versions of all charts where stack version changes were made, and updated the eck-stack Chart.yaml's dependency versions to match.

This should be ready for another set of 👀

deploy/eck-beats/examples/metricbeat_hosts.yaml

deploy/eck-stack/examples/metricbeat_hosts.yaml

pebrc · 2022-11-03T19:48:50Z

deploy/eck-stack/examples/metricbeat_hosts.yaml

+    subjects:
+    - kind: ServiceAccount
+      name: metricbeat
+      namespace: default


can we do something clever to get the release namespace here?

We actually do default to the Release.Namespace. This was an oversight keeping this in the example. It's ben removed.

https://github.com/elastic/cloud-on-k8s/pull/5899/files#diff-7a54af9bb234c05618afe6d34207f8931b534d186a560cebe506c9584414fb3fR26

namespace: {{ .namespace | default $.Release.Namespace | quote }}

I think it's still there. Forgot to push a commit?

Increment eck-stack chart version

Update tests for new stack vesrion, and consistent naming changes.

…e of stack version change.

pebrc

LGTM

pebrc · 2022-11-21T14:58:43Z

deploy/eck-agent/values.yaml

@@ -45,7 +45,7 @@ spec:
  # Reference to ECK-managed Elasticsearch instance.
  #
  elasticsearchRefs:
-  - name: elasticsearch
+  - name: eck-elasticsearch


Is this a breaking change for users who already rely on the old names? Not sure what the expectations around Helm charts are when it comes to this.

These are simply the default values, and are just placeholders for users to provide their own names, as the names of these follow these rules from the values files: (they, by default, are prefixed with the Release name that the user provides, so there's no way to predict it)

# Overridable names of the Elasticsearch resource. # By default, this is the Release name set for the chart, # followed by 'eck-elasticsearch'. # # nameOverride will override the name of the Chart with the name set here, # so nameOverride: quickstart, would convert to '{{ Release.name }}-quickstart' # # nameOverride: "quickstart" # # fullnameOverride will override both the release name, and the chart name, # and will name the Elasticsearch resource exactly as specified. # # fullnameOverride: "quickstart"

Since we are also incrementing the semver already, it may make sense to indicate that these are breaking changes, if any users are using this default behavior. I'll do so.

pebrc · 2022-11-21T15:04:18Z

hack/update-stack-version.sh

@@ -34,7 +34,7 @@ for_all_yaml_do() {
 	local function="$1"
 	# Directories containing Yaml files with version references to replace
 	# Note: hack/operatorhub/config.yaml will need to be updated manually
-	local dirs=(config/samples config/recipes config/e2e test/e2e)
+	local dirs=(config/samples config/recipes config/e2e test/e2e deploy/eck-stack deploy/eck-beats deploy/eck-kibana deploy/eck-elasticsearch)


Why not just deploy? Assuming you want to exclude the operator Helm Chart why is are the Agent charts missing?

pebrc · 2022-11-21T15:12:37Z

deploy/eck-stack/examples/metricbeat_hosts.yaml

+    subjects:
+    - kind: ServiceAccount
+      name: metricbeat
+      namespace: default


I think it's still there. Forgot to push a commit?

…ault values change update the stack version update script to include agent/fleet-server-agent

naemono · 2022-11-21T15:57:06Z

I think it's still there. Forgot to push a commit?

@pebrc this namespace reference was also in an example in eck-beats/examples/*, which is where I removed the previous one from. I've removed this one as well. Ty

Also, to the hack/update-stack-version.sh comment, I believe the initial intent was to not include the operator chart. I've updated this to include agent/fleet.

brsolomon-deloitte · 2023-01-05T13:13:00Z

@naemono Has this chart been pushed to the https://helm.elastic.co repository?

* Add initial version of ECK-Managed Beats Helm Chart

naemono · 2023-01-12T22:42:42Z

@naemono Has this chart been pushed to the https://helm.elastic.co repository?

@brsolomon-deloitte apologies for the delays in response. You should see the Helm chart within the repository in about 1 hour.

naemono added 7 commits July 28, 2022 08:41

Add initial version of ECK-Managed Beats Helm Chart

4344045

don't reference fullname in notes, as each instance is named differen…

6c2bd6c

…tly. adjust apiversion in template. adjust beat labels in template. add type of beat to the default values, along with a description.

Move beats chart to not use a list of instances.

6eaa51f

Adjust examples to override the default values for those values.

adding additional beats examples.

8009bac

renaming beats templates, as they're singular now. adding beats helm unit tests to eck-stack chart. adding beats to eck-stack values file. adding beats to eck-stack chart.yaml

rename test suite name for beats.

f623f15

Adding missing newlines in files, and removing an errant space.

52bf7ac

Update eck-stack chart readme

b84235e

naemono added the >enhancement Enhancement of existing functionality label Jul 28, 2022

naemono marked this pull request as ready for review July 28, 2022 16:49

barkbay self-assigned this Aug 9, 2022

barkbay reviewed Aug 10, 2022

View reviewed changes

naemono added 3 commits August 23, 2022 10:48

Add documentation for helm unit tests.

ac019e8

Update update-stack-version.sh to support updating helm chart stack versions as well Add notes about where the k8s version requirement comes from.

Add license file to beat helm chart

26bbd21

Merge branch 'main' into 5505-beats-eck-resource-helm-chart

0f12ceb

barkbay removed their assignment Sep 13, 2022

naemono added 6 commits September 13, 2022 10:48

Merge branch 'main' into 5505-beats-eck-resource-helm-chart

9f71258

Update to use nindent, not indent for beat annotations

f4f56f6

Making svc/clusterRole/ClusterRoleBinding consistent across Charts.

a98b3c0

Use toYaml properly in helper.

8cbdcad

ensure cluster role binding ref exists before templating. Tests for clusterrolebinding/clusterrole/serviceaccount

Adding/removing newlines

c8116eb

Merge branch 'main' into 5505-beats-eck-resource-helm-chart

57f35ed

naemono added 3 commits October 31, 2022 08:13

wip

aad783c

Add examples and tests for all supported beat types.

02df23a

Consistent versions

2f93c12

newlines at end of all files remove plural from all beat names

Merge branch 'main' into 5505-beats-eck-resource-helm-chart

330bfad

pebrc reviewed Nov 3, 2022

View reviewed changes

deploy/eck-beats/examples/filebeat_no_autodiscover.yaml Outdated Show resolved Hide resolved

pebrc reviewed Nov 3, 2022

View reviewed changes

Review comments.

5762226

naemono added 5 commits November 21, 2022 07:39

Merge branch 'main' into 5505-beats-eck-resource-helm-chart

89516ad

Update examples/values to be consistent on naming of es/kibana/*refs.

690bf12

Increment eck-stack chart version

Update one final example

b493116

Update all versions to latest stack 8.5.0

6c14b30

Update tests for new stack vesrion, and consistent naming changes.

Bumping agent/elasticsearch/fleet-server/kibana chart versions becaus…

1c53785

…e of stack version change.

pebrc reviewed Nov 21, 2022

View reviewed changes

naemono added 2 commits November 21, 2022 09:46

Increment Minor version to be more clear about potential breaking def…

b5fc323

…ault values change update the stack version update script to include agent/fleet-server-agent

remove from eck-stack example as well.

e50c6b3

pebrc approved these changes Nov 21, 2022

View reviewed changes

naemono added the v2.6.0 label Nov 22, 2022

naemono merged commit 170ebe7 into elastic:main Nov 23, 2022

thbkrkr added >feature Adds or discusses adding a feature to the product and removed >enhancement Enhancement of existing functionality labels Jan 5, 2023

naemono added a commit to naemono/cloud-on-k8s that referenced this pull request Jan 9, 2023

ECK resources Helm chart - Beats (elastic#5899)

28a0e5b

* Add initial version of ECK-Managed Beats Helm Chart

naemono mentioned this pull request Jan 9, 2023

[2.6] ECK resources Helm chart - Beats #6286

Merged

naemono added a commit that referenced this pull request Jan 9, 2023

ECK resources Helm chart - Beats (#5899) (#6286)

6a61269

* Add initial version of ECK-Managed Beats Helm Chart

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ECK resources Helm chart - Beats #5899

ECK resources Helm chart - Beats #5899

naemono commented Jul 28, 2022

barkbay left a comment

barkbay Aug 10, 2022

naemono Aug 23, 2022

barkbay Aug 10, 2022

naemono Aug 23, 2022

barkbay Sep 5, 2022

naemono Sep 6, 2022

barkbay Aug 10, 2022

naemono Aug 23, 2022

barkbay Sep 5, 2022 •

edited

Loading

naemono Sep 6, 2022

naemono Sep 6, 2022

barkbay Aug 10, 2022

naemono Aug 23, 2022

naemono commented Oct 27, 2022

naemono commented Oct 31, 2022

pebrc Nov 3, 2022

naemono Nov 21, 2022

naemono Nov 21, 2022

pebrc Nov 3, 2022

naemono Nov 21, 2022

pebrc Nov 21, 2022

pebrc left a comment

pebrc Nov 21, 2022

naemono Nov 21, 2022

pebrc Nov 21, 2022

pebrc Nov 21, 2022

naemono commented Nov 21, 2022

brsolomon-deloitte commented Jan 5, 2023

naemono commented Jan 12, 2023

ECK resources Helm chart - Beats #5899

ECK resources Helm chart - Beats #5899

Conversation

naemono commented Jul 28, 2022

barkbay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barkbay Sep 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naemono commented Oct 27, 2022

naemono commented Oct 31, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pebrc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naemono commented Nov 21, 2022

brsolomon-deloitte commented Jan 5, 2023

naemono commented Jan 12, 2023

barkbay Sep 5, 2022 •

edited

Loading