Azure: Add support for Gov Cloud #7664

N-o-Z · 2024-04-11T19:31:12Z

Change Description

Background

Add support for Azure users using the Gov Cloud endpoint

New Feature

Add support for additional Azure cloud endpoints in a more modular way

Testing Details

Added tests for the namespace and endpoint validations
Very hard to actually test usage as we have no access to the gov cloud endpoint

Breaking Change?

No - but we have depracated the China cloud configuration flag

github-actions · 2024-04-11T19:37:46Z

E2E Test Results - DynamoDB Local - Local Block Adapter

itaiad200

Overall looks great. Couple of things that prevents me from approving:

Failing esti tests. They seem related to this change.
Missing docs update for this change.
I believe @arielshaqed opposed to passing a domain. I was on the same side too, but this PR convinced me otherwise.

itaiad200 · 2024-04-11T19:37:24Z

pkg/config/config.go

-			ChinaCloud         bool          `mapstructure:"china_cloud"`
-			// TestEndpointURL for testing purposes
+			// Deprecated: Value ignored
+			ChinaCloud      bool   `mapstructure:"china_cloud"`


Suggested change

ChinaCloud bool `mapstructure:"china_cloud"`

DeprecatedChinaCloud bool `mapstructure:"china_cloud"`

itaiad200 · 2024-04-11T19:38:13Z

pkg/config/config.go

