Adds authorization for sidecar-promclient #4104

someshkoli · 2021-04-24T12:07:10Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Changes

Added new flag prometheus.http-client to sidecar to pass http client config via file or string(similar to alertmanager). Using this flag an instance of thanoshttp.Client is created and is passed to promclient.NewClient method which uses this client as base client instead of whole new client which then makes authorized api calls.
The same client is passed to reloader config which then makes authorized api calls to prom api.

There are some code duplication which has been duplicated from prometheus which can be easily removed by exposing one of the functions in prometheus http_config
🙌
Close: #3975

Verification

Added a e2e test:
Checks if query is able to query when prom uses auth

someshkoli · 2021-04-26T12:40:32Z

cmd/thanos/sidecar.go

@@ -212,10 +231,13 @@ func runSidecar(
 	}

 	{
+		// todo(someshkoli): Not sure how to properly implement RoundTripper for these properties or


@Namanl2001 please have a look at this 🤔 I guess there's no clean way to set these properties. (might need to rewrite auth for promclient or make some changes thanoshttp for custom transporter)

I think we need some 'generic' transporter that we can re-use everywhere. I had the same 'issue': https://github.com/thanos-io/thanos/pull/3970/files/e34e60e2848f2233809d67d5398151b670341ea1#diff-95c18d923c79bf32e4f8b3370d9a925aa910643559d351fab6bc307691c8f672R119

The thanoshttp has one func which is:

// NewTransport creates a new http.Transport with default settings.

I guess we could add an other function that has httpConfig as argument for re-usability in the entire project.

I think we need some 'generic' transporter that we can re-use everywhere. I had the same 'issue': https://github.com/thanos-io/thanos/pull/3970/files/e34e60e2848f2233809d67d5398151b670341ea1#diff-95c18d923c79bf32e4f8b3370d9a925aa910643559d351fab6bc307691c8f672R119

The thanoshttp has one func which is:

// NewTransport creates a new http.Transport with default settings.

I guess we could add an other function that has httpConfig as argument for re-usability in the entire project.

Agreed, this is being used at 2-3 i guess and making a generic one can improve reusablity. @wiardvanrij I hope the overall idea in implementation is correct ? I can move forward with fixing this transporter issue ?

I'm not the right person to make a definitive review. I miss the exact technical knowledge on it. However I was at the meeting and I think you made what was discussed. Also it looks logical and makes sense.

You could always wait for a review if you think you might invest a lot of time into a feature you are unsure about. However if you have a good feeling about it, go make awesome stuff and continue 👍

Alright thanks

I think this transport and connection flags

conf.connection.maxIdleConnsPerHost conf.connection.maxIdleConns

Are colliding with out promethues.http-config flag. The idea would be to unify those? E.g Deprecating connection flags if that makes sense? Or reusing them?

The goal is to have one way of configuring HTTP configuration.

Let's remove

--receive.connection-pool-size=RECEIVE.CONNECTION-POOL-SIZE
Controls the http MaxIdleConns. Default is 0,
which is unlimited
--receive.connection-pool-size-per-host=100

And use our prometheus flags.

bwplotka · 2021-04-29T11:15:24Z

pkg/promclient/promclient.go

 		"",
 	)
 }

 // NewWithTracingClient returns client with tracing tripperware.
