Refactoring of azure obj store #3957 #3970

wiardvanrij · 2021-03-24T20:19:08Z

Signed-off-by: Wiard van Rij wiard@outlook.com

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

Changes

Adds configuration options for the Azure Pipeline method
Adds configuration options for the Azure Reader method
Adds configuration options for the HTTP transport method
Adds HTTP Transport override
Adds MSI authentication (Cherry-pick from master: Fix parseStep bug introduced in PR #3740 (#3887) #3889)
Made parsing of the config strict
~~Breaks configuration by moving max_retries to it's own reader_config~~
Rewrote tests

Verification

How you tested it?
No, other than the tests included

Especially the MSI authentication part is untested.

How do you know it works?
This is tested via other users on #3957 :

@wiardvanrij ran your image all night locally with no failures.

Originally posted by @airkewld in #3957 (comment)

I hope this PR can get tested by users of Azure to confirm the working of the changes AND if this has positive impact for on, for example #3952 - relates to #3957

wiardvanrij · 2021-03-25T17:24:53Z

Need to check why the test fails on the cortex dep. :D

wiardvanrij · 2021-04-01T13:31:30Z

We use this part; https://github.com/cortexproject/cortex/blob/master/pkg/storage/bucket/azure/bucket_client.go#L10 and Cortex uses the Thanos part.
I think for this change both Cortex and Thanos need to be updated but I'm not sure on how to proceed with this.

@pracucci could you weight in here if my statement is even correct? Thank you!

bwplotka · 2021-04-08T17:18:33Z

Cycling dependency problem, so:

Fork Cortex, fix this on your fork, push remote branch to your fork
Update go.mod to replace cortex dep with your fork URL and Commit SHA
Let's review AND merge this PR
Propose the change in the fork to Cortex
Remove replace.

bwplotka

Some suggestions otherwise look good!

pkg/objstore/azure/azure.go

pkg/objstore/azure/helpers.go

phillebaba · 2021-04-29T11:29:22Z

Is there anything I can do to help move this along? :) Or is this dependent on changes that have to be made in Cortex?

wiardvanrij · 2021-04-29T11:52:33Z

Is there anything I can do to help move this along? :) Or is this dependent on changes that have to be made in Cortex?

It's currently not blocking but just needs some fixes in packages. Which eventually also requires some changes at the Cortex side. I did however not foresee the complexity at first :p

But it's good to know that people need this, so I can make it a higher prio!

airkewld · 2021-04-29T18:42:10Z

Glad to see this moving along. My compact pod is dying multiple times a day at this point, hopefully this will resolve the issue for me and others.
Thanks

pkg/objstore/azure/helpers.go

pkg/objstore/azure/azure.go

wiardvanrij · 2021-04-29T20:33:49Z

@airkewld & @phillebaba see #3957 - I've build an image. But feel free to build it yourself. Wouldn't mind more testers on this feature :)

phillebaba · 2021-04-29T20:40:18Z

I can probably test this as I am already running Thanos in AKS. Hopefully this will work with aad-pod-identity.

wiardvanrij · 2021-04-29T20:54:17Z

I can probably test this as I am already running Thanos in AKS. Hopefully this will work with aad-pod-identity.

Don't use it on production yet tho.. :D Also can we move potential discussions to the issue? Makes it a bit easier. Thanks so much btw :)

wiardvanrij · 2021-04-30T14:30:01Z

Cycling dependency problem, so:

Fork Cortex, fix this on your fork, push remote branch to your fork

Update go.mod to replace cortex dep with your fork URL and Commit SHA

Let's review AND merge this PR

Propose the change in the fork to Cortex

Remove replace.

I changed the configuration in a way that it's not breaking anymore AND it has no dependency issue anymore.
There is a technical trade-off here by implementing a configuration value that is somewhat obsolete.

We now use the max_retries config value if no specific other 'retry related' configuration is set. This means there is 0 change for those we do not use the extra added configuration options. If one sets specific retry value configurations, the max_retries config value is ignored in favor of the more specific config values.

So this resolved two things:

No dependency issue anymore
Backwards compatible

nicolastakashi · 2021-05-05T20:38:38Z

Hi dudes!
any prevision to release it?

yashrsharma44 · 2021-05-06T17:11:35Z

pkg/objstore/azure/azure.go

+	}
+
+	// If we don't have config specific retry values but we do have the generic MaxRetries.
+	// This is part backwards compatibility but also ease of configuration


unless I have misunderstood, do you mean

Suggested change

// This is part backwards compatibility but also ease of configuration

// This part backwards compatibility but also ease of configuration

😅

Yes, thanks, I will make this proper English :D

yashrsharma44

Small question about the comment 😛

bwplotka

LGTM, 💪🏽

