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

Use token-based authentication instead of the access key in the AzureFileShareService #779

Merged
merged 24 commits into from
Jul 24, 2024

Conversation

motus
Copy link
Member

@motus motus commented Jul 9, 2024

Make AzureFileShareService class use token-based credential instead of the access key. This PR is part of #777

NOTE:

  • We have to use late initialization of the _share_client to avoid issues with JSON schema validation tests.
  • More PRs will follow:
    • Use to azcopy on the remote VMs instead of mounting the file share using the access key
    • Remove references to "storageAccountKey" from configurations and tests and document the new mechanism

@motus motus requested a review from a team as a code owner July 9, 2024 00:57
@motus motus self-assigned this Jul 9, 2024
@motus motus added ready for review Ready for review mlos-bench labels Jul 9, 2024
@bpkroth
Copy link
Contributor

bpkroth commented Jul 22, 2024

Part of why we went with the mount smb method was reduced dependencies on the target VM.

Use of azcopy instead might be a little more challenging on that front, but let's see.

@bpkroth
Copy link
Contributor

bpkroth commented Jul 22, 2024

As long as we're removing account storage key, does removing fileshare also make sense? We could use blobs instead I guess. Maybe less nice though and too many changes.

@bpkroth bpkroth changed the title Use token-based authentication instead of the access key in the AzureFlieShareService Use token-based authentication instead of the access key in the AzureFileShareService Jul 23, 2024
Copy link
Contributor

@bpkroth bpkroth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly looks good. Only feedback was to move the SupportsAuth check to __init__ so we can check for proper configs at startup time rather than at file transfer need time which might be after some experiment trial has already run.

@motus motus enabled auto-merge (squash) July 23, 2024 22:23
@motus motus disabled auto-merge July 23, 2024 22:29
@motus motus force-pushed the sergiym/svc/fs_token_auth branch from 35e77b4 to 827492f Compare July 23, 2024 23:58
@motus motus merged commit 73f0708 into microsoft:main Jul 24, 2024
12 checks passed
@motus motus deleted the sergiym/svc/fs_token_auth branch August 1, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants