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

Added validations in include and exclude keys #3181

Merged
merged 1 commit into from
Aug 18, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
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 @@ -12,13 +12,11 @@
import com.fasterxml.jackson.databind.annotation.JsonSerialize;
import org.opensearch.dataprepper.model.plugin.InvalidPluginConfigurationException;

import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.stream.Collectors;

/**
* Represents an extension of the {@link PluginModel} which is specific to Sink
Expand Down Expand Up @@ -120,10 +118,11 @@ private SinkInternalJsonModel(@JsonProperty("routes") final List<String> routes,
private SinkInternalJsonModel(final List<String> routes, final String tagsTargetKey, final List<String> includeKeys, final List<String> excludeKeys, final Map<String, Object> pluginSettings) {
super(pluginSettings);
this.routes = routes != null ? routes : Collections.emptyList();
this.includeKeys = includeKeys != null ? preprocessingKeys(includeKeys) : Collections.emptyList();
this.excludeKeys = excludeKeys != null ? preprocessingKeys(excludeKeys) : Collections.emptyList();
this.includeKeys = includeKeys != null ? includeKeys : Collections.emptyList();
this.excludeKeys = excludeKeys != null ? excludeKeys : Collections.emptyList();
this.tagsTargetKey = tagsTargetKey;
validateConfiguration();
validateKeys();
}

void validateConfiguration() {
Expand All @@ -132,24 +131,18 @@ void validateConfiguration() {
}
}


/**
* Pre-processes a list of Keys and returns a sorted list
* The keys must start with `/` and not end with `/`
*
* @param keys a list of raw keys
* @return a sorted processed keys
* Validates both include and exclude keys if they contain /
*/
private List<String> preprocessingKeys(final List<String> keys) {
if (keys.contains("/")) {
return new ArrayList<>();
}
List<String> result = keys.stream()
.map(k -> k.startsWith("/") ? k : "/" + k)
.map(k -> k.endsWith("/") ? k.substring(0, k.length() - 1) : k)
.collect(Collectors.toList());
Collections.sort(result);
return result;
private void validateKeys() {
includeKeys.forEach(key -> {
if(key.contains("/"))
throw new InvalidPluginConfigurationException("include_keys cannot contain /");
});
excludeKeys.forEach(key -> {
if(key.contains("/"))
throw new InvalidPluginConfigurationException("exclude_keys cannot contain /");
Copy link
Member

Choose a reason for hiding this comment

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

Does this mean we don't support nested keys? seems like it should be supported like we do for delete_entries processor

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We decided not to support it for now, and add it later.
#3163 (comment)

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 do not support it.

So this change just validates that users don't try to use this. See this comment and thread for more details:

#3163 (comment)

});
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,27 +144,41 @@ void serialize_with_just_pluginModel() throws IOException {
}

@Test
void sinkModel_with_include_keys() throws IOException {
void sinkModel_with_include_keys() {
final Map<String, Object> pluginSettings = new LinkedHashMap<>();
final SinkModel sinkModel = new SinkModel("customSinkPlugin", Arrays.asList("routeA", "routeB"), null, Arrays.asList("bcd", "/abc", "efg/"), null, pluginSettings);
final SinkModel sinkModel = new SinkModel("customSinkPlugin", Arrays.asList("routeA", "routeB"), null, Arrays.asList("bcd", "abc", "efg"), null, pluginSettings);

assertThat(sinkModel.getExcludeKeys(), equalTo(new ArrayList<String>()));
assertThat(sinkModel.getIncludeKeys(), equalTo(Arrays.asList("/abc", "/bcd", "/efg")));
assertThat(sinkModel.getIncludeKeys(), equalTo(Arrays.asList("bcd", "abc", "efg")));

}

@Test
void sinkModel_with_exclude_keys() throws IOException {
void sinkModel_with_invalid_include_keys() {
final Map<String, Object> pluginSettings = new LinkedHashMap<>();
final SinkModel sinkModel = new SinkModel("customSinkPlugin", Arrays.asList("routeA", "routeB"), null, List.of("/"), Arrays.asList("bcd", "/abc", "efg/"), pluginSettings);
assertThrows(InvalidPluginConfigurationException.class, () -> new SinkModel("customSinkPlugin", Arrays.asList("routeA", "routeB"), null, List.of("/bcd"), List.of(), pluginSettings));
}

@Test
void sinkModel_with_exclude_keys() {
final Map<String, Object> pluginSettings = new LinkedHashMap<>();
final SinkModel sinkModel = new SinkModel("customSinkPlugin", Arrays.asList("routeA", "routeB"), null, List.of(), Arrays.asList("abc", "bcd", "efg"), pluginSettings);

assertThat(sinkModel.getIncludeKeys(), equalTo(new ArrayList<String>()));
assertThat(sinkModel.getExcludeKeys(), equalTo(Arrays.asList("/abc", "/bcd", "/efg")));
assertThat(sinkModel.getExcludeKeys(), equalTo(Arrays.asList("abc", "bcd", "efg")));

}

@Test
void sinkModel_with_invalid_exclude_keys() {
final Map<String, Object> pluginSettings = new LinkedHashMap<>();
assertThrows(InvalidPluginConfigurationException.class, () -> new SinkModel("customSinkPlugin", Arrays.asList("routeA", "routeB"), null, List.of(), List.of("/bcd"), pluginSettings));
}



@Test
void sinkModel_with_both_include_and_exclude_keys() throws IOException {
void sinkModel_with_both_include_and_exclude_keys() {
final Map<String, Object> pluginSettings = new LinkedHashMap<>();
assertThrows(InvalidPluginConfigurationException.class, () -> new SinkModel("customSinkPlugin", Arrays.asList("routeA", "routeB"), null, List.of("abc"), List.of("bcd"), pluginSettings));
}
Expand Down
8 changes: 4 additions & 4 deletions data-prepper-plugins/opensearch/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ With the `document_root_key` set to `status`. The document structure would be `{
duration: "15 ms"
}
```
- `include_keys`: A list of keys to be included (retained). The key in the list can be a valid JSON path, such as 'request/status'. This option can work together with `document_root_key`.
- `include_keys`: A list of keys to be included (retained). The key in the list cannot contain '/'. This option can work together with `document_root_key`.

For example, If we have the following sample event:
```
Expand All @@ -224,7 +224,7 @@ For example, If we have the following sample event:
}
}
```
if `include_keys` is set to ["status", "metadata/sourceIp"], the document written to OpenSearch would be:
if `include_keys` is set to ["status", "metadata"], the document written to OpenSearch would be:
```
{
status: 200,
Expand Down Expand Up @@ -256,11 +256,11 @@ For example, If we have the following sample event:
}
}
```
if `exclude_keys` is set to ["status", "metadata/sourceIp"], the document written to OpenSearch would be:
if `exclude_keys` is set to ["message", "status"], the document written to OpenSearch would be:
```
{
message: null,
metadata: {
sourceIp: "123.212.49.58",
destinationIp: "79.54.67.231",
bytes: 3545,
duration: "15 ms"
Expand Down
Loading