pkg/objstore/azure/azure.go

wiardvanrij · 2021-05-08T10:08:33Z

I will update this PR as soon as this is merged: #4104 (comment)
Then I can use that generic http method 💯

phillebaba · 2021-05-09T21:17:11Z

@wiardvanrij had some time now to try this PR. The container is created without an issue but I am seeing authorization issues when uploading blocks from the receiver. I am using aad-pod-identity which adds additional complexities, but unless someone else has verified that this feature works I think you should hold off on merging.

wiardvanrij · 2021-05-10T07:42:33Z

@wiardvanrij had some time now to try this PR. The container is created without an issue but I am seeing authorization issues when uploading blocks from the receiver. I am using aad-pod-identity which adds additional complexities, but unless someone else has verified that this feature works I think you should hold off on merging.

Thanks! Could you perhaps provide more information in #3957 ?

phillebaba

Just small suggestions to fix the msi resource value.

docs/storage.md

pkg/objstore/azure/azure_test.go

bwplotka

LGTM! There are minor style nits, otherwise I would be happy to merge this.

Great work and sorry for major lag on review!

pkg/objstore/azure/azure.go

pkg/objstore/azure/helpers.go

go.mod

tomasfreund · 2021-07-02T10:35:24Z

docs/storage.md

+  http_config:
+    idle_conn_timeout: 0s
+    response_header_timeout: 0s
+    insecure_skip_verify: false


Could tls_config be added instead of just insecure_skip_verify?
I know blob storage itself does not support it but we'd like to use a proxy (by changing the endpoint in the configuration) with mTLS for blob storage to avoid having to provide the storage account key to every cluster we monitor.
I also think it would be better to make the config as similar to Prometheus as possible for consistency and code resuse (if possible)

