[feature][broker] Allow to configure the entry filters per namespace and per topic

[gaozhangmin]

Fixes #16870

Motivation

Allow to configure the filters per namespace and per topic

Modifications

Support config namespace and topic level entry filter policy through admin api.

pulsar-admin namespaces set-entry-filters
pulsar-admin namespaces get-entry-filters
pulsar-admin namespaces remove-entry-filters

pulsar-admin topics set-entry-filters
pulsar-admin topics get-entry-filters
pulsar-admin topics remove-entry-filters

Verifying this change

org.apache.pulsar.broker.service.plugin.FilterEntryTest
org.apache.pulsar.broker.service.AbstractBaseDispatcherTest#testFilterEntriesForConsumerOfEntryFilter
org.apache.pulsar.broker.admin.AdminApi2Test#testSetNamespaceEntryFilters
org.apache.pulsar.broker.admin.AdminApi2Test#testSetTopicLevelEntryFilters

Documentation

Check the box below or label this PR directly.

Need to update docs?

• doc-required
  (Your PR needs to update docs and you will update later)

• doc-not-needed
  (Please explain why)

• doc
  (Your PR contains doc changes)

• doc-complete
  (Docs have been already added)

[eolivelli]

This is a great work!
thank you very much for providing this implementation.

I have left some feedback:

• do not set the directory, it is broker specific (and not all the brokers may have the same filesystem layout)
• do not add a description, we never do that in Pulsar, and it is for very low benefit
• still keep the global list of EntryFilters configured on broker.conf (otherwise local caches won't work)

you can see this production implementation of a EntryFilter

https://github.com/datastax/pulsar-jms/blob/master/pulsar-jms-filters/src/main/java/com/datastax/oss/pulsar/jms/selectors/JMSFilter.java

[eolivelli]

I have two questions.

It seems to me that with this patch the entryFilters cannot be configured in broker.conf anymore, is this correct ? (in that case we have to restore such support, otherwise existing users that upgrade will have problems)

Ideally I would like to see that in a topic the entryFilters that are applied are the sum of: per-broker entry filters + per-namespace entry filters and per-topic entry filters.

This way in a multi-tenant environment the administrator can have control over what is happening.

Also we should add a flag to allow setting per-namespace and per-topic entry filters.

in some environments the system administrator doesn't want to let the users activate custom behaviour to the users (tenant/namespace operators)

[eolivelli]

@gaozhangmin we can chat on slack if you want

[gaozhangmin]

@Jason918 PTAL, Thx

[gaozhangmin]

entryFilters cannot be configured in broker.conf anymore, is this correct

1、it's still supported configuring filters via broker.conf, default value is initialized in
org.apache.pulsar.broker.service.AbstractTopic#updateTopicPolicyByBrokerConfig
2、Reference other topic level policies, I don't think we need a flag there.
@eolivelli

[eolivelli]

@gaozhangmin thanks for your clarification. it makes sense to me.

I have two remaining points:

• I don't find tests about configuring the filters "per broker"
• I do think that the filters must be "additive" and the namespace/topic filters should not override the broker filters

The broker filters may have been set by the system administrator to enforce some rules and if a user (tenant admin?) is able to override the list of filters that that would be some kind of security hole.

If you feel strong that we should keep the behaviour of this patch, then I would ask you to add a configuration parameter to make this behaviour configurable, in order to allow system administrators to enforce some entry filters

[gaozhangmin]

@eolivelli Add a configuration parameter allowOverrideEntryFilters, default to false.
About tests on filters "per broker", I don't change the logic, I believe it's already covered.

[eolivelli]

@gaozhangmin thank you for adding the parameter.
The patch now looks generally good.

One last comment: we are missing tests for the new option: we need tests that validate that the option is really working.
After adding the tests I am +1 to this patch, great work !

[gaozhangmin]

@eolivelli What you mean for new option is the configuration allowOverrideEntryFilters, Right?

[eolivelli]

@eolivelli What you mean for new option is the configuration allowOverrideEntryFilters, Right?

correct

[eolivelli]

LGTM