pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

Add new tenant operation LIST_ANTI_AFFINITY_NAMESPACES and fix validation of getAntiAffinityNamespaces

Open equanz opened this issue 3 years ago • 6 comments

Motivation

Since https://github.com/apache/pulsar/pull/6428, we can define authz policy to each namespace operation. However, I think the operation policy validateNamespacePolicyOperation(namespaceName, PolicyName.ANTI_AFFINITY, PolicyOperation.READ); is not suitable for getAntiAffinityNamespaces method because this method is a tenant-wise operation. (is not specific namespace-wise operation)

Currently, the validation method throws NPE when using default authz provider org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider. https://github.com/apache/pulsar/pull/13723#issuecomment-1021007049

Modifications

  • Add new tenant operation LIST_ANTI_AFFINITY_NAMESPACES
  • Modify operate validation to LIST_ANTI_AFFINITY_NAMESPACES

If this approach is not accepted, I think we can use an existing operation like LIST_NAMESPACES instead of adding a new tenant operation.

Verifying this change

  • [x] Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

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: (yes)
    • modified endpoint's validation
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

  • [x] no-need-doc

equanz avatar Jan 12 '22 08:01 equanz

@equanz After the change, how do the users using the old version migrate to the new version? We will introduce a breaking change in this PR, we should discuss in the dev email thread first.

codelipenghui avatar Jan 12 '22 14:01 codelipenghui

@codelipenghui Sorry for the late reply.

If user defines custom authz provider and follow namespaceName=null, policy=PolicyName.ANTI_AFFINITY, operation=PolicyOperation.READ as correct state, they should change definition when updating the version. Therefore, we should consider migration plan just like you said.

By the way, I think the validation method may throw NPE when using default authz provider org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider. https://github.com/apache/pulsar/blob/8c8738f26cad0edf91d6b97f4c103680517900d9/pulsar-broker-common/src/main/java/org/apache/pulsar/broker/authorization/PulsarAuthorizationProvider.java#L598 Therefore, it is one of issue fix for default authz provider. Moreover, this endpoint doesn't return correct value in the latest version. (Please see https://github.com/apache/pulsar/pull/13644)

