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

Jump to bottom

feat: allow endpoint overrides in AwsSecretsManagerVault #485

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

feat: allow endpoint overrides in AwsSecretsManagerVault #485

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 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
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
18 changes: 14 additions & 4 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 @@ -36,6 +39,9 @@ public class AwsSecretsManagerVaultExtension implements ServiceExtension {
@Setting
private static final String VAULT_AWS_REGION = "edc.vault.aws.region";

@Setting
private static final String AWS_ENDPOINT_OVERRIDE = "edc.aws.endpoint.override";
Copy link
Member

Choose a reason for hiding this comment

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

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


@Override
public String name() {
return NAME;
Expand All @@ -44,17 +50,21 @@ public String name() {
@Provider
public Vault createVault(ServiceExtensionContext context) {
var vaultRegion = context.getConfig().getString(VAULT_AWS_REGION);
var vaultEndpointOverride = Optional.of(AWS_ENDPOINT_OVERRIDE)
.map(key -> context.getSetting(key, null))
.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) {
var builder = SecretsManagerClient.builder()
.region(Region.of(vaultRegion));
.region(Region.of(vaultRegion))
.endpointOverride(vaultEndpointOverride);
return builder.build();
}

}
11 changes: 11 additions & 0 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 @@ -47,4 +47,15 @@ void configOptionRegionProvided_shouldNotThrowException() {
extension.createVault(validContext);
}

@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.

ServiceExtensionContext validContext = mock(ServiceExtensionContext.class);
Config cfg = mock();
when(cfg.getString("edc.vault.aws.region")).thenReturn("eu-west-1");
when(cfg.getString("edc.aws.endpoint.override")).thenReturn("http://localhost:4566");
when(validContext.getConfig()).thenReturn(cfg);
when(validContext.getMonitor()).thenReturn(monitor);

extension.createVault(validContext);
}
}
Loading