fix: don't use dashboard cache if url is different (#841)

* fix: lift restriction on having both url and grafanacom specified in a dashboard spec

There's a fallback logic in (r *DashboardPipelineImpl) obtainJson() that allows a user to have multiple sources for configuration with falling back to the next one in case of an error. For unknown reasons, having both url and grafanacom in spec was not allowed. This restriction should be lifted.

* fix: don't use dashboard cache if URL is different, and other minor fixes

- for code simplification, moved from &metav1.Duration to metav1.Duration where the former was not really needed (GrafanaDashboardStatus, GrafanaSpec);
- fix: for unknown reasons, NewDashboardPipeline would fallback to 24h instead of 0(=infinity) if dashboard.Spec.ContentCacheDuration is not defined. Removed that;
- moved cache extraction from obtainJson() to loadDashboardFromURL(). - I think that location makes more sense;
- fix: don’t use dashboard cache if url is different.

api/integreatly/v1alpha1/grafana_types.go

@@ -41,7 +41,7 @@ type GrafanaSpec struct {
 
 	// DashboardContentCacheDuration sets a default for when a `GrafanaDashboard` resource doesn't specify a `contentCacheDuration`.
 	// If left unset or 0 the default behavior is to cache indefinitely.
-	DashboardContentCacheDuration *metav1.Duration `json:"dashboardContentCacheDuration,omitempty"`
+	DashboardContentCacheDuration metav1.Duration `json:"dashboardContentCacheDuration,omitempty"`
 }
 
 type ReadinessProbeSpec struct {

api/integreatly/v1alpha1/grafanadashboard_types.go

@@ -18,14 +18,14 @@ package v1alpha1
 
 import (
 	"bytes"
-	"crypto/sha1" // nolint
+	"compress/gzip"
+	"crypto/sha1" //nolint
 	"crypto/sha256"
 	"encoding/json"
 	"fmt"
 	"io"
 	"io/ioutil"
-
-	"compress/gzip"
+	"time"
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -82,7 +82,7 @@ type GrafanaDashboardRef struct {
 
 type GrafanaDashboardStatus struct {
 	ContentCache     []byte                 `json:"contentCache,omitempty"`
-	ContentTimestamp *metav1.Time           `json:"contentTimestamp,omitempty"`
+	ContentTimestamp metav1.Time            `json:"contentTimestamp,omitempty"`
 	ContentUrl       string                 `json:"contentUrl,omitempty"`
 	Error            *GrafanaDashboardError `json:"error,omitempty"`
 }
@@ -171,7 +171,7 @@ func (d *GrafanaDashboard) Parse(optional string) (map[string]interface{}, error
 		dashboardBytes = []byte(optional)
 	}
 
-	var parsed = make(map[string]interface{})
+	parsed := make(map[string]interface{})
 	err := json.Unmarshal(dashboardBytes, &parsed)
 	return parsed, err
 }
@@ -191,6 +191,34 @@ func (d *GrafanaDashboard) UID() string {
 	return fmt.Sprintf("%x", sha1.Sum([]byte(d.Namespace+d.Name))) // nolint
 }
 
+func (d *GrafanaDashboard) GetContentCache(url string) string {
+	var cacheDuration time.Duration
+	if d.Spec.ContentCacheDuration != nil {
+		cacheDuration = d.Spec.ContentCacheDuration.Duration
+	}
+
+	return d.Status.getContentCache(url, cacheDuration)
+}
+
+// getContentCache returns content cache when the following conditions are met: url is the same, data is not expired, gzipped data is not corrupted
+func (s *GrafanaDashboardStatus) getContentCache(url string, cacheDuration time.Duration) string {
+	if s.ContentUrl != url {
+		return ""
+	}
+
+	notExpired := cacheDuration <= 0 || s.ContentTimestamp.Add(cacheDuration).After(time.Now())
+	if !notExpired {
+		return ""
+	}
+
+	cache, err := Gunzip(s.ContentCache)
+	if err != nil {
+		return ""
+	}
+
+	return string(cache)
+}
+
 func Gunzip(compressed []byte) ([]byte, error) {
 	decoder, err := gzip.NewReader(bytes.NewReader(compressed))
 	if err != nil {

api/integreatly/v1alpha1/grafanadashboard_types_test.go

@@ -7,6 +7,10 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 // Encoded via cat | gzip | base64
@@ -78,3 +82,86 @@ func TestDecompress(t *testing.T) {
 		t.Fail()
 	}
 }
+
+func TestGrafanaDashboardStatus_getContentCache(t *testing.T) {
+	timestamp := metav1.Time{Time: time.Now().Add(-1 * time.Hour)}
+	infinite := 0 * time.Second
+	dashboardJSON := `{"dummyField": "dummyData"}`
+
+	cachedDashboard, err := Gzip(dashboardJSON)
+	assert.Nil(t, err)
+
+	url := "http://127.0.0.1:8080/1.json"
+
+	// Correctly populated cache
+	status := GrafanaDashboardStatus{
+		ContentCache:     cachedDashboard,
+		ContentTimestamp: timestamp,
+		ContentUrl:       url,
+	}
+
+	// Corrupted cache
+	statusCorrupted := GrafanaDashboardStatus{
+		ContentCache:     []byte("abc"),
+		ContentTimestamp: timestamp,
+		ContentUrl:       url,
+	}
+
+	tests := []struct {
+		name     string
+		status   GrafanaDashboardStatus
+		url      string
+		duration time.Duration
+		want     string
+	}{
+		{
+			name:     "no cache: fields are not populated",
+			url:      status.ContentUrl,
+			duration: infinite,
+			status:   GrafanaDashboardStatus{},
+			want:     "",
+		},
+		{
+			name:     "no cache: url is different",
+			url:      "http://another-url/2.json",
+			duration: infinite,
+			status:   status,
+			want:     "",
+		},
+		{
+			name:     "no cache: expired",
+			url:      status.ContentUrl,
+			duration: 1 * time.Minute,
+			status:   status,
+			want:     "",
+		},
+		{
+			name:     "no cache: corrupted gzip",
+			url:      statusCorrupted.ContentUrl,
+			duration: infinite,
+			status:   statusCorrupted,
+			want:     "",
+		},
+		{
+			name:     "valid cache: not expired yet",
+			url:      status.ContentUrl,
+			duration: 24 * time.Hour,
+			status:   status,
+			want:     dashboardJSON,
+		},
+		{
+			name:     "valid cache: not expired yet (infinite)",
+			url:      status.ContentUrl,
+			duration: infinite,
+			status:   status,
+			want:     dashboardJSON,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.status.getContentCache(tt.url, tt.duration)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}

controllers/common/controllerState.go

@@ -7,7 +7,7 @@ var ControllerEvents = make(chan ControllerState, 1)
 type ControllerState struct {
 	DashboardSelectors            []*v1.LabelSelector
 	DashboardNamespaceSelector    *v1.LabelSelector
-	DashboardContentCacheDuration *v1.Duration
+	DashboardContentCacheDuration v1.Duration
 	AdminUrl                      string
 	GrafanaReady                  bool
 	ClientTimeout                 int

controllers/grafana/grafana_controller.go

@@ -16,7 +16,6 @@ import (
 	v1 "k8s.io/api/core/v1"
 	netv1 "k8s.io/api/networking/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/client-go/tools/record"
@@ -308,10 +307,6 @@ func (r *ReconcileGrafana) manageSuccess(cr *grafanav1alpha1.Grafana, state *com
 		cr.Status.InstalledDashboards = r.Config.GetDashboards("")
 	}
 
-	if cr.Spec.DashboardContentCacheDuration == nil {
-		cr.Spec.DashboardContentCacheDuration = &metav1.Duration{Duration: 0}
-	}
-
 	instance := &grafanav1alpha1.Grafana{}
 	err := r.Client.Get(r.Context, request.NamespacedName, instance)
 	if err != nil {

controllers/grafanadashboard/dashboard_pipeline.go

@@ -53,9 +53,6 @@ type DashboardPipelineImpl struct {
 }
 
 func NewDashboardPipeline(client client.Client, dashboard *v1alpha1.GrafanaDashboard, ctx context.Context) DashboardPipeline {
-	if dashboard.Spec.ContentCacheDuration == nil {
-		dashboard.Spec.ContentCacheDuration = &metav1.Duration{Duration: 24 * time.Hour}
-	}
 	return &DashboardPipelineImpl{
 		Client:    client,
 		Dashboard: dashboard,
@@ -117,16 +114,6 @@ func (r *DashboardPipelineImpl) validateJson() error {
 	return err
 }
 
-func (r *DashboardPipelineImpl) shouldUseContentCache() bool {
-	if r.Dashboard.Status.ContentCache != nil && r.Dashboard.Spec.ContentCacheDuration != nil && r.Dashboard.Status.ContentTimestamp != nil {
-		cacheDuration := r.Dashboard.Spec.ContentCacheDuration.Duration
-		contentTimeStamp := r.Dashboard.Status.ContentTimestamp
-
-		return cacheDuration <= 0 || contentTimeStamp.Add(cacheDuration).After(time.Now())
-	}
-	return false
-}
-
 // Try to get the dashboard json definition either from a provided URL or from the
 // raw json in the dashboard resource. The priority is as follows:
 //  0. try to use previously fetched content from url or grafanaCom if it is valid
@@ -136,23 +123,8 @@ func (r *DashboardPipelineImpl) shouldUseContentCache() bool {
 //  3. no configmap specified: try to use embedded json
 //  4. no json specified: try to use embedded jsonnet
 func (r *DashboardPipelineImpl) obtainJson() error {
-	// TODO(DeanBrunt): Add earlier validation for this
-	if r.Dashboard.Spec.Url != "" && r.Dashboard.Spec.GrafanaCom != nil {
-		return errors.New("both dashboard url and grafana.com source specified")
-	}
-
 	var returnErr error
 
-	if r.shouldUseContentCache() {
-		jsonBytes, err := v1alpha1.Gunzip(r.Dashboard.Status.ContentCache)
-		if err != nil {
-			returnErr = fmt.Errorf("failed to decode/decompress gzipped json: %w", err)
-		} else {
-			r.JSON = string(jsonBytes)
-			return nil
-		}
-	}
-
 	if r.Dashboard.Spec.GrafanaCom != nil {
 		url, err := r.getGrafanaComDashboardUrl()
 		if err != nil {
@@ -237,6 +209,11 @@ func (r *DashboardPipelineImpl) loadJsonnet(source string) (string, error) {
 
 // Try to obtain the dashboard json from a provided url
 func (r *DashboardPipelineImpl) loadDashboardFromURL(source string) error {
+	r.JSON = r.Dashboard.GetContentCache(source)
+	if r.JSON != "" {
+		return nil
+	}
+
 	url, err := url.ParseRequestURI(source)
 	if err != nil {
 		return fmt.Errorf("invalid url %v", source)
@@ -264,7 +241,7 @@ func (r *DashboardPipelineImpl) loadDashboardFromURL(source string) error {
 				Code:    resp.StatusCode,
 				Retries: retries + 1,
 			},
-			ContentTimestamp: &metav1.Time{Time: time.Now()},
+			ContentTimestamp: metav1.Time{Time: time.Now()},
 		}
 
 		if err := r.Client.Status().Update(r.Context, r.Dashboard); err != nil {
@@ -305,7 +282,7 @@ func (r *DashboardPipelineImpl) loadDashboardFromURL(source string) error {
 	r.refreshDashboard()
 	r.Dashboard.Status = v1alpha1.GrafanaDashboardStatus{
 		ContentCache:     content,
-		ContentTimestamp: &metav1.Time{Time: time.Now()},
+		ContentTimestamp: metav1.Time{Time: time.Now()},
 		ContentUrl:       source,
 	}
 

controllers/grafanadashboard/grafanadashboard_controller.go

@@ -24,6 +24,7 @@ import (
 	"math"
 	"net/http"
 	"os"
+	"time"
 
 	"github.com/go-logr/logr"
 	grafanav1alpha1 "github.com/grafana-operator/grafana-operator/v4/api/integreatly/v1alpha1"
@@ -38,8 +39,6 @@ import (
 	"k8s.io/client-go/tools/record"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 
-	"time"
-
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -289,7 +288,6 @@ func (r *GrafanaDashboardReconciler) reconcileDashboards(request reconcile.Reque
 		}
 
 		folder, err := grafanaClient.CreateOrUpdateFolder(folderName)
-
 		if err != nil {
 			log.Log.Error(err, "failed to get or create namespace folder for dashboard", "folder", folderName, "dashboard", request.Name)
 			r.manageError(&dashboard, err)
@@ -303,8 +301,9 @@ func (r *GrafanaDashboardReconciler) reconcileDashboards(request reconcile.Reque
 			folderId = *folder.ID
 		}
 
+		// If ContentCacheDuration is not defined at a dashboard level, fallback to the instance-level value
 		if dashboard.Spec.ContentCacheDuration == nil {
-			dashboard.Spec.ContentCacheDuration = r.state.DashboardContentCacheDuration
+			dashboard.Spec.ContentCacheDuration = &r.state.DashboardContentCacheDuration
 		}
 
 		// Process the dashboard. Use the known hash of an existing dashboard
@@ -359,7 +358,7 @@ func (r *GrafanaDashboardReconciler) reconcileDashboards(request reconcile.Reque
 
 		_, err = grafanaClient.CreateOrUpdateDashboard(processed, folderId, folderName)
 		if err != nil {
-			//log.Log.Error(err, "cannot submit dashboard %v/%v", "namespace", dashboard.Namespace, "name", dashboard.Name)
+			// log.Log.Error(err, "cannot submit dashboard %v/%v", "namespace", dashboard.Namespace, "name", dashboard.Name)
 			r.manageError(&dashboard, err)
 
 			continue
@@ -453,7 +452,6 @@ func (r *GrafanaDashboardReconciler) checkNamespaceLabels(dashboard *grafanav1al
 		return false, err
 	}
 	selector, err := metav1.LabelSelectorAsSelector(r.state.DashboardNamespaceSelector)
-
 	if err != nil {
 		return false, err
 	}