Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Select ProviderConfig via labels #319

Open
MisterMX opened this issue Feb 11, 2022 · 9 comments
Open

Select ProviderConfig via labels #319

MisterMX opened this issue Feb 11, 2022 · 9 comments
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@MisterMX
Copy link
Contributor

What problem are you facing?

Crossplane managed resources currently support referencing a ProviderConfig via its object name through spec.providerConfigRef.name.

This requires the name to be known when creating the MR. However, this is not always the case. For example, if one creates an EKS cluster through a composition that also generates a new helm.crossplane.io/ProviderConfig, the name will have a dynamic hash appended as prefix. In this case one would have to wait for the composite resource and copy-paste the name before deploying resources on the cluster.

How could Crossplane help solve your problem?

Since one can add the composite labels to the ProviderConfig such as crossplane.io/claim-name and crossplane.io/claim-namespace one could select the respective provider config using this labels.

This way I could install a helm Release by referencing the config through a providerConfigSelector, i.e.

apiVersion: helm.crossplane.io/v1beta1
kind: Release
spec:
  forProvider:
    # ...
  providerConfigSelector:
    matchLabels:
       crossplane.io/claim-name: my-cluster
       crossplane.io/claim-namespace: my-project
@MisterMX MisterMX added the enhancement New feature or request label Feb 11, 2022
@negz
Copy link
Member

negz commented Feb 12, 2022

I think this is a duplicate of crossplane/crossplane#1699, though I'm tempted to keep this newer issue as it has a little more detail.

This is also loosely related to crossplane/crossplane#2255.

FWIW I'm a strong +1 on this and would be happy to see it implemented.

@muvaf
Copy link
Member

muvaf commented Apr 21, 2022

@MisterMX would you be interested in contributing this? I think we can have another reference resolver specific to this and possibly have it placed in a chain before the default APISimpleReferenceResolver.

@MisterMX
Copy link
Contributor Author

@muvaf yes, I can have a look at it, although my time is currently limited. But I am very interested in this feature.

@jbw976 jbw976 added the help wanted Extra attention is needed label Apr 26, 2022
@jbw976
Copy link
Member

jbw976 commented Apr 26, 2022

cool @MisterMX! if you don't get the time to do this soon, that's OK. There may be some other folks interested in taking on this somewhat scoped but beneficial feature.

If you start to work on it officially, please do assign yourself to it so we can coordinate :)

@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Aug 13, 2022
@AaronME
Copy link

AaronME commented Sep 12, 2023

We're running into this as well, given that we want to create both Kubernetes Clusters and things that launch to those kubernetes clusters from various Claims.

We currently make a best-effort attempt to match a generic environment parameter to the name of a cluster. And then we write every composite with a clusterOverride parameter, which tends to become the default.

With a selector, the environment could be patched as a label to generated ProviderConfigs and then targeted by claims with a similar label.

@jeanduplessis jeanduplessis removed the wontfix This will not be worked on label Sep 12, 2023
@muvaf muvaf added the good first issue Good for newcomers label Sep 19, 2023
@jeanduplessis jeanduplessis added this to the v1.15 milestone Oct 9, 2023
@jbw976 jbw976 removed this from the v1.15 milestone Jan 24, 2024
@jbw976 jbw976 moved this to Backlog in Crossplane Roadmap Jan 24, 2024
@twobiers
Copy link

I think this feature would be also very useful for us and I would like to take a look into this.
My plan would be the following:

  • Add a ProviderConfigSelector of type LabelSelector to MR Spec
  • Create a new APIProviderConfigReferenceResolver
  • The resolving logic will be delegated to the MR (Same as for Resource References)
    • This means, every MR needs to implement some ResolveProviderConfigReference() function
  • I think resolving a provider configuration reference and resource reference can take place after each other within the reconcilation phase. Maybe those can be consolidated somehow.

Does it sound reasonable? I'm wondering if there's a way to implement that without much effort of provider maintainers, e.g. by generating the reference resolver with angryjet.

@jbw976
Copy link
Member

jbw976 commented Aug 31, 2024

Thanks for your enthusiasm/willingness to take this on @twobiers! 🙇‍♂️

A question for other maintainers, since this isn't an area I'm super familiar with: would further investments in a cross resource reference based experience still be supported now that "extra resources" are available? I think this was suggested in crossplane/crossplane#4627 (comment), but I'm not sure if ProviderConfigs would be a special case or not 🤔

/cc @negz @phisco @turkenh @bobh66

For your proposal @twobiers, it could be a good idea to explore further with some prototyping to better learn the boundaries of this problem and how the existing framework could support it. Question for you: Does this need to be a separate type of reference resolver, or can the existing ResolveReferences() for MRs be used to also resolve a ProviderConfig reference, just like it does all other types? Invocation in the managed reconciler: https://github.com/crossplane/crossplane-runtime/blob/release-1.17/pkg/reconciler/managed/reconciler.go#L930

@twobiers
Copy link

Thanks for pointing extra resources out, I think that should do the trick in general.

Speaking of "native" support, the ResolveReferences() method for most of the providers is generated when // +crossplane:generate:reference is set on a type. Resolving a ProviderConfig reference from that method would have the following impact:

  • The ResolveReferences() method technically becomes mandatory and the APIs responsibility would be extended to not only resolve MR references, but also ProviderConfig references
  • The code generation tools need a way to generate the method for every provided MR

Actually thinking about that more, that makes a lot of sense. Because it would require nearly none intervention of a provider developer (assuming using angryjet/upjet).
For instance the argocd provider would not need to touch this hand-written code part:
https://github.com/crossplane-contrib/provider-argocd/blob/c3e58f3024372f15f26f9cbb72897a1f1a79cb23/pkg/clients/argocd.go#L49-L58.

However, I'm wondering about the change in the APIs responsibility, because the ProviderConfigSelector would become part of every MR, while the ResolveReference() method might not support that. For instance assuming a fully hand written provider would get the selector once the runtime is upgraded and if not paying attention to the release notes forget to implement the part, leading to users rely on a runtime feature the provider is not supporting.

An alternative would be to use the existing ReferenceResolver but extend it with an explicit method ResolveProviderConfigReference(). This way a compiler error would lead to awareness on the provider side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
Status: Backlog
Development

No branches or pull requests

7 participants