From d9b3fa4f59aeb931869c5bea5501976eaf719159 Mon Sep 17 00:00:00 2001 From: QuentinBisson Date: Tue, 10 Dec 2024 10:53:59 +0100 Subject: [PATCH 1/3] support-multiple-tenants-in-one-org --- api/v1alpha1/grafanaorganization_types.go | 3 + ...ty.giantswarm.io_grafanaorganizations.yaml | 2 + .../grafanaorganization_controller.go | 27 ++- .../controller/predicates/predicates_test.go | 210 ++++++++++++++++++ pkg/grafana/grafana.go | 6 +- pkg/grafana/types.go | 14 +- 6 files changed, 242 insertions(+), 20 deletions(-) create mode 100644 internal/controller/predicates/predicates_test.go diff --git a/api/v1alpha1/grafanaorganization_types.go b/api/v1alpha1/grafanaorganization_types.go index aebfe810..eb39d4a0 100644 --- a/api/v1alpha1/grafanaorganization_types.go +++ b/api/v1alpha1/grafanaorganization_types.go @@ -29,6 +29,8 @@ const ( // GrafanaOrganizationSpec defines the desired state of GrafanaOrganization type GrafanaOrganizationSpec struct { // DisplayName is the name displayed when viewing the organization in Grafana. It can be different from the actual org's name. + // +kubebuilder:example="Giant Swarm" + // +kubebuilder:validation:MinLength=1 DisplayName string `json:"displayName"` // Access rules defines user permissions for interacting with the organization in Grafana. @@ -36,6 +38,7 @@ type GrafanaOrganizationSpec struct { // Tenants is a list of tenants that are associated with the Grafana organization. // +kubebuilder:example={"giantswarm"} + // +kube:validation:MinItems=1 Tenants []TenantID `json:"tenants,omitempty"` } diff --git a/config/crd/observability.giantswarm.io_grafanaorganizations.yaml b/config/crd/observability.giantswarm.io_grafanaorganizations.yaml index 31927d97..89c11fab 100644 --- a/config/crd/observability.giantswarm.io_grafanaorganizations.yaml +++ b/config/crd/observability.giantswarm.io_grafanaorganizations.yaml @@ -50,6 +50,8 @@ spec: displayName: description: DisplayName is the name displayed when viewing the organization in Grafana. It can be different from the actual org's name. + example: Giant Swarm + minLength: 1 type: string rbac: description: Access rules defines user permissions for interacting diff --git a/internal/controller/grafanaorganization_controller.go b/internal/controller/grafanaorganization_controller.go index ca52bea4..ffb7a252 100644 --- a/internal/controller/grafanaorganization_controller.go +++ b/internal/controller/grafanaorganization_controller.go @@ -224,14 +224,23 @@ func (r GrafanaOrganizationReconciler) configureSharedOrg(ctx context.Context) e return nil } +func newOrganization(grafanaOrganization *v1alpha1.GrafanaOrganization) *grafana.Organization { + tenantIDs := make([]string, len(grafanaOrganization.Spec.Tenants)) + for i, tenant := range grafanaOrganization.Spec.Tenants { + tenantIDs[i] = string(tenant) + } + + return &grafana.Organization{ + ID: grafanaOrganization.Status.OrgID, + Name: grafanaOrganization.Spec.DisplayName, + TenantIDs: tenantIDs, + } +} + func (r GrafanaOrganizationReconciler) configureOrganization(ctx context.Context, grafanaOrganization *v1alpha1.GrafanaOrganization) (err error) { logger := log.FromContext(ctx) // Create or update organization in Grafana - var organization = &grafana.Organization{ - ID: grafanaOrganization.Status.OrgID, - Name: grafanaOrganization.Spec.DisplayName, - TenantID: grafanaOrganization.Name, - } + var organization = newOrganization(grafanaOrganization) if organization.ID == 0 { // if the CR doesn't have an orgID, create the organization in Grafana @@ -265,13 +274,9 @@ func (r GrafanaOrganizationReconciler) configureDatasources(ctx context.Context, logger.Info("configuring data sources") // Create or update organization in Grafana - var organization = grafana.Organization{ - ID: grafanaOrganization.Status.OrgID, - Name: grafanaOrganization.Spec.DisplayName, - TenantID: grafanaOrganization.Name, - } + var organization = newOrganization(grafanaOrganization) - datasources, err := grafana.ConfigureDefaultDatasources(ctx, r.GrafanaAPI, organization) + datasources, err := grafana.ConfigureDefaultDatasources(ctx, r.GrafanaAPI, *organization) if err != nil { return errors.WithStack(err) } diff --git a/internal/controller/predicates/predicates_test.go b/internal/controller/predicates/predicates_test.go new file mode 100644 index 00000000..44c6ec44 --- /dev/null +++ b/internal/controller/predicates/predicates_test.go @@ -0,0 +1,210 @@ +package predicates + +import ( + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/event" +) + +func TestIsGrafanaPod(t *testing.T) { + tests := []struct { + name string + pod *corev1.Pod + expected bool + }{ + { + name: "nil pod", + pod: nil, + expected: false, + }, + { + name: "non-Grafana pod", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-grafana-pod", + Namespace: "default", + }, + }, + expected: false, + }, + { + name: "Grafana pod with correct labels", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grafana-pod", + Namespace: "monitoring", + Labels: map[string]string{ + "app.kubernetes.io/instance": "grafana", + }, + }, + }, + expected: true, + }, + { + name: "Grafana pod with incorrect namespace", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grafana-pod", + Namespace: "default", + Labels: map[string]string{ + "app.kubernetes.io/instance": "grafana", + }, + }, + }, + expected: false, + }, + { + name: "Grafana pod with incorrect label", + pod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grafana-pod", + Namespace: "monitoring", + Labels: map[string]string{ + "app.kubernetes.io/instance": "not-grafana", + }, + }, + }, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isGrafanaPod(tt.pod) + if result != tt.expected { + t.Errorf("expected %v, got %v", tt.expected, result) + } + }) + } +} + +func TestGrafanaPodRecreatedPredicate_Update(t *testing.T) { + tests := []struct { + name string + oldPod *corev1.Pod + newPod *corev1.Pod + expected bool + }{ + { + name: "nil old object", + oldPod: nil, + newPod: &corev1.Pod{}, + expected: false, + }, + { + name: "nil new object", + oldPod: &corev1.Pod{}, + newPod: nil, + expected: false, + }, + { + name: "non-Grafana pod", + oldPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-grafana-pod", + Namespace: "default", + }, + }, + newPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "non-grafana-pod", + Namespace: "default", + }, + }, + expected: false, + }, + { + name: "Grafana pod not ready to ready", + oldPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grafana-pod", + Namespace: "monitoring", + Labels: map[string]string{ + "app.kubernetes.io/instance": "grafana", + }, + }, + Status: corev1.PodStatus{ + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionFalse, + }, + }, + }, + }, + newPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grafana-pod", + Namespace: "monitoring", + Labels: map[string]string{ + "app.kubernetes.io/instance": "grafana", + }, + }, + Status: corev1.PodStatus{ + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + expected: true, + }, + { + name: "Grafana pod ready to not ready", + oldPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grafana-pod", + Namespace: "monitoring", + Labels: map[string]string{ + "app.kubernetes.io/instance": "grafana", + }, + }, + Status: corev1.PodStatus{ + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + }, + }, + }, + }, + newPod: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "grafana-pod", + Namespace: "monitoring", + Labels: map[string]string{ + "app.kubernetes.io/instance": "grafana", + }, + }, + Status: corev1.PodStatus{ + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionFalse, + }, + }, + }, + }, + expected: false, + }, + } + + predicate := GrafanaPodRecreatedPredicate{} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + e := event.UpdateEvent{ + ObjectOld: tt.oldPod, + ObjectNew: tt.newPod, + } + result := predicate.Update(e) + if result != tt.expected { + t.Errorf("expected %v, got %v", tt.expected, result) + } + }) + } +} diff --git a/pkg/grafana/grafana.go b/pkg/grafana/grafana.go index 3c7ffe11..cfd1a2f2 100644 --- a/pkg/grafana/grafana.go +++ b/pkg/grafana/grafana.go @@ -18,9 +18,9 @@ const ( ) var SharedOrg = Organization{ - ID: 1, - Name: "Shared Org", - TenantID: "giantswarm", + ID: 1, + Name: "Shared Org", + TenantIDs: []string{"giantswarm"}, } // We need to use a custom name for now until we can replace the existing datasources. diff --git a/pkg/grafana/types.go b/pkg/grafana/types.go index 595bbea0..6ded75be 100644 --- a/pkg/grafana/types.go +++ b/pkg/grafana/types.go @@ -1,9 +1,11 @@ package grafana +import "strings" + type Organization struct { - ID int64 - Name string - TenantID string + ID int64 + Name string + TenantIDs []string } type Datasource struct { @@ -33,12 +35,12 @@ func (d Datasource) buildJSONData() map[string]interface{} { } func (d Datasource) buildSecureJSONData(organization Organization) map[string]string { - tenant := organization.TenantID + tenantIDs := organization.TenantIDs if d.Type != "loki" { // We do not support multi-tenancy for Mimir yet - tenant = "anonymous" + tenantIDs = []string{"anonymous"} } return map[string]string{ - "httpHeaderValue1": tenant, + "httpHeaderValue1": strings.Join(tenantIDs, "|"), } } From af028f143f486a9c7d3c8174b3201099a9e8397d Mon Sep 17 00:00:00 2001 From: QuentinBisson Date: Tue, 10 Dec 2024 17:25:52 +0100 Subject: [PATCH 2/3] remove useless pointer --- .../grafanaorganization_controller.go | 19 ++++++--------- pkg/grafana/grafana.go | 23 ++++++++++--------- 2 files changed, 19 insertions(+), 23 deletions(-) diff --git a/internal/controller/grafanaorganization_controller.go b/internal/controller/grafanaorganization_controller.go index ffb7a252..67bebcfa 100644 --- a/internal/controller/grafanaorganization_controller.go +++ b/internal/controller/grafanaorganization_controller.go @@ -210,7 +210,7 @@ func (r GrafanaOrganizationReconciler) configureSharedOrg(ctx context.Context) e sharedOrg := grafana.SharedOrg logger.Info("configuring shared organization") - if _, err := grafana.UpdateOrganization(ctx, r.GrafanaAPI, sharedOrg); err != nil { + if err := grafana.UpdateOrganization(ctx, r.GrafanaAPI, &sharedOrg); err != nil { logger.Error(err, "failed to rename shared org") return errors.WithStack(err) } @@ -224,13 +224,13 @@ func (r GrafanaOrganizationReconciler) configureSharedOrg(ctx context.Context) e return nil } -func newOrganization(grafanaOrganization *v1alpha1.GrafanaOrganization) *grafana.Organization { +func newOrganization(grafanaOrganization *v1alpha1.GrafanaOrganization) grafana.Organization { tenantIDs := make([]string, len(grafanaOrganization.Spec.Tenants)) for i, tenant := range grafanaOrganization.Spec.Tenants { tenantIDs[i] = string(tenant) } - return &grafana.Organization{ + return grafana.Organization{ ID: grafanaOrganization.Status.OrgID, Name: grafanaOrganization.Spec.DisplayName, TenantIDs: tenantIDs, @@ -241,12 +241,11 @@ func (r GrafanaOrganizationReconciler) configureOrganization(ctx context.Context logger := log.FromContext(ctx) // Create or update organization in Grafana var organization = newOrganization(grafanaOrganization) - if organization.ID == 0 { // if the CR doesn't have an orgID, create the organization in Grafana - organization, err = grafana.CreateOrganization(ctx, r.GrafanaAPI, *organization) + err = grafana.CreateOrganization(ctx, r.GrafanaAPI, &organization) } else { - organization, err = grafana.UpdateOrganization(ctx, r.GrafanaAPI, *organization) + err = grafana.UpdateOrganization(ctx, r.GrafanaAPI, &organization) } if err != nil { @@ -276,7 +275,7 @@ func (r GrafanaOrganizationReconciler) configureDatasources(ctx context.Context, // Create or update organization in Grafana var organization = newOrganization(grafanaOrganization) - datasources, err := grafana.ConfigureDefaultDatasources(ctx, r.GrafanaAPI, *organization) + datasources, err := grafana.ConfigureDefaultDatasources(ctx, r.GrafanaAPI, organization) if err != nil { return errors.WithStack(err) } @@ -311,11 +310,7 @@ func (r GrafanaOrganizationReconciler) reconcileDelete(ctx context.Context, graf } // Delete organization in Grafana - var organization = grafana.Organization{ - ID: grafanaOrganization.Status.OrgID, - Name: grafanaOrganization.Spec.DisplayName, - TenantID: grafanaOrganization.Name, - } + var organization = newOrganization(grafanaOrganization) // Delete organization in Grafana if it exists if grafanaOrganization.Status.OrgID > 0 { diff --git a/pkg/grafana/grafana.go b/pkg/grafana/grafana.go index cfd1a2f2..c1a39213 100644 --- a/pkg/grafana/grafana.go +++ b/pkg/grafana/grafana.go @@ -69,13 +69,13 @@ var defaultDatasources = []Datasource{ }, } -func CreateOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization Organization) (*Organization, error) { +func CreateOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization *Organization) error { logger := log.FromContext(ctx) logger.Info("creating organization") err := assertNameIsAvailable(ctx, grafanaAPI, organization) if err != nil { - return nil, errors.WithStack(err) + return errors.WithStack(err) } createdOrg, err := grafanaAPI.Orgs.CreateOrg(&models.CreateOrgCommand{ @@ -83,15 +83,15 @@ func CreateOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, }) if err != nil { logger.Error(err, "failed to create organization") - return nil, errors.WithStack(err) + return errors.WithStack(err) } logger.Info("created organization") organization.ID = *createdOrg.Payload.OrgID - return &organization, nil + return nil } -func UpdateOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization Organization) (*Organization, error) { +func UpdateOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization *Organization) error { logger := log.FromContext(ctx) logger.Info("updating organization") @@ -103,18 +103,18 @@ func UpdateOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, return CreateOrganization(ctx, grafanaAPI, organization) } logger.Error(err, fmt.Sprintf("failed to find organization with ID: %d", organization.ID)) - return nil, errors.WithStack(err) + return errors.WithStack(err) } // If both name matches, there is nothing to do. if found.Name == organization.Name { logger.Info("the organization already exists in Grafana and does not need to be updated.") - return &organization, nil + return nil } err = assertNameIsAvailable(ctx, grafanaAPI, organization) if err != nil { - return nil, errors.WithStack(err) + return errors.WithStack(err) } // if the name of the CR is different from the name of the org in Grafana, update the name of the org in Grafana using the CR's display name. @@ -123,12 +123,12 @@ func UpdateOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, }) if err != nil { logger.Error(err, "failed to update organization name") - return nil, errors.WithStack(err) + return errors.WithStack(err) } logger.Info("updated organization") - return &organization, nil + return nil } func DeleteOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization Organization) error { @@ -158,6 +158,7 @@ func DeleteOrganization(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, func ConfigureDefaultDatasources(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization Organization) ([]Datasource, error) { logger := log.FromContext(ctx) + // TODO using a serviceaccount later would be better as they are scoped to an organization var err error @@ -278,7 +279,7 @@ func isNotFound(err error) bool { } // assertNameIsAvailable is a helper function to check if the organization name is available in Grafana -func assertNameIsAvailable(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization Organization) error { +func assertNameIsAvailable(ctx context.Context, grafanaAPI *client.GrafanaHTTPAPI, organization *Organization) error { logger := log.FromContext(ctx) found, err := findByName(grafanaAPI, organization.Name) From af7b678fcb42e4c34d074c9ad6a5985d103265bc Mon Sep 17 00:00:00 2001 From: QuentinBisson Date: Tue, 10 Dec 2024 17:51:34 +0100 Subject: [PATCH 3/3] enforce tenants --- api/v1alpha1/grafanaorganization_types.go | 2 +- .../crd/observability.giantswarm.io_grafanaorganizations.yaml | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/api/v1alpha1/grafanaorganization_types.go b/api/v1alpha1/grafanaorganization_types.go index eb39d4a0..293c1340 100644 --- a/api/v1alpha1/grafanaorganization_types.go +++ b/api/v1alpha1/grafanaorganization_types.go @@ -39,7 +39,7 @@ type GrafanaOrganizationSpec struct { // Tenants is a list of tenants that are associated with the Grafana organization. // +kubebuilder:example={"giantswarm"} // +kube:validation:MinItems=1 - Tenants []TenantID `json:"tenants,omitempty"` + Tenants []TenantID `json:"tenants"` } // TenantID is a unique identifier for a tenant. It must be lowercase. diff --git a/config/crd/observability.giantswarm.io_grafanaorganizations.yaml b/config/crd/observability.giantswarm.io_grafanaorganizations.yaml index 89c11fab..d98023d2 100644 --- a/config/crd/observability.giantswarm.io_grafanaorganizations.yaml +++ b/config/crd/observability.giantswarm.io_grafanaorganizations.yaml @@ -94,6 +94,7 @@ spec: required: - displayName - rbac + - tenants type: object status: description: GrafanaOrganizationStatus defines the observed state of GrafanaOrganization