Skip to content

Comments

fix: ProviderConfig reconciler to filter usages by namespace#935

Open
kostyaplis wants to merge 1 commit intocrossplane:mainfrom
kostyaplis:fix/providerconfig-namespace-filter
Open

fix: ProviderConfig reconciler to filter usages by namespace#935
kostyaplis wants to merge 1 commit intocrossplane:mainfrom
kostyaplis:fix/providerconfig-namespace-filter

Conversation

@kostyaplis
Copy link

Description of your changes

Fixes crossplane/crossplane#7154

The fix adds client.InNamespace(pc.GetNamespace()) to the list options when the ProviderConfig is namespaced. ClusterProviderConfigs are unaffected and they continue to list across all namespaces.

Manual test:

  • Built provider-upjet-azure binary with the patched crossplane-runtime
  • Packaged it as a Docker image using crossplane xpkg build and pushed to public repo using crossplane xpkg push
  • Deployed to a cluster with multiple namespaced ProviderConfigs and verified that status.users counts are scoped correctly per namespace. While ClusterProviderConfigs are not affected. Simple test case included in the issue: Namespaced ProviderConfigUsage should consider ProviderConfig namespace. crossplane#7154

I have:

Need help with this checklist? See the cheat sheet.

@kostyaplis kostyaplis requested a review from a team as a code owner February 21, 2026 12:07
@kostyaplis kostyaplis requested a review from negz February 21, 2026 12:07
Signed-off-by: Konstantin Plis <plis.konstantin@gmail.com>
@kostyaplis kostyaplis force-pushed the fix/providerconfig-namespace-filter branch from 5a6881e to 75f8eb9 Compare February 21, 2026 12:09
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 21, 2026

📝 Walkthrough

Walkthrough

The reconciler now builds a variadic slice of client ListOptions (including an InNamespace option when the ProviderConfig has a namespace) and passes them to client.List to enumerate ProviderConfigUsages scoped appropriately; error handling and events on List failures are preserved.

Changes

Cohort / File(s) Summary
ProviderConfig Reconciliation
pkg/reconciler/providerconfig/reconciler.go
Replaced a direct client.List call that passed a single matching-label argument with a variadic ListOptions slice. When the ProviderConfig specifies a namespace, an InNamespace option is appended so ProviderConfigUsages are listed within that namespace. Error handling and logging remain unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the fix—filtering ProviderConfig usages by namespace—and stays within the 72-character limit.
Description check ✅ Passed The description is thorough and directly related to the changeset, referencing the linked issue and explaining the manual testing performed.
Linked Issues check ✅ Passed The code changes align with issue #7154's objective: adding namespace filtering to ProviderConfig usage counting ensures deletion checks only consider usages within the same namespace.
Out of Scope Changes check ✅ Passed The changes are narrowly scoped to the ProviderConfig reconciler's List operation, directly addressing the namespace filtering requirement with no extraneous modifications.
Breaking Changes ✅ Passed The PR modifies only internal implementation in pkg/reconciler/providerconfig/reconciler.go without changing any exported API functions, types, or methods.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
pkg/reconciler/providerconfig/reconciler.go (1)

165-169: Nitpick: consider adding "namespace" to the log context alongside the new namespace-scoped behavior.

Now that pc.GetNamespace() drives a meaningful branch in the reconcile loop, including it in the structured log values would make per-namespace debugging much easier (e.g., when two same-named ProviderConfigs are reconciled concurrently, log lines will be distinguishable without needing to cross-reference the UID):

🔍 Suggested observability improvement
 log = log.WithValues(
     "uid", pc.GetUID(),
     "version", pc.GetResourceVersion(),
     "name", pc.GetName(),
+    "namespace", pc.GetNamespace(),
 )
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/reconciler/providerconfig/reconciler.go` around lines 165 - 169, Add the
ProviderConfig namespace to the structured logger context so per-namespace
debugging is possible: update the log.WithValues call in reconciler.go (the
block that sets log = log.WithValues("uid", pc.GetUID(), "version",
pc.GetResourceVersion(), "name", pc.GetName())) to include "namespace" with
pc.GetNamespace(); ensure the key is exactly "namespace" and the value uses
pc.GetNamespace() so logs for Reconcile/ProviderConfig handling (e.g., in the
reconcile loop and any functions using that log) are disambiguated when
same-named resources exist in different namespaces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@pkg/reconciler/providerconfig/reconciler.go`:
- Around line 181-184: Add unit tests that verify the namespace-scoping behavior
around the listOpts construction in reconciler.go: create two table-driven test
cases for the Reconciler handling of ProviderConfig (one where
fake.ProviderConfig has a Namespace set and one with an empty Namespace), then
assert that the client.List call is invoked with
client.InNamespace(pc.GetNamespace()) when pc.GetNamespace() != "" and that no
InNamespace option is passed when cluster-scoped; exercise the code path that
builds listOpts (the code using listOpts := []client.ListOption{matchingLabels}
and the branch with pc.GetNamespace()) and use a fake/client that records
received List options to make the assertions.

---

Nitpick comments:
In `@pkg/reconciler/providerconfig/reconciler.go`:
- Around line 165-169: Add the ProviderConfig namespace to the structured logger
context so per-namespace debugging is possible: update the log.WithValues call
in reconciler.go (the block that sets log = log.WithValues("uid", pc.GetUID(),
"version", pc.GetResourceVersion(), "name", pc.GetName())) to include
"namespace" with pc.GetNamespace(); ensure the key is exactly "namespace" and
the value uses pc.GetNamespace() so logs for Reconcile/ProviderConfig handling
(e.g., in the reconcile loop and any functions using that log) are disambiguated
when same-named resources exist in different namespaces.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@pkg/reconciler/providerconfig/reconciler.go`:
- Around line 181-184: Add unit tests for the namespaced vs cluster-scoped
list-scoping behavior introduced around listOpts/listOpts =
append(...client.InNamespace...) in reconciler.go: create a table-driven test in
pkg/reconciler/providerconfig/reconciler_test.go that constructs
fake.ProviderConfig instances with Namespace: "test-ns" and Namespace: ""
(fake.ProviderConfig embeds metav1.ObjectMeta), calls the reconciler method that
invokes client.List (use the same function under test that builds listOpts), and
assert that when pc.GetNamespace() == "test-ns" the client.List call receives
both matchingLabels and client.InNamespace("test-ns"), and when
pc.GetNamespace() == "" the client.List call receives only matchingLabels; use
the existing fake client/test harness to inspect the List call arguments and add
two assertions (one per case) to prevent regression.

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.

Namespaced ProviderConfigUsage should consider ProviderConfig namespace.

1 participant