-func NewWithTracingClient(logger log.Logger, userAgent string) *Client {
+func NewWithTracingClient(logger log.Logger, httpClient http.Client, userAgent string) *Client {


Suggested change

func NewWithTracingClient(logger log.Logger, httpClient http.Client, userAgent string) *Client {

func NewWithTracingClient(logger log.Logger, httpClient *http.Client, userAgent string) *Client {

bwplotka · 2021-04-29T11:19:29Z

cmd/thanos/sidecar.go

@@ -212,10 +231,13 @@ func runSidecar(
 	}

 	{
+		// todo(someshkoli): Not sure how to properly implement RoundTripper for these properties or


I think this transport and connection flags

conf.connection.maxIdleConnsPerHost conf.connection.maxIdleConns

Are colliding with out promethues.http-config flag. The idea would be to unify those? E.g Deprecating connection flags if that makes sense? Or reusing them?

The goal is to have one way of configuring HTTP configuration.

bwplotka · 2021-04-29T11:21:54Z

cmd/thanos/sidecar.go

@@ -212,10 +231,13 @@ func runSidecar(
 	}

 	{
+		// todo(someshkoli): Not sure how to properly implement RoundTripper for these properties or


Let's remove

--receive.connection-pool-size=RECEIVE.CONNECTION-POOL-SIZE
Controls the http MaxIdleConns. Default is 0,
which is unlimited
--receive.connection-pool-size-per-host=100

And use our prometheus flags.

pkg/http/http.go

cmd/thanos/sidecar.go

pkg/http/http.go

someshkoli · 2021-05-03T15:47:29Z

This implementation helps in providing a generic way to create http clients used across. There are still many instances where directly http library is used to create client which can be replaced with thanos http client instance and can be configured flexibly.
Also solves the issue mentined by @wiardvanrij is here.

One question, while updating docs, the ruler docs got updated too because the http.ClientConfig structure was modified. Is this healthy to do ?

wiardvanrij · 2021-05-08T10:14:20Z

pkg/http/http.go

+			MaxIdleConnsPerHost: opts.maxIdleConnsPerHost,
+			DisableKeepAlives:   true,
+			TLSClientConfig:     tlsConfig,
+			DisableCompression:  true,


I think we default to this because there was 'something with it' for Thanos side (citation needed). As this is more a generic package we should default to false and explicitly set this too true on functionalities via arguments that require this to be disabled.

Tl;dr IMO: set 'defaults' to something that would be defaults for all packages, and override different cases when needed. Not the other way around. :)

I think we default to this because there was 'something with it' for Thanos side (citation needed). As this is more a generic package we should default to false and explicitly set this too true on functionalities via arguments that require this to be disabled.

Tl;dr IMO: set 'defaults' to something that would be defaults for all packages, and override different cases when needed. Not the other way around. :)

Yes Yes, sounds fair. I wonder if we can use exthttp.NewTransport here and then override them with help of options 🤔 ?

We can, but we need to be aware who is using exthttp.NewTransport and how

pkg/http/http.go

bwplotka

Don't have time to review fully, but did part of it, looks good generally, some nits.

cmd/thanos/config.go

bwplotka · 2021-05-18T09:20:46Z

pkg/http/http.go

 	"gopkg.in/yaml.v2"

 	"github.com/thanos-io/thanos/pkg/discovery/cache"
 )

+var defaultHTTPClientOptions = httpClientOptions{
+	maxIdleConns:        0,


0 feels wrong, see default Go impl:

// DefaultTransport is the default implementation of Transport and is // used by DefaultClient. It establishes network connections as needed // and caches them for reuse by subsequent calls. It uses HTTP proxies // as directed by the $HTTP_PROXY and $NO_PROXY (or $http_proxy and // $no_proxy) environment variables. var DefaultTransport RoundTripper = &Transport{ Proxy: ProxyFromEnvironment, DialContext: (&net.Dialer{ Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, DualStack: true, }).DialContext, ForceAttemptHTTP2: true, MaxIdleConns: 100, IdleConnTimeout: 90 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, }

0 feels wrong, see default Go impl:

// DefaultTransport is the default implementation of Transport and is // used by DefaultClient. It establishes network connections as needed // and caches them for reuse by subsequent calls. It uses HTTP proxies // as directed by the $HTTP_PROXY and $NO_PROXY (or $http_proxy and // $no_proxy) environment variables. var DefaultTransport RoundTripper = &Transport{ Proxy: ProxyFromEnvironment, DialContext: (&net.Dialer{ Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, DualStack: true, }).DialContext, ForceAttemptHTTP2: true, MaxIdleConns: 100, IdleConnTimeout: 90 * time.Second, TLSHandshakeTimeout: 10 * time.Second, ExpectContinueTimeout: 1 * time.Second, }

Yes right

bwplotka · 2021-05-18T09:20:55Z

pkg/http/http.go

 	"gopkg.in/yaml.v2"

 	"github.com/thanos-io/thanos/pkg/discovery/cache"
 )

+var defaultHTTPClientOptions = httpClientOptions{
+	maxIdleConns:        0,
+	maxIdleConnsPerHost: 100, // see https://github.com/golang/go/issues/13801


Suggested change

maxIdleConnsPerHost: 100, // see https://github.com/golang/go/issues/13801

maxIdleConnsPerHost: 100, // See https://github.com/golang/go/issues/13801.

bwplotka · 2021-05-18T09:22:45Z

pkg/http/http.go

+			MaxIdleConnsPerHost: opts.maxIdleConnsPerHost,
+			DisableKeepAlives:   true,
+			TLSClientConfig:     tlsConfig,
+			DisableCompression:  true,


We can, but we need to be aware who is using exthttp.NewTransport and how

bwplotka · 2021-05-18T09:24:03Z

pkg/http/http.go

-	client, err := config_util.NewClientFromConfig(httpClientConfig, name, config_util.WithHTTP2Disabled())
+	// see https://github.com/golang/go/issues/13801
+	if cfg.MaxIdleConns == 0 {
+		cfg.MaxIdleConns = 20000


I don't get it, why we don't use default? Also 0 means no limit, so 0 has meaning too, we should tolerate such option

I don't get it, why we don't use default? Also 0 means no limit, so 0 has meaning too, we should tolerate such option

Actually, this value is picked from prometheus http which we were using before, so thought it would be better to keep it the same. Also yeah 0 does for this is acceptable, 'll fix it.

bwplotka · 2021-05-18T09:24:21Z

pkg/http/http.go

@@ -95,10 +212,23 @@ func NewHTTPClient(cfg ClientConfig, name string) (*http.Client, error) {
 		return nil, err
 	}

-	client, err := config_util.NewClientFromConfig(httpClientConfig, name, config_util.WithHTTP2Disabled())
+	// see https://github.com/golang/go/issues/13801


Weird haging comment (and not full sentence), can we remove it?

bwplotka · 2021-05-18T09:24:40Z

pkg/http/http.go

+		cfg.MaxIdleConns = 20000
+	}
+	if cfg.MaxIdleConnsPerHost == 0 {
+		cfg.MaxIdleConnsPerHost = 1000


Why those numbers?

Again from prometheus implementation.

pkg/http/http.go

bwplotka · 2021-05-18T09:27:50Z

pkg/http/http.go

@@ -262,3 +392,151 @@ func (c *Client) Discover(ctx context.Context) {
 func (c *Client) Resolve(ctx context.Context) error {
 	return c.provider.Resolve(ctx, append(c.fileSDCache.Addresses(), c.staticAddresses...))
 }
+
+// Duplicated from github.com/prometheus/common/config
+// better option would be to expose config_util.newTLSRoundTripper function from prometheus


Yes, can we do that (you can contribute there, we are happy to review and merge it there)

Great, I'll raise a PR there

prometheus/common#301

bwplotka

LGTM generally, just small nits. Thanks.

cmd/thanos/sidecar.go

pkg/http/http.go

bwplotka · 2021-05-20T17:56:37Z

pkg/http/http.go

 	if err := httpClientConfig.Validate(); err != nil {
 		return nil, err
 	}

-	client, err := config_util.NewClientFromConfig(httpClientConfig, name, config_util.WithHTTP2Disabled())


Can we still use it? and append middleware we want on top?

sonatype-lift · 2021-06-01T15:40:09Z

test/e2e/e2ethanos/services.go

+func NewPrometheusAndSidecarWithBasicAuth(sharedDir string, netName string, name string, promConfig, webConfig, promImage string) (*e2e.HTTPService, *Service, error) {
+	dir := filepath.Join(sharedDir, "data", "prometheus", name)
+	container := filepath.Join(e2e.ContainerSharedDir, "data", "prometheus", name)
+	if err := os.MkdirAll(dir, 0777); err != nil {


G301: Expect directory permissions to be 0750 or less
(at-me in a reply with help or ignore)

sonatype-lift · 2021-06-01T15:40:11Z

test/e2e/e2ethanos/services.go

+		return nil, nil, errors.Wrap(err, "create prometheus dir")
+	}
+
+	if err := ioutil.WriteFile(filepath.Join(dir, "prometheus.yml"), []byte(promConfig), 0666); err != nil {


G306: Expect WriteFile permissions to be 0600 or less
(at-me in a reply with help or ignore)

bwplotka · 2021-06-17T11:54:59Z

Any update on this?

someshkoli · 2021-06-17T13:17:33Z

Any update on this?

Its ready for a review, I guess I forgot to put a review request :)

yeya24 · 2021-06-26T16:26:22Z

@someshkoli Can you rebase on main and fix the conflicts? Thank you!

Added http-client flag to sidecar which takes yaml file or string as input Passed auth client to prom client to build new prom client instead of using default http client todo: check if this is the proper way to implement this - can we reuse thanoshttp for auth - if yes then how to add maxidelconns and maxidleconnsperhost properties to http client returned by thanoshttp.NewHTTPClient Signed-off-by: someshkoli <[email protected]>

Signed-off-by: someshkoli <[email protected]>

merged maxidleconns flag to http-config added additinal properties to ClientConfig Signed-off-by: someshkoli <[email protected]>

Defines default TransportConfig and uses the same for yaml unmarshalling Adds checks for bearer token auth Signed-off-by: someshkoli <[email protected]>

Signed-off-by: someshkoli <[email protected]>

sonatype-lift · 2021-07-05T15:32:52Z

test/e2e/e2ethanos/services.go

+func NewPrometheusAndSidecarWithBasicAuth(sharedDir string, netName string, name string, promConfig, webConfig, promImage string) (*e2e.HTTPService, *Service, error) {
+	dir := filepath.Join(sharedDir, "data", "prometheus", name)
+	container := filepath.Join(e2e.ContainerSharedDir, "data", "prometheus", name)
+	if err := os.MkdirAll(dir, 0777); err != nil {


G301: Expect directory permissions to be 0750 or less
(at-me in a reply with help or ignore)

sonatype-lift · 2021-07-05T15:32:53Z

test/e2e/e2ethanos/services.go

+		return nil, nil, errors.Wrap(err, "create prometheus dir")
+	}
+
+	if err := ioutil.WriteFile(filepath.Join(dir, "prometheus.yml"), []byte(promConfig), 0666); err != nil {


G306: Expect WriteFile permissions to be 0600 or less
(at-me in a reply with help or ignore)

yeya24 · 2021-08-05T06:10:10Z

Hi, @someshkoli, sorry for the late reply. Can you fix the conflict and get CI passing? Your work is really amazing and we really want to have this feature!

someshkoli · 2021-08-06T15:25:59Z

Hi, @someshkoli, sorry for the late reply. Can you fix the conflict and get CI passing? Your work is really amazing and we really want to have this feature!

will update it soon, have a lot to rebase :")

…s into feature/promclient-auth Signed-off-by: someshkoli <[email protected]>

Signed-off-by: someshkoli <[email protected]>

someshkoli · 2021-08-30T15:54:14Z

@yeya24 Moving this to #4612

someshkoli · 2021-09-06T18:29:44Z

@yeya24 can you please take a look at #4612 ?

yeya24 · 2021-11-08T04:15:13Z

Close in favor of #4612

* pull work from #4104 Signed-off-by: someshkoli <[email protected]> * update docs Signed-off-by: someshkoli <[email protected]> * fix fs perms Signed-off-by: someshkoli <[email protected]> * remove flags from exthttp, make prom http config configurable, Signed-off-by: someshkoli <[email protected]> * typo fix Signed-off-by: someshkoli <[email protected]> * adds method to set http client to realoder Signed-off-by: someshkoli <[email protected]> * Merge branch 'main' into feature/promclient-authorization Signed-off-by: Ben Ye <[email protected]> * fix format Signed-off-by: Ben Ye <[email protected]> * fix e2e Signed-off-by: Ben Ye <[email protected]> * fix prom e2e test Signed-off-by: Ben Ye <[email protected]> * update changelog Signed-off-by: Ben Ye <[email protected]> * update dependency Signed-off-by: Ben Ye <[email protected]> Co-authored-by: Ben Ye <[email protected]>

someshkoli changed the title ~~Feature/promclient auth~~ Authorization for sidecar-promclient Apr 24, 2021

someshkoli changed the title ~~Authorization for sidecar-promclient~~ Adds authorization for sidecar-promclient Apr 24, 2021

someshkoli marked this pull request as draft April 24, 2021 12:32

someshkoli commented Apr 26, 2021

View reviewed changes

bwplotka reviewed Apr 29, 2021

View reviewed changes

sonatype-lift bot reviewed May 2, 2021

View reviewed changes

pkg/http/http.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed May 2, 2021

View reviewed changes

cmd/thanos/sidecar.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed May 2, 2021

View reviewed changes

pkg/http/http.go Outdated Show resolved Hide resolved

someshkoli marked this pull request as ready for review May 3, 2021 15:38

someshkoli requested review from bwplotka and wiardvanrij May 3, 2021 15:50

wiardvanrij mentioned this pull request May 8, 2021

Refactoring of azure obj store #3957 #3970

Merged

2 tasks

wiardvanrij reviewed May 8, 2021

View reviewed changes

pkg/http/http.go Show resolved Hide resolved

bwplotka reviewed May 18, 2021

View reviewed changes

someshkoli mentioned this pull request May 18, 2021

http_config: Exposes NewTLSRoundTripper method prometheus/common#301

Merged

someshkoli force-pushed the feature/promclient-auth branch from 239bad8 to d4b1e1d Compare May 20, 2021 16:59

bwplotka reviewed May 20, 2021

View reviewed changes

someshkoli force-pushed the feature/promclient-auth branch from d4b1e1d to 1b929d3 Compare May 28, 2021 10:24

sonatype-lift bot reviewed Jun 1, 2021

View reviewed changes

someshkoli force-pushed the feature/promclient-auth branch from 61a7e16 to 69776e5 Compare June 1, 2021 15:56

someshkoli requested a review from bwplotka June 17, 2021 13:17

someshkoli mentioned this pull request Jun 26, 2021

sidecar support https and basic auth access to Prometheus #3975

Closed

someshkoli added 2 commits July 5, 2021 20:47

Added httpclient property to reloader

5b58893

Signed-off-by: someshkoli <[email protected]>

someshkoli added 10 commits July 5, 2021 20:52

lint fix

23603ca

Signed-off-by: someshkoli <[email protected]>

Removed redundant maxidleconns flag from sidecar

1e85b8d

merged maxidleconns flag to http-config added additinal properties to ClientConfig Signed-off-by: someshkoli <[email protected]>

Adds TransportConfig to client config

3ba0361

Defines default TransportConfig and uses the same for yaml unmarshalling Adds checks for bearer token auth Signed-off-by: someshkoli <[email protected]>

Updates docs for ruler

aa72a72

Signed-off-by: someshkoli <[email protected]>

lint fix

b21971d

Signed-off-by: someshkoli <[email protected]>

update prometheus/common version from v21 -> v25

ff6babe

Signed-off-by: someshkoli <[email protected]>

Added integration test for prom<-sidecar<-query

1610ada

Signed-off-by: someshkoli <[email protected]>

lint fix

14e6ff4

Signed-off-by: someshkoli <[email protected]>

update new query service

9c7f78e

Signed-off-by: someshkoli <[email protected]>

update new query service

ac569d9

Signed-off-by: someshkoli <[email protected]>

someshkoli force-pushed the feature/promclient-auth branch from 8a01ab9 to ac569d9 Compare July 5, 2021 15:26

sonatype-lift bot reviewed Jul 5, 2021

View reviewed changes

someshkoli added 2 commits August 25, 2021 01:05

Merge branch 'main' into feature/promclient-auth

fb3c7bb

updates docs and fixes kingpin issue

c572865

someshkoli force-pushed the feature/promclient-auth branch from 8efaac1 to 8dca580 Compare August 24, 2021 19:48

Merge branch 'feature/promclient-auth' of github.com:someshkoli/thano…

eb37e71

…s into feature/promclient-auth Signed-off-by: someshkoli <[email protected]>

someshkoli force-pushed the feature/promclient-auth branch from 8dca580 to eb37e71 Compare August 24, 2021 19:49

someshkoli added 2 commits August 25, 2021 01:27

updates gomod

bfacbeb

Signed-off-by: someshkoli <[email protected]>

lint fix

af518e4

Signed-off-by: someshkoli <[email protected]>

someshkoli force-pushed the feature/promclient-auth branch from 5c71354 to af518e4 Compare August 24, 2021 20:41

someshkoli added a commit to someshkoli/thanos that referenced this pull request Aug 30, 2021

pull work from thanos-io#4104

2076b8b

Signed-off-by: someshkoli <[email protected]>

someshkoli mentioned this pull request Aug 30, 2021

Adds authorization for sidecar-promclient #4612

Merged

2 tasks

yeya24 closed this Nov 8, 2021

	func NewWithTracingClient(logger log.Logger, httpClient http.Client, userAgent string) *Client {
	func NewWithTracingClient(logger log.Logger, httpClient http.Client, userAgent string) Client {

	maxIdleConnsPerHost: 100, // see https://github.com/golang/go/issues/13801
	maxIdleConnsPerHost: 100, // See https://github.com/golang/go/issues/13801.

Adds authorization for sidecar-promclient #4104

Adds authorization for sidecar-promclient #4104

Conversation

someshkoli commented Apr 24, 2021 • edited Loading

Changes

Verification

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

someshkoli commented May 3, 2021 • edited Loading

Choose a reason for hiding this comment

someshkoli May 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka left a 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

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonatype-lift bot Jun 1, 2021

Choose a reason for hiding this comment

sonatype-lift bot Jun 1, 2021

Choose a reason for hiding this comment

bwplotka commented Jun 17, 2021

someshkoli commented Jun 17, 2021 • edited Loading

yeya24 commented Jun 26, 2021

sonatype-lift bot Jul 5, 2021

Choose a reason for hiding this comment

sonatype-lift bot Jul 5, 2021

Choose a reason for hiding this comment

yeya24 commented Aug 5, 2021 • edited Loading

someshkoli commented Aug 6, 2021

someshkoli commented Aug 30, 2021 • edited Loading

someshkoli commented Sep 6, 2021

yeya24 commented Nov 8, 2021

someshkoli commented Apr 24, 2021 •

edited

Loading

someshkoli commented May 3, 2021 •

edited

Loading

someshkoli May 8, 2021 •

edited

Loading

someshkoli commented Jun 17, 2021 •

edited

Loading

yeya24 commented Aug 5, 2021 •

edited

Loading

someshkoli commented Aug 30, 2021 •

edited

Loading