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

Do no use x-opaque-id for deduplicating Elastic originating requests #82855

Merged
merged 9 commits into from
Jan 25, 2022
Merged

Do no use x-opaque-id for deduplicating Elastic originating requests #82855

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
909a1f6
Do no use x-opaque-id for deduplicating kibana requests
pgomulka Jan 19, 2022
d788bf0
Merge remote-tracking branch 'upstream/master' into disable_xopaqueid…
pgomulka Jan 20, 2022
0335702
constant and npe fix
pgomulka Jan 20, 2022
cc82822
use product origin as x-opaque-id
pgomulka Jan 24, 2022
e3d95eb
remove kibana constant
pgomulka Jan 24, 2022
6dcfa57
Merge remote-tracking branch 'upstream/master' into disable_xopaqueid…
pgomulka Jan 24, 2022
d184961
Update docs/changelog/82855.yaml
pgomulka Jan 24, 2022
f654de4
setter rename
pgomulka Jan 25, 2022
4667cc0
Merge branch 'disable_xopaqueid_kibana_originating' of github.com:pgo…
pgomulka Jan 25, 2022
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
15 changes: 9 additions & 6 deletions server/src/main/java/org/elasticsearch/common/logging/RateLimitingFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import java.util.Map;
import java.util.Set;

import static org.elasticsearch.common.logging.DeprecatedMessage.ELASTIC_ORIGIN_FIELD_NAME;
import static org.elasticsearch.common.logging.DeprecatedMessage.KEY_FIELD_NAME;
import static org.elasticsearch.common.logging.DeprecatedMessage.X_OPAQUE_ID_FIELD_NAME;

Expand All @@ -35,13 +36,14 @@
* This filter works by using a lruKeyCache - a set of keys which prevents a second message with the same key to be logged.
* The lruKeyCache has a size limited to 128, which when breached will remove the oldest entries.
*
* It is possible to disable use of `x-opaque-id` as a key with {@link RateLimitingFilter#setUseXOpaqueId(boolean) }
* It is possible to disable use of `x-opaque-id` as a key with {@link RateLimitingFilter#setUseXOpaqueIdEnabled(boolean) }
* @see <a href="https://logging.apache.org/log4j/2.x/manual/filters.htmlf">Log4j2 Filters</a>
*/
@Plugin(name = "RateLimitingFilter", category = Node.CATEGORY, elementType = Filter.ELEMENT_TYPE)
public class RateLimitingFilter extends AbstractFilter {

private volatile boolean useXOpaqueId = true;
private static final String KIBANA_OPRDUCT_ORIGIN = "kibana";
Copy link

Choose a reason for hiding this comment

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

is this a typo or intentional?

Suggested change
private static final String KIBANA_OPRDUCT_ORIGIN = "kibana";
private static final String KIBANA_PRODUCT_ORIGIN = "kibana";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that was a typo. The constant is not needed as this change will apply to all elastic originating requests

// a flag to disable/enable use of xOpaqueId controlled by changing cluster setting
private volatile boolean useXOpaqueIdEnabled = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why the addition of "enabled"? The "use" prefix already denotes a boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true - it does not make sens to me now either. I can't remember what I was thinking when renaming..


private final Set<String> lruKeyCache = Collections.newSetFromMap(Collections.synchronizedMap(new LinkedHashMap<>() {
@Override
Expand Down Expand Up @@ -76,7 +78,8 @@ public Result filter(Message message) {

private String getKey(ESLogMessage esLogMessage) {
final String key = esLogMessage.get(KEY_FIELD_NAME);
if (useXOpaqueId) {
final String productOrigin = esLogMessage.get(ELASTIC_ORIGIN_FIELD_NAME);
if (useXOpaqueIdEnabled && KIBANA_OPRDUCT_ORIGIN.equals(productOrigin) == false) {
String xOpaqueId = esLogMessage.get(X_OPAQUE_ID_FIELD_NAME);
return xOpaqueId + key;
}
Expand All @@ -101,7 +104,7 @@ public static RateLimitingFilter createFilter(
return new RateLimitingFilter(match, mismatch);
}

public void setUseXOpaqueId(boolean useXOpaqueId) {
this.useXOpaqueId = useXOpaqueId;
public void setUseXOpaqueIdEnabled(boolean useXOpaqueIdEnabled) {
this.useXOpaqueIdEnabled = useXOpaqueIdEnabled;
}
}
16 changes: 15 additions & 1 deletion server/src/test/java/org/elasticsearch/common/logging/RateLimitingFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ public void testFilterCanBeReset() {

public void testMessagesXOpaqueIsIgnoredWhenDisabled() {
RateLimitingFilter filter = new RateLimitingFilter();
filter.setUseXOpaqueId(false);
filter.setUseXOpaqueIdEnabled(false);
filter.start();

// Should NOT be rate-limited because it's not in the cache
Expand All @@ -169,4 +169,18 @@ public void testMessagesXOpaqueIsIgnoredWhenDisabled() {
message = DeprecatedMessage.of(DeprecationCategory.OTHER, "key 1", "opaque-id 1", "productName", "msg 0");
assertThat(filter.filter(message), equalTo(Result.ACCEPT));
}

public void testXOpaqueIdNotBeingUsedFromKibanaOriginatingRequests() {
RateLimitingFilter filter = new RateLimitingFilter();
filter.setUseXOpaqueIdEnabled(true);
filter.start();

// Should NOT be rate-limited because it's not in the cache
Message message = DeprecatedMessage.of(DeprecationCategory.OTHER, "key 0", "opaque-id 0", "kibana", "msg 0");
assertThat(filter.filter(message), equalTo(Result.ACCEPT));

// Should be rate-limited even though the x-opaque-id is unique because it originates from kibana
message = DeprecatedMessage.of(DeprecationCategory.OTHER, "key 0", "opaque-id 1", "kibana", "msg 0");
assertThat(filter.filter(message), equalTo(Result.DENY));
}
}
4 changes: 2 additions & 2 deletions x-pack/plugin/deprecation/src/main/java/org/elasticsearch/xpack/deprecation/Deprecation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,9 +109,9 @@ public Collection<Object> createComponents(

final RateLimitingFilter rateLimitingFilterForIndexing = new RateLimitingFilter();
// enable on start.
rateLimitingFilterForIndexing.setUseXOpaqueId(USE_X_OPAQUE_ID_IN_FILTERING.get(environment.settings()));
rateLimitingFilterForIndexing.setUseXOpaqueIdEnabled(USE_X_OPAQUE_ID_IN_FILTERING.get(environment.settings()));
clusterService.getClusterSettings()
.addSettingsUpdateConsumer(USE_X_OPAQUE_ID_IN_FILTERING, rateLimitingFilterForIndexing::setUseXOpaqueId);
.addSettingsUpdateConsumer(USE_X_OPAQUE_ID_IN_FILTERING, rateLimitingFilterForIndexing::setUseXOpaqueIdEnabled);

final DeprecationIndexingComponent component = DeprecationIndexingComponent.createDeprecationIndexingComponent(
client,
Expand Down