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

feature: support azure MangedIdentity TokenCredential #590

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

Meir017
Copy link
Contributor

@Meir017 Meir017 commented Aug 14, 2024

closes #558

Tests:

  • test managed identity in azure
  • Unit tests

@Meir017 Meir017 marked this pull request as ready for review August 14, 2024 10:08
@badrishc
Copy link
Contributor

How does this feature work exactly? Is it compatible with the older connection strings? And how does it interface with unit tests?

@Meir017
Copy link
Contributor Author

Meir017 commented Aug 15, 2024

How does this feature work exactly?

Assuming the garnet application will run inside of azure (ex: vm / aks) and managed identity will be able to be assigned to the environment

Is it compatible with the older connection strings?

I think this us instead of using a connection string, so any Metadata provided in the connection string will need to be provided in a different way

And how does it interface with unit tests?

We might consider using DefaultAzureCredential https://learn.microsoft.com/en-us/dotnet/api/azure.identity.defaultazurecredential?view=azure-dotnet then for unit tests we could run AZ login in order to populate the AzureCli credential layer

Currently the unit tests don't use the managed identity credential to perform the login

{
Assert.Ignore("Azure tests are disabled.");
}

var AzureTestDirectory = $"{TestContext.CurrentContext.Test.MethodName.ToLowerInvariant()}";
var configPath = $"{AzureTestDirectory}/test1.config";
var AzureEmulatedStorageString = "UseDevelopmentStorage=true;";
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a unit test that uses the new version of identity.

Copy link
Contributor

@badrishc badrishc Sep 2, 2024

Choose a reason for hiding this comment

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

Any progress on this? We would need it to be confident that the feature works (now, and in future as the codebase evolves).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hoping to get to this by the end of this week

Copy link
Contributor

Choose a reason for hiding this comment

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

Ping on this, thank you!

Copy link
Contributor

Choose a reason for hiding this comment

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

Any update on this? Feel free to close and re-open when you have made progress on it, thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@badrishc I'm not sure how to test this locally, I got garnet to run but I'm not sure how to force garnet to save into the blob storage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I got garnet to interact with the azure blob storage I created.
image

@Meir017
Copy link
Contributor Author

Meir017 commented Oct 20, 2024

Somthing that might be concerning, I noticed that the constructor of public AzureStorageNamedDeviceFactory(string serviceUri, TokenCredential credential, ILogger logger = null) is being called multiple times with the exact same argument when starting the server

running locally with

.\bin\Debug\net8.0\GarnetServer.exe "--use-azure-storage" "--storage-managed-identity" "fake-managed-identity" "--storage-service-uri" "https://garnettest.blob.core.windows.net" "--storage-tier" "--logdir" "garnettestcontainer"

called the constructor 5 times, this causes the time until the service prints * Ready to accept connections be to ~20s

created #739

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow using managed identity when using Azure Page Blobs
2 participants