[fix][broker] Fix create ns by nodece · Pull Request #17864 · apache/pulsar

nodece · 2022-09-28T02:11:28Z

Motivation

The bundles policy of the namespace is ignored if the namespace is created directly by the NamespaceResources, see the logic of admin service:

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/v2/Namespaces.java

Lines 1956 to 1969 in 9e4daea

    
           private Policies getDefaultPolicesIfNull(Policies policies) { 
        
               if (policies == null) { 
        
                   policies = new Policies(); 
        
               } 
        
               int defaultNumberOfBundles = config().getDefaultNumberOfNamespaceBundles(); 
        
               if (policies.bundles == null) { 
        
                   policies.bundles = getBundles(defaultNumberOfBundles); 
        
               } 
        
               return policies; 
        
           }

Modifications

Use admin to create a namespace instead of the NamespaceResources

Documentation

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)

Matching PR in forked repository

nodece#4

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

Jason918 · 2022-09-30T09:09:13Z

-            Policies nsp = new Policies();
-            nsp.replication_clusters = Collections.singleton(config.getClusterName());
-            nsr.createPolicies(ns, nsp);
+            broker.getAdminClient().namespaces().createNamespace(ns.toString());


Sorry, I don't get it, why nsr.createPolicies(ns, nsp); is ignored?

Some policy is ignored because the conf file effects this policy.

eolivelli

Lgtm

What about adding a test case?

nodece · 2022-09-30T16:16:22Z

Lgtm

What about adding a test case?

I think we should add an integration test, what do you think about that?

BewareMyPower · 2022-12-09T07:59:38Z

Could you clarify which policies are affected?

nodece · 2022-12-09T08:23:59Z

Could you clarify which policies are affected?

See the PR description.

BewareMyPower · 2022-12-09T08:25:45Z

If only the number of bundles were affected, could you use another solution? This PR brings a breaking change to the standalone. We need to add an integration test to protect it.

nodece · 2022-12-09T08:28:20Z

If only the number of bundles were affected, could you use another solution?

No.

This PR brings a breaking change to the standalone. We need to add an integration test to protect it.

Good catch. Let me add an integration test for this.

BewareMyPower · 2022-12-09T08:38:08Z

For how to reproduce it, you can reference the pulsar-test-service-start.sh, which uses standalone-ssl.conf as the config file. In additional, even if Pulsar service is ready was printed, the standalone actually didn't start successfully. You can log in the docker container and run bin/pulsar standalone -nss -nfw directly, you should see the following logs:

2022-12-09T07:45:12,311+0000 [main] INFO  org.apache.pulsar.broker.PulsarService - created admin with url http://localhost:8080
2022-12-09T07:45:12,664+0000 [pulsar-web-46-15] ERROR org.apache.pulsar.broker.admin.v2.Namespaces - [anonymous] Failed to create namespace public/default
java.util.concurrent.CompletionException: org.apache.pulsar.broker.web.RestException: Unauthorized to validateTenantOperation for originalPrincipal [null] and clientAppId [anonymous] about operation [CREATE_NAMESPACE] on tenant [public]
        at java.util.concurrent.CompletableFuture.encodeThrowable(CompletableFuture.java:315) ~[?:?]
        at java.util.concurrent.CompletableFuture.uniAcceptNow(CompletableFuture.java:761) ~[?:?]
        at java.util.concurrent.CompletableFuture.uniAcceptStage(CompletableFuture.java:735) ~[?:?]
        at java.util.concurrent.CompletableFuture.thenAccept(CompletableFuture.java:2182) ~[?:?]
        at org.apache.pulsar.broker.web.PulsarWebResource.validateTenantOperationAsync(PulsarWebResource.java:995) ~[org.apache.pulsar-pulsar-broker-2.11.0.jar:2.11.0]
        at org.apache.pulsar.broker.admin.impl.NamespacesBase.internalCreateNamespace(NamespacesBase.java:138) ~[org.apache.pulsar-pulsar-broker-2.11.0.jar:2.11.0]
        at org.apache.pulsar.broker.admin.v2.Namespaces.createNamespace(Namespaces.java:166) ~[org.apache.pulsar-pulsar-broker-2.11.0.jar:2.11.0]

nodece · 2022-12-09T08:43:38Z

Because you didn't set up the brokerClientAuthenticationPlugin and brokerClientAuthenticationParameters in the standalone-ssl.conf.

BewareMyPower · 2022-12-09T09:27:26Z

-            Policies nsp = new Policies();
-            nsp.replication_clusters = Collections.singleton(config.getClusterName());
-            nsr.createPolicies(ns, nsp);


Why not fix it by something like:

nsp.bundles = /* ... */;

It would be more clear for what's the default bundle policy of the standalone and not affected by the PulsarAdmin implementations.

So far, I've only found the bundle issue, I'm not sure about the other factors, so using the admin.

BewareMyPower · 2022-12-09T09:28:19Z

Because you didn't set up the brokerClientAuthenticationPlugin and brokerClientAuthenticationParameters in the standalone-ssl.conf.

The previous deployment also didn't set up these two configs and it has worked for a long time.

