[kibana] added dashboard import + some fixes #244

monotek · 2019-08-02T14:00:07Z

Signed-off-by: André Bauer [email protected]

added dashboard import via file & url as available in stable/kibana
added missing mandatory field apiVersion to Chart.yaml
fixed fullname template
use labels as recomended by helm (create) 2.14.3
added Makefile to helmignore
changed image variables
use semverCompare to decide if ELASTICSEARCH_URL or ELASTICSEARCH_HOSTS var is used
made resource limits optional

Chart version not bumped (the versions are all bumped and released at the same time)
README.md updated with any new values or changes
Updated template tests in ${CHART}/tests/*.py
Updated integration tests in ${CHART}/examples/*/test/goss.yaml

Signed-off-by: André Bauer <[email protected]>

elasticmachine · 2019-08-02T14:00:10Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Signed-off-by: André Bauer <[email protected]>

Crazybus

Sorry for leaving so many comments. There are a lot of different changes all in this pull request which makes the reviews large too. In the future splitting them into separate pull requests would make things a lot easier for both of us.

I reviewed everything in this PR as is. However I have a couple of ideas for alternative approaches that may be more maintainable than what you are proposing.

Using a postStart hook. Right now you are starting up an initContainer, that starts Kibana with the full configuration of the main container then run some commands against it. It's more or less just re-implementing exactly what a postStart hook does except with a whole bunch of extra code duplication.
Not implementing this specifically into the chart itself. Making sure these import script can cover all different use cases is quite hard (especially in bash...maybe using only python would make it easier and more testable). An alternative approach would be to just expose the lifecycle block like the Elasticsearch chart does. Then users could add whatever kind of imports they wanted without needing to come up with a generic solution that works for everyone. Things get more complicated if you want to do things like supporting importing dashboards from private GitHub repos.

Crazybus · 2019-08-06T18:15:24Z

kibana/Chart.yaml

 name: kibana
-version: 7.3.0
+version: 7.3.0-1


Can you revert the version bump. The pull request template and the contributing guide both explain why we don't do this.

Crazybus · 2019-08-06T18:17:30Z

kibana/Chart.yaml

 appVersion: 7.3.0
+description: Official Elastic helm chart for Kibana
+home: https://github.com/elastic/helm-charts/kibana


This link is broken and gives a 404.

Crazybus · 2019-08-06T18:19:36Z

kibana/README.md

