Reworking the datasource format #84

Nexucis · 2021-09-16T18:08:41Z

This PR is changing deeply the datamodel of the datasource. The idea is to introduce the concept of multi different type of datasource.

Every different datasources will share these attributes :

type BasicDatasource struct {
	Kind    Kind `json:"kind" yaml:"kind"`
	Default bool `json:"default" yaml:"default"`
}

Kind will give you the type of the datasource.
Default will say if it's the default datasource for the same type. Meaning that you will be able to define one default datasource per type of datasource. One default datasource for Prometheus, one for MySQL ...etc.

This PR is also changing the configuration for the Prometheus datasource itself by introducing more complexe HTTP configuration:

type HTTPConfiguration struct {
	// URL is the url required to contact the datasource
	URL *url.URL `json:"url" yaml:"url"`
	// The way the UI will contact the datasource. Or through the Backend or directly.
	// By default, Access is set with the value 'server'
	Access HTTPAccess `json:"access,omitempty" yaml:"access,omitempty"`
	// WhiteList is a list of tuple of http method and http endpoint that will be accessible.
	// These parameters are only used when access is set to 'server'
	WhiteList []HTTPWhiteListConfig `json:"white_list" yaml:"white_list"`
	// Auth is holding any security configuration for the http configuration.
	// When defined, it's impossible to set the value of Access with 'browser'
	Auth *HTTPAuth `json:"auth,omitempty" yaml:"auth,omitempty"`
	// Headers can be used to provide additional header that needs to be forwarded when requesting the datasource
	// When defined, it's impossible to set the value of Access with 'browser'
	Headers map[string]string `yaml:"headers,omitempty"`
}

As you can see, it introduced:

an HTTP datasource could be used directly in the UI
some auth configuration (basic auth, bearer token, ca cert)

The Prometheus Datasource define a default whiteList configuration. That's the only thing really specialize for Prometheus itself.

It also introduced the notion of the local datasource. You can define a datasource in your project. The datasource definition is exactly the same as the one you can define globaly.
Only dashboard in the same project will be able to use it. Dashboards from other project won't be able to use it. Only the global datasources are shared across the different dashboards / projects.

A consequency of changing the spec of the datasource and introducing the local datasource is the recode of the Proxy.
For the moment, we only support Prometheus which exposed an HTTP API, but at some point we likely have not HTTP datasource.
The proxy is taking care of that, by introducing a proxy factory. Depending of the datasource type, an http proxy will be created or something else.

I will add more unit test, but in the meantine I will be interesting by your opinion about these changes @juliusv and @robskillington :).

juliusv · 2021-09-17T10:15:40Z

internal/api/core/middleware/proxy.go

-	}
-	logrus.Tracef("'%s' is a request to a datasource", requestPath)
-	matchingGroups := proxyMatcher.FindAllStringSubmatch(requestPath, -1)
+func catchDatasourceAndPath(c echo.Context, dts datasource.DAO, globalDTS globaldatasource.DAO) (v1.DatasourceSpec, string, error) {


catch sounds a bit unusual for this kind of operation, how about extract?

extract is definitely better :). Thanks !

juliusv · 2021-09-17T10:33:25Z

internal/api/core/middleware/proxy.go

+		logrus.Errorf(proxyErr.Error())
+		proxyErr = err
+	}
+	// use a dedicated HTTP transport to avoid any TSL encrypt issue


Suggested change

// use a dedicated HTTP transport to avoid any TSL encrypt issue

// use a dedicated HTTP transport to avoid any TLS encryption issues

juliusv · 2021-09-17T10:43:26Z

internal/api/core/middleware/proxy.go