BewareMyPower · 2022-12-09T09:35:08Z

It's a real-world breaking change, not something like "it's an API that might already be used somewhere". The current semantics have already been applied in the standalone deployment of the C++/Python client tests.

I just confirmed that the Golang client tests would also fail after this change.

https://github.com/apache/pulsar-client-go/blob/055b00b83ccfef4ea12a593e3678cb4bdd18311c/integration-tests/conf/standalone.conf#L112-L115

Other ecosystems are not confirmed. But I think it's enough.

Breaking change is a breaking change not for "how it should be used", but for "whether it has already been used".

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

### Motivation apache#17864 and apache#18837 tried to fix the regression of the namespace bundle policy in standalone by creating the namespace via `PulsarAdmin`. It brings a problem that the namespace would not be created successfully if the authentication of the built-in admin client was not configured. It brings the regression that makes the set up of the C++ client's unit tests in branch-2.11 fail. Because the steps are: - Start a standalone - Update the policy of `public/default` via `pulsar-admin` without creating the namespace ### Modifications Do not use `PulsarAdmin` to create the namespace when starting the standalone. Instead, set the bundle policy according to the `defaultNumberOfNamespaceBundles` config. Add a `testStandaloneWithRocksDB` to test the namespace can still be created even if no broker client authentication is configured. Then move the bundle policy test from `SmokeTest` to this test.

### Motivation apache#15186 changed the behavior of how to create the default namespace. However, it brings a regression that even if the built-in admin didn't have the authentication configured while the standalone enabled the authentication, the namespace could still be created successfully. This PR also changed the deploy script to remove the creation of "public/default" namespace. Instead, it grants the permission to this namespace directly. After apache#17864 and apache#18837, the namespace will be created by the built-in admin again. But the deploy script would fail. ### Modifications Create the default namespace via `pulsar-admin` in the deploy script.

nodece requested review from codelipenghui, eolivelli and merlimat September 28, 2022 02:11

nodece added release/2.11.1 area/broker release/blocker Indicate the PR or issue that should block the release until it gets resolved labels Sep 28, 2022

nodece requested review from Jason918, RobertIndie, Technoboy-, lhotari, mattisonchao and michaeljmarshall September 28, 2022 02:12

github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 28, 2022

[fix][broker] Fix create ns

a1835b0

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

nodece force-pushed the fix-create-ns branch 2 times, most recently from 4981f0d to a1835b0 Compare September 30, 2022 04:06

nodece added the ready-to-test label Sep 30, 2022

Jason918 reviewed Sep 30, 2022

View reviewed changes

Jason918 approved these changes Sep 30, 2022

View reviewed changes

RobertIndie approved these changes Sep 30, 2022

View reviewed changes

eolivelli approved these changes Sep 30, 2022

View reviewed changes

nodece merged commit e1b537f into apache:master Oct 8, 2022

nodece mentioned this pull request Dec 6, 2022

[branch-2.11][fix][broker] Fix create ns #18755

Merged

4 tasks

BewareMyPower removed release/blocker Indicate the PR or issue that should block the release until it gets resolved release/2.11.1 labels Dec 9, 2022

BewareMyPower reviewed Dec 9, 2022

View reviewed changes

Technoboy- assigned nodece Dec 12, 2022

Technoboy- added this to the 2.12.0 milestone Dec 12, 2022

Technoboy- added release/2.11.0 cherry-picked/branch-2.11 labels Dec 12, 2022

Technoboy- pushed a commit that referenced this pull request Dec 12, 2022

[fix][broker] Fix create ns (#17864)

4294a0f

Signed-off-by: Zixuan Liu <nodeces@gmail.com>

This was referenced Dec 13, 2022

[branch-2.11][fix][cpp] Fix the standalone set up BewareMyPower/pulsar#13

Closed

[branch-2.11][fix][cpp] Fix the standalone set up #18907

Closed

	private Policies getDefaultPolicesIfNull(Policies policies) {
	if (policies == null) {
	policies = new Policies();
	}

	int defaultNumberOfBundles = config().getDefaultNumberOfNamespaceBundles();

	if (policies.bundles == null) {
	policies.bundles = getBundles(defaultNumberOfBundles);
	}

	return policies;
	}

Uh oh!

Conversation

nodece commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modifications

Documentation

Matching PR in forked repository

Uh oh!

Jason918 Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

nodece Sep 30, 2022

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

nodece commented Sep 30, 2022

Uh oh!

BewareMyPower commented Dec 9, 2022

Uh oh!

nodece commented Dec 9, 2022

Uh oh!

BewareMyPower commented Dec 9, 2022

Uh oh!

nodece commented Dec 9, 2022

Uh oh!

BewareMyPower commented Dec 9, 2022

Uh oh!

nodece commented Dec 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BewareMyPower Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

nodece Dec 9, 2022

Choose a reason for hiding this comment

Uh oh!

BewareMyPower commented Dec 9, 2022

Uh oh!

BewareMyPower commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nodece commented Sep 28, 2022 •

edited

Loading

nodece commented Dec 9, 2022 •

edited

Loading