Skip to content

fix(extraresources): preserve namespace when matching by labels - #612

Open
pujitha24 wants to merge 1 commit into
crossplane-contrib:mainfrom
pujitha24:auto/issue-597
Open

fix(extraresources): preserve namespace when matching by labels#612
pujitha24 wants to merge 1 commit into
crossplane-contrib:mainfrom
pujitha24:auto/issue-597

Conversation

@pujitha24

Copy link
Copy Markdown
Contributor

Description of your changes

ToResourceSelector converts an ExtraResourcesRequirement into a fnv1.ResourceSelector, but only copied the Namespace field onto the selector inside the matchName branch. Namespace is a top-level field on the proto ResourceSelector, independent of the match oneof (matchLabels vs matchName), so when a requirement combined matchLabels with a namespace, the namespace was silently dropped and the query matched labelled resources across all namespaces instead of the requested one. This reproduces the exact behavior reported in the issue: HTTPRoutes selected by matchLabels with a namespace set came back from every namespace, not just the specified one.

The fix moves the namespace assignment above the matchLabels/matchName branch so it is applied to both selector types instead of only matchName.

Added extraresources_test.go with TestToResourceSelector, covering matchLabels+namespace (the bug scenario), matchLabels without a namespace (cluster-scoped, must remain unset), and matchName+namespace (regression guard for the previously-working path). Ran go test -v -cover ./..., which passed (82.9% coverage); confirmed via git stash that the matchLabels+namespace case fails against the pre-fix code and passes with the fix. Also ran go build ./..., go vet ./..., go mod tidy (no diff), and golangci-lint run ./... at v2.4.0 (the version pinned in this repo's CI workflow), all with no issues. User-visible behavior for matchName selectors is unchanged; the only behavior change is that matchLabels selectors now honor the namespace field as documented, narrowing results to the requested namespace instead of matching cluster-wide.

Fixes #597

I have:


AI assistance: this change was drafted with Claude Code.

Motivation: ToResourceSelector converted an ExtraResourcesRequirement
into a fnv1.ResourceSelector, but only copied the Namespace field onto
the selector inside the matchName branch. Namespace is a top-level
field on the proto ResourceSelector, independent of the match oneof
(matchLabels vs matchName), so when a requirement combined matchLabels
with a namespace, the namespace was silently dropped and the query
matched labelled resources across all namespaces instead of the
requested one. This reproduces the exact behavior reported in the
issue: HTTPRoutes selected by matchLabels with a namespace set came
back from every namespace, not just the specified one.

Approach: move the namespace assignment above the matchLabels/
matchName branch so it is applied to both selector types instead of
only matchName.

Validation: added extraresources_test.go with TestToResourceSelector,
covering matchLabels+namespace (the bug scenario), matchLabels without
a namespace (cluster-scoped, must remain unset), and matchName+
namespace (regression guard for the previously-working path). Ran
`go test -v -cover ./...`, which passed (82.9% coverage); confirmed
via `git stash` that the matchLabels+namespace case fails against the
pre-fix code and passes with the fix. Also ran `go build ./...`,
`go vet ./...`, `go mod tidy` (no diff), and
`golangci-lint run ./...` at v2.4.0 (the version pinned in this repo's
CI workflow), all with no issues. User-visible behavior for matchName
selectors is unchanged; the only behavior change is that
matchLabels selectors now honor the namespace field as documented,
narrowing results to the requested namespace instead of matching
cluster-wide.

Report: crossplane-contrib#597
Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
@pujitha24

Copy link
Copy Markdown
Contributor Author

Just checking in on this one — it's rebased and all checks are green, happy to make any changes if something would help review go faster.

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.

Matchlabel criteria excludes namespace on ExtraResource

1 participant