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

feat: add extra resources support #83

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phisco
Copy link
Collaborator

@phisco phisco commented Apr 8, 2024

Description of your changes

Fixes #77

I have:

@phisco
Copy link
Collaborator Author

phisco commented Apr 8, 2024

misses an example, I'll try to find a meaningful one.

@phisco phisco mentioned this pull request Apr 10, 2024
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
@danielloader
Copy link

danielloader commented May 15, 2024

misses an example, I'll try to find a meaningful one.

If you want an example the ability to provision an s3 bucket external to the composition (in a composition of its own or not) and be able to add bucket notifications from another composition would be a good way to demonstrate a one to many relationship.

Often one bucket will have a dozen notification handlers into SQS queues etc.

@danielloader
Copy link

Comments from my day of using this branch:

  • Document or change the nested "items" key on each extraResources object.
  • Documentation around the lazy evaluation and guarding needed for the composition function in general, it's a wider composition function issue but it really bites when you're reaching out for additional resources and can't make use of the sequencer function. Especially when you're grabbing the external resource to grab some connection secret strings off it and the sole use it to combine it into the composition connection secrets (grabbing MongoDB Atlas hostname for example).

Ideas to track, if they should become issues or not is up to you:

  • A function or documented functionality of how to guard the MRs from all the externalResource requests not being fulfilled at the templating time - causing malformed or missing fields in MRs to be templated.

Otherwise it's worked as advertised and enabled the workflows I hoped it would!

@dariozachow
Copy link

Is it planned to merge this one soon?

@markussiebert
Copy link

markussiebert commented May 22, 2024

Can you provide a full working example? I can't get this running ...

OK - you have to use Crossplane > 15 in order to use extraResources :-/

@markussiebert
Copy link

markussiebert commented May 22, 2024

My problem is, that I am able to retrieve extraResources with the function, but it's not possible to use them as described:

cannot compose resources: pipeline step "go-templating" returned a fatal result: cannot execute template: template: manifests:10:26: executing "manifests" at <index .extraResources "namespaces">

with this template:

apiVersion: 'meta.gotemplating.fn.crossplane.io/v1alpha1'
kind: 'ExtraResources'
requirements:
  namespaces:
    apiVersion: 'kubernetes.crossplane.io/v1alpha2'
    kind: 'Object'
    matchLabels:
      'something/claimRefNamespace': 'string'
---
{{ $someExtraResources := index .extraResources "namespaces" }}
{{- range $i, $extraResource := $someExtraResources.items }}
apiVersion: kubernetes.crossplane.io/v1alpha1
kind: Object
metadata:
  annotations:
    gotemplating.fn.crossplane.io/composition-resource-name: default-serviceaccount-prod
spec:
  forProvider:
    manifest:
      apiVersion: v1
      kind: Configmap
      metadata:
        name: $extraResource.resource.metadata.name
        namespace: default
      data:
        somekey: value
  providerConfigRef:
    name: "kubernetes"
{{- end }}

So we do encapsulate everything in {{ with .extreResources }} to make it work

So all in all - nice work - in our tests this just works.

@take-five
Copy link

This seems super useful! Will this be merged soon?

@@ -587,6 +592,124 @@ func TestRunFunction(t *testing.T) {
},
},
},
"ExtraResources": {
reason: "The Function should return a fatal result if the extra resource key is duplicated.",

Choose a reason for hiding this comment

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

wrong reason?

@markussiebert
Copy link

Sorry - have the question, does this PR has any future?

@phisco
Copy link
Collaborator Author

phisco commented Jun 4, 2024

Yes, @markussiebert, I only have to find the time to refine it a bit further.

README.md Show resolved Hide resolved
ApiVersion: e.APIVersion,
Kind: e.Kind,
}
if len(e.MatchLabels) == 0 {
Copy link

Choose a reason for hiding this comment

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

when trying to retrieve ALL resources of a kind I would usually specify an empty label Selector.
Maybe this should check for empty string in MatchName instead?

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.

Support requesting extra resources
7 participants