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.

Sign up for GitHub

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

feat: allow endpoint overrides in AwsSecretsManagerVault #485

Merged
merged 4 commits into from
Dec 2, 2024
Merged

feat: allow endpoint overrides in AwsSecretsManagerVault #485

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a531597
feat: allow endpoint overrides in AwsSecretsManagerVault
chlorochrule Nov 13, 2024
00bce73
accept review
chlorochrule Nov 25, 2024
6a233c3
fix by self review
chlorochrule Nov 25, 2024
27ef0a2
accept review
chlorochrule Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 18 additions & 7 deletions ...lt/vault-aws/src/main/java/org/eclipse/edc/vault/aws/AwsSecretsManagerVaultExtension.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient;

import java.net.URI;
import java.util.Optional;

/**
* This extension registers an implementation of the Vault interface for AWS Secrets Manager.
* It also registers a VaultPrivateKeyResolver and VaultCertificateResolver, which store and retrieve certificates
Expand All @@ -33,8 +36,14 @@
public class AwsSecretsManagerVaultExtension implements ServiceExtension {
public static final String NAME = "AWS Secrets Manager Vault";

@Setting
private static final String VAULT_AWS_REGION = "edc.vault.aws.region";
@Setting(key = "edc.vault.aws.region",
description = "The AWS Secrets Manager client will point to the specified region")
String vaultRegion;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

package private for testing

Copy link
Member

Choose a reason for hiding this comment

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

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


@Setting(key = "edc.vault.aws.endpoint.override",
description = "If valued, the AWS Secrets Manager client will point to the specified endpoint",
required = false)
String vaultAwsEndpointOverride;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

package private for testing


@Override
public String name() {
Expand All @@ -43,18 +52,20 @@ public String name() {

@Provider
public Vault createVault(ServiceExtensionContext context) {
var vaultRegion = context.getConfig().getString(VAULT_AWS_REGION);
var vaultEndpointOverride = Optional.ofNullable(vaultAwsEndpointOverride)
.map(URI::create)
.orElse(null);

var smClient = buildSmClient(vaultRegion);
var smClient = buildSmClient(vaultRegion, vaultEndpointOverride);

return new AwsSecretsManagerVault(smClient, context.getMonitor(),
new AwsSecretsManagerVaultDefaultSanitationStrategy(context.getMonitor()));
}

private SecretsManagerClient buildSmClient(String vaultRegion) {
private SecretsManagerClient buildSmClient(String vaultRegion, URI vaultEndpointOverride) {
Copy link
Member

Choose a reason for hiding this comment

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

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

var builder = SecretsManagerClient.builder()
.region(Region.of(vaultRegion));
.region(Region.of(vaultRegion))
.endpointOverride(vaultEndpointOverride);
return builder.build();
}

}
44 changes: 34 additions & 10 deletions ...ault-aws/src/test/java/org/eclipse/edc/vault/aws/AwsSecretsManagerVaultExtensionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,35 +16,59 @@

import org.eclipse.edc.spi.monitor.Monitor;
import org.eclipse.edc.spi.system.ServiceExtensionContext;
import org.eclipse.edc.spi.system.configuration.Config;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import software.amazon.awssdk.regions.Region;
import software.amazon.awssdk.services.secretsmanager.SecretsManagerClient;

import java.net.URI;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.InstanceOfAssertFactories.type;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

class AwsSecretsManagerVaultExtensionTest {
private static ServiceExtensionContext context;

@BeforeAll
public static void beforeAll() {
context = mock(ServiceExtensionContext.class);
when(context.getMonitor()).thenReturn(mock(Monitor.class));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AwsSecretsManagerVaultExtension uses only monitor, does not use ServiceExtensionContext

Copy link
Member

Choose a reason for hiding this comment

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

using DependencyInjectionExtension will make these lines unnecessary


private final Monitor monitor = mock(Monitor.class);
private final AwsSecretsManagerVaultExtension extension = new AwsSecretsManagerVaultExtension();

@Test
void configOptionRegionNotProvided_shouldThrowException() {
var extension = new AwsSecretsManagerVaultExtension();
ServiceExtensionContext invalidContext = mock(ServiceExtensionContext.class);
when(invalidContext.getMonitor()).thenReturn(monitor);

Assertions.assertThrows(NullPointerException.class, () -> extension.createVault(invalidContext));
}

@Test
void configOptionRegionProvided_shouldNotThrowException() {
ServiceExtensionContext validContext = mock(ServiceExtensionContext.class);
Config cfg = mock();
when(cfg.getString("edc.vault.aws.region")).thenReturn("eu-west-1");
when(validContext.getConfig()).thenReturn(cfg);
when(validContext.getMonitor()).thenReturn(monitor);
var extension = new AwsSecretsManagerVaultExtension();
extension.vaultRegion = "eu-west-1";

var vault = extension.createVault(context);

extension.createVault(validContext);
assertThat(vault).extracting("smClient", type(SecretsManagerClient.class)).satisfies(client -> {
assertThat(client.serviceClientConfiguration().region()).isEqualTo(Region.of("eu-west-1"));
});
}

@Test
void configOptionEndpointOverrideProvided_shouldNotThrowException() {
Copy link
Member

Choose a reason for hiding this comment

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

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.

var extension = new AwsSecretsManagerVaultExtension();
extension.vaultRegion = "eu-west-1";
extension.vaultAwsEndpointOverride = "http://localhost:4566";
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 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!

Copy link
Member

Choose a reason for hiding this comment

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

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


var vault = extension.createVault(context);

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