INTLY-2712 check cr for valid json #25

StevenTobin · 2019-07-16T10:40:31Z

Check cr for valid JSON before importing dashboard.

Verification steps:

Install the operator from this branch
Create a dashboard with invalid json and try to import it
The operator should log a meaningful error and the dashboard should not get imported
Check the dashboard CR: a status message with the json error should have been appended
fix the json and make sure the dashboard is imported now

pkg/controller/grafanadashboard/dashboard_controller.go

david-martin · 2019-07-17T09:35:45Z

closes #18

pb82 · 2019-07-18T13:36:30Z

pkg/controller/grafana/pluginsHelper.go

@@ -118,7 +104,8 @@ func (h *PluginsHelperImpl) FilterPlugins(cr *integreatly.Grafana, requested int
 		// Don't allow to install multiple versions of the same plugin
 		if filteredPlugins.HasSomeVersionOf(&plugin) == true {
 			installedVersion := filteredPlugins.GetInstalledVersionOf(&plugin)
-			h.AppendMessage(fmt.Sprintf("Not installing version %s of %s because %s is already installed", plugin.Version, plugin.Name, installedVersion.Version), plugin.Origin)
+			msg := fmt.Sprintf("Not installing version %s of %s because %s is already installed", plugin.Version, plugin.Name, installedVersion.Version)
+			common.KubeHelperImpl{}.AppendMessage(msg, plugin.Origin)


This is creating a new instance of KubeHelper for every iteration of this line. Would be better to not make AppendMessage part of the KubeHelperImpl struct (by removing the (h KubeHelperImpl) part of the function definition).

Or by using a shared instance of KubeHelper (could also be passed in or as a property of PluginHelper)

pb82 · 2019-07-18T15:30:11Z

pkg/controller/grafanadashboard/dashboard_controller.go

+		}
+		if known {
+			r.deleteDashboard(cr)
+			cr.Status.Phase = common.StatusResourceUninitialized


Doesn't this lead to the dashboard being reimported?

No, because we return reconcile.Result after it and the json check won't allow it to get imported the next time.

This is to catch where the json was valid, so the dashboard is in the configmap, but now the json has changed and is invalid.

pb82 · 2019-07-18T15:36:40Z

pkg/controller/grafanadashboard/dashboard_controller.go

+			return reconcile.Result{}, nil
+		}
+		if known {
+			r.deleteDashboard(cr)


This could lead to a valid dashboard to be removed from the configmap when a change is made that breaks the json and the dashboard is reconciled a second time (e.g. by adding a label).

I'm unsure if we should remove a dashboard from the configmap, it shouldn't be possible for it to be imported in the first place when the json was invalid.

Yeah just typing out an answer to your last comment I realise we should leave the valid dashboard in place if the json is now invalid.

pb82 · 2019-07-18T15:43:49Z

pkg/controller/grafanadashboard/dashboard_controller.go

+func isJSON(s string) (bool, error) {
+	var js map[string]interface{}
+	err := json.Unmarshal([]byte(s), &js)
+	return json.Unmarshal([]byte(s), &js) == nil, err


shouldn't this be return err == nil, err, to avoid doing the check twice?

pb82 · 2019-07-18T15:44:55Z

pkg/controller/grafanadashboard/dashboard_controller.go

+	// Dashboard is in cm and JSON is invalid
+	if jsonTest == false && changed {
+		jsonError := err.Error()
+		msg := fmt.Sprintf("Invalid JSON, Error: %s", jsonError)


per convention, all golang log messages should start with lowercase

ah sorry, this is what's going into the status message

david-martin

@StevenTobin @pb82 Logs after creating a dashboard with invalid json

{"level":"info","ts":1563525778.167916,"logger":"controller_grafanadashboard","caller":"grafanadashboard/dashboard_controller.go:193","msg":"dashboard prerequisite check failed"}
{"level":"info","ts":1563525778.168719,"logger":"controller_grafanadashboard","caller":"grafanadashboard/dashboard_controller.go:159","msg":"dashboard reconciled but no changes"}
{"level":"info","ts":1563525778.168788,"logger":"controller_grafanadashboard","caller":"grafanadashboard/dashboard_controller.go:171","msg":"dashboard reconciled but json still invalid"}
{"level":"info","ts":1563525778.168808,"logger":"controller_grafanadashboard","caller":"grafanadashboard/dashboard_controller.go:193","msg":"dashboard prerequisite check failed"}

status of CR

status:
  lastConfig: a6bc7800395833e792c7be2240ecbd0d
  messages:
    - message: >-
        invalid JSON, error: invalid character ',' looking for beginning of
        object key string
      timestamp: 'Friday, 19-Jul-19 09:42:58 IST'
  phase: 2

Fixed the invalid json and the dashboard was added to Grafana.
However, the status still shows:

status:
  lastConfig: ac51e61e86cc26f00717240114511787
  messages:
    - message: >-
        invalid JSON, error: invalid character ',' looking for beginning of
        object key string
      timestamp: 'Friday, 19-Jul-19 09:42:58 IST'
  phase: 2

Is that expected?

pb82 · 2019-07-19T08:48:20Z

@david-martin yes, at the moment we don't remove the error message. No i'm thinking, maybe it would be a good idea to do so?

david-martin · 2019-07-19T08:52:18Z

@pb82 The reason I mention it is because it made me think there was still a problem with the dashboard. However, that wasn't the case as the Dashboard was added OK to Grafana when I checked the grafana-dashboards configmap & UI.

newline at EOF add JSON error to status fix for code check add error checking for phases, JSON error changing and fixing error fix for code check remove debug print use hash to control when to run

StevenTobin · 2019-07-19T09:52:32Z

Tested changes locally, all looks good. CR with invalid JSON is being ignored and an error logged in status.

  messages:
    - message: 'invalid JSON, error: invalid character ''\n'' in string literal'
      timestamp: 'Friday, 19-Jul-19 10:49:11 IST'
    - message: dashboard 'ferwa/dashboard-with-plugins.json' imported
      timestamp: 'Friday, 19-Jul-19 10:49:39 IST'
    - message: installing plugin grafana-piechart-panel@1.3.6
      timestamp: 'Friday, 19-Jul-19 10:49:44 IST'
    - message: installing plugin grafana-clock-panel@1.0.2
      timestamp: 'Friday, 19-Jul-19 10:49:44 IST'

Import message is being created after fixing the invalid JSON.

I can't approve as I'm the author of the PR but 👍

StevenTobin marked this pull request as ready for review July 16, 2019 10:40

StevenTobin requested a review from pb82 July 16, 2019 10:40

pb82 reviewed Jul 16, 2019

View reviewed changes

pkg/controller/grafanadashboard/dashboard_controller.go Outdated Show resolved Hide resolved

StevenTobin changed the title ~~check cr for valid json~~ INTLY-2712 check cr for valid json Jul 16, 2019

pb82 reviewed Jul 16, 2019

View reviewed changes

pkg/controller/grafanadashboard/dashboard_controller.go Outdated Show resolved Hide resolved

pb82 reviewed Jul 16, 2019

View reviewed changes

pkg/controller/grafanadashboard/dashboard_controller.go Outdated Show resolved Hide resolved

StevenTobin force-pushed the INTLY-2712 branch from fe929bc to a25b13f Compare July 18, 2019 07:43

pb82 reviewed Jul 18, 2019

View reviewed changes

pb82 self-assigned this Jul 18, 2019

pb82 reviewed Jul 18, 2019

View reviewed changes

david-martin reviewed Jul 19, 2019

View reviewed changes

StevenTobin and others added 6 commits July 19, 2019 11:42

check cr for valid json

6c6dc29

newline at EOF add JSON error to status fix for code check add error checking for phases, JSON error changing and fixing error fix for code check remove debug print use hash to control when to run

travis code check changes

d03cda1

fix appendMessage function

f8c8bf5

move dashboard prerequisites check to import phase

6158fdf

linter fixes

ff0269d

append success message when dashboard is imported

5d46c70

pb82 force-pushed the INTLY-2712 branch from ecc5b95 to 5d46c70 Compare July 19, 2019 09:43

pb82 merged commit fdf7dae into grafana:master Jul 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTLY-2712 check cr for valid json #25

INTLY-2712 check cr for valid json #25

StevenTobin commented Jul 16, 2019 •

edited by pb82

Loading

david-martin commented Jul 17, 2019

pb82 Jul 18, 2019

pb82 Jul 18, 2019

pb82 Jul 18, 2019

StevenTobin Jul 18, 2019

pb82 Jul 18, 2019

StevenTobin Jul 18, 2019

pb82 Jul 18, 2019

pb82 Jul 18, 2019

pb82 Jul 18, 2019

david-martin left a comment •

edited

Loading

pb82 commented Jul 19, 2019

david-martin commented Jul 19, 2019

StevenTobin commented Jul 19, 2019

INTLY-2712 check cr for valid json #25

INTLY-2712 check cr for valid json #25

Conversation

StevenTobin commented Jul 16, 2019 • edited by pb82 Loading

david-martin commented Jul 17, 2019

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

david-martin left a comment • edited Loading

Choose a reason for hiding this comment

pb82 commented Jul 19, 2019

david-martin commented Jul 19, 2019

StevenTobin commented Jul 19, 2019

StevenTobin commented Jul 16, 2019 •

edited by pb82

Loading

david-martin left a comment •

edited

Loading