Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

backend/azureblob/imds hybrid imds support #6132

Closed
ian-bowler opened this issue Apr 27, 2022 · 18 comments
Closed

backend/azureblob/imds hybrid imds support #6132

ian-bowler opened this issue Apr 27, 2022 · 18 comments

Comments

@ian-bowler
Copy link

The associated forum post URL from https://forum.rclone.org

https://forum.rclone.org/t/azure-blob-backend-azure-arc-managed-identity-support-himds/30441

What is your current rclone version (output from rclone version)?

rclone v1.58.0

  • os/version: debian 11.3 (64 bit)
  • os/kernel: 5.10.0-11-amd64 (x86_64)
  • os/type: linux
  • os/arch: amd64
  • go/version: go1.17.8
  • go/linking: static
  • go/tags: none

What problem are you are trying to solve?

Use a different endpoint for IMDS

How do you think rclone should be changed to solve that?

Add config level endpoint specification

OR

Read optional endpoint from environment variable

How to use GitHub

  • Please use the 👍 reaction to show that you are affected by the same issue.
  • Please don't comment if you have no relevant information to add. It's just extra noise for everyone subscribed to this issue.
  • Subscribe to receive notifications on status change and new comments.
@ian-bowler
Copy link
Author

ian-bowler commented Apr 27, 2022

It appears that Microsoft has modified the oauthTokenManager.go code which imds.go is based on to support Arc, however it is not as simple as changing the endpoint as the token is saved into a local file and must be read from that file.
https://github.com/Azure/azure-storage-azcopy/blob/main/common/oauthTokenManager.go#L825

@ncw
Copy link
Member

ncw commented Apr 28, 2022

It looks like it might be public enough so we could use the code from https://github.com/Azure/azure-storage-azcopy/blob/main/common/oauthTokenManager.go directly which would make maintenance easier.

@backerman since you wrote this code originally in #3213 I wonder if you have any thoughts about this?

@ian-bowler
Copy link
Author

ian-bowler commented Apr 28, 2022

I have made my own merge of the oauthTokenManager code into imds.go and will use that version locally until it can be integrated/ properly tested. I have attached my imds.go as a txt. I haven't made any changes to make the error messages relevant to rclone instead of azcopy
imds.txt

@backerman
Copy link
Contributor

@ncw I'll take a look.

@backerman
Copy link
Contributor

I'm going to try to get this done in the next week or so. A lot of my previous patch to enable MSI was working around the storage SDK not being compatible with the azidentity package in the main SDK; there's now github.com/Azure/azure-sdk-for-go/sdk/storage/azblob so I'll port the Azure code to that. It does support Arc (and also browser authentication which is definitely nice to have).

@backerman
Copy link
Contributor

That new SDK requires Go 1.18, and rclone currently requires 1.16 - will it be okay to bump the version?

@ncw
Copy link
Member

ncw commented Jul 5, 2022

@backerman what we normally do here is use conditional compilation on go version to only include the new feature in go1.18 compilations. When go1.18 becomes the oldest supported version we remove the conditional complilation.

This would typically involve one file with a // +build go1.18 header with the implementation and another with // +build !go1.18 with a shim implementation.

Though in this case we are talking about the whole azureblob backend aren't we? That's fine too - non go1.18 compilations will just be missing the azureblob backend.

So you'd just add a build constraint here

//go:build !plan9 && !solaris && !js
// +build !plan9,!solaris,!js

Something like

//go:build !plan9 && !solaris && !js && go1.18
// +build !plan9,!solaris,!js,go1.18

Though we should probably check to see on which archs the new SDK does actually compile!

@ian-bowler
Copy link
Author

@backerman is there a branch with your changes to identity that I can build locally/use until it is released?

@backerman
Copy link
Contributor

backerman commented Jul 31, 2022

@ian-bowler I did some of this work today and expect to have something at least minimally usable tomorrow. I'll let you know when I've got a branch that compiles.

Edit: nope, not today. Still a dozen references to fix and then I can do some basic tests to make sure it doesn't panic all over the place with all these pointers in the new SDK.

@backerman
Copy link
Contributor

backerman commented Aug 7, 2022

@ian-bowler I've got something that, er, compiles in my fork. I'm now going through the integration test failures to make it actually work.

@backerman
Copy link
Contributor

