Support ADLS Gen2 #5027

N-o-Z · 2023-01-12T04:16:31Z

# Conflicts: # go.mod # go.sum

# Conflicts: # go.mod # pkg/block/azure/adapter.go # pkg/block/factory/build.go # pkg/ingest/store/factory.go

# Conflicts: # esti/import_test.go

N-o-Z · 2023-01-14T20:29:28Z

PR ready to review.
Added an Esti Azure with HNS flavor job to the pipeline which runs our integration tests with ADLS Gen2.

@ozkatz adding you to reviewers as I had to make some changes to the pre signed URL code

guy-har

Great addition, the new SDK seems much better than the previous one. Moving to the new SDK should even solve more issues other than support ADLS Gen2. Thanks!
Added some comments

guy-har · 2023-01-15T07:47:10Z

pkg/block/factory/build.go

-	case azure.AuthMethodMSI:
-		credentials, err = azure.GetMSICredentials()
-	default:
-		err = ErrAuthMethodNotSupported


It seems like we aren't supporting MSI anymore, not sure it's used, but this will be a breaking change. Can we still support it?
Another thing, that shouldn't be part of this PR. IIRC the new SDK has a way to connect and take the configurations from the running environment which was once requested, maybe we should open an issue for that and fix it as well.

You are right - I originally added the code which checks for default credentials (CLI, ENV etc..)
Re-added it - let me know if it solves the issue

pkg/block/factory/build.go

guy-har · 2023-01-15T08:07:16Z

pkg/block/azure/chunkwriting.go

+		ctx:     ctx,
+		cancel:  cancel,
+		reader:  from,
+		to:      to,
+		id:      newID(),
+		o:       o,
+		errCh:   make(chan error, 1),
+		buffers: buffers,


General message on this file, previously it was copy of the chunkwriting file from the SDK with small modifications, that way if there is a significant change in a new version applying it would be straightforward. I noticed that the implementation in the new Azure SDK is different. As it looks now we have a copy of the old implementation with our modifications, is there a way to adjust somehow the implementation from the new SDK?

The new implementation is using a tracker to track the parts which we are already doing in a different way to support multiple requests. I did use the new mmbPool struct they are using and modified the old code to work similarly to how they do it. I found it difficult to align to their implementation but if you have any suggestions - I would love to hear

guy-har · 2023-01-15T08:09:43Z

pkg/block/azure/credentials.go

-func GetAccessKeyCredentials(accountName, accountKey string) (azblob.Credential, error) {
-	if len(accountName) == 0 && len(accountKey) == 0 {
-		// fallback to Azure environment variables
-		accountName, accountKey = os.Getenv("AZURE_STORAGE_ACCOUNT"), os.Getenv("AZURE_STORAGE_ACCESS_KEY")
-	}
-	return azblob.NewSharedKeyCredential(accountName, accountKey)
-}


IIUC we are losing the support of taking the AZURE configurations... this might be a breaking change, can we keep this support. (Also here I think that the new way of generating an azure client might solve this as well)

guy-har · 2023-01-15T08:25:09Z

pkg/block/azure/multipart_block_writer.go

+func (m *MultipartBlockWriter) Upload(_ context.Context, _ io.ReadSeekCloser, _ *blockblob.UploadOptions) (blockblob.UploadResponse, error) {
+	panic("Should not be called")
+}


Are we sure we want to panic here? would logging and returning and error be enough?

Why not support it?

We needed to implement the interface - but we don't really need to use it.
I'm not sure what is the right implementation as I do not know how this will be used.

pkg/ingest/store/factory.go

pkg/ingest/store/azure.go

guy-har · 2023-01-15T10:25:46Z

pkg/block/azure/adapter.go

-
-	// TODO(Guys): remove this work around once azure fixes panic issue and use azblob.UploadStreamToBlockBlob
-	transferManager, err := azblob.NewStaticBuffer(_1MiB, MaxBuffers)
-	if err != nil {
-		return err
-	}
-	uploadOpts := a.translatePutOpts(ctx, opts)
-	uploadOpts.TransferManager = transferManager
-	defer transferManager.Close()
-	resp, err := copyFromReader(ctx, reader, blobURL, uploadOpts)
-	if err != nil {
-		return err
-	}
-	_ = resp == nil // this is done in order to ignore "result 0 is never used" error ( copyFromReader is copied from azure, and we want to keep it with minimum changes)
-	return nil


guy-har · 2023-01-15T10:33:45Z

pkg/block/azure/adapter.go

+	qp, err := client.GetAccountSASURL(aztables.AccountSASResourceTypes{
+		Container: true,
+		Object:    true,
+	}, permissions, time.Now(), a.preSignedURLDurationGenerator())


Not sure we should use time.Now() it seems like we pass a time that "IsZero" it isn't specified, which is what I believe we want in this case.

pkg/block/azure/adapter.go

.github/workflows/esti.yaml

pkg/block/azure/adapter.go

pkg/block/factory/build.go

pkg/ingest/store/factory.go

nopcoder · 2023-01-15T12:06:36Z

