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

Compatible API header parsing plugin #60516

Closed
Closed
Show file tree
Hide file tree
Changes from 31 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
c6aeb38
draft
pgomulka Jul 27, 2020
e85833e
compatible version from a plugin
pgomulka Jul 27, 2020
7f8b84a
moving compatibility function to a plugin
pgomulka Jul 28, 2020
88598dc
remove unused interface
pgomulka Jul 28, 2020
c009235
suggested changes
jakelandis Jul 28, 2020
dce47ac
cleanup
jakelandis Jul 28, 2020
3470081
more cleanup
jakelandis Jul 28, 2020
72d7b13
Merge pull request #21 from jakelandis/jake-xpack-spi-request
pgomulka Jul 29, 2020
6b06d7f
interface instead of function
pgomulka Jul 29, 2020
5ec8038
testcase for combinations
pgomulka Jul 29, 2020
a45171e
add hascontent argumetn and pass tests
pgomulka Jul 29, 2020
4ccf457
precommit
pgomulka Jul 29, 2020
8770870
RestCompatibility plugin injected into RestRequest
pgomulka Jul 31, 2020
0c6a5f7
Merge branch 'master' into compat_plugin_inside_rest_request
pgomulka Aug 20, 2020
8e834b6
precommit
pgomulka Aug 20, 2020
29fcc81
header validation test
pgomulka Aug 21, 2020
8a0952b
precommit
pgomulka Aug 21, 2020
b3c626e
compatibleWithVersion on RestHandler
pgomulka Aug 21, 2020
8f42c1d
import
pgomulka Aug 21, 2020
924a35e
fake request with a default to current
pgomulka Aug 24, 2020
0e0e590
split plugin interface
pgomulka Sep 1, 2020
51376bb
rename file
pgomulka Sep 1, 2020
03e9a63
remove evaluaiton depends on
pgomulka Sep 1, 2020
e790a5d
remove compatible function fron http server transport
pgomulka Sep 3, 2020
a2bc52f
cleanup
pgomulka Sep 3, 2020
6724dda
add version to channel
pgomulka Sep 3, 2020
3f606a3
fix nullpointer
pgomulka Sep 3, 2020
e834a9c
Apply suggestions from code review
pgomulka Sep 4, 2020
4014a8d
javadoc and exception repalce
pgomulka Sep 7, 2020
1a9339a
Merge branch 'master' into compat_plugin_channel2
pgomulka Sep 7, 2020
541cf6d
remove exception and empty lines
pgomulka Sep 8, 2020
fd34207
Apply suggestions from code review
pgomulka Sep 14, 2020
b3e7654
add testcase to node init
pgomulka Sep 14, 2020
1d5b099
unused import
pgomulka Sep 14, 2020
1bb983b
Update server/src/main/java/org/elasticsearch/node/Node.java
pgomulka Sep 17, 2020
448a8e7
Merge branch 'master' into compat_plugin_inside_rest_request
pgomulka Oct 5, 2020
91011cc
Merge branch 'compat_plugin_inside_rest_request' of github.com:pgomul…
pgomulka Oct 5, 2020
89d50f5
use media type parser
pgomulka Oct 5, 2020
97baf91
spotless
pgomulka Oct 6, 2020
324dd57
Merge branch 'master' into compat_plugin_inside_rest_request
pgomulka Oct 7, 2020
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 @@ -137,6 +137,10 @@ public static XContentBuilder builder(XContent xContent, Set<String> includes, S
DATE_TRANSFORMERS = Collections.unmodifiableMap(dateTransformers);
}

public XContentBuilder withCompatibleMajorVersion(byte compatibleVersion) {
return this;
}

