fix(namespace): converge bookie affinity group instead of blind-writing

[david-streamlio]

Fixes #420.

spec.bookieAffinityGroup already existed on PulsarNamespace and was already reconciled, so this is not a new feature — it is a fix to the reconcile path, plus the validation and docs the field was missing. Rationale for the reframing is in this issue comment.

The bug

Every PulsarNamespace that did not declare bookieAffinityGroup issued an unconditional DELETE .../persistence/bookieAffinity on each reconcile, with no IsNotFound tolerance — unlike the neighbouring RemoveTopicAutoCreation and RemoveInactiveTopicPolicies branches in the same function:

} else {
    err = p.adminClient.Namespaces().DeleteBookieAffinityGroup(completeNSName)
    if err != nil {
        return err
    }
}

Pulsar implements that delete as a set of a null group (NamespacesBase#internalDeleteBookieAffinityGroupAsync → internalSetBookieAffinityGroupAsync(null)), and both the setter and the getter call validateSuperUserAccessAsync(). Consequences:

1. A tenant-admin connection cannot reconcile any namespace. It gets 401/403 on that DELETE, ApplyNamespace fails, and the namespace never reaches Ready — for users who never touched bookie affinity at all.
2. Spurious metadata writes. setLocalPoliciesWithCreate creates the local-policies znode with default bundle data even when there was nothing to delete.
3. No read of current state, so "removing the setting removes the policy" was assumed rather than verified.

The fix

applyBookieAffinityGroup reads the current group first and writes only on an actual diff. Both of Pulsar's "no group" answers are normalized to nil: a 404 "Namespace local-policies does not exist" when the namespace has no local policies, and a 200 carrying an empty group when they were cleared.

Reading is superuser-gated too (internalGetBookieAffinityGroupAsync calls validateSuperUserAccessAsync, same as the setter), so read-before-write on its own does not lift the superuser requirement — it just moves it from a DELETE to a GET. A tenant-admin connection is denied either way, and the error still aborts applyNamespacePolicies before every policy that follows.

So the denial is handled explicitly, split by intent:

• No group requested (bookieAffinityGroup absent) — there is nothing to converge, so a 401/403 on the read is logged and skipped rather than failed. This is what actually unblocks reconcile for namespaces that never use the feature. The trade-off is that a group set out of band is retained rather than removed; the log says so.
• A group requested — the operator has been told to write, so a permission failure surfaces. Silently skipping it would leave the CR claiming a configuration Pulsar never received.

Any other read failure still propagates rather than being mistaken for "unset".

The 401/403 test is IsPermissionDenied(err), a new helper in pkg/admin/errors.go. ReasonUnauthorized and ReasonForbidden were already declared there but had no consumers anywhere in the repo; this is the first.

Also in this PR

• Corrected the BookieAffinityGroup godoc. It read "is the name of the namespace isolation policy to apply to the namespace", which describes PulsarNSIsolationPolicy — a different feature. That wording is why Expose bookie affinity groups (namespace-level bookie isolation) as a managed resource #420 was filed as "not exposed through any CRD".
• MinLength=1 on both group names. An empty primary passed CRD validation and was pushed to Pulsar, where BookieRackAffinityMapping cannot place ledgers. Only "" becomes invalid, so no working configuration is rejected.
• Documented the end-to-end carve-out — new "Broker and Bookie Isolation" section in docs/pulsar_namespace.md pairing PulsarNSIsolationPolicy (brokers) with bookieAffinityGroup (bookies), including the rack-metadata prerequisite and the superuser requirement. Cross-linked from docs/pulsar_ns_isolation_policy.md.

Tests

• pkg/admin/bookie_affinity_group_test.go — 7 table cases over an httptest server in the style of namespace_backlog_quota_test.go: unset stays unset without writing, cleared group is not deleted again, removal deletes, set when absent, matching group left alone, changed group rewritten, dropped secondary rewritten. Plus read-failure propagation.
• tests/operator/resources_test.go — a PulsarNamespace Bookie Affinity Group context covering create → update → empty-primary rejection → field removal, mirroring the offload-policies coverage from feat(namespace): add offload policies support #413.
• pkg/admin/bookie_affinity_group_test.go — the permission cases from @freeznet's review, driven through applyNamespacePolicies rather than the helper in isolation, because the combination that broke is "field omitted" and "read denied": affinity endpoint returns 401/403 with no group requested → no error and a later policy still applied; the same denial with a group requested → still fails, and the reconcile does not continue past a write it could not make. Both tolerance cases fail on the previous head of this branch.
• pkg/admin/errors_test.go — IsPermissionDenied across 401/403/404/500, wrapped and pointer rest.Error, and non-REST errors.

Verified: go build ./..., go test ./pkg/... ./api/... (including the existing applyNamespacePolicies backlog-quota tests, which now traverse the new read path), make fmt vet, make license-check (479 files, 0 invalid). E2E not executed — needs a live cluster via ADMIN_SERVICE_URL.

Note for reviewers

The CRD schema was updated by hand, deliberately. make manifests pulls controller-gen v0.17.0 (pinned at Makefile:201) but the committed CRDs were generated with v0.15.0. Running it rewrites all 19 CRDs — ~1300 lines of unrelated churn — and strips the Apache license header from every YAML. Only the bookieAffinityGroup block was applied here, byte-matching the generator's output. The Makefile/CRD version skew is pre-existing and wants its own PR.

The sn-operator half of #420 (declarative bookie-to-isolation-group membership on BookKeeperCluster) is a genuinely separate gap and belongs on that repo — nothing here can express it.

🤖 Generated with Claude Code

[github-actions]

@david-streamlio:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

[freeznet]

The read-before-write approach reduces redundant writes, but the tenant-admin reconciliation failure remains because the new read requires the same superuser permission. The existing affinity unit tests pass; additional HTTP-backed cases with an omitted group and a 401/403 GET both fail. Please address the permission boundary before merging.

[freeznet]

[P1] Avoid failing on the privileged read when no affinity group is requested

This GET is unconditional, including when desired == nil. Pulsar's NamespacesBase.internalGetBookieAffinityGroupAsync() calls validateSuperUserAccessAsync() before reading local policies, so a tenant-admin connection still receives 401 (or 403) even when no group exists. The error returned here still aborts applyNamespacePolicies before subsequent policies and prevents the namespace from becoming Ready; replacing DELETE with GET therefore does not fix the permission problem described in this PR. Please make permission failures non-blocking for the omitted-field path (with a log explaining that an existing group may be retained), while preserving failures for explicitly requested groups and other errors. Add cases that call applyNamespacePolicies with BookieAffinityGroup: nil, return 401/403 from the affinity endpoint, and assert that a later policy is applied. I ran those two cases against this head and both fail; the existing TestApplyBookieAffinityGroup* tests pass because they do not cover this combination.

[david-streamlio]

Fixed in cc0ed65.

applyBookieAffinityGroup now splits the denial by intent rather than treating every read error the same way:

• desired == nil — nothing to converge, so a 401/403 on the read is logged and skipped instead of returned. This is the path that was blocking reconcile for namespaces that never touch the feature.
• group requested — unchanged, the failure still surfaces. Skipping it would leave the CR claiming a configuration Pulsar never received.

Everything else propagates as before.

The test is a new IsPermissionDenied(err) next to IsNotFound in pkg/admin/errors.go. ReasonUnauthorized and ReasonForbidden were already declared there but had no consumers anywhere in the repo, so an inline status-code comparison would just have left the same gap for the next person.

Tests are the two you specified, driven through applyNamespacePolicies rather than the helper — you were right that the isolation was the reason the existing cases missed this, since the breaking combination is "field omitted" and "read denied" together:

• 401 and 403 with BookieAffinityGroup: nil → no error, and autoTopicCreation (the policy immediately after the affinity block) still applied.
• 401 and 403 with a group requested → still fails, and the reconcile does not continue past the write it could not make.
• IsPermissionDenied across 401/403/404/500, wrapped and pointer rest.Error, non-REST errors.

I confirmed both tolerance cases fail on 754d4cb and pass on cc0ed65 — matching what you saw. go build ./..., go vet, and go test ./pkg/... are green.

I also corrected the PR description: the "removes the superuser dependency" claim was wrong, and it now says plainly that read-before-write alone just moved the requirement from a DELETE to a GET.

Still open from my earlier comment, and I did not implement it unilaterally: the retained-group case is currently visible only in operator logs, so a tenant-admin user who removes bookieAffinityGroup gets a Ready CR while Pulsar keeps the old group. Want a status condition on the PulsarNamespace alongside the log, or is the log sufficient for now? Happy to add it in this PR or leave it for a follow-up.

[david-streamlio]

@freeznet Thanks — the P1 is correct, and it's a real hole in this PR. I traced it end to end and every link holds:

1. internalGetBookieAffinityGroup() opens with validateSuperUserAccess() — NamespacesBase.java:936 on branch-3.0, same gate as the setter at line 893. My own PR description says this and then draws the wrong conclusion from it.
2. applyBookieAffinityGroup calls getBookieAffinityGroup as its first statement, three lines above the if desired == nil branch. So every namespace pays the privileged read whether or not it uses the feature.
3. IsNotFound is ErrorReason(err) == ReasonNotFound (pkg/admin/errors.go:74) — 404 only. A 401/403 falls straight through return nil, err.
4. applyNamespacePolicies returns on that error, and the affinity block sits ahead of topic auto-creation and inactive-topic policies, so those are skipped and the namespace never reaches Ready.

So consequence #1 in the PR description — the headline bug — is not fixed here. I swapped an unconditional superuser-gated DELETE for an unconditional superuser-gated GET. Consequences #2 (spurious setLocalPoliciesWithCreate znode writes) and #3 (blind write with no read of current state) are genuinely fixed, but the sentence "removes the superuser dependency for namespaces that don't use the feature" is false as implemented. I'll correct the description along with the code.

Proposed approach

Your shape is the right one, and I want to be explicit about why the two obvious alternatives are worse:

• Skip the GET entirely when desired == nil — simpler, but it also stops a superuser connection from removing a group when the field is dropped. That's a regression against today's behaviour.
• Tolerate 401/403 on every path — silently no-ops a group the user explicitly asked for.

So: tolerate permission failures only on the omitted-field path, keep them fatal when a group is explicitly requested. Concretely —

1. Add an IsPermissionDenied(err) helper next to IsNotFound in pkg/admin/errors.go. ReasonUnauthorized and ReasonForbidden are already declared there but have no consumers anywhere in the repo — this would be the first, and I'd rather add the helper than inline a status-code comparison that the next person has to rediscover.
2. In applyBookieAffinityGroup, treat a permission-denied read as "unknown, and not our business" only when desired == nil: log and return nil. Any other read error, and any read error at all when a group is requested, still propagates.

One thing I'd like your read on

Your suggestion was a log line explaining that an existing group may be retained. My concern is that this creates spec/reality drift that's only visible in operator logs: a tenant-admin user who removes bookieAffinityGroup gets a CR reporting Ready while Pulsar quietly keeps the old group.

Would you prefer a status condition on the PulsarNamespace (e.g. BookieAffinityGroupNotReconciled) alongside the log, so it surfaces where the user is actually looking? It's more surface area than you asked for, so I don't want to add it unilaterally.

Tests

Taking your guidance as written — driving applyNamespacePolicies rather than the helper in isolation is the part my current tests miss:

• BookieAffinityGroup: nil + affinity endpoint returns 401 → no error, and a later policy still applied.
• Same with 403.
• Group explicitly requested + 401/403 → still fails loudly.

The existing TestApplyBookieAffinityGroup* cases pass because they exercise the helper directly and never combine "field omitted" with "read denied." That combination is exactly the reconcile path real tenant-admin users hit.

Two areas still unreviewed

While you're back in here, these didn't draw comment and I'd value a look:

• MinLength=1 on the group names. I claimed "no working configuration is rejected," which is true functionally, but stored CRs aren't revalidated until someone updates them — so an existing CR with an empty primary would start getting its updates rejected. I think that's the behaviour we want; flagging it as a slightly stronger change than I first described.
• The hand-edited CRD schema. Rationale is in the PR description under "Note for reviewers" — the controller-gen version skew between Makefile:201 (v0.17.0) and the committed CRDs (v0.15.0). Happy to split that into its own PR either way.

I'll push the fix once you've weighed in on the status-condition question.

[freeznet]

Re-reviewed cc0ed65. The previous P1 is addressed: an omitted affinity group now tolerates a denied read (401/403) and continues applying subsequent namespace policies, while explicitly requested groups and other read errors still fail. The retained-group behavior is logged.

Validation: go test ./pkg/admin -count=1 passed, including the two independent omitted-field 401/403 reproductions that failed on the previous head. The affinity and permission tests also passed with -race. All current CI checks are successful, including both operator E2E jobs. I did not run a live tenant-admin cluster locally.

No new blocking findings. Logging the retained-group case is sufficient for this scoped fix; a dedicated status condition can be considered separately.