@@ -33,7 +33,7 @@ Examples of installing older major versions can be found in the [examples](./exa
 While only the latest releases are tested, it is possible to easily install old or new releases by overriding the `imageTag`. To install version `7.3.0` of Kibana it would look like this:

 ```
-helm install --name kibana elastic/kibana --set imageTag=7.3.0
+helm install --name kibana elastic/kibana --set image.tag=7.3.0


This would be a breaking change and cause a lot of unexpected breakages for users. It would silently cause users to be upgraded to the latest version since the old imageTag would be ignored. I understand that this is the latest value name used in the current helm template but we only do breaking changes along with the rest of the stack (so 8.0.0 being the next version with breaking changes).

Crazybus · 2019-08-06T18:29:22Z

kibana/templates/_helpers.tpl

+app.kubernetes.io/name: {{ include "kibana.name" . }}
+helm.sh/chart: {{ include "kibana.chart" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+{{- if .Chart.AppVersion }}


This is hardcoded into the Chart.yaml so I don't think the if statement is needed.

Crazybus · 2019-08-06T18:31:28Z

kibana/templates/_helpers.tpl

+helm.sh/chart: {{ include "kibana.chart" . }}
+app.kubernetes.io/instance: {{ .Release.Name }}
+{{- if .Chart.AppVersion }}
+app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}


I think setting this might be misleading as the appVersion will always be whatever the latest version is, rather than whatever version the user has installed. I think it would be better to leave it out or have a way for the user to configure this.

Crazybus · 2019-08-06T19:08:00Z

kibana/values.yaml

+  timeout: 60
+  xpackauth:
+    enabled: false
+    username: myuser


Its not a good idea to store secrets directly in the values.yaml. If you take a look at the readinessProbe for the Kibana container there is a function that can be used to properly use http/https and the right authentication without needing to duplicate the password and add it to the values file.

helm-charts/kibana/templates/deployment.yaml

Lines 94 to 106 in a386536

#!/usr/bin/env bash -e

http () {

local path="${1}"

set -- -XGET -s --fail

if [ -n "${ELASTIC_USERNAME}" ] && [ -n "${ELASTIC_PASSWORD}" ]; then

set -- "$@" -u "${ELASTIC_USERNAME}:${ELASTIC_PASSWORD}"

fi

curl -k "$@" "{{ .Values.protocol }}://localhost:{{ .Values.httpPort }}${path}"

}

http "{{ .Values.healthCheckPath }}"

Crazybus · 2019-08-06T19:08:58Z

kibana/templates/configmap-dashboardimport.yaml

+    #!/usr/bin/env bash
+    #
+    # kibana dashboard import script
+    #


a set -euo pipefail up here will make sure that the script fails properly if any of the commands fail.

http://redsymbol.net/articles/unofficial-bash-strict-mode/

Crazybus · 2019-08-06T19:12:00Z

kibana/templates/configmap-dashboardimport.yaml

+    echo "Waiting up to {{ .Values.dashboardImport.timeout }} seconds for Kibana to get in green overall state..."
+
+    for i in {1..{{ .Values.dashboardImport.timeout }}}; do
+      curl -s localhost:5601/api/status | python -c 'import sys, json; print json.load(sys.stdin)["status"]["overall"]["state"]' 2> /dev/null | grep green > /dev/null && break || sleep 1


I already commented on this in the values.yaml, but hard coding the protocol and port is going to break for anyone using https or a different port. The readinessProbe has a function that handles all of the different use cases.

helm-charts/kibana/templates/deployment.yaml

Lines 94 to 106 in a386536

#!/usr/bin/env bash -e

http () {

local path="${1}"

set -- -XGET -s --fail

if [ -n "${ELASTIC_USERNAME}" ] && [ -n "${ELASTIC_PASSWORD}" ]; then

set -- "$@" -u "${ELASTIC_USERNAME}:${ELASTIC_PASSWORD}"

fi

curl -k "$@" "{{ .Values.protocol }}://localhost:{{ .Values.httpPort }}${path}"

}

http "{{ .Values.healthCheckPath }}"

Anyone using a custom webroot will also have issues causing all of the api calls to fail.

Crazybus · 2019-08-06T19:15:06Z

kibana/README.md

+| `dashboardImport.xpackauth.enabled`        | Enable Xpack auth                                                      | `false`                               |
+| `dashboardImport.xpackauth.username`       | Optional Xpack username                                                | `myuser`                              |
+| `dashboardImport.xpackauth.password`       | Optional Xpack password                                                | `mypass`                              |
+| `dashboardImport.dashboards`               | Dashboards                                                             | `{}`                                  |


Could you explain what formats are supported? From the import script I can see that you can specify http URLs or files that are mounted within the initContainer.

Crazybus · 2019-08-06T19:18:12Z

kibana/templates/configmap-dashboardimport.yaml

+      fi
+
+      if [ "$?" != "0" ]; then
+        echo -e "\nImport of ${DASHBOARD_FILE} dashboard failed... Exiting..."


Do you actually want this script to fail if it can't install a dashboard. If GitHub is down it would mean that you would be unable to start up Kibana without modifying the install.

botelastic · 2020-01-07T15:58:02Z

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. To track this PR (even if closed), please open a corresponding issue if one does not already exist.

monotek · 2020-01-07T16:14:27Z

To be honest i've stopped using kibana as we use stackdriver now so my interest in implementing these changes is very low at the moment.

From my perspective we can close this pr and deprecate the stable/kibana chart as soon as you want.

morganchristiansson · 2020-01-08T13:31:44Z

This is actually a useful PR - if someone took over and submitted new PR solving all issues would it be merged? I'm currently managing these manually through Kibana and .kibana index is periodically backed up in s3 snapshots. Codifying them somehow is on my todo list.

jmlrt · 2020-01-08T17:40:34Z

@monotek many thanks for this PR and your work on stable/kibana.

Despite this is an interesting feature, I'm not sure this is something we should include in Kibana chart.

Handling all cases without impacting Kibana reliability can become complicated (handling different sources for dashboard files, not failing deployment/upgrade if the dashboard files are not available or corrupted, handling kibana api url change if protocol or port is changed, as well as if the endpoint change in a future version of kibana...).

I would prefer not to implement this feature in the chart and let users implement it outside of the chart to keep it as simple and reliable as possible and focus on providing Kubernetes and Elastic production best practices and flexibility.

As mentionned in #244 (review), this is typically something that you could implement while handling only your own case using Helm postStart hook which are now overridable since #197.

added dashboard import + some fixes

61e168d

Signed-off-by: André Bauer <[email protected]>

add not commited file

21a4cc7

Signed-off-by: André Bauer <[email protected]>

monotek mentioned this pull request Aug 6, 2019

[stable/kibana] Migration Tracker helm/charts#14935

Closed

10 tasks

Crazybus suggested changes Aug 6, 2019

View reviewed changes

jmlrt added the feature label Oct 17, 2019

botelastic bot added the triage/stale label Jan 7, 2020

botelastic bot removed the triage/stale label Jan 7, 2020

jmlrt closed this Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kibana] added dashboard import + some fixes #244

[kibana] added dashboard import + some fixes #244

monotek commented Aug 2, 2019 •

edited

Loading

elasticmachine commented Aug 2, 2019

Crazybus left a comment

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

Crazybus Aug 6, 2019

botelastic bot commented Jan 7, 2020

monotek commented Jan 7, 2020

morganchristiansson commented Jan 8, 2020 •

edited

Loading

jmlrt commented Jan 8, 2020

	#!/usr/bin/env bash -e
	http () {
	local path="${1}"
	set -- -XGET -s --fail

	if [ -n "${ELASTIC_USERNAME}" ] && [ -n "${ELASTIC_PASSWORD}" ]; then
	set -- "$@" -u "${ELASTIC_USERNAME}:${ELASTIC_PASSWORD}"
	fi

	curl -k "$@" "{{ .Values.protocol }}://localhost:{{ .Values.httpPort }}${path}"
	}

	http "{{ .Values.healthCheckPath }}"

[kibana] added dashboard import + some fixes #244

[kibana] added dashboard import + some fixes #244

Conversation

monotek commented Aug 2, 2019 • edited Loading

elasticmachine commented Aug 2, 2019

Crazybus 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

botelastic bot commented Jan 7, 2020

monotek commented Jan 7, 2020

morganchristiansson commented Jan 8, 2020 • edited Loading

jmlrt commented Jan 8, 2020

monotek commented Aug 2, 2019 •

edited

Loading

morganchristiansson commented Jan 8, 2020 •

edited

Loading