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

Support patching from common data sources? #2099

Closed
negz opened this issue Jan 26, 2021 · 15 comments · Fixed by #3007
Closed

Support patching from common data sources? #2099

negz opened this issue Jan 26, 2021 · 15 comments · Fixed by #3007
Assignees
Labels
composition enhancement New feature or request

Comments

@negz
Copy link
Member

negz commented Jan 26, 2021

References

Design: #3008
Implementation: #3007

What problem are you facing?

When platform teams design their XRs they may want to use environment scoped data to render out composed resources.
It's possible that this data varies enough that it's not ideal for it to be a fixed value in a Composition, but that it's also not something that the platform team desired claim owners (i.e. their API consumers) to have control over.

Some concrete examples:

  1. Which VPC an RDS instance should be created in. Imagine a platform team that manages 10 Kubernetes clusters. Each cluster runs Crossplane, and each cluster is in its own VPC. Any databases used by each cluster should be created in its VPC. Currently this platform team could not use a single database Composition across all clusters unless they exposed the target VPC as an XR (and thus claim) field - i.e. left it up to their API consumers to set.
  2. Which cloud provider credentials should be used to create managed resources. Imagine a platform team that runs one Kubernetes cluster. The cluster runs Crossplane, and each of the teams the platform team supports have their own namespace within said cluster. Each team also has their own set of cloud credentials. All teams should be able to create database XRs, but they must do so using their own credentials. Currently this is impossible. The platform team could create a ProviderConfig for each team, but they would then need to either let their API consumers specify which PlatformConfig to use (without restriction) or fork their database Composition to reflect each team's ProviderConfig (teams could again select which Composition to use without restriction).

How could Crossplane help solve your problem?

Crossplane could allow Compositions to patch from common Kubernetes data sources, possibly including ConfigMaps, Secrets, and (claim) Namespaces. Doing so would enable the above use cases:

  1. The platform team can add a well known ConfigMap or Secret to each Kubernetes cluster - perhaps a ConfigMap named cluster-info in the crossplane-system namespace, then write a Composition that patched from data.vpc-name within that ConfigMap.
  2. The platform team can annotate each team's namespace with their ProviderConfig name, then write a Composition that patched from metadata.annotations[crossplane.io/provider-config on the claim's namespace through to spec.providerConfigRef.name on each composed resource.