...
2022-01-25T18:43:37,980+0900 [pulsar-web-30-6] ERROR org.apache.pulsar.broker.web.AuthenticationFilter - [127.0.0.1] Error performing authentication for HTTP
javax.servlet.ServletException: java.lang.NullPointerException
	at org.glassfish.jersey.servlet.WebComponent.serviceImpl(WebComponent.java:410) ~[jersey-container-servlet-core-2.34.jar:?]
	at org.glassfish.jersey.servlet.WebComponent.service(WebComponent.java:346) ~[jersey-container-servlet-core-2.34.jar:?]
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:366) ~[jersey-container-servlet-core-2.34.jar:?]
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:319) ~[jersey-container-servlet-core-2.34.jar:?]
	at org.glassfish.jersey.servlet.ServletContainer.service(ServletContainer.java:205) ~[jersey-container-servlet-core-2.34.jar:?]
	at org.eclipse.jetty.servlet.ServletHolder.handle(ServletHolder.java:799) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1631) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.apache.pulsar.broker.web.ResponseHandlerFilter.doFilter(ResponseHandlerFilter.java:67) ~[classes/:?]
	at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.apache.pulsar.broker.web.AuthenticationFilter.doFilter(AuthenticationFilter.java:81) ~[pulsar-broker-common-2.10.0-SNAPSHOT.jar:?]
	at org.eclipse.jetty.servlet.FilterHolder.doFilter(FilterHolder.java:193) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1601) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.servlet.ServletHandler.doHandle(ServletHandler.java:548) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.session.SessionHandler.doHandle(SessionHandler.java:1624) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextHandle(ScopedHandler.java:233) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ContextHandler.doHandle(ContextHandler.java:1434) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:188) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.servlet.ServletHandler.doScope(ServletHandler.java:501) ~[jetty-servlet-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.session.SessionHandler.doScope(SessionHandler.java:1594) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ScopedHandler.nextScope(ScopedHandler.java:186) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ContextHandler.doScope(ContextHandler.java:1349) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ScopedHandler.handle(ScopedHandler.java:141) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:234) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.HandlerCollection.handle(HandlerCollection.java:146) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.StatisticsHandler.handle(StatisticsHandler.java:179) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.handler.HandlerWrapper.handle(HandlerWrapper.java:127) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.Server.handle(Server.java:516) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.HttpChannel.lambda$handle$1(HttpChannel.java:400) ~[jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.HttpChannel.dispatch(HttpChannel.java:645) [jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:392) [jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:277) [jetty-server-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:311) [jetty-io-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105) [jetty-io-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.io.ssl.SslConnection$DecryptedEndPoint.onFillable(SslConnection.java:555) [jetty-io-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:410) [jetty-io-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.io.ssl.SslConnection$2.succeeded(SslConnection.java:164) [jetty-io-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:105) [jetty-io-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.io.ChannelEndPoint$1.run(ChannelEndPoint.java:104) [jetty-io-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.runTask(EatWhatYouKill.java:338) [jetty-util-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.doProduce(EatWhatYouKill.java:315) [jetty-util-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.tryProduce(EatWhatYouKill.java:173) [jetty-util-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.util.thread.strategy.EatWhatYouKill.run(EatWhatYouKill.java:131) [jetty-util-9.4.44.v20210927.jar:9.4.44.v20210927]
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:409) [jetty-util-9.4.44.v20210927.jar:9.4.44.v20210927]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_312]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_312]
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30) [netty-common-4.1.72.Final.jar:4.1.72.Final]
	at java.lang.Thread.run(Thread.java:748) [?:1.8.0_312]
Caused by: java.lang.NullPointerException
	at org.apache.pulsar.broker.authorization.PulsarAuthorizationProvider.allowNamespacePolicyOperationAsync(PulsarAuthorizationProvider.java:598) ~[pulsar-broker-common-2.10.0-SNAPSHOT.jar:2.10.0-SNAPSHOT]
	at org.apache.pulsar.broker.authorization.AuthorizationService.allowNamespacePolicyOperationAsync(AuthorizationService.java:445) ~[pulsar-broker-common-2.10.0-SNAPSHOT.jar:2.10.0-SNAPSHOT]
	at org.apache.pulsar.broker.authorization.AuthorizationService.allowNamespacePolicyOperationAsync(AuthorizationService.java:463) ~[pulsar-broker-common-2.10.0-SNAPSHOT.jar:2.10.0-SNAPSHOT]
	at org.apache.pulsar.broker.authorization.AuthorizationService.allowNamespacePolicyOperation(AuthorizationService.java:474) ~[pulsar-broker-common-2.10.0-SNAPSHOT.jar:2.10.0-SNAPSHOT]
	at org.apache.pulsar.broker.web.PulsarWebResource.validateNamespacePolicyOperation(PulsarWebResource.java:898) ~[classes/:?]
	at org.apache.pulsar.broker.admin.impl.NamespacesBase.internalGetAntiAffinityNamespaces(NamespacesBase.java:1873) ~[classes/:?]
	at org.apache.pulsar.broker.admin.v1.Namespaces.getAntiAffinityNamespaces(Namespaces.java:453) ~[classes/:?]
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) ~[?:1.8.0_312]
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) ~[?:1.8.0_312]
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) ~[?:1.8.0_312]
	at java.lang.reflect.Method.invoke(Method.java:498) ~[?:1.8.0_312]
	at org.glassfish.jersey.server.model.internal.ResourceMethodInvocationHandlerFactory.lambda$static$0(ResourceMethodInvocationHandlerFactory.java:52) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher$1.run(AbstractJavaResourceMethodDispatcher.java:124) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.invoke(AbstractJavaResourceMethodDispatcher.java:167) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.model.internal.JavaResourceMethodDispatcherProvider$TypeOutInvoker.doDispatch(JavaResourceMethodDispatcherProvider.java:219) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.model.internal.AbstractJavaResourceMethodDispatcher.dispatch(AbstractJavaResourceMethodDispatcher.java:79) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.invoke(ResourceMethodInvoker.java:475) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:397) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.model.ResourceMethodInvoker.apply(ResourceMethodInvoker.java:81) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.ServerRuntime$1.run(ServerRuntime.java:255) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:248) ~[jersey-common-2.34.jar:?]
	at org.glassfish.jersey.internal.Errors$1.call(Errors.java:244) ~[jersey-common-2.34.jar:?]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:292) ~[jersey-common-2.34.jar:?]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:274) ~[jersey-common-2.34.jar:?]
	at org.glassfish.jersey.internal.Errors.process(Errors.java:244) ~[jersey-common-2.34.jar:?]
	at org.glassfish.jersey.process.internal.RequestScope.runInScope(RequestScope.java:265) ~[jersey-common-2.34.jar:?]
	at org.glassfish.jersey.server.ServerRuntime.process(ServerRuntime.java:234) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.server.ApplicationHandler.handle(ApplicationHandler.java:680) ~[jersey-server-2.34.jar:?]
	at org.glassfish.jersey.servlet.WebComponent.serviceImpl(WebComponent.java:394) ~[jersey-container-servlet-core-2.34.jar:?]
	... 48 more
2022-01-25T18:43:38,000+0900 [pulsar-web-30-6] INFO  org.eclipse.jetty.server.RequestLog - 127.0.0.1 - - [25/1/2022:18:43:37 +0900] "GET /admin/namespaces/test/antiAffinity/group?property=my-property HTTP/1.1" 500 174 "-" "Pulsar-Java-v2.10.0-SNAPSHOT" 35
...

equanz avatar Jan 25 '22 09:01 equanz

One of the alternative approaches is using an existing definition of operation. For example, validate TenantOperation.LIST_NAMESPACES for a tenant and PolicyName.ANTI_AFFINITY, PolicyOperation.READ for each namespace. https://github.com/apache/pulsar/blob/8c8738f26cad0edf91d6b97f4c103680517900d9/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L1886 https://github.com/apache/pulsar/blob/8c8738f26cad0edf91d6b97f4c103680517900d9/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/NamespacesBase.java#L1888-L1898

This approach doesn't add a new definition of operation. However, it requires TenantOperation.LIST_NAMESPACES to run the operation. Therefore, I think it is one of the breaking changes. In addition, validate tenant admin many times on the default authz provider.

Another approach is using only TenantOperation.LIST_NAMESPACES. It validates tenant admin single times on the default authz provider. However, it also doesn't follow existing behavior and doesn't support granularity.

equanz avatar Jan 27 '22 03:01 equanz

@codelipenghui Could you check these comments please?

equanz avatar Feb 07 '22 07:02 equanz

The pr had no activity for 30 days, mark with Stale label.

github-actions[bot] avatar Mar 10 '22 02:03 github-actions[bot]

The pr had no activity for 30 days, mark with Stale label.

github-actions[bot] avatar Jun 10 '22 02:06 github-actions[bot]