yes and no. This is in line with all other storage options in Thanos: https://thanos.io/tip/thanos/storage.md/
There is some work being done to make these configs more logical, but for now I try to keep it aligned with how everything in Thanos works (:

edit:
We must also not confuse it on which part the TLS is for. I.e. this is for the connection towards whatever object store you have. Not to implement TLS on the component itself. Just for clarification.

thanks for the answer, I guess the solution to this problem is to implement a custom proxy and run it as a sidecar, using https with a dummy certificate and insecure_skip_verify

thanks for the answer, I guess the solution to this problem is to implement a custom proxy and run it as a sidecar, using https with a dummy certificate and insecure_skip_verify

Yea I understand the case, it's just that I don't feel making one specific change here. However, it would make sense to implement a generic tls_config for non-component related configs, but for 'object storage connections'. As this would/should be implemented not only for Azure Blob, but also for all the other options Thanos supports.
Perhaps you could create a new issue for this?

Signed-off-by: Wiard van Rij <wiard@outlook.com>

Co-authored-by: Philip Laine <philip.laine@gmail.com> Signed-off-by: Wiard van Rij <wiard@outlook.com>

Signed-off-by: Wiard van Rij <wiard@outlook.com>

bwplotka

Amazing! Let's get this in and get some nice, sweet feedback.

Nice work @wiardvanrij , Mr Golang dev! (:

amrmahdi · 2021-09-03T01:50:52Z

@wiardvanrij Thanks for adding these changes!
Couple of questions:

The msi_resource why is it needed? Isn't it just a concat of storage account name and the endpoint?
I'm assuming this does not support user assigned managed identity, correct?

amrmahdi · 2021-09-03T19:30:45Z

@wiardvanrij Thanks for adding these changes!
Couple of questions:

The msi_resource why is it needed? Isn't it just a concat of storage account name and the endpoint?

I'm assuming this does not support user assigned managed identity, correct?

I think got my questions answered. I have a follow up PR to support user-assigned managed identity. #4636

wiardvanrij · 2021-09-03T20:05:41Z

@wiardvanrij Thanks for adding these changes!
Couple of questions:

The msi_resource why is it needed? Isn't it just a concat of storage account name and the endpoint?

I'm assuming this does not support user assigned managed identity, correct?

I think got my questions answered. I have a follow up PR to support user-assigned managed identity. #4636

Wow thanks! Really good and thanks for helping out!

* Introduce `http` config settings in Azure storage Cortex v1.11.0 included thanos-io/thanos#3970, which added configuration options to Azure's http client and transport, replacing usage of `http.DefaultClient`. Unfortunately since Cortex was not setting this config, Cortex implicitly switched from `http.DefaultClient` to all empty values (e.g. `MaxIdleConns: 0` rather than 100). Introduce `http` config settings to Azure storage. This motivated moving `s3.HTTPConfig` into a new `pkg/storage/bucket/config` package, to allow `azure` and `s3` to share it. Also update the instructions for running the website to include installing `embedmd`. Signed-off-by: Andrew Seigner <andrew@sig.gy> * feedback: `config.HTTP` -> `http.Config` also back out changelog cleanup Signed-off-by: Andrew Seigner <andrew@sig.gy> * Back out accidental changelog addition Signed-off-by: Andrew Seigner <andrew@sig.gy>

* Introduce `http` config settings in Azure storage Cortex v1.11.0 included thanos-io/thanos#3970, which added configuration options to Azure's http client and transport, replacing usage of `http.DefaultClient`. Unfortunately since Cortex was not setting this config, Cortex implicitly switched from `http.DefaultClient` to all empty values (e.g. `MaxIdleConns: 0` rather than 100). Introduce `http` config settings to Azure storage. This motivated moving `s3.HTTPConfig` into a new `pkg/storage/bucket/config` package, to allow `azure` and `s3` to share it. Also update the instructions for running the website to include installing `embedmd`. Signed-off-by: Andrew Seigner <andrew@sig.gy> * feedback: `config.HTTP` -> `http.Config` also back out changelog cleanup Signed-off-by: Andrew Seigner <andrew@sig.gy> * Back out accidental changelog addition Signed-off-by: Andrew Seigner <andrew@sig.gy> Signed-off-by: Alvin Lin <alvinlin@amazon.com>

bwplotka reviewed Apr 8, 2021

View reviewed changes

pkg/objstore/azure/azure.go Show resolved Hide resolved

pkg/objstore/azure/helpers.go Show resolved Hide resolved

simonswine mentioned this pull request Apr 23, 2021

when msi is enabled, use a msi authorizer to fetch the storage accoun… cortexproject/cortex#4100

Closed

3 tasks

sonatype-lift bot reviewed Apr 29, 2021

View reviewed changes

pkg/objstore/azure/helpers.go Show resolved Hide resolved

sonatype-lift bot reviewed Apr 29, 2021

View reviewed changes

pkg/objstore/azure/azure.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Apr 29, 2021

View reviewed changes

pkg/objstore/azure/azure.go Outdated Show resolved Hide resolved

wiardvanrij requested review from kakkoyun and bwplotka April 30, 2021 14:36

yashrsharma44 reviewed May 6, 2021

View reviewed changes

bwplotka previously approved these changes May 6, 2021

View reviewed changes

pkg/objstore/azure/azure.go Show resolved Hide resolved

phillebaba reviewed May 10, 2021

View reviewed changes

docs/storage.md Outdated Show resolved Hide resolved

pkg/objstore/azure/azure_test.go Outdated Show resolved Hide resolved

pkg/objstore/azure/azure_test.go Outdated Show resolved Hide resolved

wiardvanrij dismissed bwplotka’s stale review via 25d5415 May 10, 2021 19:05

bwplotka previously approved these changes Jun 17, 2021

View reviewed changes

wiardvanrij dismissed bwplotka’s stale review via 2710576 June 17, 2021 18:27

sonatype-lift bot reviewed Jun 17, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

tomasfreund reviewed Jul 2, 2021

View reviewed changes

wiardvanrij and others added 18 commits July 5, 2021 16:36

Rebased feature - initial commit of azure obj store extend

2d9d9bc

Signed-off-by: Wiard van Rij <wiard@outlook.com>

use custom cortex to fix config change

321c0a8

Signed-off-by: Wiard van Rij <wiard@outlook.com>

modules acting up

dcb5060

Signed-off-by: Wiard van Rij <wiard@outlook.com>

remove sprint

ad9b8cf

Signed-off-by: Wiard van Rij <wiard@outlook.com>

adds dot

5487f3e

Signed-off-by: Wiard van Rij <wiard@outlook.com>

removed need for changes on cortex side

27444e7

Signed-off-by: Wiard van Rij <wiard@outlook.com>

adds changelog

9897864

Signed-off-by: Wiard van Rij <wiard@outlook.com>

Update docs/storage.md

be8533a

Co-authored-by: Philip Laine <philip.laine@gmail.com> Signed-off-by: Wiard van Rij <wiard@outlook.com>

Update pkg/objstore/azure/azure_test.go

9184805

Co-authored-by: Philip Laine <philip.laine@gmail.com> Signed-off-by: Wiard van Rij <wiard@outlook.com>

Update pkg/objstore/azure/azure_test.go

bed1bb9

Co-authored-by: Philip Laine <philip.laine@gmail.com> Signed-off-by: Wiard van Rij <wiard@outlook.com>

update a few cleanups

a3b8715

Signed-off-by: Wiard van Rij <wiard@outlook.com>

fixes go.mod and go.sum?

105d2be

Signed-off-by: Wiard van Rij <wiard@outlook.com>

fixes whitespace

cdfa94e

Signed-off-by: Wiard van Rij <wiard@outlook.com>

fixes space

fc2461e

Signed-off-by: Wiard van Rij <wiard@outlook.com>

updates azure Go-autorest

a8aa4e5

Signed-off-by: Wiard van Rij <wiard@outlook.com>

fixes readme

e8a0e0f

Signed-off-by: Wiard van Rij <wiard@outlook.com>

fixes go.sum.. (╯°□°)╯︵ ┻━┻

f094476

Signed-off-by: Wiard van Rij <wiard@outlook.com>

e2e retest

092b159

Signed-off-by: Wiard van Rij <wiard@outlook.com>

bwplotka approved these changes Jul 5, 2021

View reviewed changes

bwplotka merged commit 9355a82 into thanos-io:main Jul 5, 2021

scoof mentioned this pull request Aug 6, 2021

feat: azure storage managed identity cortexproject/cortex#4358

Closed

3 tasks

bill3tt mentioned this pull request Aug 9, 2021

Qurey：Can not query from Object storage #4466

Closed

NissesSenap mentioned this pull request Aug 22, 2021

Add MSI authentication for Azure Storage Account to chunks grafana/loki#4201

Closed

siggy mentioned this pull request Dec 8, 2021

Introduce http config settings in Azure storage cortexproject/cortex#4581

Merged

3 tasks

wiardvanrij mentioned this pull request Jan 18, 2022

Thanos Compact Crashing due to Azure Timeout #3952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of azure obj store #3957 #3970

Refactoring of azure obj store #3957 #3970

wiardvanrij commented Mar 24, 2021 •

edited

Loading

wiardvanrij commented Mar 25, 2021

wiardvanrij commented Apr 1, 2021

bwplotka commented Apr 8, 2021

bwplotka left a comment

phillebaba commented Apr 29, 2021

wiardvanrij commented Apr 29, 2021

airkewld commented Apr 29, 2021

wiardvanrij commented Apr 29, 2021

phillebaba commented Apr 29, 2021

wiardvanrij commented Apr 29, 2021

wiardvanrij commented Apr 30, 2021

nicolastakashi commented May 5, 2021

yashrsharma44 May 6, 2021

wiardvanrij May 8, 2021

yashrsharma44 left a comment

bwplotka left a comment

wiardvanrij commented May 8, 2021

phillebaba commented May 9, 2021 •

edited

Loading

wiardvanrij commented May 10, 2021

phillebaba left a comment

bwplotka left a comment •

edited

Loading

tomasfreund Jul 2, 2021

wiardvanrij Jul 2, 2021 •

edited

Loading

tomasfreund Jul 2, 2021

wiardvanrij Jul 5, 2021

bwplotka left a comment

amrmahdi commented Sep 3, 2021

amrmahdi commented Sep 3, 2021

wiardvanrij commented Sep 3, 2021

	// This is part backwards compatibility but also ease of configuration
	// This part backwards compatibility but also ease of configuration

Refactoring of azure obj store #3957 #3970

Refactoring of azure obj store #3957 #3970

Conversation

wiardvanrij commented Mar 24, 2021 • edited Loading

Changes

Verification

wiardvanrij commented Mar 25, 2021

wiardvanrij commented Apr 1, 2021

bwplotka commented Apr 8, 2021

bwplotka left a comment

Choose a reason for hiding this comment

phillebaba commented Apr 29, 2021

wiardvanrij commented Apr 29, 2021

airkewld commented Apr 29, 2021

wiardvanrij commented Apr 29, 2021

phillebaba commented Apr 29, 2021

wiardvanrij commented Apr 29, 2021

wiardvanrij commented Apr 30, 2021

nicolastakashi commented May 5, 2021

yashrsharma44 May 6, 2021

Choose a reason for hiding this comment

wiardvanrij May 8, 2021

Choose a reason for hiding this comment

yashrsharma44 left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

wiardvanrij commented May 8, 2021

phillebaba commented May 9, 2021 • edited Loading

wiardvanrij commented May 10, 2021

phillebaba left a comment

Choose a reason for hiding this comment

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

tomasfreund Jul 2, 2021

Choose a reason for hiding this comment

wiardvanrij Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

tomasfreund Jul 2, 2021

Choose a reason for hiding this comment

wiardvanrij Jul 5, 2021

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

amrmahdi commented Sep 3, 2021

amrmahdi commented Sep 3, 2021

wiardvanrij commented Sep 3, 2021

wiardvanrij commented Mar 24, 2021 •

edited

Loading

phillebaba commented May 9, 2021 •

edited

Loading

bwplotka left a comment •

edited

Loading

wiardvanrij Jul 2, 2021 •

edited

Loading