Note that this enhancement needs a little more thought. Specifically I'm wondering:

  1. Do we need this at the Composition layer if we allow arbitrary cross resource references at the managed resource layer per Generic Cross-Resource References #1770. i.e. Could you cover at least the first use case by writing your Composition such that each managed resource's spec.forProvider.vpcName was a cross-resource-reference to a ConfigMap?
  2. If we do this at the Composition layer do we want to limit it to well known data sources (e.g. ConfigMaps etc) or open it to up patching from any kind of resource (and if so do we still have any impetus to build Generic Cross-Resource References #1770 at the managed resource layer?)
@negz negz added enhancement New feature or request composition labels Jan 26, 2021
@negz
Copy link
Member Author

negz commented Jan 26, 2021

CC @prasek

@prasek
Copy link
Member

prasek commented Jan 26, 2021

@negz CRRs were in part motivated by the GitOps scenario of applying multiple related resources with dependencies on non-deterministic CSP generated names (e.g. AzureDB and VnetRule). IIRC #1770 was focused on making these CRRs more generic at the managed resource level to simplify codegen while ensuring that the correct creation order could entirely be handled behind the API line to preserve a clean GitOps UX. AFAIK composed resources benefit from this behavior as well, so if there were a new set of code generated managed resources would that make a case for doing both this and #1770? If one approach can solve all use cases would be helpful to see some example YAML showing what it would look like.

Also would be a fan of opening up the resources that could be referenced to all kinds unless there were significant downsides to doing so.

@negz
Copy link
Member Author

negz commented Jan 26, 2021

IIRC #1770 was focused on making these CRRs more generic at the managed resource level to simplify codegen

Exactly.

AFAIK composed resources benefit from this behavior as well, so if there were a new set of code generated managed resources would that make a case for doing both this and #1770?

What I was trying to get across is that I can envision designs for #1770 that would make this feature redundant. Let's say for example that CRRs are implemented as an array of arbitrary references on a managed resource, each of which is a reference to an arbitrary fieldpath within an arbitrary object (i.e. GVK and name). That means you could have a CRR to a field within a ConfigMap and just patch to that field in your Composition - you wouldn't need this feature.

That said there are some quirks to work out - I think we need to play around with both designs a little before we know for sure whether we should do one, the other, or both. I want to call out that we should consider the two holistically just to make sure we don't needlessly (and arguably confusingly) build two ways of doing one thing.

@muvaf
Copy link
Member

muvaf commented Jan 27, 2021

I believe if we do this on managed resource layer #1770 in a way that covers all kinds, using it to solve both of these problems would be trivial. An example Composition could look like the following:

...
resources:
- base:
    apiVersion: database.aws.crossplane.io/v1beta1
    kind: RDSInstance
    spec:
      forProvider: somevalues...
      references:
      - source:
          apiVersion: v1
          kind: ConfigMap
          name: config-store
          namespace: default
        fromFieldPath: data.providerConfigName
        toFieldPath: spec.providerConfigRef.name
  patches:
  - fromFieldPath: spec.claimRef.namespace
    toFieldPath: spec.references[0].source.namespace

The namespace (and name if needed) of the source object could be retrieved from composite resource so that we make sure to get the value from the namespace you're able to create your claim. IMO, #1770 would solve both problems without introducing more complexity to composition layer.

FWIW, there is a workaround to the second problem today. Admins can create ProviderConfig objects for each namespace. Then they can enforce use of ProviderConfig that has the same name with your namespace.

apiVersion: v1
kind: Namespace
metadata:
  name: team-a
---
apiVersion: aws.crossplane.io/v1beta1
kind: ProviderConfig
metadata:
  name: team-a
spec:
  credentials: some settings here
---
apiVersion: v1
kind: Namespace
metadata:
  name: team-b
---
apiVersion: aws.crossplane.io/v1beta1
kind: ProviderConfig
metadata:
  name: team-b
spec:
  credentials: some other settings here

Then Composition would look like the following:

...
resources:
- base:
    apiVersion: database.aws.crossplane.io/v1beta1
    kind: RDSInstance
    spec:
      forProvider: somevalues...
  patches:
  - fromFieldPath: spec.claimRef.namespace
    toFieldPath: spec.providerConfigRef.name
    policy:
      fromFieldPath: Required

If they want to have multiple ProviderConfig per namespace and let the users choose from their pool, they can use some suffixes and allow the members to provide them, though this depends on #2093

...
resources:
- base:
    apiVersion: database.aws.crossplane.io/v1beta1
    kind: RDSInstance
    spec:
      forProvider: somevalues...
  patches:
  - fromFieldPath: spec.claimRef.namespace
    toFieldPath: spec.providerConfigRef.name
  - fromFieldPath: spec.pcSuffix
    toFieldPath: spec.providerConfigRef.name
    transforms:
    - type: string
      format: "%s-%s" # we need multiple input PR #2093 for this to work.

@muvaf muvaf added this to Accepted in v1.3 via automation Jun 3, 2021
@muvaf muvaf self-assigned this Jun 3, 2021
@muvaf muvaf moved this from Accepted to In Progress in v1.3 Jun 3, 2021
@jbw976 jbw976 moved this from In Progress to PR Open in v1.3 Jun 17, 2021
@jbw976 jbw976 added this to Review in progress in v1.4 Jul 1, 2021
@negz negz moved this from Review in progress to In Design in v1.4 Aug 12, 2021
@negz negz added this to In design in Roadmap Aug 26, 2021
@negz negz removed this from In Design in v1.4 Aug 26, 2021
@jbhennin
Copy link

jbhennin commented Oct 8, 2021

@muvaf @negz As discussed in the community meeting, we strive to make an excellent application developer user experience. There are pieces of the K8s cluster like the VPC ID, Security Groups, DB Subnet Group, etc. that should not have to be known by someone creating a resource like an RDS Database.

Terraform has the data block that allows the platform developer to incorporate these common resources. Crossplane should allow for common resources to be referenced regardless if they are managed by crossplane or not.

Having this type of functionality is critical in the successful adoption and use of crossplane, else the experience is just too cumbersome.

@yogeek
Copy link
Contributor

yogeek commented Jan 27, 2022

As explained in a Slack discussion with @muvaf, it seems that this proposition could help with the following use case :

When a Crossplane resource depends on a piece of infrastructure managed by Kubernetes (e.g. a LoadBalancer type K8S service that manages an AWS NetworkLoadBalancer), it would be very useful if Crossplane could access the Kubernetes resource details to use it into a managed resource manifest.

Real-life example :
We need to create an AWS VPCEndpointServiceConfiguration with Crossplane. For this, the ARN of the targeted loadbalancer is needed (https://github.com/crossplane/provider-aws/blob/master/examples/ec2/vpcendpointserviceconfiguration.yaml#L9).
But in the case of a LoadBalancer that has previously been dynamically created by K8S from a K8S Service with type: LoadBalancer (in our case, the ingress-gateway service deployed by Istio installation), how can we indicate the ARN of this LoadBalancer to Crossplane ? This LoadBalancer being dynamic, we cannot hard-code its ARN as it can change anytime.
It would be great if Crossplane could retrieve the LoadBalancer information from the K8S service resource to specify it as a field of the VPCEndpointServiceConfiguration CustomResource definition.

@yogeek
Copy link
Contributor

yogeek commented Jan 27, 2022

Following the research after my previous comment, I saw this syntax in the kubernetes crossplane provider : https://github.com/crossplane-contrib/provider-kubernetes/blob/main/examples/in-composition/composition.yaml#L162

Is this possible only because the Service is part of the Crossplane Helm Release or would it be possible for every K8S resources ...? (even the one not managed by Crossplane)

@MisterMX
Copy link
Contributor

MisterMX commented Mar 3, 2022

This feature would help us a lot as we have to serve the same compositions for different environments.

Would it be possible to extend the Compositions API similar to the generic patches in provider-kuberntes?

We would propose a patch structure like this:

      patches:
        - type: FromObjectFieldPath
          fromObjectRef:
            apiVersion: v1
            kind: ConfigMap
            name: sample-config
            namespace: sample-ns
          fromFieldPath: data.value
          toFieldPath: spec.forProvider.sampleField
          policy:
            fromFieldPath: Required # Dont render if referenced resource does not exist

This way we it would integrate seamlessly into the composition workflow we already have.

@muvaf
Copy link
Member

muvaf commented Mar 31, 2022

This would allow using an arbitrary Secret in the cluster in an arbitrary namespace, hence allow privilege escalation through accessing the service account tokens. Users with Composition permission are able to access all Crossplane and provider types but it seems to me that this PR makes them available to access even non-Crossplane resources, which could be fine, but I'd like us to be explicit about this and/or think about how to not pay that cost to get this feature.

@MisterMX
Copy link
Contributor

MisterMX commented Apr 1, 2022

@muvaf yes, this is indeed a problem that might lead to a big security loophole. We therefore suggested an alternative approach by introducing a new CR ValueSet that can be referenced in compositions without leading to privilege escalation. See #3008.

@ksawerykarwacki
Copy link

@MisterMX I currently have workflow where I patch from one composition's connection secrets to another (shared infrastructure credentials for multiple application instances) and currently I have to do this in the application that generates claims. Is there any assumption that ValueSet could replace connection secret or extend that functionality?

@MisterMX
Copy link
Contributor

MisterMX commented Apr 7, 2022

@ksawerykarwacki I don't think that will happen (though its not up to me) and I am not sure if its actually desirable to replace the common Secret resource with something Crossplane specific.

However, you could add the ValueSet as a dedicated resource in your composition, patch the data you need and reuse it somewhere else.

@jbw976 jbw976 added this to To do in v1.8 via automation Apr 7, 2022
@jbw976 jbw976 moved this from To do to In progress in v1.8 Apr 7, 2022
@muvaf muvaf removed their assignment Apr 22, 2022
@jbw976 jbw976 added this to To do in v1.9 via automation Jun 2, 2022
@jbw976 jbw976 moved this from To do to In progress in v1.9 Jun 2, 2022
@stale
Copy link

stale bot commented Aug 17, 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 stale label Aug 17, 2022
@muvaf muvaf removed the stale label Aug 17, 2022
@github-actions
Copy link

Crossplane does not currently have enough maintainers to address every issue and pull request. This issue has been automatically marked as stale because it has had no activity in the last 90 days. It will be closed in 7 days if no further activity occurs. Leaving a comment starting with /fresh will mark this issue as not stale.

@github-actions github-actions bot added the stale label Nov 17, 2022
@jbw976
Copy link
Member

jbw976 commented Nov 17, 2022

/fresh this is actively being worked on in #3007 and in high demand.

@github-actions github-actions bot removed the stale label Nov 17, 2022
Roadmap automation moved this from In design to Released Dec 26, 2022
v1.8 automation moved this from In progress to Done Dec 26, 2022
v1.9 automation moved this from In progress to Done Dec 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
composition enhancement New feature or request
Projects
Status: Done
Roadmap
Released
v1.3
PR Open
v1.8
Done
v1.9
Done
8 participants