+}
+
+func (h *httpProxy) prepareRequest(c echo.Context) error {
+	req := c.Request()
 	// We have to modify the HOST of the request in order to match the host of the targetURL
 	// So far I'm not sure to understand exactly why, but if you are going to remove it, be sure of what you are doing.
 	// It has been done to fix an error returned by Openshift itself saying the target doesn't exist.
 	// Since we are using HTTP/1, setting the HOST is setting also an header so if the host and the header are different


Aside: In HTTP/1.1 the host is only sent in the Host: header, so that's why it definitely needs to be set so that the backend router / proxy knows who you want to talk to.

ah that's where it comes from. Perhaps I should change the comment then ?

juliusv · 2021-09-17T10:45:58Z

internal/api/core/middleware/proxy.go

 	// Fix header
 	if len(req.Header.Get(echo.HeaderXRealIP)) == 0 {
 		req.Header.Set(echo.HeaderXRealIP, c.RealIP())
 	}
 	if len(req.Header.Get(echo.HeaderXForwardedProto)) == 0 {
 		req.Header.Set(echo.HeaderXForwardedProto, c.Scheme())
 	}
+	// set header according to the configuration
+	if len(h.config.Headers) > 0 {


Do we want to allow overriding even special headers like the user agent or the host header here? Maybe fine for now, but I'd add a TODO to think about this later.

indeed we should have a list of header that cannot be overridden

will put a todo

juliusv · 2021-09-17T10:51:49Z

internal/api/core/middleware/proxy.go

+		if authConfig.BasicAuth != nil {
+			password, err := authConfig.BasicAuth.GetPassword()
+			if err != nil {
+				logrus.WithError(err).Error("unable to get access to the password for the basic auth")


"unable to get access" sounds like a permissions / auth problem... looking at the code of GetPassword() the most likely reason would be if there's a file configured and probably the file doesn't exist or has a wrong name. "unable to read" / "unable to retrieve"?

juliusv · 2021-09-17T10:54:19Z

internal/api/interface/v1/datasource/interface.go

@@ -24,17 +24,20 @@ type Query struct {
 	// Name is a prefix of the Datasource.metadata.name that is used to filter the list of the Datasource.
 	// Name can be empty in case you want to return the full list of Datasource available.
 	Name string `query:"name"`
+	// Project is the exact name of the project.
+	// The value can come or from the path of the URL or from the query parameter


Suggested change

// The value can come or from the path of the URL or from the query parameter

// The value can come from the path of the URL or from the query parameter

juliusv · 2021-09-17T14:19:02Z

internal/api/interface/v1/globaldatasource/interface.go

+
+type Query struct {
+	etcd.Query
+	// Name is a prefix of the GlobalDatasource.metadata.name that is used to filter the list of the GlobalDatasource.


NamePrefix could be a less confusing name for this field then?

pkg/model/api/v1/datasource.go

Nexucis · 2021-09-18T08:57:54Z

alright I think I tackled all comments @juliusv. I also renamed the WhiteList attribute with AllowedEndpoints. I think it describes a bit better the purpose of this attribute.

pkg/model/api/v1/datasource_test.go

juliusv · 2021-09-20T10:09:16Z

It's a lot of code, so I didn't manage to analyze everything in detail, but generally it looks good 👍

robskillington · 2021-09-23T15:15:37Z

internal/api/core/middleware/proxy.go

+		desc := h.config.URL.String()
+		proxyErr = fmt.Errorf("error proxying, remote unreachable: target=%s, err=%w", desc, err)
+		logrus.Errorf(proxyErr.Error())
+		proxyErr = err


Hm this seems to clobber the definition proxyErr assignment on line 158?

Perhaps use a different variable if you want to retain the error, i.e.

logrus.WithError(err).Errorf("error proxying, remote unreachable: target=%s, err=%w", desc, err) proxyErr = err

or just

proxyErr = fmt.Errorf("error proxying, remote unreachable: target=%s, err=%w", desc, err) logrus.Errorf(proxyErr.Error())

robskillington · 2021-09-23T15:24:57Z

internal/api/core/middleware/proxy.go

+	if err != nil {
+		if etcd.IsKeyNotFound(err) {
+			logrus.Debugf("unable to find the Datasource '%s' in project '%s'", datasourceName, projectName)
+			return nil, "", echo.NewHTTPError(http.StatusBadGateway, fmt.Sprintf("unable to forward the request to the datasource '%s', datasource doesn't exist", datasourceName))


nit: Should this be http.StatusNotFound?

that's a good question. Looking at the definition of the error BadGateway probably it's indeed better to return the status not found

robskillington · 2021-09-23T15:25:13Z

internal/api/core/middleware/proxy.go

 	}
 	datasourceName := matchingGroups[0][1]
 	// getting the datasource object
 	dts, err := dao.Get(datasourceName)
 	if err != nil {
 		if etcd.IsKeyNotFound(err) {
 			logrus.Debugf("unable to find the Datasource '%s'", datasourceName)
-			return nil, echo.NewHTTPError(http.StatusBadGateway, fmt.Sprintf("unable to forward the request to the datasource '%s', datasource doesn't exist", datasourceName))
+			return nil, "", echo.NewHTTPError(http.StatusBadGateway, fmt.Sprintf("unable to forward the request to the datasource '%s', datasource doesn't exist", datasourceName))


nit: Should this be http.StatusNotFound?

robskillington · 2021-09-23T15:28:21Z

internal/api/impl/v1/datasource/persistence.go

@@ -41,13 +41,13 @@ func (d *dao) Update(entity *v1.Datasource) error {
 	return d.client.Upsert(key, entity)
 }

-func (d *dao) Delete(name string) error {
-	key := v1.GenerateDatasourceID(name)
+func (d *dao) Delete(project string, name string) error {


nit: Can be project, name string here (since types are same for the two args)

robskillington · 2021-09-23T15:28:24Z

internal/api/impl/v1/datasource/persistence.go

 	return d.client.Delete(key)
 }

-func (d *dao) Get(name string) (*v1.Datasource, error) {
-	key := v1.GenerateDatasourceID(name)
+func (d *dao) Get(project string, name string) (*v1.Datasource, error) {


nit: Can be project, name string here (since types are same for the two args)

robskillington · 2021-09-23T15:29:42Z

internal/api/interface/v1/datasource/interface.go

 }

 type DAO interface {
 	Create(entity *v1.Datasource) error
 	Update(entity *v1.Datasource) error
-	Delete(name string) error
-	Get(name string) (*v1.Datasource, error)
+	Delete(project string, name string) error


nit: Can be project, name string here (since types are same for the two args)

robskillington · 2021-09-23T15:29:47Z

internal/api/interface/v1/datasource/interface.go

-	Delete(name string) error
-	Get(name string) (*v1.Datasource, error)
+	Delete(project string, name string) error
+	Get(project string, name string) (*v1.Datasource, error)


nit: Can be project, name string here (since types are same for the two args)

robskillington · 2021-09-23T15:37:36Z

pkg/model/api/v1/datasource/http.go

+	return nil
+}
+
+func (h *HTTPConfig) validate(conf tmpHTTPConfig) error {


nit: Hm this method also seems to be setting variables on the HTTPConfig itself. Perhaps this would be better named validateAndParse(...) or something?

mmm I don't know, this 'convention' is used on every object. Well it's fine to change it everywhere, perhaps I'm a bit too familiar that in a validate method you have code that is changing and setting data. It's a sort of way to validate the data, because at the end, well your data is valid and contained the proper value.
That's the way I see it. WDYT ?

robskillington · 2021-09-23T15:38:16Z

pkg/model/api/v1/datasource/prometheus.go

+var defaultPrometheusAllowedEndpoints = []HTTPAllowedEndpoint{
+	{
+		Endpoint: "/api/v1/labels",
+		Method:   http.MethodPost,
+	},
+	{
+		Endpoint: "/api/v1/series",
+		Method:   http.MethodPost,
+	},
+	{
+		Endpoint: "/api/v1/metadata",
+		Method:   http.MethodGet,
+	},
+	{
+		Endpoint: "/api/v1/query",
+		Method:   http.MethodPost,
+	},
+	{
+		Endpoint: "/api/v1/query_range",
+		Method:   http.MethodPost,
+	},
+}


Probably want to add the /api/v1/label/FOO/values endpoint too?

yeah that's a TODO (I forgot to add it), we should be able to managed endpoint with variable inside.

and this is not yet used by the Proxy

robskillington · 2021-09-23T15:38:30Z

pkg/model/api/v1/datasource/prometheus.go

+var defaultPrometheusAllowedEndpoints = []HTTPAllowedEndpoint{
+	{
+		Endpoint: "/api/v1/labels",
+		Method:   http.MethodPost,


Hm, is this not GET?

We support both methods:

https://github.com/prometheus/prometheus/blob/d77c985f8c122cf5947b0eb621999fcc8dcca9bf/web/api/v1/api.go#L294-L295

...I think someone in the past once ran into problems with GET and the URL length limit for match parameters, so for newer Prometheus versions, we support POST as well for some of these endpoints.

actually both are supported. It's just POST is avoiding the size limit of the URL, because the parameters are passed in the body

robskillington

LGTM other than minor comments

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

codecov-commenter · 2021-09-26T07:45:21Z

Codecov Report

Merging #84 (da9dabd) into master (827a9ef) will decrease coverage by 3.80%.
The diff coverage is 37.65%.

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   59.20%   55.39%   -3.81%     
==========================================
  Files          47       54       +7     
  Lines        1934     2381     +447     
==========================================
+ Hits         1145     1319     +174     
- Misses        651      880     +229     
- Partials      138      182      +44

Impacted Files	Coverage Δ
cmd/perses/main.go	`0.00% <0.00%> (ø)`
internal/api/core/middleware/proxy.go	`0.00% <0.00%> (ø)`
internal/api/interface/v1/dashboard/interface.go	`0.00% <0.00%> (ø)`
...nal/api/interface/v1/globaldatasource/interface.go	`0.00% <0.00%> (ø)`
internal/api/shared/utils.go	`73.33% <ø> (+6.66%)`	⬆️
pkg/client/api/v1/globaldatasource.go	`0.00% <0.00%> (ø)`
pkg/model/api/v1/kind.go	`66.66% <ø> (+20.83%)`	⬆️
pkg/model/api/v1/metadata.go	`100.00% <ø> (+40.00%)`	⬆️
internal/api/impl/v1/globaldatasource/service.go	`5.66% <5.66%> (ø)`
...ternal/api/impl/v1/globaldatasource/persistence.go	`15.00% <15.00%> (ø)`
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 827a9ef...da9dabd. Read the comment docs.

Nexucis requested review from robskillington and juliusv September 16, 2021 18:08

juliusv reviewed Sep 17, 2021

View reviewed changes

pkg/model/api/v1/datasource.go Show resolved Hide resolved

Nexucis force-pushed the feature/rework-datasource branch 2 times, most recently from f317de7 to 25a2e5d Compare September 18, 2021 08:10

juliusv reviewed Sep 20, 2021

View reviewed changes

pkg/model/api/v1/datasource_test.go Outdated Show resolved Hide resolved

robskillington reviewed Sep 23, 2021

View reviewed changes

robskillington approved these changes Sep 23, 2021

View reviewed changes

Nexucis added 4 commits September 26, 2021 09:40

rework the datasource model

08832e2

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

fix generating dao

2a60472

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

define datasource and globalDatasource

3207c64

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

override unmarshall for HTTPWhiteListConfig

358ad6d

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

Nexucis added 13 commits September 26, 2021 09:40

define default whitelist for prometheus

c03d191

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

review how the proxy is working to match the new datasource model

620b6e3

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

add test on unmarshal datasource and fix bugs

3074155

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

update datasources for the dev environment

20ddd07

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

simplify catchDatasource method

942bbb9

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

fix e2e datasource test

289006c

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

rename httpConfiguration with httpConfig and add more unit test

9a4d6e2

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

fix datasource route in e2e test

85ef82d

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

rename WhiteList by AllowedEndpoint

81d6edb

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

re wording based on the PR review

b2b1035

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

missing apiPrefix in the route for e2e test

87fe0d8

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

fix typo in unmarshal

c9ee227

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

fix review

b326e93

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

Nexucis force-pushed the feature/rework-datasource branch from 981188e to b326e93 Compare September 26, 2021 07:40

fix license check

da9dabd

Signed-off-by: Augustin Husson <husson.augustin@gmail.com>

Nexucis merged commit c39c3ee into master Sep 26, 2021

Nexucis deleted the feature/rework-datasource branch September 26, 2021 07:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reworking the datasource format #84

Reworking the datasource format #84

Nexucis commented Sep 16, 2021 •

edited

Loading

juliusv Sep 17, 2021

Nexucis Sep 18, 2021

juliusv Sep 17, 2021

juliusv Sep 17, 2021

Nexucis Sep 18, 2021

juliusv Sep 17, 2021

Nexucis Sep 18, 2021 •

edited

Loading

Nexucis Sep 18, 2021

juliusv Sep 17, 2021

juliusv Sep 17, 2021

juliusv Sep 17, 2021

Nexucis commented Sep 18, 2021

juliusv commented Sep 20, 2021

robskillington Sep 23, 2021 •

edited

Loading

robskillington Sep 23, 2021

Nexucis Sep 23, 2021

robskillington Sep 23, 2021

robskillington Sep 23, 2021

robskillington Sep 23, 2021

robskillington Sep 23, 2021

robskillington Sep 23, 2021

robskillington Sep 23, 2021

Nexucis Sep 23, 2021

robskillington Sep 23, 2021

Nexucis Sep 23, 2021

Nexucis Sep 23, 2021

robskillington Sep 23, 2021

juliusv Sep 23, 2021

Nexucis Sep 23, 2021

robskillington left a comment

codecov-commenter commented Sep 26, 2021

	// use a dedicated HTTP transport to avoid any TSL encrypt issue
	// use a dedicated HTTP transport to avoid any TLS encryption issues

	// The value can come or from the path of the URL or from the query parameter
	// The value can come from the path of the URL or from the query parameter

Reworking the datasource format #84

Reworking the datasource format #84

Conversation

Nexucis commented Sep 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nexucis Sep 18, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nexucis commented Sep 18, 2021

juliusv commented Sep 20, 2021

robskillington Sep 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robskillington left a comment

Choose a reason for hiding this comment

codecov-commenter commented Sep 26, 2021

Codecov Report

Nexucis commented Sep 16, 2021 •

edited

Loading

Nexucis Sep 18, 2021 •

edited

Loading

robskillington Sep 23, 2021 •

edited

Loading