pkg/ingest/store/factory.go

 		if err != nil {
 			return nil, err
 		}
 	} else {
 		var err error
-		p, err = getAzureClient()
+		c, err = getAzureClient()
 		if err != nil {
 			return nil, err
 		}


can extract out the if err != nil block

pkg/block/azure/adapter.go

pkg/block/factory/build.go

guy-har · 2023-01-15T15:21:36Z

pkg/block/factory/build.go

+func BuildAzureServiceClient(params params.Azure) (*service.Client, error) {
+	url := fmt.Sprintf(azure.AzURLTemplate, params.StorageAccount)
+	options := service.ClientOptions{ClientOptions: azcore.ClientOptions{Retry: policy.RetryOptions{TryTimeout: params.TryTimeout}}}
+	if params.StorageAccessKey != "" {


We should change the documentation to reflect this

Opened #5044

nopcoder

LGTM! nice upgrade from the previous SDK.

nopcoder · 2023-01-15T14:37:26Z

pkg/block/azure/adapter.go


-	preSignedBlobPattern = "https://%s.blob.core.windows.net/%s/%s?%s"
+	AzURLTemplate = "https://%s.blob.core.windows.net/"


already in azure package - so maybe just URLTemplate? or something without the Az prefix?

nopcoder · 2023-01-15T19:58:12Z

pkg/block/azure/adapter.go

+
+	// TODO (niro): copy is limited to 256MB, should we handle it somehow?
+	_, err = destClient.CopyFromURL(ctx, sasKey, nil)
+	return err


So maybe we should document this limitation as we do not support a working copy for large objects.

guy-har and others added 2 commits January 11, 2023 15:43

Dont expect it to work

11abdac

Nir's changes

e9272b5

N-o-Z added the team/versioning-engine Team versioning engine label Jan 12, 2023

N-o-Z assigned N-o-Z and guy-har Jan 12, 2023

N-o-Z marked this pull request as ready for review January 12, 2023 04:16

N-o-Z added 3 commits January 12, 2023 06:18

Merge remote-tracking branch 'origin/master' into azure-playground

2d1f274

# Conflicts: # go.mod # go.sum

Nir's changes

dc4bbf2

Add esti flavor for ADLS fix import test

6ce89e6

N-o-Z added the include-changelog PR description should be included in next release changelog label Jan 12, 2023

fix esti job

3a5c213

N-o-Z force-pushed the azure-playground branch from 0cf99e2 to 3a5c213 Compare January 12, 2023 09:41

N-o-Z and others added 11 commits January 12, 2023 12:10

fix Walk

8418c18

Fix multipart upload

f0cd03f

Fix walker

5485ab2

Revert changes in auth

a29f856

Fix buffer release race

a272fb8

Merge remote-tracking branch 'origin/master' into azure-playground

8ad494c

# Conflicts: # go.mod # pkg/block/azure/adapter.go # pkg/block/factory/build.go # pkg/ingest/store/factory.go

Merge fixes

28a0706

Merge fixes

cad6c78

Fix Copy

c34fde6

Merge branch 'master' into azure-playground

3337caf

# Conflicts: # esti/import_test.go

Final touches

6327232

N-o-Z requested review from ozkatz, guy-har and a team January 14, 2023 20:27

N-o-Z changed the title ~~Azure playground - test new SDK~~ Support ADLS Gen2 Jan 14, 2023

Fix proto

131750a

guy-har requested changes Jan 15, 2023

View reviewed changes

N-o-Z added 2 commits January 15, 2023 13:15

CR Fixes

3ced647

Add default credentials in factory

fdc0df2

nopcoder suggested changes Jan 15, 2023

View reviewed changes

N-o-Z added 2 commits January 15, 2023 14:40

Add default credentials in factory and fix presigned URL

4b69172

More fixes

5497bcd

N-o-Z requested review from nopcoder and guy-har January 15, 2023 13:34

N-o-Z mentioned this pull request Jan 15, 2023

Improvements in azure chunkwriting #5041

Closed

N-o-Z added 4 commits January 15, 2023 16:26

More fixes

bd89ac7

More fixes

58e1c8b

dumb down MMB

0de7322

Rename AzURLTemplate

890d32b

guy-har approved these changes Jan 15, 2023

View reviewed changes

Fix lint

7cd1c53

N-o-Z enabled auto-merge (squash) January 15, 2023 15:50

Remove tag from blob write permissions

933cd54

nopcoder approved these changes Jan 15, 2023

View reviewed changes

N-o-Z merged commit 7f53849 into master Jan 15, 2023

N-o-Z deleted the azure-playground branch January 15, 2023 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ADLS Gen2 #5027

Support ADLS Gen2 #5027

N-o-Z commented Jan 12, 2023 •

edited

Loading

N-o-Z commented Jan 14, 2023

guy-har left a comment

guy-har Jan 15, 2023

N-o-Z Jan 15, 2023

guy-har Jan 15, 2023

N-o-Z Jan 15, 2023

guy-har Jan 15, 2023

N-o-Z Jan 15, 2023

guy-har Jan 15, 2023

guy-har Jan 15, 2023

N-o-Z Jan 15, 2023

guy-har Jan 15, 2023

guy-har Jan 15, 2023

nopcoder Jan 15, 2023

guy-har Jan 15, 2023

N-o-Z Jan 15, 2023

nopcoder left a comment

nopcoder Jan 15, 2023

nopcoder Jan 15, 2023


		preSignedBlobPattern = "https://%s.blob.core.windows.net/%s/%s?%s"
		AzURLTemplate = "https://%s.blob.core.windows.net/"

Support ADLS Gen2 #5027

Support ADLS Gen2 #5027

Conversation

N-o-Z commented Jan 12, 2023 • edited Loading

N-o-Z commented Jan 14, 2023

guy-har 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopcoder left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z commented Jan 12, 2023 •

edited

Loading