feat: allow endpoint overrides in AwsSecretsManagerVault

[chlorochrule]

What this PR changes/adds

AwsSecretsManagerVaultExtension do not allow to override AWS endpoint. However, S3CoreExtension allows it. So, I implements endpoint overrides in AwsSecretsManagerVault using edc.aws.endpoint.override the same param of S3CoreExtension.

Why it does that

For AWS-compatible services like localstack.

Closes #486

[github-actions]

We are always happy to welcome new contributors ❤️ To make things easier for everyone, please make sure to follow our contribution guidelines, check if you have already signed the ECA, and relate this pull request to an existing issue or discussion.

[paullatzelsperger]

@chlorochrule do NOT submit surprise PRs.
As per our PR Etiquette, a discussion or an issue must be created beforehand, to outline requirements, motivation and a technical approach for the work.

Committers will then triage the issue and approve it for implementation by removing the triage label. Once that is done you are good to go.

[chlorochrule]

@paullatzelsperger I apologize for the breaking etiquette, and thank you for pointing that out! I created a ticket #486

[github-actions]

This pull request is stale because it has been open for 7 days with no activity.

[ndr-brt]

I think this setting should be different than the one used to create the S3 client, and be called edc.vault.aws.endpoint.override, to align with the region setting.

plus, settings can be injected in a different way, as described in this DR

Please adapt and add some description to the annotation

[ndr-brt]

this test is asserting nothing. I know that you inherited from the already existing tests, but tests without assertions should be avoided.
Demonstration: if you try to add a proper assertion like:

        var vault = extension.createVault(validContext);

        assertThat(vault).extracting("smClient", type(SecretsManagerClient.class)).satisfies(client -> {
            assertThat(client.serviceClientConfiguration().endpointOverride()).contains(URI.create("http://localhost:4566"));
        });

it won't pass, that's because you set the mock to respond a call where only the setting key is passed, but not the default value.
Another suggestion would be not to mock the config object but to create one using ConfigFactory.fromMap, that will make everything easier.

[chlorochrule]

@ndr-brt Thank you for reviewing! I fixed!

[chlorochrule]

AwsClientBuilder#region can identify the endpoint automatically, so it might be better to support required = false
https://github.com/aws/aws-sdk-java-v2/blob/891cf4fb6f37ac671fa91663c23965585fbdd504/core/aws-core/src/main/java/software/amazon/awssdk/awscore/client/builder/AwsClientBuilder.java#L85-L97

[ndr-brt]

yes, but to be tackled eventually in a separated issue/PR.

[chlorochrule]

package private for testing

[ndr-brt]

this is not a good pattern, please see my previous comment

[chlorochrule]

package private for testing

[chlorochrule]

AwsSecretsManagerVaultExtension uses only monitor, does not use ServiceExtensionContext

[ndr-brt]

using DependencyInjectionExtension will make these lines unnecessary

[chlorochrule]

I'm trying to use key of @Setting, but I can't find the way to inject the testing value to the field variable. Please let me know how to test @Setting annotated value with key overrides if you know a better way!

[ndr-brt]

My suggestion is to use the DependencyInjectionExtension that can be found in the edc-junit module.

Then this test can be written this way:

    @Test
    void configOptionEndpointOverrideProvided_shouldNotThrowException(ObjectFactory factory, ServiceExtensionContext context) {
        var config = ConfigFactory.fromMap(Map.of(
                "edc.vault.aws.region", "region",
                "edc.vault.aws.endpoint.override", "http://localhost:4566"
        ));
        when(context.getConfig()).thenReturn(config);
        var extension = factory.constructInstance(AwsSecretsManagerVaultExtension.class);

        var vault = extension.createVault(context);

        assertThat(vault).extracting("smClient", type(SecretsManagerClient.class)).satisfies(client -> {
            assertThat(client.serviceClientConfiguration().endpointOverride()).contains(URI.create("http://localhost:4566"));
        });
    }

and no need to make the setting fields package private

[ndr-brt]

@chlorochrule when you are ready for another review please re-request my review, so this will be listed on my "review-to-be-done" tab ;)

[ndr-brt]

My suggestion is to use the DependencyInjectionExtension that can be found in the edc-junit module.

Then this test can be written this way:

    @Test
    void configOptionEndpointOverrideProvided_shouldNotThrowException(ObjectFactory factory, ServiceExtensionContext context) {
        var config = ConfigFactory.fromMap(Map.of(
                "edc.vault.aws.region", "region",
                "edc.vault.aws.endpoint.override", "http://localhost:4566"
        ));
        when(context.getConfig()).thenReturn(config);
        var extension = factory.constructInstance(AwsSecretsManagerVaultExtension.class);

        var vault = extension.createVault(context);

        assertThat(vault).extracting("smClient", type(SecretsManagerClient.class)).satisfies(client -> {
            assertThat(client.serviceClientConfiguration().endpointOverride()).contains(URI.create("http://localhost:4566"));
        });
    }

and no need to make the setting fields package private

[ndr-brt]

yes, but to be tackled eventually in a separated issue/PR.

[ndr-brt]

this is not a good pattern, please see my previous comment

[ndr-brt]

using DependencyInjectionExtension will make these lines unnecessary

[ndr-brt]

I would inline this method as it's not really giving any improvement.

[ndr-brt]

LGTM