diff --git a/api/v1beta1/grafanadatasource_types.go b/api/v1beta1/grafanadatasource_types.go index f45da5887..dbdcd5a4e 100644 --- a/api/v1beta1/grafanadatasource_types.go +++ b/api/v1beta1/grafanadatasource_types.go @@ -17,16 +17,15 @@ limitations under the License. package v1beta1 import ( - "bytes" "encoding/json" - "errors" - "fmt" "time" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) type GrafanaDatasourceInternal struct { + // Deprecated field, use spec.uid instead + // +optional UID string `json:"uid,omitempty"` Name string `json:"name,omitempty"` Type string `json:"type,omitempty"` @@ -40,7 +39,9 @@ type GrafanaDatasourceInternal struct { // Deprecated field, it has no effect OrgID *int64 `json:"orgId,omitempty"` - // Deprecated field, it has no effect + + // Whether to enable/disable editing of the datasource in Grafana UI + // +optional Editable *bool `json:"editable,omitempty"` // +kubebuilder:validation:Schemaless @@ -57,7 +58,13 @@ type GrafanaDatasourceInternal struct { } // GrafanaDatasourceSpec defines the desired state of GrafanaDatasource +// +kubebuilder:validation:XValidation:rule="((!has(oldSelf.uid) && !has(self.uid)) || (has(oldSelf.uid) && has(self.uid)))", message="spec.uid is immutable" type GrafanaDatasourceSpec struct { + // The UID, for the datasource, fallback to the deprecated spec.datasource.uid and metadata.uid + // +optional + // +kubebuilder:validation:XValidation:rule="self == oldSelf",message="spec.uid is immutable" + CustomUID string `json:"uid,omitempty"` + Datasource *GrafanaDatasourceInternal `json:"datasource"` // selects Grafana instances for import @@ -146,37 +153,26 @@ func (in *GrafanaDatasource) Unchanged(hash string) bool { return in.Status.Hash == hash } -func (in *GrafanaDatasource) IsUpdatedUID(uid string) bool { +func (in *GrafanaDatasource) IsUpdatedUID() bool { // Datasource has just been created, status is not yet updated if in.Status.UID == "" { return false } - if uid == "" { - uid = string(in.UID) - } - - return in.Status.UID != uid + return in.Status.UID != in.CustomUIDOrUID() } -func (in *GrafanaDatasource) ExpandVariables(variables map[string][]byte) ([]byte, error) { - if in.Spec.Datasource == nil { - return nil, errors.New("data source is empty, can't expand variables") - } - - raw, err := json.Marshal(in.Spec.Datasource) - if err != nil { - return nil, err +// Wrapper around CustomUID, datasourcelUID or default metadata.uid +func (in *GrafanaDatasource) CustomUIDOrUID() string { + if in.Spec.CustomUID != "" { + return in.Spec.CustomUID } - for key, value := range variables { - patterns := []string{fmt.Sprintf("$%v", key), fmt.Sprintf("${%v}", key)} - for _, pattern := range patterns { - raw = bytes.ReplaceAll(raw, []byte(pattern), value) - } + if in.Spec.Datasource.UID != "" { + return in.Spec.Datasource.UID } - return raw, nil + return string(in.ObjectMeta.UID) } func (in *GrafanaDatasource) IsAllowCrossNamespaceImport() bool { diff --git a/api/v1beta1/grafanadatasource_types_test.go b/api/v1beta1/grafanadatasource_types_test.go index e1727afee..97daedc04 100644 --- a/api/v1beta1/grafanadatasource_types_test.go +++ b/api/v1beta1/grafanadatasource_types_test.go @@ -1,62 +1,70 @@ package v1beta1 import ( - "bytes" - "fmt" - "testing" -) + "context" -func TestGrafanaDatasources_expandVariables(t *testing.T) { - type testcase struct { - name string - variables map[string][]byte - in GrafanaDatasource - out []byte - } + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) - testcases := []testcase{ - { - name: "basic replacement", - variables: map[string][]byte{ - "PROMETHEUS_USERNAME": []byte("root"), - }, - in: GrafanaDatasource{ - Spec: GrafanaDatasourceSpec{ - Datasource: &GrafanaDatasourceInternal{ - Name: "prometheus", - User: "${PROMETHEUS_USERNAME}", - }, - }, - }, - out: []byte("{\"name\":\"prometheus\",\"user\":\"root\"}"), +func newDatasource(name string, uid string) *GrafanaDatasource { + return &GrafanaDatasource{ + TypeMeta: v1.TypeMeta{ + APIVersion: APIVersion, + Kind: "GrafanaDatasource", }, - { - name: "replacement without curly braces", - variables: map[string][]byte{ - "PROMETHEUS_USERNAME": []byte("root"), - }, - in: GrafanaDatasource{ - Spec: GrafanaDatasourceSpec{ - Datasource: &GrafanaDatasourceInternal{ - Name: "prometheus", - User: "$PROMETHEUS_USERNAME", - }, + ObjectMeta: v1.ObjectMeta{ + Name: name, + Namespace: "default", + }, + Spec: GrafanaDatasourceSpec{ + CustomUID: uid, + InstanceSelector: &v1.LabelSelector{ + MatchLabels: map[string]string{ + "test": "datasource", }, }, - out: []byte("{\"name\":\"prometheus\",\"user\":\"root\"}"), + Datasource: &GrafanaDatasourceInternal{ + Name: "testdata", + Type: "grafana-testdata-datasource", + Access: "proxy", + }, }, } +} - for _, tc := range testcases { - t.Run(tc.name, func(t *testing.T) { - b, err := tc.in.ExpandVariables(tc.variables) - if err != nil { - t.Fatal(err) - } +var _ = Describe("Datasource type", func() { + Context("Ensure Datasource spec.uid is immutable", func() { + ctx := context.Background() + It("Should block adding uid field when missing", func() { + ds := newDatasource("missing-uid", "") + By("Create new Datasource without uid") + Expect(k8sClient.Create(ctx, ds)).To(Succeed()) - if !bytes.Equal(b, tc.out) { - t.Error(fmt.Errorf("expected %v, but got %v", string(tc.out), string(b))) - } + By("Adding a uid") + ds.Spec.CustomUID = "new-uid" + Expect(k8sClient.Update(ctx, ds)).To(HaveOccurred()) }) - } -} + + It("Should block removing uid field when set", func() { + ds := newDatasource("existing-uid", "existing-uid") + By("Creating Datasource with existing UID") + Expect(k8sClient.Create(ctx, ds)).To(Succeed()) + + By("And setting UID to ''") + ds.Spec.CustomUID = "" + Expect(k8sClient.Update(ctx, ds)).To(HaveOccurred()) + }) + + It("Should block changing value of uid", func() { + ds := newDatasource("removing-uid", "existing-uid") + By("Create new Datasource with existing UID") + Expect(k8sClient.Create(ctx, ds)).To(Succeed()) + + By("Changing the existing UID") + ds.Spec.CustomUID = "new-uid" + Expect(k8sClient.Update(ctx, ds)).To(HaveOccurred()) + }) + }) +}) diff --git a/api/v1beta1/suite_test.go b/api/v1beta1/suite_test.go new file mode 100644 index 000000000..9a4a36826 --- /dev/null +++ b/api/v1beta1/suite_test.go @@ -0,0 +1,80 @@ +/* +Copyright 2022. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1beta1 + +import ( + "path/filepath" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "k8s.io/client-go/kubernetes/scheme" + "k8s.io/client-go/rest" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/envtest" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + //+kubebuilder:scaffold:imports +) + +// These tests use Ginkgo (BDD-style Go testing framework). Refer to +// http://onsi.github.io/ginkgo/ to learn more about Ginkgo. + +const ( + APIVersion = "grafana.integreatly.org/v1beta1" +) + +var ( + cfg *rest.Config + k8sClient client.Client + testEnv *envtest.Environment +) + +func TestAPIs(t *testing.T) { + RegisterFailHandler(Fail) + + RunSpecs(t, "CRDs Suite") +} + +var _ = BeforeSuite(func() { + logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true))) + + By("bootstrapping test environment") + testEnv = &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")}, + ErrorIfCRDPathMissing: true, + } + + cfg, err := testEnv.Start() + Expect(err).NotTo(HaveOccurred()) + Expect(cfg).NotTo(BeNil()) + + err = AddToScheme(scheme.Scheme) + Expect(err).NotTo(HaveOccurred()) + + //+kubebuilder:scaffold:scheme + + k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) + Expect(err).NotTo(HaveOccurred()) + Expect(k8sClient).NotTo(BeNil()) +}) + +var _ = AfterSuite(func() { + By("tearing down the test environment") + err := testEnv.Stop() + Expect(err).NotTo(HaveOccurred()) +}) diff --git a/config/crd/bases/grafana.integreatly.org_grafanadatasources.yaml b/config/crd/bases/grafana.integreatly.org_grafanadatasources.yaml index 8ee3e9faa..28596b2a5 100644 --- a/config/crd/bases/grafana.integreatly.org_grafanadatasources.yaml +++ b/config/crd/bases/grafana.integreatly.org_grafanadatasources.yaml @@ -67,7 +67,8 @@ spec: database: type: string editable: - description: Deprecated field, it has no effect + description: Whether to enable/disable editing of the datasource + in Grafana UI type: boolean isDefault: type: boolean @@ -86,6 +87,7 @@ spec: type: type: string uid: + description: Deprecated field, use spec.uid instead type: string url: type: string @@ -161,6 +163,13 @@ spec: format: duration pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ type: string + uid: + description: The UID, for the datasource, fallback to the deprecated + spec.datasource.uid and metadata.uid + type: string + x-kubernetes-validations: + - message: spec.uid is immutable + rule: self == oldSelf valuesFrom: description: environments variables from secrets or config maps items: @@ -231,6 +240,10 @@ spec: - datasource - instanceSelector type: object + x-kubernetes-validations: + - message: spec.uid is immutable + rule: ((!has(oldSelf.uid) && !has(self.uid)) || (has(oldSelf.uid) && + has(self.uid))) status: description: GrafanaDatasourceStatus defines the observed state of GrafanaDatasource properties: diff --git a/controllers/datasource_controller.go b/controllers/datasource_controller.go index a34cf783f..c99a3cc66 100644 --- a/controllers/datasource_controller.go +++ b/controllers/datasource_controller.go @@ -182,6 +182,9 @@ func (r *GrafanaDatasourceReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{}, nil } + // Overwrite OrgID to ensure the field is useless + cr.Spec.Datasource.OrgID = nil + instances, err := r.GetMatchingDatasourceInstances(ctx, cr, r.Client) if err != nil { controllerLog.Error(err, "could not find matching instances", "name", cr.Name, "namespace", cr.Namespace) @@ -196,7 +199,7 @@ func (r *GrafanaDatasourceReconciler) Reconcile(ctx context.Context, req ctrl.Re return ctrl.Result{RequeueAfter: RequeueDelay}, err } - if cr.IsUpdatedUID(datasource.UID) { + if cr.IsUpdatedUID() { controllerLog.Info("datasource uid got updated, deleting datasources with the old uid") err = r.onDatasourceDeleted(ctx, req.Namespace, req.Name) if err != nil { @@ -258,7 +261,7 @@ func (r *GrafanaDatasourceReconciler) Reconcile(ctx context.Context, req ctrl.Re if cr.ResyncPeriodHasElapsed() { cr.Status.LastResync = metav1.Time{Time: time.Now()} } - cr.Status.UID = datasource.UID + cr.Status.UID = cr.CustomUIDOrUID() return ctrl.Result{RequeueAfter: cr.GetResyncPeriod()}, r.Client.Status().Update(ctx, cr) } else { // if there was an issue with the datasource, update the status @@ -445,9 +448,7 @@ func (r *GrafanaDatasourceReconciler) getDatasourceContent(ctx context.Context, return nil, "", err } - if cr.Spec.Datasource.UID == "" { - simpleContent.Set("uid", string(cr.UID)) - } + simpleContent.Set("uid", cr.CustomUIDOrUID()) for _, ref := range cr.Spec.ValuesFrom { ref := ref diff --git a/controllers/fetchers/suite_test.go b/controllers/fetchers/suite_test.go index 7f6d977a0..6a53a5494 100644 --- a/controllers/fetchers/suite_test.go +++ b/controllers/fetchers/suite_test.go @@ -5,7 +5,7 @@ import ( grafanav1beta1 "github.com/grafana/grafana-operator/v5/api/v1beta1" - "github.com/onsi/ginkgo" + "github.com/onsi/ginkgo/v2" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -13,7 +13,7 @@ import ( logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/log/zap" - . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) @@ -42,18 +42,12 @@ var _ = ginkgo.BeforeSuite(func() { err = grafanav1beta1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) - err = grafanav1beta1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - - err = grafanav1beta1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - //+kubebuilder:scaffold:scheme k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) -}, 60) +}) var _ = AfterSuite(func() { By("tearing down the test environment") diff --git a/controllers/fetchers/url_fetcher_test.go b/controllers/fetchers/url_fetcher_test.go index 42d6e5f20..4f6c04349 100644 --- a/controllers/fetchers/url_fetcher_test.go +++ b/controllers/fetchers/url_fetcher_test.go @@ -10,7 +10,7 @@ import ( "github.com/grafana/grafana-operator/v5/api/v1beta1" - . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" ) diff --git a/controllers/suite_test.go b/controllers/suite_test.go index 231d9b9bd..db67f41e2 100644 --- a/controllers/suite_test.go +++ b/controllers/suite_test.go @@ -20,7 +20,7 @@ import ( "path/filepath" "testing" - . "github.com/onsi/ginkgo" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" @@ -64,18 +64,12 @@ var _ = BeforeSuite(func() { err = grafanav1beta1.AddToScheme(scheme.Scheme) Expect(err).NotTo(HaveOccurred()) - err = grafanav1beta1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - - err = grafanav1beta1.AddToScheme(scheme.Scheme) - Expect(err).NotTo(HaveOccurred()) - //+kubebuilder:scaffold:scheme k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme}) Expect(err).NotTo(HaveOccurred()) Expect(k8sClient).NotTo(BeNil()) -}, 60) +}) var _ = AfterSuite(func() { By("tearing down the test environment") diff --git a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanadatasources.yaml b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanadatasources.yaml index 8ee3e9faa..28596b2a5 100644 --- a/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanadatasources.yaml +++ b/deploy/helm/grafana-operator/crds/grafana.integreatly.org_grafanadatasources.yaml @@ -67,7 +67,8 @@ spec: database: type: string editable: - description: Deprecated field, it has no effect + description: Whether to enable/disable editing of the datasource + in Grafana UI type: boolean isDefault: type: boolean @@ -86,6 +87,7 @@ spec: type: type: string uid: + description: Deprecated field, use spec.uid instead type: string url: type: string @@ -161,6 +163,13 @@ spec: format: duration pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ type: string + uid: + description: The UID, for the datasource, fallback to the deprecated + spec.datasource.uid and metadata.uid + type: string + x-kubernetes-validations: + - message: spec.uid is immutable + rule: self == oldSelf valuesFrom: description: environments variables from secrets or config maps items: @@ -231,6 +240,10 @@ spec: - datasource - instanceSelector type: object + x-kubernetes-validations: + - message: spec.uid is immutable + rule: ((!has(oldSelf.uid) && !has(self.uid)) || (has(oldSelf.uid) && + has(self.uid))) status: description: GrafanaDatasourceStatus defines the observed state of GrafanaDatasource properties: diff --git a/deploy/kustomize/base/crds.yaml b/deploy/kustomize/base/crds.yaml index e7680805a..8afd9d2d4 100644 --- a/deploy/kustomize/base/crds.yaml +++ b/deploy/kustomize/base/crds.yaml @@ -1141,7 +1141,8 @@ spec: database: type: string editable: - description: Deprecated field, it has no effect + description: Whether to enable/disable editing of the datasource + in Grafana UI type: boolean isDefault: type: boolean @@ -1160,6 +1161,7 @@ spec: type: type: string uid: + description: Deprecated field, use spec.uid instead type: string url: type: string @@ -1235,6 +1237,13 @@ spec: format: duration pattern: ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ type: string + uid: + description: The UID, for the datasource, fallback to the deprecated + spec.datasource.uid and metadata.uid + type: string + x-kubernetes-validations: + - message: spec.uid is immutable + rule: self == oldSelf valuesFrom: description: environments variables from secrets or config maps items: @@ -1305,6 +1314,10 @@ spec: - datasource - instanceSelector type: object + x-kubernetes-validations: + - message: spec.uid is immutable + rule: ((!has(oldSelf.uid) && !has(self.uid)) || (has(oldSelf.uid) && + has(self.uid))) status: description: GrafanaDatasourceStatus defines the observed state of GrafanaDatasource properties: diff --git a/docs/docs/api.md b/docs/docs/api.md index 8e249f1e7..18db5cd58 100644 --- a/docs/docs/api.md +++ b/docs/docs/api.md @@ -2258,6 +2258,8 @@ GrafanaDatasource is the Schema for the grafanadatasources API