Support azure storage of azurechina,azuregerman,azureusgov #2988

zbbkeepgoing · 2020-08-06T12:59:55Z

Signed-off-by: Binbin Zou [email protected]

What this PR does:
support azure storage of AzureChinaCloud,AzureGermanCloud,GerUSGovernment

Which issue(s) this PR fixes:
NONE

Checklist

Tests updated
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Test Images:

Signed-off-by: Binbin Zou <[email protected]>

zbbkeepgoing · 2020-08-07T01:29:35Z

Add support for other Azure Clouds On #1319

zbbkeepgoing · 2020-08-07T01:47:55Z

@khaines Hi haines, please review if you are free.

pracucci

Thanks @zbbkeepgoing for working on this! ❤️

I left few comments. Most are minor things, but please be aware that CircleCI for this PR is failing to run: could you try to login to https://circleci.com/ with your GitHub account and follow the cortexproject there before submitting your next commit, please?

I'm marking this PR as "request changes" mainly to signal CI hasn't run.

pracucci · 2020-08-07T15:38:41Z

pkg/chunk/azure/blob_storage_client.go

 // BlobStorageConfig defines the configurable flags that can be defined when using azure blob storage.
 type BlobStorageConfig struct {
+	CloudEnvironment   string         `yaml:"cloud_environment"`


What if we simply call it environment?

pracucci · 2020-08-07T15:39:22Z

pkg/chunk/azure/blob_storage_client.go

@@ -41,6 +55,7 @@ func (c *BlobStorageConfig) RegisterFlags(f *flag.FlagSet) {

 // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet
 func (c *BlobStorageConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
+	f.StringVar(&c.CloudEnvironment, prefix+"azure.cloud_environment", "AzureGlobal", "Environment of Azure Cloud. This value can be AzureGlobal, AzureChinaCloud, AzureGermanCloud, AzureUSGovernment.")


We have a convention to use _ in YAML and - in CLI flags. Here it should be cloud-environment, even if I would suggest to just call it environment.

Also if you have a constant for AzureGlobal and the other environments and you create a global slice variable with all supported environments, then here you could dinamically generate it using strings.Join(supportedEnvironments, ",").

pracucci · 2020-08-07T15:40:41Z

pkg/chunk/azure/blob_storage_client.go

 const blobURLFmt = "https://%s.blob.core.windows.net/%s/%s"
 const containerURLFmt = "https://%s.blob.core.windows.net/%s"


Could you prefix these by global, please?

pracucci · 2020-08-07T15:41:59Z

pkg/chunk/azure/blob_storage_client.go

+
+func (b *BlobStorage) selectBlobUrlFmt() string {
+	switch b.cfg.CloudEnvironment {
+	case "AzureGlobal":


Could you create constants for these AzureGlobal, AzureChinaCloud, etc please?

pkg/chunk/azure/blob_storage_client.go

pracucci · 2020-08-07T15:46:01Z

CHANGELOG.md

@@ -46,6 +46,7 @@
    - `-experimental.tsdb.flush-blocks-on-shutdown` changed to `-experimental.blocks-storage.tsdb.flush-blocks-on-shutdown`
 * [CHANGE] Flags `-bigtable.grpc-use-gzip-compression`, `-ingester.client.grpc-use-gzip-compression`, `-querier.frontend-client.grpc-use-gzip-compression` are now deprecated. #2940
 * [CHANGE] Limit errors reported by ingester during query-time now return HTTP status code 422. #2941
+* [FEATURE] Add support for azure storage of AzureChinaCloud,AzureGermanCloud,AzureUSGovernment. #2988


Please move the CHANGELOG entry to the top, under "master / unrelease". This is the section for 1.3.0 which has already been cut. Thanks!

pracucci · 2020-08-07T15:50:22Z

pkg/chunk/azure/blob_storage_client_test.go

+	"github.com/cortexproject/cortex/pkg/util/flagext"
+)
+
+func TestGetObject(t *testing.T) {


I tried to run these tests locally, but looks failing with the error illegal base64 data at input byte 8. Also I'm not sure how they're supposed to work, considering the backend storage is not mocked 🤔

Yes, I think it is difficult for me to mock azure storage, so I deleted the test file 😂

… feature/azure-storage-others-cloud

pracucci

Thanks @zbbkeepgoing for addressing my feedback. I left a last round of feedback and then we should be good to go. Thanks!

CHANGELOG.md

pkg/chunk/azure/blob_storage_client.go

pracucci · 2020-08-10T08:06:20Z

pkg/chunk/azure/blob_storage_client.go

+
+// Validate the config.
+func (c *BlobStorageConfig) Validate() error {
+	if c.Environment != azureGlobal && c.Environment != azureChinaCloud && c.Environment != azureGermanCloud && c.Environment == azureUSGovernment {


You can use supportedEnvironments here:

if !util.StringsContain(supportedEnvironments, c.Environment) {

pracucci · 2020-08-10T08:06:55Z

pkg/chunk/azure/blob_storage_client.go

+// Validate the config.
+func (c *BlobStorageConfig) Validate() error {
+	if c.Environment != azureGlobal && c.Environment != azureChinaCloud && c.Environment != azureGermanCloud && c.Environment == azureUSGovernment {
+		return fmt.Errorf("unsupported environment: %s, please select one of: %s ", c.Environment, strings.Join(supportedEnvironments, ","))


Suggested change

return fmt.Errorf("unsupported environment: %s, please select one of: %s ", c.Environment, strings.Join(supportedEnvironments, ","))

return fmt.Errorf("unsupported Azure blob storage environment: %s, please select one of: %s ", c.Environment, strings.Join(supportedEnvironments, ", "))

pracucci · 2020-08-10T08:08:00Z

pkg/chunk/azure/blob_storage_client.go

@@ -41,6 +69,7 @@ func (c *BlobStorageConfig) RegisterFlags(f *flag.FlagSet) {

 // RegisterFlagsWithPrefix adds the flags required to config this to the given FlagSet
 func (c *BlobStorageConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
+	f.StringVar(&c.Environment, prefix+"azure.environment", azureGlobal, fmt.Sprintf("Environment of Azure Cloud. This value can be %s.", strings.Join(supportedEnvironments, ",")))


Suggested change

f.StringVar(&c.Environment, prefix+"azure.environment", azureGlobal, fmt.Sprintf("Environment of Azure Cloud. This value can be %s.", strings.Join(supportedEnvironments, ",")))

f.StringVar(&c.Environment, prefix+"azure.environment", azureGlobal, fmt.Sprintf("Azure Cloud environment. Supported values are: %s.", strings.Join(supportedEnvironments, ", ")))

pracucci · 2020-08-10T08:08:25Z

pkg/chunk/azure/blob_storage_client.go

+)
+
+// Environment
+const (


Merge the two const please.

pracucci

LGTM (modulo fixing the linter issue), thanks! @pstibrany could you take a second look, please?

Signed-off-by: Binbin Zou <[email protected]>

pstibrany

LGTM, thank you! I've left two non-blocking comments.

pstibrany · 2020-08-10T13:51:13Z

CHANGELOG.md

@@ -1,7 +1,7 @@
 # Changelog

 ## master / unreleased
-
+* [ENHANCEMENT] Add support for azure storage of AzureChinaCloud,AzureGermanCloud,AzureUSGovernment. #2988


Suggested change

* [ENHANCEMENT] Add support for azure storage of AzureChinaCloud,AzureGermanCloud,AzureUSGovernment. #2988

* [ENHANCEMENT] Add support for azure storage in China, German and US Government environments. #2988

pstibrany · 2020-08-10T14:27:23Z

pkg/chunk/azure/blob_storage_client.go

+	//Blob service endpoint of AzureGlobal
+	globalBlobURLFmt      = "https://%s.blob.core.windows.net/%s/%s"
+	globalContainerURLFmt = "https://%s.blob.core.windows.net/%s"
+
+	// Blob service endpoint of AzureChinaCloud
+	chinaBlobURLFmt      = "https://%s.blob.core.chinacloudapi.cn/%s/%s"
+	chinaContainerURLFmt = "https://%s.blob.core.chinacloudapi.cn/%s"
+
+	// Blob service endpoint of AzureGermanCloud
+	germanyBlobURLFmt      = "https://%s.blob.core.cloudapi.de/%s/%s"
+	germanyContainerURLFmt = "https://%s.blob.core.cloudapi.de/%s"
+
+	// Blob service endpoint of AzureUSGovernment
+	govBlobURLFmt      = "https://%s.blob.core.usgovcloudapi.net/%s/%s"
+	govContainerURLFmt = "https://%s.blob.core.usgovcloudapi.net/%s"


I'm trying to verify these endpoints, but cannot find authoritative summary documentation. Do you have any relevant link?

Would it make sense to use something like:

var endpoints = map[string]struct{blobURL, containerURL string} { azureGlobal: { blobURL: "https://%s.blob.core.windows.net/%s/%s", containerURL: "https://%s.blob.core.windows.net/%s", }, azureChinaCloud: { blobURL: "https://%s.blob.core.chinacloudapi.cn/%s/%s", containerURL: "https://%s.blob.core.chinacloudapi.cn/%s", }, // ... }

and then use the map instead of selectBlobURLFmt and selectContainerURLFmt?

Please refer to the following document：

China: https://docs.microsoft.com/en-us/azure/china/resources-developer-guide
Germany: https://docs.microsoft.com/ca-es/azure////germany/germany-services-storage
US Government: https://docs.microsoft.com/en-us/azure/azure-government/compare-azure-government-global-azure

I'm trying to verify these endpoints, but cannot find authoritative summary documentation. Do you have any relevant link?

Would it make sense to use something like:

var endpoints = map[string]struct{blobURL, containerURL string} { azureGlobal: { blobURL: "https://%s.blob.core.windows.net/%s/%s", containerURL: "https://%s.blob.core.windows.net/%s", }, azureChinaCloud: { blobURL: "https://%s.blob.core.chinacloudapi.cn/%s/%s", containerURL: "https://%s.blob.core.chinacloudapi.cn/%s", }, // ... }

and then use the map instead of selectBlobURLFmt and selectContainerURLFmt?

done

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

pstibrany

Thank you!

Support azure storage of azurechina,azuregerman,azureusgov

77566bb

Signed-off-by: Binbin Zou <[email protected]>

pull-request-size bot added the size/L label Aug 6, 2020

change pr number

6c2ea12

Signed-off-by: Binbin Zou <[email protected]>

zbbkeepgoing force-pushed the feature/azure-storage-others-cloud branch from 7350dd6 to 6c2ea12 Compare August 6, 2020 13:02

pracucci suggested changes Aug 7, 2020

View reviewed changes

Merge branch 'master' of https://github.com/cortexproject/cortex into…

2e854f0

… feature/azure-storage-others-cloud

pull-request-size bot added size/M and removed size/L labels Aug 8, 2020

pracucci reviewed Aug 10, 2020

View reviewed changes

zbbkeepgoing force-pushed the feature/azure-storage-others-cloud branch from f4fd5b9 to 492095a Compare August 10, 2020 09:37

pracucci approved these changes Aug 10, 2020

View reviewed changes

Optimize the code

c5d97d6

Signed-off-by: Binbin Zou <[email protected]>

zbbkeepgoing force-pushed the feature/azure-storage-others-cloud branch from 492095a to c5d97d6 Compare August 10, 2020 10:16

Conflict resolution

ceceb6a

Signed-off-by: Binbin Zou <[email protected]>

pstibrany approved these changes Aug 10, 2020

View reviewed changes

Optimize the code

52dd841

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

pstibrany approved these changes Aug 11, 2020

View reviewed changes

pstibrany merged commit 43cef5a into cortexproject:master Aug 11, 2020

		const blobURLFmt = "https://%s.blob.core.windows.net/%s/%s"
		const containerURLFmt = "https://%s.blob.core.windows.net/%s"

	return fmt.Errorf("unsupported environment: %s, please select one of: %s ", c.Environment, strings.Join(supportedEnvironments, ","))
	return fmt.Errorf("unsupported Azure blob storage environment: %s, please select one of: %s ", c.Environment, strings.Join(supportedEnvironments, ", "))

	f.StringVar(&c.Environment, prefix+"azure.environment", azureGlobal, fmt.Sprintf("Environment of Azure Cloud. This value can be %s.", strings.Join(supportedEnvironments, ",")))
	f.StringVar(&c.Environment, prefix+"azure.environment", azureGlobal, fmt.Sprintf("Azure Cloud environment. Supported values are: %s.", strings.Join(supportedEnvironments, ", ")))

	* [ENHANCEMENT] Add support for azure storage of AzureChinaCloud,AzureGermanCloud,AzureUSGovernment. #2988
	* [ENHANCEMENT] Add support for azure storage in China, German and US Government environments. #2988

Support azure storage of azurechina,azuregerman,azureusgov #2988

Support azure storage of azurechina,azuregerman,azureusgov #2988

Uh oh!

Conversation

zbbkeepgoing commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zbbkeepgoing commented Aug 7, 2020

Uh oh!

zbbkeepgoing commented Aug 7, 2020

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

pstibrany Aug 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

zbbkeepgoing commented Aug 6, 2020 •

edited

Loading

pracucci left a comment •

edited

Loading

pstibrany Aug 10, 2020 •

edited

Loading