@@ -509,17 +511,17 @@ func (c *Config) BlockstoreAzureParams() (blockparams.Azure, error) {
 		logging.ContextUnavailable().Warn("blockstore.azure.auth_method is deprecated. Value is no longer used.")
 	}
 	if c.Blockstore.Azure.ChinaCloud {
-		logging.ContextUnavailable().Warn("blockstore.azure.china_cloud is enabled. lakeFS will only function on Azure China Cloud")
+		logging.ContextUnavailable().Warn("blockstore.azure.china_cloud is deprecated. Value is no longer used.")


Shouldn't we exit? If someone is using this, by design, lakeFS won't work for him.

I think we should actually do something else so it wouldn't be a breaking change - fixing

itaiad200 · 2024-04-11T19:44:25Z

pkg/block/azure/adapter.go

+var (
+	ErrInvalidDomain = errors.New("invalid Azure Domain")
+
+	endpointRegex      = regexp.MustCompile(`https://(?P<account>[\w]+).(?P<domain>[\w.-]+)[/:][\w-/]*$`)


Now you have 2 problems.....

Not sure I understand your comment

https://blog.codinghorror.com/regular-expressions-now-you-have-two-problems/

Sometimes you just have to do it :)

itaiad200 · 2024-04-11T19:46:48Z

pkg/block/azure/adapter.go

+	if params.Domain == "" {
+		params.Domain = BlobEndpointDefaultDomain
+	} else {
+		domain := strings.TrimSuffix(params.Domain, "/")


Not blocking, but I think we should be more strict here.. Accepting things like blob.core.usgovcloudapi.net/ is weird

I can go either way, removing

itaiad200 · 2024-04-11T19:47:49Z

pkg/block/azure/adapter.go

@@ -605,3 +631,20 @@ func (a *Adapter) GetPresignUploadPartURL(_ context.Context, _ block.ObjectPoint
 func (a *Adapter) ListParts(_ context.Context, _ block.ObjectPointer, _ string, _ block.ListPartsOpts) (*block.ListPartsResponse, error) {
 	return nil, block.ErrOperationNotSupported
 }
+
+func ParseURL(raw string) (string, string, error) {


Let's add documentation and named args.. (string, string, error) is not very indicative.

github-actions · 2024-04-11T21:31:03Z

♻️ PR Preview 1b229b3 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

N-o-Z · 2024-04-11T21:37:33Z

Overall looks great. Couple of things that prevents me from approving:

Failing esti tests. They seem related to this change.

Missing docs update for this change.

I believe @arielshaqed opposed to passing a domain. I was on the same side too, but this PR convinced me otherwise.

Small hiccup with how we build the pre-signed urls. Should be fixed now

itaiad200 · 2024-04-12T09:58:08Z

docs/reference/configuration.md

+* ~~`blockstore.azure.china_cloud` `(bool : false)`~~ - Enable for using lakeFS on Azure China Cloud.  
+  **Note:** Deprecated - In favor of `blockstore.azure.domain` 
+  {: .note }
+* `blockstore.azure.domain` `(string : blob.core.windows.net)` - Enables support of different Azure cloud domains. Current supported domains: [`blob.core.chinacloudapi.cn`, `blob.core.usgovcloudapi.net`]


You missed the default one

Also - some are beta (untested, might be broken in future versions).
Let's be explicit about what's 'first tier' (blob.core.windows.net) and what are 'beta'.

The default is the default, no need to add it to the supported domains.
Added a remark that supported domains are in Beta stage (Though this was not mentioned explicitly on the China flag)

itaiad200 · 2024-04-13T12:37:41Z

pkg/block/azure/adapter.go

+var (
+	ErrInvalidDomain = errors.New("invalid Azure Domain")
+
+	endpointRegex      = regexp.MustCompile(`https://(?P<account>[\w]+).(?P<domain>[\w.-]+)[/:][\w-/]*$`)


https://blog.codinghorror.com/regular-expressions-now-you-have-two-problems/

itaiad200 · 2024-04-13T12:40:36Z

pkg/config/config.go

+	if c.Blockstore.Azure.ChinaCloudDeprecated {
+		logging.ContextUnavailable().Warn("blockstore.azure.china_cloud is deprecated. Value is no longer used. Please pass Domain = 'blob.core.chinacloudapi.cn'")
+		c.Blockstore.Azure.Domain = "blob.core.chinacloudapi.cn"


I wouldn't warn here, I would panic. We're allowed to drop support for beta features.
It was released only recently, it's easier to drop support now instead of later.

I don't want to break backward compatibility to current users. We didn't advertise this as a Beta feature, I don't see this becoming a bigger problem in the future

itaiad200 · 2024-04-13T12:43:08Z

pkg/block/azure/client_cache.go

 	} else {
-		url = buildAccountEndpoint(params.StorageAccount, params.ChinaCloud)
+		domain := strings.TrimSuffix(params.Domain, "/")


Again with the suffix? 🙈

* Azure: Add support for Gov Cloud * Small fix * CR Fixes * Fix lint * CR Fixes 2 * Add TODO * Fix upload * Fix upload * Fix upload * Fix upload

Azure: Add support for Gov Cloud

e4f4876

N-o-Z added area/block-adapter azure Issues regarding azure block adapter and support labels Apr 11, 2024

N-o-Z requested a review from itaiad200 April 11, 2024 19:31

N-o-Z self-assigned this Apr 11, 2024

N-o-Z added the include-changelog PR description should be included in next release changelog label Apr 11, 2024

itaiad200 requested changes Apr 11, 2024

View reviewed changes

N-o-Z added 2 commits April 11, 2024 23:41

Small fix

6c2adcf

CR Fixes

8f01f41

Fix lint

49c98d2

N-o-Z requested a review from itaiad200 April 11, 2024 21:39

itaiad200 approved these changes Apr 13, 2024

View reviewed changes

N-o-Z added 6 commits April 13, 2024 21:23

CR Fixes 2

1daf6ce

Add TODO

88e43f6

Fix upload

46787d3

Fix upload

78ddb8b

Fix upload

600248e

Fix upload

1b229b3

N-o-Z enabled auto-merge (squash) April 13, 2024 20:34

N-o-Z merged commit f75304c into master Apr 14, 2024
36 checks passed

N-o-Z deleted the task/support-azure-gov-cloud-7662 branch April 14, 2024 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Azure: Add support for Gov Cloud #7664

Azure: Add support for Gov Cloud #7664

N-o-Z commented Apr 11, 2024

github-actions bot commented Apr 11, 2024

itaiad200 left a comment

itaiad200 Apr 11, 2024

itaiad200 Apr 11, 2024

N-o-Z Apr 11, 2024

itaiad200 Apr 11, 2024

N-o-Z Apr 11, 2024

itaiad200 Apr 13, 2024

N-o-Z Apr 13, 2024

itaiad200 Apr 11, 2024

N-o-Z Apr 11, 2024

itaiad200 Apr 11, 2024

github-actions bot commented Apr 11, 2024 •

edited

Loading

N-o-Z commented Apr 11, 2024

itaiad200 Apr 12, 2024

itaiad200 Apr 13, 2024

N-o-Z Apr 13, 2024

itaiad200 Apr 13, 2024

itaiad200 Apr 13, 2024

N-o-Z Apr 13, 2024

itaiad200 Apr 13, 2024

	ChinaCloud bool `mapstructure:"china_cloud"`
	DeprecatedChinaCloud bool `mapstructure:"china_cloud"`

Azure: Add support for Gov Cloud #7664

Azure: Add support for Gov Cloud #7664

Conversation

N-o-Z commented Apr 11, 2024

Change Description

Background

New Feature

Testing Details

Breaking Change?

github-actions bot commented Apr 11, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

itaiad200 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

github-actions bot commented Apr 11, 2024 • edited Loading

N-o-Z commented Apr 11, 2024

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

github-actions bot commented Apr 11, 2024 •

edited

Loading