Update: down to a much smaller number of integration test failures.

@ncw
Copy link
Member

ncw commented Aug 9, 2022

@backerman nice one :-) Let me know if you need help.

Try not to break -vv --dump headers which involves using rclone's http Client or Transport - it looks like you might have broken that in a quick skim of the code, but I wasn't sure! There isn't a test for it, which there probably should be.

@bowleri
Copy link

bowleri commented Sep 1, 2022

@backerman I was able to build and test your fork with success even a windows build. Looking forward to this becoming mainline at some point

@ncw
Copy link
Member

ncw commented Nov 23, 2022

@backerman I've been doing this port myself - I had completely forgotten you'd started already so I started from scratch - sorry :-(

I've found 5 relevant bugs (I reported 3 of them) in the SDK which are the reason the integration tests aren't 100%

  1. azblob UploadStream produces panic: send on closed channel if input stream has error Azure/azure-sdk-for-go#19612
  2. azblob: when using SharedKey credentials, can't reference some blob names with ? in Azure/azure-sdk-for-go#19613
  3. Azure Blob Storage paths are not URL-escaped Azure/azure-sdk-for-go#19475
  4. Controlling TransferManager in azblob 0.5.1 Azure/azure-sdk-for-go#19579
  5. azblob: blob.StartCopyFromURL doesn't work with UTF-8 characters in the source blob Azure/azure-sdk-for-go#19614

I've managed to work around most of them but there are still a couple of failures from 2) (? in blob name) and 3) (% in blob name). I had to take ? out of the test suite because of 2) so that will need fixing and 1) is a panic waiting to happen so not ideal. It would be nice if 4) was fixed - that would mean we could fix 1) by installing our own TransferManager.

I'm going to re-work the auth next which should include fixing this issue.

When I've done that I'll post a beta.

@ncw
Copy link
Member

ncw commented Nov 29, 2022

@backerman I've pulled the auth port you did out of your tree and added to mine as separate commit - that saved me loads of time - thank you! Hope that is OK.

ncw added a commit that referenced this issue Nov 30, 2022
This commit switches from using the old Azure go modules

    github.com/Azure/azure-pipeline-go/pipeline
    github.com/Azure/azure-storage-blob-go/azblob
    github.com/Azure/go-autorest/autorest/adal

To the new SDK

    github.com/Azure/azure-sdk-for-go/

This stops rclone using deprecated code and enables the full range of
authentication with Azure.

See #6132 and #5284
ncw added a commit that referenced this issue Dec 4, 2022
This commit switches from using the old Azure go modules

    github.com/Azure/azure-pipeline-go/pipeline
    github.com/Azure/azure-storage-blob-go/azblob
    github.com/Azure/go-autorest/autorest/adal

To the new SDK

    github.com/Azure/azure-sdk-for-go/

This stops rclone using deprecated code and enables the full range of
authentication with Azure.

See #6132 and #5284
ncw added a commit that referenced this issue Dec 6, 2022
This commit switches from using the old Azure go modules

    github.com/Azure/azure-pipeline-go/pipeline
    github.com/Azure/azure-storage-blob-go/azblob
    github.com/Azure/go-autorest/autorest/adal

To the new SDK

    github.com/Azure/azure-sdk-for-go/

This stops rclone using deprecated code and enables the full range of
authentication with Azure.

See #6132 and #5284
@ncw
Copy link
Member

ncw commented Dec 6, 2022

I've merged this work for v1.61.

I took the opportunity to rework the auth to make it a bit easier to configure but it is backwards compatible.

Thank you @backerman for you work here - I used your work as a basis for this commit f746b2f

I'd appreciate testing of the latest beta which contains the code. You can see the docs for the auth here: https://tip.rclone.org/azureblob/#authenticating-with-azure-blob-storage

@ncw ncw added this to the v1.61 milestone Dec 6, 2022
@ian-bowler
Copy link
Author

@ncw I have confirmed that the latest beta works with my AzureArc authentication scheme on local machines outside azure.

Thank you @ncw and @backerman for the work to properly update Azure Auth integration, looking forward to switching from my custom branch back to the main release

@ncw
Copy link
Member

ncw commented Dec 6, 2022

Thanks for confirming it works @ian-bowler

I'll close this issue now

@ncw ncw closed this as completed Dec 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants