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

Add broker interceptor for Intercepting all Pulsar command and REST API requests #7143

Merged
merged 18 commits into from
Jun 9, 2020

Conversation

codelipenghui
Copy link
Contributor

Motivation

Broker Interceptors

Modifications

Describe the modifications you've done.

Verifying this change

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@jerrypeng
Copy link
Contributor

@codelipenghui thanks for working on something like this. The funny thing is we have developed an "interceptor" framework as well. It covers all the REST endpoints as well. Maybe we can collaborate?

@sijie
Copy link
Member

sijie commented Jun 3, 2020

@jerrypeng do you want to send out the pull request? so we can merge efforts here.

@merlimat
Copy link
Contributor

merlimat commented Jun 4, 2020

Added PR: #7159

@sijie sijie added this to the 2.6.0 milestone Jun 8, 2020
category = CATEGORY_INTERCEPTORS,
doc = "The directory to locate interceptors"
)
private String interceptorDirectory = "./interceptors";
Copy link
Member

Choose a reason for hiding this comment

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

overall looks good to me. I see there are two sets of settings here, one is for listeners and the other one is for interceptors? I think we should just rename the listener interfaces to Interceptors. So it is consistent with client-side interceptor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

@codelipenghui codelipenghui changed the title [WIP] Broker Interceptors Intercept all Pulsar command and REST API requests. Jun 8, 2020
@@ -450,7 +453,9 @@ public void start() throws PulsarServerException {

this.defaultOffloader = createManagedLedgerOffloader(
OffloadPolicies.create(this.getConfiguration().getProperties()));

this.brokerInterceptor = BrokerInterceptors.load(config);
brokerService.setInterceptor(getBrokerInterceptor());
Copy link
Member

Choose a reason for hiding this comment

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

We should support multiple broker interceptors. Can you add a broker interceptor implementation that wraps a list of interceptors into one interceptor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation is supporting multiple broker interceptors, I added BrokerInterceptors.java also implement BrokerInterceptor interface.

@merlimat merlimat requested a review from jerrypeng June 8, 2020 13:49
@codelipenghui codelipenghui merged commit dbc0649 into apache:master Jun 9, 2020
@codelipenghui codelipenghui deleted the server_interceptors branch June 9, 2020 05:16
@codelipenghui codelipenghui changed the title Intercept all Pulsar command and REST API requests. Add broker interceptor for Intercepting all Pulsar command and REST API requests Jun 9, 2020
@jerrypeng
Copy link
Contributor

@merlimat put up our version of this. I thought we were going to work together merge both our implementation?

@merlimat
Copy link
Contributor

merlimat commented Jun 9, 2020

There are many points that are missing in this implementation. Especially on how to give a proper error to the client when some operation is denied.

@merlimat
Copy link
Contributor

merlimat commented Jun 9, 2020

Also, this is exposing various internal classes to the interceptor API. That means that these classes are now becoming part of public API, which is not a good idea since they were not meant to be.

@sijie
Copy link
Member

sijie commented Jun 10, 2020

@merlimat @jerrypeng I think this is to set up the framework so the broker can load interceptors from NAR packages. The interface is very basic and only focuses on intercepting connections. We don't expect this interface to the public at this moment. It is the first step to implement interceptors. We should iterate the interface and add the pull request #7159 that @merlimat sent for intercepting per restful call.

Since these two pull requests have different focuses (NAR class loading, connection level interception vs per-rest-call interception) and they can be merged together, I don't see we need to block one pull request for the other pull request. Especially in any case, we need to load the plugin from NAR package just as what we did for other plugins, correct?

cdbartholomew pushed a commit to kafkaesque-io/pulsar that referenced this pull request Jul 24, 2020
…PI requests. (apache#7143)

Add broker interceptor for Intercepting all Pulsar command and REST API requests
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
…PI requests. (apache#7143)

Add broker interceptor for Intercepting all Pulsar command and REST API requests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants