Skip to content

fix: [Bug] NamespaceV2 is not working#10608

Open
123123213weqw wants to merge 2 commits into
apache:developfrom
123123213weqw:wangyue/issue-9341
Open

fix: [Bug] NamespaceV2 is not working#10608
123123213weqw wants to merge 2 commits into
apache:developfrom
123123213weqw:wangyue/issue-9341

Conversation

@123123213weqw

Copy link
Copy Markdown

What is the purpose of the change

Fixes #9341.

NamespaceV2 does not isolate resources: a consumer in one namespace can consume messages produced by a producer in a different namespace.

Brief changelog

namespaceV2 was designed to be resolved server-side — the client attaches nsd/ns extension fields to each remoting request through NamespaceRpcHook, expecting the server to wrap the resource. However, the broker never reads those fields; it derives the namespace from the (already-wrapped) resource name via NamespaceUtil.getNamespaceFromResource. Meanwhile, all client-side resource wrapping (ClientConfig.withNamespace / withoutNamespace and their call sites in the producer/consumer) is driven by ClientConfig.getNamespace(), which only returns the deprecated V1 namespace and therefore stays empty when only namespaceV2 is configured. The net effect is that neither side wraps the resource, so producers and consumers in different namespaces hit the same physical topic.

This change makes ClientConfig.getNamespace() fall back to namespaceV2 when the V1 namespace is not set (the V1 namespace still takes precedence when both are configured). Because getNamespace() is the single source of truth used by every resource wrapping/unwrapping call site, topics and groups are now correctly prefixed with the namespaceV2 value, restoring isolation:

  • Producer (namespaceV2 = InstanceTest1) → InstanceTest1%NAMESPACE_TOPIC
  • Consumer (namespaceV2 = InstanceTest) → InstanceTest%NAMESPACE_TOPIC

NamespaceUtil.wrapNamespace is idempotent (isAlreadyWithNamespace), so this is safe both for the direct-to-broker path (which ignores nsd/ns) and for a proxy that performs its own resolution.

Verifying this change

Added unit tests in client/.../ClientConfigTest:

  • getNamespace() returns namespaceV2 when the V1 namespace is unset.
  • withNamespace / withoutNamespace wrap and unwrap using namespaceV2, and wrapping is idempotent.
  • The V1 namespace takes precedence over namespaceV2 when both are set.

Does this pull request potentially affect one of the following parts

  • Dependencies: No
  • Broker: No
  • Client: Yes — ClientConfig.getNamespace() now falls back to namespaceV2 for resource wrapping when the V1 namespace is unset.
  • Consumer (pull/push): No code change beyond the shared ClientConfig behavior.
  • Namesrv: No
  • Proxy: No

Documentation

  • Does this pull request introduce a new feature? No

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot

Summary

This PR fixes #9341 by making ClientConfig.getNamespace() fall back to namespaceV2 when the V1 namespace is not set, restoring namespace isolation for users who only configure namespaceV2.

Findings

  • [Info] client/src/main/java/org/apache/rocketmq/client/ClientConfig.java:417-419 — The fallback is correctly placed after the V1 check and before the namesrvAddr parsing, preserving the intended priority: V1 > V2 > namesrvAddr-derived. The return is side-effect-free (does not mutate namespace or set namespaceInitialized), which is consistent with how the V1 check on lines 413-415 behaves.

  • [Info] client/src/main/java/org/apache/rocketmq/client/ClientConfig.java:417-419 — One subtle point: since the V2 fallback does not cache the result into namespace or set namespaceInitialized = true, every call to getNamespace() will re-evaluate the StringUtils.isNotEmpty(namespaceV2) check. This is lightweight and matches the existing V1 pattern, so no action needed — just noting for awareness.

  • [Info] The fix is purely client-side, which is the right approach here. Since the broker derives namespace from the resource name via NamespaceUtil.getNamespaceFromResource, making the client correctly prefix resources with the V2 namespace value (e.g., InstanceTest%TOPIC) is sufficient to restore isolation. The NamespaceUtil.wrapNamespace idempotency ensures safety when a proxy also performs its own resolution.

  • [Info] Test coverage is solid — four test cases cover: basic fallback, wrapping with V2, unwrapping with V2, and V1 precedence. The idempotent wrapping test (withNamespace("InstanceTest%resource") returning the same value) is a nice touch.

Suggestions

No blocking concerns. The change is minimal, well-targeted, and correctly addresses the root cause described in the issue.

Cross-repo Note

No changes needed in apache/rocketmq-clients — this fix is in the legacy Java client (client/ module), not the gRPC-based multi-language SDK.


Automated review by github-manager-bot

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot

PR: Fix: NamespaceV2 is not working
Verdict: ✅ Approved

Summary

Fixes ClientConfig.getNamespace() to fall back to namespaceV2 when namespace (v1) is not set. Previously, if a user configured only namespaceV2 without namespace, the getNamespace() method would skip namespaceV2 entirely and attempt to parse the namespace from namesrvAddr, leading to incorrect behavior.

Analysis

Correctness

  • The fix adds the namespaceV2 fallback in the right position — after checking namespace but before falling back to parsing namesrvAddr.
  • The precedence is correct: namespace (v1) > namespaceV2 > namesrvAddr parsing.

Tests

  • 4 new test cases covering:
    • Fallback to namespaceV2 when v1 is unset
    • withNamespace / withoutNamespace using namespaceV2
    • V1 precedence over V2
  • Good coverage of the fix.

Compatibility

  • No breaking changes. Existing behavior with namespace set is unchanged.
  • Only affects the previously broken case where only namespaceV2 was configured.

Performance

  • Simple string check, no performance impact.

Verdict

Clean fix with good test coverage. Addresses a real usability gap in the namespace v2 support.

When only namespaceV2 is set, topics and consumer/producer groups are not
wrapped on the client side, because ClientConfig.getNamespace() — the single
value used by withNamespace/withoutNamespace and every resource wrapping call
site — only returns the deprecated V1 namespace, which stays empty.
namespaceV2 was intended to be resolved server-side via the nsd/ns extension
fields attached by NamespaceRpcHook, but the broker never consumes those
fields; instead it derives the namespace from already-wrapped resource names
(NamespaceUtil.getNamespaceFromResource). With neither side wrapping the
resource, producers and consumers configured with different namespaces operate
on the same physical topic, so a namespace can consume messages from another
namespace.
Make getNamespace() fall back to namespaceV2 when the V1 namespace is unset,
so that resources are wrapped with the namespaceV2 value and isolated
correctly. The V1 namespace still takes precedence when both are set.

Signed-off-by: 王越 <1939455790@qq.com>
@123123213weqw

Copy link
Copy Markdown
Author

@lizhimins @RongtongJin gentle ping — this fixes #9341 (NamespaceV2 is not working), which multiple users have confirmed in the issue ("5.3.2 also has this problem", "Namespace settings are completely a mystery"). The root cause is in ClientConfig — see the PR body for the trace and the one-line fix.

The PR is MERGEABLE against develop but CI runs are stuck in action_required (first-time contributor approval). Could one of you approve the workflow runs so the Maven/bazel/CodeQL checks can start? Happy to address any review feedback.

@itxaiohanglover

Copy link
Copy Markdown

Confirmed the namespace-isolation gap this addresses. The added ClientConfigTest covers the nsd/ns resolution — could you also assert the cross-namespace negative case (a consumer in ns-A must NOT see ns-B messages)? That's the exact symptom reported in #9341 and would harden the regression guard.

Adds a regression guard for the cross-namespace isolation symptom
reported in apache#9341: two clients configured with different namespaceV2
values must resolve the same resource to different fully-qualified
names, so a consumer in InstanceA cannot accidentally address
InstanceB's resources.

The test also covers the negative case explicitly — configA.withoutNamespace
on a resource resolved under configB's namespace must NOT reduce to the
bare resource, confirming the prefix is namespace-pinned and the two
clients stay isolated.

Requested in apache#10608 code review.
@123123213weqw

Copy link
Copy Markdown
Author

@itxaiohanglover good catch — added the negative case you asked for in c2c4e86.

testNamespaceV2IsolatesResourcesAcrossNamespaces builds two clients (InstanceA / InstanceB) and asserts:

  1. Each client pins its own namespace prefix: configA.withNamespace("resource") == "InstanceA%resource", configBInstanceB%resource.
  2. The two resolutions don't collide (positive guard).
  3. Negative isolation case: configA.withoutNamespace("InstanceB%resource") does NOT reduce to the bare resource — it stays prefixed with the foreign namespace, so configA can't "claim" it. This is the exact symptom reported in [Bug] NamespaceV2 is not working #9341: before the fix, namespaceV2 was ignored, both clients resolved to the same un-prefixed resource, and consumers in one namespace could see the other.

Local CI is in action_required (first-time-contributor approval), so the test hasn't been validated by the project's GitHub Actions yet — happy to share repro steps or run them on a box if that helps.

@123123213weqw

Copy link
Copy Markdown
Author

@itxaiohanglover done — pushed in c2c4e86. Added testNamespaceV2IsolatesResourcesAcrossNamespaces covering both the positive (each client pins its own namespace prefix) and the negative (a resource resolved under InstanceB does NOT reduce to bare resource when stripped via configA, so the two clients stay isolated). Ready for another look.

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Review by github-manager-bot

Summary

Fixes NamespaceV2 isolation failure (#9341): when only namespaceV2 is configured (V1 namespace unset), ClientConfig.getNamespace() now falls back to namespaceV2, ensuring producers and consumers in different namespaces correctly resolve to different fully-qualified resource names.

Findings

  • [Info] ClientConfig.java:417-419 — The 4-line fix is correctly placed in the precedence chain: V1 namespacenamespaceV2 → namesrvAddr parsing. V1 still takes precedence when both are set, preserving backward compatibility.
  • [Info] ClientConfig.java:417 — Unlike the namesrvAddr branch below, the namespaceV2 return does not cache into the namespace field. This is correct — namespaceV2 is a direct field read (no parsing cost), and caching it into namespace would conflate the two distinct configuration sources.
  • [Info] NamespaceUtil.wrapNamespace is idempotent (isAlreadyWithNamespace check), so the fix is safe for both direct-to-broker and proxy-mediated paths.

Suggestions

  • No code changes suggested. The fix is minimal and correct.
  • Consider adding a @since or inline comment noting that namespaceV2 fallback was added in this PR, to help future maintainers understand the precedence chain's evolution.

Verdict

Excellent minimal fix with outstanding test coverage — especially testNamespaceV2IsolatesResourcesAcrossNamespaces which serves as a regression guard for the exact cross-namespace isolation symptom. LGTM.


Automated review by github-manager-bot

@itxaiohanglover

Copy link
Copy Markdown

Verified the new testNamespaceV2IsolatesResourcesAcrossNamespaces in c2c4e86 — the negative assertions (resolutions must not collide, and configA.withoutNamespace(resolvedB) must not strip B's prefix) cover exactly the #9341 symptom. LGTM from my side, thanks for the quick turnaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] NamespaceV2 is not working

3 participants