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

Fix the lease coordination table permissions #5097

Merged
merged 2 commits into from
Oct 22, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,28 @@
*/
public class AwsCredentialsOptions {
private static final AwsCredentialsOptions DEFAULT_OPTIONS = new AwsCredentialsOptions();
private static final AwsCredentialsOptions DEFAULT_OPTIONS_WITH_DEFAULT_CREDS =
AwsCredentialsOptions.builder().withUseDefaultCredentials(true).build();
private final String stsRoleArn;
private final String stsExternalId;
private final Region region;
private final Map<String, String> stsHeaderOverrides;
private final boolean useDefaultCredentials;

private AwsCredentialsOptions(final Builder builder) {
this.stsRoleArn = builder.stsRoleArn;
this.stsExternalId = builder.stsExternalId;
this.region = builder.region;
this.stsHeaderOverrides = builder.stsHeaderOverrides != null ? new HashMap<>(builder.stsHeaderOverrides) : Collections.emptyMap();
this.useDefaultCredentials = builder.useDefaultCredentials;
}

private AwsCredentialsOptions() {
this.stsRoleArn = null;
this.stsExternalId = null;
this.region = null;
this.stsHeaderOverrides = Collections.emptyMap();
this.useDefaultCredentials = false;
}

/**
Expand All @@ -49,6 +54,10 @@ public static AwsCredentialsOptions defaultOptions() {
return DEFAULT_OPTIONS;
}

public static AwsCredentialsOptions defaultOptionsWithDefaultCreds() {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static AwsCredentialsOptions defaultOptionsWithDefaultCreds() {
public static AwsCredentialsOptions defaultOptionsWithDefaultCredentialsProvider() {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dlvenable. I have made the name change.

return DEFAULT_OPTIONS_WITH_DEFAULT_CREDS;
}

public String getStsRoleArn() {
return stsRoleArn;
}
Expand All @@ -65,6 +74,10 @@ public Map<String, String> getStsHeaderOverrides() {
return stsHeaderOverrides;
}

public boolean isUseDefaultCredentials() {
Copy link
Member

Choose a reason for hiding this comment

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

We already have a concept of default in the plugin configuration. So using "default" here is rather confusing.

@JsonProperty("default")
private AwsStsConfiguration defaultStsConfiguration = new AwsStsConfiguration();

So I think we should use a different name here.

One idea: isUseDefaultCredentialsProvider to clarify that this is coupled with the SDK's DefaultCredentialsProvider.

Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dlvenable for your input. I will make the change.

return useDefaultCredentials;
}

/**
* Builder class for {@link AwsCredentialsOptions}.
*/
Expand All @@ -73,6 +86,7 @@ public static class Builder {
private String stsExternalId;
private Region region;
private Map<String, String> stsHeaderOverrides = Collections.emptyMap();
private boolean useDefaultCredentials = false;

/**
* Sets the STS role ARN to use.
Expand Down Expand Up @@ -122,6 +136,17 @@ public Builder withStsHeaderOverrides(final Map<String, String> stsHeaderOverrid
return this;
}

/**
* Configures whether to use default credentials.
*
* @param useDefaultCredentials
* @return The {@link Builder} for continuing to build
*/
public Builder withUseDefaultCredentials(final boolean useDefaultCredentials) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public Builder withUseDefaultCredentials(final boolean useDefaultCredentials) {
public Builder withUseDefaultCredentialsProvider(final boolean useDefaultCredentialsProvider) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dlvenable. I have made the name change.

this.useDefaultCredentials = useDefaultCredentials;
return this;
}

/**
* Builds the {@link AwsCredentialsOptions}.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.CoreMatchers.sameInstance;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;

class AwsCredentialsOptionsTest {
@Test
Expand Down Expand Up @@ -150,4 +151,20 @@ void defaultOptions_returns_same_instance_on_multiple_calls() {
assertThat(AwsCredentialsOptions.defaultOptions(),
sameInstance(AwsCredentialsOptions.defaultOptions()));
}


@Test
void with_DefaultRole() {
final AwsCredentialsOptions awsCredentialsOptions = AwsCredentialsOptions.defaultOptionsWithDefaultCreds();

assertThat(awsCredentialsOptions, notNullValue());
assertThat(awsCredentialsOptions.getStsRoleArn(), nullValue());
assertTrue(awsCredentialsOptions.isUseDefaultCredentials());
}

@Test
void defaultCredentialsOptions_returns_same_instance_on_multiple_calls() {
assertThat(AwsCredentialsOptions.defaultOptionsWithDefaultCreds(),
sameInstance(AwsCredentialsOptions.defaultOptionsWithDefaultCreds()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ Region getDefaultRegion() {
AwsCredentialsProvider providerFromOptions(final AwsCredentialsOptions credentialsOptions) {
Objects.requireNonNull(credentialsOptions);

if (credentialsOptions.isUseDefaultCredentials()) {
return DefaultCredentialsProvider.create();
}

if(credentialsOptions.getStsRoleArn() != null || defaultStsConfiguration.getAwsStsRoleArn() != null) {
return createStsCredentials(credentialsOptions);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,42 @@ void test_AwsPlugin_without_STS_role_and_with_default_role_uses_default_role() {
assertThat(awsCredentialsProvider2, sameInstance(awsCredentialsProvider1));
}

@Test
void test_AwsPlugin_without_STS_role_and_without_default_role_uses_default_role() {

createObjectUnderTest().apply(extensionPoints);

final ArgumentCaptor<ExtensionProvider<AwsCredentialsSupplier>> extensionProviderArgumentCaptor = ArgumentCaptor.forClass(ExtensionProvider.class);
verify(extensionPoints).addExtensionProvider(extensionProviderArgumentCaptor.capture());

final ExtensionProvider<AwsCredentialsSupplier> extensionProvider = extensionProviderArgumentCaptor.getValue();

final Optional<AwsCredentialsSupplier> optionalSupplier = extensionProvider.provideInstance(context);
assertThat(optionalSupplier, notNullValue());
assertThat(optionalSupplier.isPresent(), equalTo(true));

final AwsCredentialsSupplier awsCredentialsSupplier = optionalSupplier.get();

final AwsCredentialsOptions awsCredentialsOptions1 = AwsCredentialsOptions.builder()
.withRegion(Region.US_EAST_1)
.withUseDefaultCredentials(true)
.build();

final AwsCredentialsProvider awsCredentialsProvider1 = awsCredentialsSupplier.getProvider(awsCredentialsOptions1);

assertThat(awsCredentialsProvider1, instanceOf(DefaultCredentialsProvider.class));

final AwsCredentialsOptions awsCredentialsOptions2 = AwsCredentialsOptions.builder()
.withRegion(Region.US_EAST_1)
.withUseDefaultCredentials(true)
.build();

final AwsCredentialsProvider awsCredentialsProvider2 = awsCredentialsSupplier.getProvider(awsCredentialsOptions2);

assertThat(awsCredentialsProvider2, instanceOf(DefaultCredentialsProvider.class));
assertThat(awsCredentialsProvider2, sameInstance(awsCredentialsProvider1));
}

private String createStsRole() {
return String.format("arn:aws:iam::123456789012:role/%s", UUID.randomUUID());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ public KinesisClientFactory(final AwsCredentialsSupplier awsCredentialsSupplier,
.withStsExternalId(awsAuthenticationConfig.getAwsStsExternalId())
.withStsHeaderOverrides(awsAuthenticationConfig.getAwsStsHeaderOverrides())
.build());
defaultCredentialsProvider = awsCredentialsSupplier.getProvider(AwsCredentialsOptions.defaultOptions());
defaultCredentialsProvider = awsCredentialsSupplier.getProvider(
AwsCredentialsOptions.defaultOptionsWithDefaultCreds());
this.awsAuthenticationConfig = awsAuthenticationConfig;
}

Expand Down
Loading