[fix][broker] Fix wrong behaviour when using namespace.allowed_clusters, such as namespace deletion and namespace policies updating

[poorbarcode]

Motivation

Before PIP-321 Introduce allowed-cluster at the namespace level, Pulsar does not support enabling topic level Geo Replication without enabling namespace level Geo Replication, because the policies namespace.replication_clusters has two meanings:

1. Which cluster is allowed to access the namespace?
2. Which clusters enabled Geo Replication?

PIP-321 Introduce allowed-cluster at the namespace level defined a new policy namespace.allowed_clusters, which splits the two definitions.

• if allowed_clusters is not empty
  • allowed_clusters can be used to define which cluster is allowed to access the namespace, if it is set.
• otherwise: the replication_clusters defines both statuses.

---

3 Issues

PIP-321 did not complete all the adaptations of codes, such as follows

• 1. Expected behaviour: when a cluster permission was removed from a namespace, Pulsar will unload the namespace to prevent the topics under the namespace from being loaded up again.
  • Issue: the cluster is still defined allowed by namespace.allowed_clusters, the namespace is also unloaded. More importantly, in this case, all brokers will trigger an unloading, which will cause this namespace to remain unavailable for a long time. Every time any policy of the namespace is updated, unload will be triggered again for a round, which will cause the problem to become so serious as to be uncontrollable. The test testUpdateNamespacePolicies is used to reproduce the issue.
• 2. Expected behaviour: a namespace can only be removed if only one cluster has permission to access it.
  • Issue: the namespace can be deleted even if allowed_cluster is defined as 2 clusters are allowed to access. The test testDeleteNamespaceIfTwoClustersAllowed is used to reproduce the issue.
• 3. Expected behaviour: broker will unload the namespace if the isolation policy is changed, to make the new policy apply.
  • Issue: the namespace will not be unloaded if the allowed permission is defined by namespace.allowed_cluster. The test testUpdateNamespaceIsolationPolicy is used to reproduce the issue.

---

Modifications

• Fix the 3 issues
• Rather than check replication_clusters and allowed_clusters anywhere, use a common method to deal with them, including the following checks
  • Whether a cluster is allowed to access a namaspace.
  • New policy checking.

Documentation

• doc
• doc-required
• doc-not-needed
• doc-complete

Matching PR in forked repository

PR in forked repository: x

[lhotari]

please check this test failure:

  Error:  Tests run: 9, Failures: 1, Errors: 0, Skipped: 4, Time elapsed: 3.943 s <<< FAILURE! -- in org.apache.pulsar.broker.lookup.http.HttpTopicLookupv2Test
  Error:  org.apache.pulsar.broker.lookup.http.HttpTopicLookupv2Test.testValidateReplicationSettingsOnNamespace -- Time elapsed: 0.018 s <<< FAILURE!
  java.lang.AssertionError: expected [404] but found [412]
  	at org.testng.Assert.fail(Assert.java:110)
  	at org.testng.Assert.failNotEquals(Assert.java:1577)
  	at org.testng.Assert.assertEqualsImpl(Assert.java:149)
  	at org.testng.Assert.assertEquals(Assert.java:131)
  	at org.testng.Assert.assertEquals(Assert.java:1418)
  	at org.testng.Assert.assertEquals(Assert.java:1382)
  	at org.testng.Assert.assertEquals(Assert.java:1428)
  	at org.apache.pulsar.broker.lookup.http.HttpTopicLookupv2Test.testValidateReplicationSettingsOnNamespace(HttpTopicLookupv2Test.java:281)
  	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
  	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
  	at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:139)
  	at org.testng.internal.invokers.InvokeMethodRunnable.runOne(InvokeMethodRunnable.java:47)
  	at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:76)
  	at org.testng.internal.invokers.InvokeMethodRunnable.call(InvokeMethodRunnable.java:11)
  	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
  	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
  	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
  	at java.base/java.lang.Thread.run(Thread.java:1583)

[poorbarcode]

please check this test failure:

Sure

[codecov-commenter]

Codecov Report

❌ Patch coverage is 74.48980% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.20%. Comparing base (a091ea7) to head (1b1d7fd).
⚠️ Report is 370 commits behind head on master.

Files with missing lines	Patch %	Lines
...pache/pulsar/broker/admin/impl/NamespacesBase.java	62.50%	9 Missing and 6 partials ⚠️
...g/apache/pulsar/common/policies/data/Policies.java	72.00%	5 Missing and 2 partials ⚠️
...rg/apache/pulsar/broker/service/BrokerService.java	85.71%	3 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #24860      +/-   ##
============================================
- Coverage     74.29%   74.20%   -0.10%     
+ Complexity    33825    33471     -354     
============================================
  Files          1913     1913              
  Lines        149281   149375      +94     
  Branches      17325    17347      +22     
============================================
- Hits         110902   110837      -65     
- Misses        29540    29682     +142     
- Partials       8839     8856      +17     

Flag	Coverage Δ
inttests	26.41% <19.38%> (+<0.01%)	⬆️
systests	22.78% <14.28%> (-0.03%)	⬇️
unittests	73.73% <74.48%> (-0.08%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
.../org/apache/pulsar/broker/admin/AdminResource.java	75.57% <ø> (ø)
.../apache/pulsar/broker/admin/impl/ClustersBase.java	84.08% <100.00%> (+0.05%)	⬆️
...pulsar/broker/admin/impl/PersistentTopicsBase.java	69.48% <ø> (ø)
...rg/apache/pulsar/broker/service/AbstractTopic.java	88.43% <100.00%> (+0.08%)	⬆️
...sar/broker/service/persistent/PersistentTopic.java	79.14% <100.00%> (+0.03%)	⬆️
...rg/apache/pulsar/broker/web/PulsarWebResource.java	65.15% <100.00%> (-0.06%)	⬇️
...rg/apache/pulsar/broker/service/BrokerService.java	83.31% <85.71%> (+<0.01%)	⬆️
...g/apache/pulsar/common/policies/data/Policies.java	59.82% <72.00%> (+3.30%)	⬆️
...pache/pulsar/broker/admin/impl/NamespacesBase.java	76.49% <62.50%> (-0.28%)	⬇️

... and 95 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lhotari]

@poorbarcode Should this fix target also branch-3.0?

[poorbarcode]

@lhotari

Should this fix target also branch-3.0?

Following the rule described on the website, I think we can skip to cherry-pick it into branch-3.0 since it is not s security issue. If other committers need it, they can drive cherry-picking.

[lhotari]

Following the rule described on the website, I think we can skip to cherry-pick it into branch-3.0 since it is not s security issue. If other committers need it, they can drive cherry-picking.

@poorbarcode The support policy is about Apache Pulsar PMC's commitment to perform releases. If there's a severe bug that impacts branch-3.0, it makes sense to target the branch-3.0 as long as necessary.
Since this seems to be an important bug fix, wouldn't it make sense to cherry-pick also to branch-3.0 ? There are a lot of community users that haven't yet upgraded to 4.0.x and beyond.