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

[Admin] Table Storage Connection Setting #299

Merged

Conversation

sikutisa
Copy link
Contributor

@sikutisa sikutisa commented Sep 3, 2024


Description

Discussion

  • ContainerName
  • ConnectionString
    • public TableServiceClient (string connectionString); 생성자를 사용했는데, 다른 생성자를 사용하는 것이 적절한지에 대해 논의할 필요가 있을 것 같습니다.

Reference

https://learn.microsoft.com/en-us/connectors/azuretables/#connect-to-azure-table-storage-connector-using-table-endpoint
https://learn.microsoft.com/en-us/azure/storage/common/storage-configure-connection-string

@justinyoo
Copy link
Contributor

bicep 리뷰 코멘트 남겨뒀는데요, 커넥션 스트링을 키볼트에서 가져온다고 가정해 보시면 좋겠습니다: #292 (review)

@justinyoo
Copy link
Contributor

#300 태스크 생성해 뒀으니 거기서 코멘트 하나만 남겨주세요 @sikutisa

@sikutisa
Copy link
Contributor Author

sikutisa commented Sep 6, 2024

bicep 리뷰 코멘트 남겨뒀는데요, 커넥션 스트링을 키볼트에서 가져온다고 가정해 보시면 좋겠습니다: #292 (review)

해당 리뷰 확인해서 키볼트에서 가져오는 방식으로 변경해보겠습니다.

@sikutisa sikutisa force-pushed the feature/208-table-storage-connection branch from c5744db to 71d979a Compare September 7, 2024 08:58
@sikutisa
Copy link
Contributor Author

sikutisa commented Sep 7, 2024

@justinyoo
현재 문제가 되는 테스트 코드는 아래와 같습니다(ServiceCollectionExtensionsTests.cs의 110라인 입니다).

    [Theory]
    [InlineData("http://localhost", default(string))]
    [InlineData("http://localhost", "")]
    public void Given_NullOrEmpty_SecretName_When_Invoked_AddKeyVaultService_Then_It_Should_Throw_Exception(string vaultUri, string? secretName)
    {
        // Arrange
        var services = new ServiceCollection();
        var dict = new Dictionary<string, string>()
        {
            { "Azure:KeyVault:VaultUri", vaultUri },
            { "Azure:KeyVault:SecretNames:OpenAI", secretName! },
        };
#pragma warning disable CS8620 // Argument cannot be used for parameter due to differences in the nullability of reference types.
        var config = new ConfigurationBuilder().AddInMemoryCollection(dict).Build();
#pragma warning restore CS8620 // Argument cannot be used for parameter due to differences in the nullability of reference types.
        services.AddSingleton<IConfiguration>(config);
        services.AddKeyVaultService();

        // Act
        Action action = () => services.BuildServiceProvider().GetService<SecretClient>();

        // Assert
        action.Should().Throw<InvalidOperationException>();
    }

두 시나리오 중 첫번째 시나리오인 [InlineData("http://localhost", default(string))]에서 System.Collections.Generic.KeyNotFoundException가 발생하고 있습니다. 하지만 디버그 모드로 확인해봤을 때에는 일단 config 변수까지는 키가 정상적으로 추가되는 것으로 보입니다.
스크린샷 2024-09-07 181512

그래서 추측컨데, 현재 AddKeyVaultService 함수에서 설정값을 객체에 바인드할 때 값이 null이면 Dictionary 객체 바인드가 제대로 동작하지 않고 있는 것 같습니다.

AddKeyVaultService 내부에서 중단점을 통해 변수들을 살펴보면, configuration에는 OpenAI 키가 들어있는데, settings의 SecretNames Count가 0으로 나옵니다.(기대값: 1)
image

로컬에서 테스트를 더 해본 결과, AddKeyVaultService에서 바인딩을 할 때 중첩된 딕셔너리를 명시적으로 지정하여 바인드하면 테스트가 통과하는 것을 확인했습니다.

    public static IServiceCollection AddKeyVaultService(this IServiceCollection services)
    {
        services.AddSingleton<SecretClient>(sp =>
        {
            var configuration = sp.GetService<IConfiguration>()
                ?? throw new InvalidOperationException($"{nameof(IConfiguration)} service is not registered.");

            var settings = configuration.GetSection(AzureSettings.Name).GetSection(KeyVaultSettings.Name).Get<KeyVaultSettings>()
                ?? throw new InvalidOperationException($"{nameof(KeyVaultSettings)} could not be retrieved from the configuration.");

           // Bind SecretNames Dictionary
            var secretNamesSection = configuration.GetSection($"{AzureSettings.Name}:{KeyVaultSettings.Name}:SecretNames");

            if (string.IsNullOrWhiteSpace(settings.VaultUri) == true)
            {
                throw new InvalidOperationException($"{nameof(KeyVaultSettings.VaultUri)} is not defined.");
            }

            if (string.IsNullOrWhiteSpace(secretNamesSection["OpenAI"]) == true)
            {
                throw new InvalidOperationException($"{nameof(KeyVaultSettings.SecretNames)}.OpenAI is not defined.");
            }

            var client = new SecretClient(new Uri(settings.VaultUri), new DefaultAzureCredential());

            return client;
        });

        return services;
    }

KeyVaultSettings의 SecretNames를 다른 방식으로 구현하던가 아니면 바인드 방식을 바꾸던가 하는 조치가 필요해 보입니다.

@sikutisa sikutisa changed the title [Backend API] Table Storage Connection Setting [Admin] Table Storage Connection Setting Sep 7, 2024
@sikutisa sikutisa marked this pull request as ready for review September 7, 2024 10:35
If a null secretName is given, a KeyNotFoundException is thrown instead of an InvalidOperationException.

Related to: aliencube#300
Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

LGTM! 고생하셨습니다!

@justinyoo justinyoo merged commit 999dfd7 into aliencube:main Sep 12, 2024
1 check passed
@sikutisa sikutisa deleted the feature/208-table-storage-connection branch September 13, 2024 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Admin] 스토리지 커넥션 개체 생성
2 participants