@FunctionalInterface
public interface Writer {
void write(XContentBuilder builder, Object value) throws IOException;
Expand Down
6 changes: 6 additions & 0 deletions server/src/main/java/org/elasticsearch/Version.java
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,7 @@ public static Version fromString(String version) {
public final byte revision;
public final byte build;
public final org.apache.lucene.util.Version luceneVersion;
public final int previousMajorId;

Version(int id, org.apache.lucene.util.Version luceneVersion) {
this.id = id;
Expand All @@ -248,6 +249,7 @@ public static Version fromString(String version) {
this.revision = (byte) ((id / 100) % 100);
this.build = (byte) (id % 100);
this.luceneVersion = Objects.requireNonNull(luceneVersion);
this.previousMajorId = major > 0 ? (major - 1) * 1000000 + 99 : major;
}

public boolean after(Version version) {
Expand Down Expand Up @@ -276,6 +278,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder.value(toString());
}

public Version previousMajor() {
return Version.fromId(previousMajorId);
}

/*
* We need the declared versions when computing the minimum compatibility version. As computing the declared versions uses reflection it
* is not cheap. Since computing the minimum compatibility version can occur often, we use this holder to compute the declared versions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@
import org.elasticsearch.persistent.UpdatePersistentTaskStatusAction;
import org.elasticsearch.plugins.ActionPlugin;
import org.elasticsearch.plugins.ActionPlugin.ActionHandler;
import org.elasticsearch.rest.CompatibleVersion;
import org.elasticsearch.rest.RestController;
import org.elasticsearch.rest.RestHandler;
import org.elasticsearch.rest.RestHeaderDefinition;
Expand Down Expand Up @@ -423,7 +424,7 @@ public class ActionModule extends AbstractModule {
public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpressionResolver,
IndexScopedSettings indexScopedSettings, ClusterSettings clusterSettings, SettingsFilter settingsFilter,
ThreadPool threadPool, List<ActionPlugin> actionPlugins, NodeClient nodeClient,
CircuitBreakerService circuitBreakerService, UsageService usageService) {
CircuitBreakerService circuitBreakerService, UsageService usageService, CompatibleVersion compatibleVersion) {
this.settings = settings;
this.indexNameExpressionResolver = indexNameExpressionResolver;
this.indexScopedSettings = indexScopedSettings;
Expand Down Expand Up @@ -455,10 +456,9 @@ public ActionModule(Settings settings, IndexNameExpressionResolver indexNameExpr
indicesAliasesRequestRequestValidators = new RequestValidators<>(
actionPlugins.stream().flatMap(p -> p.indicesAliasesRequestValidators().stream()).collect(Collectors.toList()));

restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService);
restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, compatibleVersion);
}


public Map<String, ActionHandler<?, ?>> getActions() {
return actions;
}
Expand Down
21 changes: 20 additions & 1 deletion server/src/main/java/org/elasticsearch/node/Node.java
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,8 @@
import org.elasticsearch.plugins.Plugin;
import org.elasticsearch.plugins.PluginsService;
import org.elasticsearch.plugins.RepositoryPlugin;
import org.elasticsearch.plugins.RestCompatibilityPlugin;
import org.elasticsearch.rest.CompatibleVersion;
import org.elasticsearch.plugins.ScriptPlugin;
import org.elasticsearch.plugins.SearchPlugin;
import org.elasticsearch.plugins.SystemIndexPlugin;
Expand Down Expand Up @@ -520,7 +522,8 @@ protected Node(final Environment initialEnvironment,

ActionModule actionModule = new ActionModule(settings, clusterModule.getIndexNameExpressionResolver(),
settingsModule.getIndexScopedSettings(), settingsModule.getClusterSettings(), settingsModule.getSettingsFilter(),
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService);
threadPool, pluginsService.filterPlugins(ActionPlugin.class), client, circuitBreakerService, usageService,
getRestCompatibleFunction());
modules.add(actionModule);

final RestController restController = actionModule.getRestController();
Expand Down Expand Up @@ -692,6 +695,22 @@ protected Node(final Environment initialEnvironment,
}
}

/**
* @return A function that can be used to determine the requested REST compatible version
*/
private CompatibleVersion getRestCompatibleFunction() {
Copy link
Member

Choose a reason for hiding this comment

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

can we add tests for this? Maybe in NodeTests

List<RestCompatibilityPlugin> restCompatibilityPlugins = pluginsService.filterPlugins(RestCompatibilityPlugin.class);
final CompatibleVersion compatibleVersion;
if (restCompatibilityPlugins.size() > 1) {
throw new IllegalStateException("Only one RestCompatibilityPlugin is allowed");
} else if (restCompatibilityPlugins.size() == 1) {
compatibleVersion = restCompatibilityPlugins.get(0)::getCompatibleVersion;
} else {
compatibleVersion = CompatibleVersion.CURRENT_VERSION;
}
return compatibleVersion;
}

protected TransportService newTransportService(Settings settings, Transport transport, ThreadPool threadPool,
TransportInterceptor interceptor,
Function<BoundTransportAddress, DiscoveryNode> localNodeFactory,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.plugins;

import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;


/**
* An extension point for Compatible API plugin implementation.
*/
public interface RestCompatibilityPlugin {
/**
* Returns a version which was requested on Accept and Content-Type headers
*
* @param acceptHeader - a media-type value from Accept header
* @param contentTypeHeader - a media-type value from Content-Type header
* @param hasContent - a flag indicating if a request has content
* @return a requested Compatible API Version
*/
Version getCompatibleVersion(@Nullable String acceptHeader, @Nullable String contentTypeHeader, Boolean hasContent)
throws ElasticsearchStatusException;
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
}
35 changes: 35 additions & 0 deletions server/src/main/java/org/elasticsearch/rest/CompatibleVersion.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.rest;

import org.elasticsearch.ElasticsearchStatusException;
import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;

/**
* An interface used to specify a function that returns a compatible API version
* Intended to be used in a code base instead of a plugin.
*/
@FunctionalInterface
public interface CompatibleVersion {
jaymode marked this conversation as resolved.
Show resolved Hide resolved
Version get(@Nullable String acceptHeader, @Nullable String contentTypeHeader, boolean hasContent) throws ElasticsearchStatusException;
pgomulka marked this conversation as resolved.
Show resolved Hide resolved

CompatibleVersion CURRENT_VERSION = (acceptHeader, contentTypeHeader, hasContent) -> Version.CURRENT;
}
28 changes: 21 additions & 7 deletions server/src/main/java/org/elasticsearch/rest/RestController.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.Strings;
Expand Down Expand Up @@ -88,11 +89,15 @@ public class RestController implements HttpServerTransport.Dispatcher {
/** Rest headers that are copied to internal requests made during a rest request. */
private final Set<RestHeaderDefinition> headersToCopy;
private final UsageService usageService;
private CompatibleVersion compatibleVersion;


public RestController(Set<RestHeaderDefinition> headersToCopy, UnaryOperator<RestHandler> handlerWrapper,
NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService) {
NodeClient client, CircuitBreakerService circuitBreakerService, UsageService usageService,
CompatibleVersion compatibleVersion) {
this.headersToCopy = headersToCopy;
this.usageService = usageService;
this.compatibleVersion = compatibleVersion;
if (handlerWrapper == null) {
handlerWrapper = h -> h; // passthrough if no wrapper set
}
Expand Down Expand Up @@ -218,7 +223,8 @@ public void dispatchBadRequest(final RestChannel channel, final ThreadContext th
}
}

private void dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler) throws Exception {
private void dispatchRequest(RestRequest request, RestChannel channel, RestHandler handler, Version compatibleApiVersion)
throws Exception {
final int contentLength = request.contentLength();
if (contentLength > 0) {
final XContentType xContentType = request.getXContentType();
Expand All @@ -240,7 +246,7 @@ private void dispatchRequest(RestRequest request, RestChannel channel, RestHandl
inFlightRequestsBreaker(circuitBreakerService).addWithoutBreaking(contentLength);
}
// iff we could reserve bytes for the request we need to send the response also over this channel
responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength);
responseChannel = new ResourceHandlingHttpChannel(channel, circuitBreakerService, contentLength, compatibleApiVersion);
// TODO: Count requests double in the circuit breaker if they need copying?
if (handler.allowsUnsafeBuffers() == false) {
request.ensureSafeBuffers();
Expand Down Expand Up @@ -309,6 +315,9 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
final String rawPath = request.rawPath();
final String uri = request.uri();
final RestRequest.Method requestMethod;
//TODO: USAGE_1 now that we have a version we can implement a REST handler that accepts path, method AND version
Version version = compatibleVersion.get(request.header("Accept"), request.header("Content-Type"), request.hasContent());

try {
// Resolves the HTTP method and fails if the method is invalid
requestMethod = request.method();
Expand All @@ -327,7 +336,7 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
return;
}
} else {
dispatchRequest(request, channel, handler);
dispatchRequest(request, channel, handler, version);
return;
}
}
Expand Down Expand Up @@ -445,12 +454,15 @@ private static final class ResourceHandlingHttpChannel implements RestChannel {
private final RestChannel delegate;
private final CircuitBreakerService circuitBreakerService;
private final int contentLength;
private final Version compatibleVersion;
private final AtomicBoolean closed = new AtomicBoolean();

ResourceHandlingHttpChannel(RestChannel delegate, CircuitBreakerService circuitBreakerService, int contentLength) {
ResourceHandlingHttpChannel(RestChannel delegate, CircuitBreakerService circuitBreakerService, int contentLength,
Version compatibleVersion) {
this.delegate = delegate;
this.circuitBreakerService = circuitBreakerService;
this.contentLength = contentLength;
this.compatibleVersion = compatibleVersion;
}

@Override
Expand All @@ -465,13 +477,15 @@ public XContentBuilder newErrorBuilder() throws IOException {

@Override
public XContentBuilder newBuilder(@Nullable XContentType xContentType, boolean useFiltering) throws IOException {
return delegate.newBuilder(xContentType, useFiltering);
return delegate.newBuilder(xContentType, useFiltering)
.withCompatibleMajorVersion(compatibleVersion.major);
}

@Override
public XContentBuilder newBuilder(XContentType xContentType, XContentType responseContentType, boolean useFiltering)
throws IOException {
return delegate.newBuilder(xContentType, responseContentType, useFiltering);
return delegate.newBuilder(xContentType, responseContentType, useFiltering)
.withCompatibleMajorVersion(compatibleVersion.major);
}

@Override
Expand Down
11 changes: 11 additions & 0 deletions server/src/main/java/org/elasticsearch/rest/RestHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.rest;

import org.elasticsearch.Version;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.xcontent.XContent;
import org.elasticsearch.rest.RestRequest.Method;
Expand Down Expand Up @@ -90,6 +91,16 @@ default List<ReplacedRoute> replacedRoutes() {
return Collections.emptyList();
}

/**
* Returns a version a handler is compatible with.
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
* This version is then used to math a handler with a request that specified a version.
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
* If no version is specified, handler is assumed to be compatible with <code>Version.CURRENT</code>
* @return a version
*/
default Version compatibleWithVersion() {
return Version.CURRENT;
}

class Route {

private final String path;
Expand Down
12 changes: 8 additions & 4 deletions server/src/main/java/org/elasticsearch/rest/RestRequest.java
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,18 @@ public boolean isContentConsumed() {
return contentConsumed;
}

// for testing
Copy link
Contributor Author

Choose a reason for hiding this comment

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

also if we make mediatypeparser even more strict, we could remove the RestRequest.parseContentType (line 504) and line 60
WDYT?

protected RestRequest(NamedXContentRegistry xContentRegistry, Map<String, String> params, String path,
Map<String, List<String>> headers, HttpRequest httpRequest, HttpChannel httpChannel) {
this(xContentRegistry, params, path, headers, httpRequest, httpChannel, requestIdGenerator.incrementAndGet());
}

protected RestRequest(RestRequest restRequest) {
this(restRequest.getXContentRegistry(), restRequest.params(), restRequest.path(), restRequest.getHeaders(),
restRequest.getHttpRequest(), restRequest.getHttpChannel(), restRequest.getRequestId());
}


private RestRequest(NamedXContentRegistry xContentRegistry, Map<String, String> params, String path,
Map<String, List<String>> headers, HttpRequest httpRequest, HttpChannel httpChannel, long requestId) {
final XContentType xContentType;
Expand All @@ -104,10 +111,7 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map<String, String>
this.requestId = requestId;
}

protected RestRequest(RestRequest restRequest) {
this(restRequest.getXContentRegistry(), restRequest.params(), restRequest.path(), restRequest.getHeaders(),
restRequest.getHttpRequest(), restRequest.getHttpChannel(), restRequest.getRequestId());
}


/**
* Invoke {@link HttpRequest#releaseAndCopy()} on the http request in this instance and replace a pooled http request
Expand Down
7 changes: 7 additions & 0 deletions server/src/test/java/org/elasticsearch/VersionTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -369,4 +369,11 @@ public void testUnreleasedVersion() {
VersionTests.assertUnknownVersion(VERSION_5_1_0_UNRELEASED);
}

public void testPreviousVersion(){
pgomulka marked this conversation as resolved.
Show resolved Hide resolved
Version current = Version.CURRENT;
assertThat(current.previousMajor(), equalTo(Version.fromString(Version.CURRENT.major - 1 + ".0.0")));
assertThat(Version.fromString("7.8.1").previousMajor(), equalTo(Version.fromString("6.0.0")));
assertThat(Version.V_EMPTY.previousMajor(), equalTo(Version.V_EMPTY));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import org.elasticsearch.common.settings.SettingsModule;
import org.elasticsearch.plugins.ActionPlugin;
import org.elasticsearch.plugins.ActionPlugin.ActionHandler;
import org.elasticsearch.rest.CompatibleVersion;
import org.elasticsearch.rest.RestChannel;
import org.elasticsearch.rest.RestController;
import org.elasticsearch.rest.RestHandler;
Expand Down Expand Up @@ -109,7 +110,7 @@ public void testSetupRestHandlerContainsKnownBuiltin() {
UsageService usageService = new UsageService();
ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(),
settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), null, emptyList(), null,
null, usageService);
null, usageService, CompatibleVersion.CURRENT_VERSION);
actionModule.initRestHandlers(null);
// At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail
Exception e = expectThrows(IllegalArgumentException.class, () ->
Expand Down Expand Up @@ -148,7 +149,7 @@ public String getName() {
UsageService usageService = new UsageService();
ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(),
settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), threadPool,
singletonList(dupsMainAction), null, null, usageService);
singletonList(dupsMainAction), null, null, usageService, CompatibleVersion.CURRENT_VERSION);
Exception e = expectThrows(IllegalArgumentException.class, () -> actionModule.initRestHandlers(null));
assertThat(e.getMessage(), startsWith("Cannot replace existing handler for [/] for method: GET"));
} finally {
Expand Down Expand Up @@ -182,7 +183,7 @@ public List<RestHandler> getRestHandlers(Settings settings, RestController restC
UsageService usageService = new UsageService();
ActionModule actionModule = new ActionModule(settings.getSettings(), new IndexNameExpressionResolver(),
settings.getIndexScopedSettings(), settings.getClusterSettings(), settings.getSettingsFilter(), threadPool,
singletonList(registersFakeHandler), null, null, usageService);
singletonList(registersFakeHandler), null, null, usageService, CompatibleVersion.CURRENT_VERSION);
actionModule.initRestHandlers(null);
// At this point the easiest way to confirm that a handler is loaded is to try to register another one on top of it and to fail
Exception e = expectThrows(IllegalArgumentException.class, () ->
Expand Down
Loading