rule: query parameter is not encoded properly in v0.11.0

[tobiaszheller]

Thanos, Prometheus and Golang version used:
Thanos v0.11.0

What happened:

After upgrading thanos from v0.10.0 to v.0.11.0 thanos rule has stopped working.
I am using following setting for query:

--query=envoy-internal:80/thanos-query

It was working fine before asking for url http://envoy-internal:80/thanos-query/api/v1/query.
After upgrading to v0.11.0 query url with after hostname is not encoded properly:
http://envoy-internal:80%2Fthanos-query/api/v1/query.

What you expected to happen:

Encode url with path after hostname in valid way.

How to reproduce it (as minimally and precisely as possible):

	scheme := "http"
	addr := "localhost:80/thanos"
	prefix := ""
	base := &url.URL{
		Scheme: scheme,
		Host:   addr,
		Path:   path.Join("/", prefix),
	}
	fmt.Println(base.String())

This is ☝️ more or less current implementation of parsing query URL which results in following output: http://localhost:80%2Fthanos/

I can prepare PR with fix.

[bwplotka]

Valid, help wanted, whoever has time (: Thanks for report!

cc @simonpasquier @pgier maybe?

[bwplotka]

I can prepare PR with fix.

Just noticed. Yes please @tobiaszheller fix would be nice

[tobiaszheller]

@bwplotka I have take a closer looks and it seems that it will require bigger changes:

thanos/pkg/http/http.go

Lines 133 to 144 in 5cb3c87

	type EndpointsConfig struct {
	// List of addresses with DNS prefixes.
	StaticAddresses []string `yaml:"static_configs"`
	// List of file configurations (our FileSD supports different DNS lookups).
	FileSDConfigs []FileSDConfig `yaml:"file_sd_configs"`
	// The URL scheme to use when talking to targets.
	Scheme string `yaml:"scheme"`
	// Path prefix to add in front of the endpoint path.
	PathPrefix string `yaml:"path_prefix"`
	}

EndpointsConfig component expects slice of static addresses but only single PathPrefix.

I am not sure if I can modify that component without break compatibility. I saw that it also used in unmarshaling from config file, right?

[simonpasquier]

what about changing this part of the code and create one query.Config instance per queryAddrs item?

thanos/cmd/thanos/rule.go

Lines 298 to 320 in 2d7b214

	for _, addr := range queryAddrs {
	if addr == "" {
	return errors.New("static querier address cannot be empty")
	}
	}
	// Build the query configuration from the legacy query flags.
	var fileSDConfigs []http_util.FileSDConfig
	if len(querySDFiles) > 0 {
	fileSDConfigs = append(fileSDConfigs, http_util.FileSDConfig{
	Files: querySDFiles,
	RefreshInterval: model.Duration(querySDInterval),
	})
	}
	queryCfg = append(queryCfg,
	query.Config{
	EndpointsConfig: http_util.EndpointsConfig{
	Scheme: "http",
	StaticAddresses: queryAddrs,
	FileSDConfigs: fileSDConfigs,
	},
	},
	)

Something like:

 for _, addr := range queryAddrs { 
 	if addr == "" { 
 		return errors.New("static querier address cannot be empty") 
 	} 
        // TODO: parse addr to extract path.
	queryCfg = append(queryCfg,
		query.Config{
			EndpointsConfig: http_util.EndpointsConfig{
				Scheme:          "http",
				StaticAddresses: []string{addr},
				PathPrefix:   path,
			},
		},
	)
 }

 var fileSDConfigs []http_util.FileSDConfig 
 if len(querySDFiles) > 0 { 
 	fileSDConfigs = append(fileSDConfigs, http_util.FileSDConfig{ 
 		Files:           querySDFiles, 
 		RefreshInterval: model.Duration(querySDInterval), 
 	})
	queryCfg = append(queryCfg,
		query.Config{
			EndpointsConfig: http_util.EndpointsConfig{
				Scheme:          "http",
				FileSDConfigs:   fileSDConfigs,
			},
		},
	)
 }

[bwplotka]

Thanks @simonpasquier for helping, I think I agree with Simon on this. (:

[bwplotka]

Fixed by #2288