From 7ed82ac0490228b553a2bd3d8bebeae3e39a8bed Mon Sep 17 00:00:00 2001 From: Santiago Date: Mon, 31 Jan 2022 12:39:55 -0300 Subject: [PATCH] Check for empty URLs when creating/updating a data source (#42837) * checks for empty URLs added * check for TimeSeriesTypeNot to fix InfluxDB alerts * log a warning when a data frame is ignored * fix: add brittle Prometheus URL input selector needs a proper aria-label or test-data-id selector * test: add URL input aria-label needs to use the grafana/e2e-selectors package * test: run ci * add URL validation for specific data sources, e2e tests * Update pkg/api/datasource/validation.go Co-authored-by: Marcus Efraimsson * delete duplicated logs * delete unnecessary leading newline Co-authored-by: gillesdemey Co-authored-by: Marcus Efraimsson --- e2e/various-suite/exemplars.spec.ts | 2 ++ .../src/selectors/components.ts | 3 +++ .../DataSourceHttpSettings.tsx | 2 ++ pkg/api/datasource/validation.go | 25 +++++++++++++++++++ pkg/api/datasources.go | 20 +++++++-------- pkg/api/pluginproxy/ds_proxy_test.go | 19 +++++++------- pkg/models/datasource.go | 8 ++++++ pkg/tsdb/cloudmonitoring/cloudmonitoring.go | 14 +++++------ 8 files changed, 66 insertions(+), 27 deletions(-) diff --git a/e2e/various-suite/exemplars.spec.ts b/e2e/various-suite/exemplars.spec.ts index f75faa114d2..13abcbefc6b 100644 --- a/e2e/various-suite/exemplars.spec.ts +++ b/e2e/various-suite/exemplars.spec.ts @@ -1,4 +1,5 @@ import { e2e } from '@grafana/e2e'; +import { selectors } from '@grafana/e2e-selectors'; const dataSourceName = 'PromExemplar'; const addDataSource = () => { @@ -9,6 +10,7 @@ const addDataSource = () => { form: () => { e2e.components.DataSource.Prometheus.configPage.exemplarsAddButton().click(); e2e.components.DataSource.Prometheus.configPage.internalLinkSwitch().check({ force: true }); + e2e.components.DataSource.DataSourceHttpSettings.urlInput().type('http://prom-url:9090'); e2e.components.DataSourcePicker.inputV2().should('be.visible').click({ force: true }); e2e().contains('gdev-tempo').scrollIntoView().should('be.visible').click(); diff --git a/packages/grafana-e2e-selectors/src/selectors/components.ts b/packages/grafana-e2e-selectors/src/selectors/components.ts index 54155ce9bf4..26f2fc7bd91 100644 --- a/packages/grafana-e2e-selectors/src/selectors/components.ts +++ b/packages/grafana-e2e-selectors/src/selectors/components.ts @@ -35,6 +35,9 @@ export const Components = { startValue: 'TestData start value', }, }, + DataSourceHttpSettings: { + urlInput: 'Datasource HTTP settings url', + }, Jaeger: { traceIDInput: 'Trace ID', }, diff --git a/packages/grafana-ui/src/components/DataSourceSettings/DataSourceHttpSettings.tsx b/packages/grafana-ui/src/components/DataSourceSettings/DataSourceHttpSettings.tsx index c8f49e5a8ee..6e44460f34f 100644 --- a/packages/grafana-ui/src/components/DataSourceSettings/DataSourceHttpSettings.tsx +++ b/packages/grafana-ui/src/components/DataSourceSettings/DataSourceHttpSettings.tsx @@ -1,6 +1,7 @@ import React, { useState, useCallback } from 'react'; import { css, cx } from '@emotion/css'; import { DataSourceSettings, SelectableValue } from '@grafana/data'; +import { selectors } from '@grafana/e2e-selectors'; import { BasicAuthSettings } from './BasicAuthSettings'; import { HttpProxySettings } from './HttpProxySettings'; import { TLSAuthSettings } from './TLSAuthSettings'; @@ -125,6 +126,7 @@ export const DataSourceHttpSettings: React.FC = (props) => { className={inputStyle} placeholder={defaultUrl} value={dataSourceConfig.url} + aria-label={selectors.components.DataSource.DataSourceHttpSettings.urlInput} onChange={(event) => onSettingsChange({ url: event.currentTarget.value })} /> ); diff --git a/pkg/api/datasource/validation.go b/pkg/api/datasource/validation.go index a8420814d7e..c9dd491dab6 100644 --- a/pkg/api/datasource/validation.go +++ b/pkg/api/datasource/validation.go @@ -1,17 +1,37 @@ package datasource import ( + "errors" "fmt" "net/url" "regexp" "strings" "github.com/grafana/grafana/pkg/infra/log" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/tsdb/mssql" ) var logger = log.New("datasource") +// requiredURL contains the set of data sources that require a URL. +var requiredURL = map[string]bool{ + models.DS_GRAPHITE: true, + models.DS_INFLUXDB: true, + models.DS_INFLUXDB_08: true, + models.DS_ES: true, + models.DS_PROMETHEUS: true, + models.DS_ALERTMANAGER: true, + models.DS_JAEGER: true, + models.DS_LOKI: true, + models.DS_OPENTSDB: true, + models.DS_TEMPO: true, + models.DS_ZIPKIN: true, + models.DS_MYSQL: true, + models.DS_POSTGRES: true, + models.DS_MSSQL: true, +} + // URLValidationError represents an error from validating a data source URL. type URLValidationError struct { Err error @@ -40,6 +60,11 @@ var reURL = regexp.MustCompile("^[^:]*://") // The data source's type and URL must be provided. If successful, the valid URL object is returned, otherwise an // error is returned. func ValidateURL(typeName, urlStr string) (*url.URL, error) { + // Check for empty URLs + if _, exists := requiredURL[typeName]; exists && strings.TrimSpace(urlStr) == "" { + return nil, URLValidationError{Err: errors.New("empty URL string"), URL: ""} + } + var u *url.URL var err error switch strings.ToLower(typeName) { diff --git a/pkg/api/datasources.go b/pkg/api/datasources.go index cf9f0b00c0f..0809a9cfc4a 100644 --- a/pkg/api/datasources.go +++ b/pkg/api/datasources.go @@ -256,13 +256,9 @@ func (hs *HTTPServer) DeleteDataSourceByName(c *models.ReqContext) response.Resp }) } -func validateURL(tp string, u string) response.Response { - if u != "" { - if _, err := datasource.ValidateURL(tp, u); err != nil { - datasourcesLogger.Error("Received invalid data source URL as part of data source command", - "url", u) - return response.Error(400, fmt.Sprintf("Validation error, invalid URL: %q", u), err) - } +func validateURL(cmdType string, url string) response.Response { + if _, err := datasource.ValidateURL(cmdType, url); err != nil { + return response.Error(400, fmt.Sprintf("Validation error, invalid URL: %q", url), err) } return nil @@ -274,10 +270,13 @@ func AddDataSource(c *models.ReqContext) response.Response { if err := web.Bind(c.Req, &cmd); err != nil { return response.Error(http.StatusBadRequest, "bad request data", err) } + datasourcesLogger.Debug("Received command to add data source", "url", cmd.Url) cmd.OrgId = c.OrgId - if resp := validateURL(cmd.Type, cmd.Url); resp != nil { - return resp + if cmd.Url != "" { + if resp := validateURL(cmd.Type, cmd.Url); resp != nil { + return resp + } } if err := bus.Dispatch(c.Req.Context(), &cmd); err != nil { @@ -306,8 +305,7 @@ func (hs *HTTPServer) UpdateDataSource(c *models.ReqContext) response.Response { datasourcesLogger.Debug("Received command to update data source", "url", cmd.Url) cmd.OrgId = c.OrgId var err error - cmd.Id, err = strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64) - if err != nil { + if cmd.Id, err = strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64); err != nil { return response.Error(http.StatusBadRequest, "id is invalid", err) } if resp := validateURL(cmd.Type, cmd.Url); resp != nil { diff --git a/pkg/api/pluginproxy/ds_proxy_test.go b/pkg/api/pluginproxy/ds_proxy_test.go index c93c2a19812..729aa365306 100644 --- a/pkg/api/pluginproxy/ds_proxy_test.go +++ b/pkg/api/pluginproxy/ds_proxy_test.go @@ -564,18 +564,18 @@ func TestDataSourceProxy_routeRule(t *testing.T) { secretsService := secretsManager.SetupTestService(t, fakes.NewFakeSecretsStore()) tests := []*testCase{ - createAuthTest(t, secretsService, models.DS_INFLUXDB_08, authTypePassword, authCheckQuery, false), - createAuthTest(t, secretsService, models.DS_INFLUXDB_08, authTypePassword, authCheckQuery, true), - createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypePassword, authCheckHeader, true), - createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypePassword, authCheckHeader, false), - createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypeBasic, authCheckHeader, true), - createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypeBasic, authCheckHeader, false), + createAuthTest(t, secretsService, models.DS_INFLUXDB_08, "http://localhost:9090", authTypePassword, authCheckQuery, false), + createAuthTest(t, secretsService, models.DS_INFLUXDB_08, "http://localhost:9090", authTypePassword, authCheckQuery, true), + createAuthTest(t, secretsService, models.DS_INFLUXDB, "http://localhost:9090", authTypePassword, authCheckHeader, true), + createAuthTest(t, secretsService, models.DS_INFLUXDB, "http://localhost:9090", authTypePassword, authCheckHeader, false), + createAuthTest(t, secretsService, models.DS_INFLUXDB, "http://localhost:9090", authTypeBasic, authCheckHeader, true), + createAuthTest(t, secretsService, models.DS_INFLUXDB, "http://localhost:9090", authTypeBasic, authCheckHeader, false), // These two should be enough for any other datasource at the moment. Proxy has special handling // only for Influx, others have the same path and only BasicAuth. Non BasicAuth datasources // do not go through proxy but through TSDB API which is not tested here. - createAuthTest(t, secretsService, models.DS_ES, authTypeBasic, authCheckHeader, false), - createAuthTest(t, secretsService, models.DS_ES, authTypeBasic, authCheckHeader, true), + createAuthTest(t, secretsService, models.DS_ES, "http://localhost:9200", authTypeBasic, authCheckHeader, false), + createAuthTest(t, secretsService, models.DS_ES, "http://localhost:9200", authTypeBasic, authCheckHeader, true), } for _, test := range tests { runDatasourceAuthTest(t, secretsService, test) @@ -900,7 +900,7 @@ const ( authCheckHeader = "header" ) -func createAuthTest(t *testing.T, secretsService secrets.Service, dsType string, authType string, authCheck string, useSecureJsonData bool) *testCase { +func createAuthTest(t *testing.T, secretsService secrets.Service, dsType string, url string, authType string, authCheck string, useSecureJsonData bool) *testCase { ctx := context.Background() // Basic user:password @@ -911,6 +911,7 @@ func createAuthTest(t *testing.T, secretsService secrets.Service, dsType string, Id: 1, Type: dsType, JsonData: simplejson.New(), + Url: url, }, } var message string diff --git a/pkg/models/datasource.go b/pkg/models/datasource.go index 005c4a1bbde..ba4184a848f 100644 --- a/pkg/models/datasource.go +++ b/pkg/models/datasource.go @@ -13,7 +13,15 @@ const ( DS_INFLUXDB_08 = "influxdb_08" DS_ES = "elasticsearch" DS_PROMETHEUS = "prometheus" + DS_ALERTMANAGER = "alertmanager" + DS_JAEGER = "jaeger" + DS_LOKI = "loki" + DS_OPENTSDB = "opentsdb" + DS_TEMPO = "tempo" + DS_ZIPKIN = "zipkin" DS_MYSQL = "mysql" + DS_POSTGRES = "postgres" + DS_MSSQL = "mssql" DS_ACCESS_DIRECT = "direct" DS_ACCESS_PROXY = "proxy" DS_ES_OPEN_DISTRO = "grafana-es-open-distro-datasource" diff --git a/pkg/tsdb/cloudmonitoring/cloudmonitoring.go b/pkg/tsdb/cloudmonitoring/cloudmonitoring.go index 8cebb467460..7c8fae24d31 100644 --- a/pkg/tsdb/cloudmonitoring/cloudmonitoring.go +++ b/pkg/tsdb/cloudmonitoring/cloudmonitoring.go @@ -58,13 +58,13 @@ var ( ) const ( - gceAuthentication string = "gce" - jwtAuthentication string = "jwt" - metricQueryType string = "metrics" - sloQueryType string = "slo" - mqlEditorMode string = "mql" - crossSeriesReducerDefault string = "REDUCE_NONE" - perSeriesAlignerDefault string = "ALIGN_MEAN" + gceAuthentication = "gce" + jwtAuthentication = "jwt" + metricQueryType = "metrics" + sloQueryType = "slo" + mqlEditorMode = "mql" + crossSeriesReducerDefault = "REDUCE_NONE" + perSeriesAlignerDefault = "ALIGN_MEAN" ) func ProvideService(httpClientProvider httpclient.Provider, tracer tracing.Tracer) *Service {