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

Convert RestWrapper to an explicit Interceptor #104291

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Jan 12, 2024

Adds a new RestInterceptor interface and converts RestServerActionPlugin.getRestHandlerInterceptor to return this new type instead of a wrapping function.

This has the following benefits:

  • Less object creation, there is 1 instance of the interceptor class (see SecurityRestFilter) rather than an instance per handler
  • More control over the sequence of steps in processing a request. The explicit interceptor separates it from the deprecation handler or any validation that might be needed, and the controller can be intentional about the order in which these operations are applied.

Adds a new `RestInterceptor` interface and converts
`RestServerActionPlugin.getRestHandlerInterceptor` to return this new
type instead of a wrapping function.

This has the following benefits:
- Less object creation, there is 1 instance of the interceptor class
  (see `SecurityRestFilter`) rather than an instance per handler
- More control over the sequence of steps in processing a request.
  The explicit interceptor separates it from the deprecation handler
  or any validation that might be needed, and the controller can be
  intentional about the order in which these operations are applied.
@tvernum tvernum added :Core/Infra/REST API REST infrastructure and utilities v8.13.0 labels Jan 12, 2024
@tvernum tvernum requested a review from rjernst January 12, 2024 04:58
@tvernum
Copy link
Contributor Author

tvernum commented Jan 12, 2024

@elasticmachine update branch

@tvernum
Copy link
Contributor Author

tvernum commented Jan 12, 2024

I'm happy enough with this as a point along the journey, but it's not in the final state.

What it has:

  • A real RestInterceptor interface that is separate from a RestHandler (no more "wrapper"). We need an interceptor (not just a preprocessor) because the SecurityRestFilter needs to cleanup the thread context after execution.
  • The interceptor is explicitly asynchronous via an ActionListener (because authentication is async)
  • The interceptor is designed to be a bit less flexible than the wrapper was (e.g. it can't replace the request object) but in return it has a simpler contract. For example, the contract now allows the interceptor can fail with an exception, and the controller will serialize that exception on the response channel. This was previously only true for as long as the wrapper was synchronous - there was no way to pass exceptions back asynchronously and the interceptor had to convert those exceptions into a rest response itself.
  • An explicit hook in the controller to "validate" the request after the interceptor has been invoked, but before the handler is invoked. This hook is empty in the RestController but subclasses can implement it.

What is missing

  • The DeprecationRestHandler is still a wrapper. That thread is a bit complex to untangle because we have a lot of state in that handler (e.g. error messages that are contextual to the route that was accessed) and no good place to put that state. There are solutions, but I haven't tackled them in this PR.
  • The interceptor has an ActionListener<Boolean> to indicate whether to execute the handler. It shouldn't - it would be better to either let the handler run, or fail with an exception (that would give an error response) with no option to say that the interceptor handled the response itself. But the contract between SecurityRestFilter and OperatorPrivilegesService.checkRest is that the operator privileges service will send a response on the rest channel if it rejects the request, and I didn't have time to untangle that bit either. We can come back to it.

I'm happy for either:

  • This to get merged as is.
  • Someone else to pick it up.
  • It to sit and wait until I am available to pick it up again myself.

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I'm good with this as a first step. I think @jakelandis should take a quick look at the security side, but otherwise I will merge this PR and open some followups.

try {
sendFailure(finalChannel, e);
} catch (IOException ex) {
logger.info("Failed to send error [{}] to HTTP client", ex.toString());
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 we want to log the error that we failed to send, not the ioexception that the http channel spit back (or probably both)?

@jakelandis
Copy link
Contributor

jakelandis commented Jan 12, 2024

I think @jakelandis should take a quick look at the security side,

only had limited time and couldn't get into the details. ++ to the general direction. I never really liked that the OperatorOnlyRegistry handles sending the response on channel ... as alluded to in the comment, with this change I think we could refactor operator privs to not do that (and just throw an exception) and in turn (i think) would no longer need the new API to expose the boolean as part of the listener contract (which is kinda awkward).

@rjernst rjernst marked this pull request as ready for review January 12, 2024 23:19
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Jan 12, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@rjernst rjernst merged commit f3bc319 into elastic:main Jan 19, 2024
15 checks passed
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Mar 14, 2024
This commit changs `OperatorOnlyRegistry.checkRest` to handle failures
via an exception rather than a return value and the use of a channel

This fits better into the way that the `SecurityRestFilter` works
(since elastic#104291) with a dedicated `RestInterceptor` interface
elasticsearchmachine pushed a commit that referenced this pull request Mar 19, 2024
This commit changs `OperatorOnlyRegistry.checkRest` to handle failures
via an exception rather than a return value and the use of a channel

This fits better into the way that the `SecurityRestFilter` works (since
#104291) with a dedicated `RestInterceptor` interface
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities >refactoring Team:Core/Infra Meta label for core/infra team v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants