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 <marcus.efraimsson@gmail.com>

* delete duplicated logs

* delete unnecessary leading newline

Co-authored-by: gillesdemey <gilles.de.mey@gmail.com>
Co-authored-by: Marcus Efraimsson <marcus.efraimsson@gmail.com>

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();

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',
     },

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<HttpSettingsProps> = (props) => {
       className={inputStyle}
       placeholder={defaultUrl}
       value={dataSourceConfig.url}
+      aria-label={selectors.components.DataSource.DataSourceHttpSettings.urlInput}
       onChange={(event) => onSettingsChange({ url: event.currentTarget.value })}
     />
   );

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) {

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 {
+func validateURL(cmdType string, url string) response.Response {
-	if u != "" {
+	if _, err := datasource.ValidateURL(cmdType, url); err != nil {
-		if _, err := datasource.ValidateURL(tp, u); err != nil {
+		return response.Error(400, fmt.Sprintf("Validation error, invalid URL: %q", url), err)
-			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)
-		}
 	}
 
 	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 {
+	if cmd.Url != "" {
-		return resp
+		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 cmd.Id, err = strconv.ParseInt(web.Params(c.Req)[":id"], 10, 64); err != nil {
-	if err != nil {
 		return response.Error(http.StatusBadRequest, "id is invalid", err)
 	}
 	if resp := validateURL(cmd.Type, cmd.Url); resp != nil {

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, "http://localhost:9090", authTypePassword, authCheckQuery, false),
-			createAuthTest(t, secretsService, models.DS_INFLUXDB_08, authTypePassword, authCheckQuery, true),
+			createAuthTest(t, secretsService, models.DS_INFLUXDB_08, "http://localhost:9090", authTypePassword, authCheckQuery, true),
-			createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypePassword, authCheckHeader, true),
+			createAuthTest(t, secretsService, models.DS_INFLUXDB, "http://localhost:9090", authTypePassword, authCheckHeader, true),
-			createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypePassword, authCheckHeader, false),
+			createAuthTest(t, secretsService, models.DS_INFLUXDB, "http://localhost:9090", authTypePassword, authCheckHeader, false),
-			createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypeBasic, authCheckHeader, true),
+			createAuthTest(t, secretsService, models.DS_INFLUXDB, "http://localhost:9090", authTypeBasic, authCheckHeader, true),
-			createAuthTest(t, secretsService, models.DS_INFLUXDB, authTypeBasic, authCheckHeader, false),
+			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, "http://localhost:9200", authTypeBasic, authCheckHeader, false),
-			createAuthTest(t, secretsService, models.DS_ES, authTypeBasic, authCheckHeader, true),
+			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

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"

pkg/tsdb/cloudmonitoring/cloudmonitoring.go

@@ -58,13 +58,13 @@ var (
 )
 
 const (
-	gceAuthentication         string = "gce"
+	gceAuthentication         = "gce"
-	jwtAuthentication         string = "jwt"
+	jwtAuthentication         = "jwt"
-	metricQueryType           string = "metrics"
+	metricQueryType           = "metrics"
-	sloQueryType              string = "slo"
+	sloQueryType              = "slo"
-	mqlEditorMode             string = "mql"
+	mqlEditorMode             = "mql"
-	crossSeriesReducerDefault string = "REDUCE_NONE"
+	crossSeriesReducerDefault = "REDUCE_NONE"
-	perSeriesAlignerDefault   string = "ALIGN_MEAN"
+	perSeriesAlignerDefault   = "ALIGN_MEAN"
 )
 
 func ProvideService(httpClientProvider httpclient.Provider, tracer tracing.Tracer) *Service {