Skip to content

feat: enable namespaced reference resolution#843

Merged
jbw976 merged 1 commit intocrossplane:mainfrom
jbw976:v2-providers
Jun 9, 2025
Merged

feat: enable namespaced reference resolution#843
jbw976 merged 1 commit intocrossplane:mainfrom
jbw976:v2-providers

Conversation

@jbw976
Copy link
Copy Markdown
Member

@jbw976 jbw976 commented Jun 6, 2025

Description of your changes

This PR updates the Resolve methods to take into a namespace when resolving references. This is needed for v2 compatible providers that have namespaced managed resources.

I considered updating ResolutionRequest.Reference itself to become a reference type that includes a namespace field, but I decided against that for the following reasons:

  • it's not needed - cluster scoped legacy MRs refer to other cluster scoped MRs and don't need to specify a namespace in the reference itself. namespaced MRs refer to other namespaced MRs and don't need to specify a namespace in the reference either - it's inferred from the namespace of the MR.
  • it would be a (semi) breaking change to add a new field to that type that is used all over the crossplane ecosystem

This PR is against main since Crossplane core development has moved onto v2 now.

Part of crossplane/crossplane#6381

Related PR: crossplane/crossplane-tools#110

These changes have been tested as demonstrated in this demo repo: https://github.com/jbw976/demo-upjet-namespaced-refs

I have:

Need help with this checklist? See the cheat sheet.

@jbw976 jbw976 requested a review from a team as a code owner June 6, 2025 08:50
@jbw976 jbw976 requested a review from negz June 6, 2025 08:50

// The reference was not set, but a selector was. Select a reference.
if err := r.client.List(ctx, req.To.List, client.MatchingLabels(req.Selector.MatchLabels)); err != nil {
if err := r.client.List(ctx, req.To.List, client.MatchingLabels(req.Selector.MatchLabels), client.InNamespace(req.Namespace)); err != nil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might be worth adding a note that InNamespace is a no-op if the namespace is empty. I always find that ambiguous when reading it.

Copy link
Copy Markdown
Member Author

@jbw976 jbw976 Jun 9, 2025

Choose a reason for hiding this comment

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

good call, just pushed an update to include a comment on this behavior!

Signed-off-by: Jared Watts <jbw976@gmail.com>
@jbw976 jbw976 merged commit 0063d29 into crossplane:main Jun 9, 2025
10 checks passed
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.

3 participants