Initial version of CEL resource expressions. #928

bigkevmcd · 2024-09-17T16:39:56Z

This is a very rough cut (needs additional error checking).

But the basic implementation works.

This is the code I referenced here #491 (comment)

I will harden it up, checking that it's receiving a json body and improve the error handling, this was a quick bit of code to see if it was possible.

bigkevmcd · 2024-09-17T16:41:46Z

api/v1/receiver_types.go

@@ -63,10 +63,24 @@ type ReceiverSpec struct {
 	// +optional
 	Events []string `json:"events,omitempty"`

+	// TODO: Validate one or other (or both?)
+


This will need to be addressed, because you could have .spec.resourceExpressions or .spec.resources (or both).

matheuscscp

Thanks for the work here @bigkevmcd, but I think we need to create an RFC or at least an issue to discuss this feature.

matheuscscp · 2024-09-17T18:48:34Z

internal/server/receiver_handlers.go

+	// Only decodes the body for the expression if the body is JSON.
+	// Technically you could generate several resources without any body.
+	if isJSONContent(r) {
+		if err := json.NewDecoder(r.Body).Decode(&body); err != nil {


This needs to be reworked, the request body was already received at this point, in the line if err := s.validate(ctx, receiver, r); err != nil { from (s *ReceiverServer) handlePayload(). Reading it again won't be possible. To fix this you will need something like a https://pkg.go.dev/io#TeeReader with a https://pkg.go.dev/bytes#Buffer.

I missed this in the code above, I'll fix this.

Another option is simply reading the body into a bytes.Buffer and passing it to all the parts of the code that need to consume it.

internal/server/receiver_handlers.go

matheuscscp · 2024-09-17T20:20:42Z

Thanks for the work here @bigkevmcd, but I think we need to create an RFC or at least an issue to discuss this feature.

Looking again at the original issue, maybe we could use that issue to discuss this feature, it looks like in this PR you're specifically trying to address this part:

It would be nice to be able to map the digest to a ImageRepository to notify (either automatically or explicitly configured). This could potentially reduce the load within and caused by Flux drastically.

matheuscscp · 2024-09-17T20:36:47Z

To fix the e2e GitHub Action failing you can run make api-docs manifests generate locally to generate the missing change for docs/api/v1/notification.md.

matheuscscp · 2024-09-18T04:13:08Z

Now there's a diff on config/crd/bases/notification.toolkit.fluxcd.io_receivers.yaml breaking the e2e GitHub Action. When I run make manifests (or my usual make api-docs manifests generate), I see the missing diff on this file locally. Can you please run this command and see if it generates this missing diff? Thanks!

bigkevmcd · 2024-09-18T08:48:09Z

Thanks for the work here @bigkevmcd, but I think we need to create an RFC or at least an issue to discuss this feature.

That's ok, it was a quick branch to explore what's possible, there's no compelling need to land this, and, it's taking on support for CEL, which is not "free".

Worst case, it gets closed as "won't fix" 😄 and informs a different solution, fine by me.

It would be nice to be able to map the digest to a ImageRepository to notify (either automatically or explicitly configured). This could potentially reduce the load within and caused by Flux drastically.

Yes, this is basically a way to dynamically generate the set of resources to notify in a receiver.

The new field .spec.resourceExpressions is basically a set of CEL expressions that can return the resources that should be notified, it could be simplified to a single resourceExpression but from experience with CEL elsewhere, this tends to lead to complex expressions.

matheuscscp · 2024-09-18T12:16:03Z

This solution sounds very elegant to me @bigkevmcd, I'd love to see it in. It would be nice if you could attend some of our dev meetings to discuss this feature, they are public and can be found here: https://fluxcd.io/community/#meetings

(The CNCF Flux Project Meeting ones.)

bigkevmcd · 2024-09-18T12:18:26Z

I'll see what I can do to be there, time permitting.

You'll see that the specific request in here #491 (comment) is tested in the code.

This adds support for dynamic generation of resources for receivers using CEL and parsing the body in the content request. Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>

Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>

This is currently failing because of this bug: kubernetes-sigs/controller-runtime#2949 Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>

stefanprodan · 2024-10-09T12:42:08Z

api/v1/receiver_types.go

+	// e.g. {"name": "test-resource-1", "kind": "Receiver", "apiVersion":
+	//       "notification.toolkit.fluxcd.io/v1"}.
+	// +optional
+	ResourceExpressions []string `json:"resourceExpressions,omitempty"`


I think this should be named ResourceFilters and act on the given list of Resources.

@stefanprodan The last test is borked because of a bug in controller-runtime which I've fixed, but is yet to be released

If I understand what you're saying here, you would have Resources and the user would declare all the possible resources, and the ResourceFilters would filter these to only a subset based on the request?

This wouldn't be dynamic tho', it's not clear how it would solve the problem outlined in #491?

If I understand what you're saying here, you would have Resources and the user would declare all the possible resources, and the ResourceFilters would filter these to only a subset based on the request?

Precisely, that's exactly what we discussed in the dev meeting today!

This wouldn't be dynamic tho', it's not clear how it would solve the problem outlined in #491?

Our reasoning is security, this style forces a more secure stance, the user always have to the declare which resources can be reconciled by the Receiver. They may end up with a long list of resources but at least it will always be clear from the Receiver object which Flux objects can be reconciled from that Receiver.

bigkevmcd commented Sep 17, 2024

View reviewed changes

bigkevmcd force-pushed the cel-notification-resources branch 3 times, most recently from 786ff7e to 5da3f91 Compare September 17, 2024 19:26

matheuscscp reviewed Sep 17, 2024

View reviewed changes

bigkevmcd force-pushed the cel-notification-resources branch from b6d22ba to 8cfcfb6 Compare September 17, 2024 20:27

bigkevmcd force-pushed the cel-notification-resources branch from de123a3 to 4e2654f Compare September 17, 2024 20:52

bigkevmcd force-pushed the cel-notification-resources branch from 1d7d611 to e25d8f3 Compare September 18, 2024 08:48

bigkevmcd force-pushed the cel-notification-resources branch from e25d8f3 to 1d7d611 Compare September 23, 2024 06:32

bigkevmcd added 5 commits October 2, 2024 18:25

Initial version of CEL resource expressions.

f3ee705

This adds support for dynamic generation of resources for receivers using CEL and parsing the body in the content request. Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>

Rebuild the docs and CRDs.

d6aae5d

Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>

Regenerate the CRD.

ba71388

Tidy up the handler code.

a6d0852

Refactor to split handler and server.

be66d3c

Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>

bigkevmcd force-pushed the cel-notification-resources branch from 904f97b to be66d3c Compare October 2, 2024 17:25

Expand test coverage.

b1c68b3

This is currently failing because of this bug: kubernetes-sigs/controller-runtime#2949 Signed-off-by: Kevin McDermott <bigkevmcd@gmail.com>

bigkevmcd force-pushed the cel-notification-resources branch from 29ce325 to b1c68b3 Compare October 8, 2024 07:44

stefanprodan reviewed Oct 9, 2024

View reviewed changes

bigkevmcd closed this Oct 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial version of CEL resource expressions. #928

Initial version of CEL resource expressions. #928

bigkevmcd commented Sep 17, 2024 •

edited

Loading

bigkevmcd Sep 17, 2024

matheuscscp left a comment

matheuscscp Sep 17, 2024

bigkevmcd Sep 17, 2024

matheuscscp Sep 17, 2024

matheuscscp commented Sep 17, 2024

matheuscscp commented Sep 17, 2024

matheuscscp commented Sep 18, 2024 •

edited

Loading

bigkevmcd commented Sep 18, 2024 •

edited

Loading

matheuscscp commented Sep 18, 2024

bigkevmcd commented Sep 18, 2024

stefanprodan Oct 9, 2024

bigkevmcd Oct 9, 2024

matheuscscp Oct 9, 2024

matheuscscp Oct 9, 2024

Initial version of CEL resource expressions. #928

Initial version of CEL resource expressions. #928

Conversation

bigkevmcd commented Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

matheuscscp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matheuscscp commented Sep 17, 2024

matheuscscp commented Sep 17, 2024

matheuscscp commented Sep 18, 2024 • edited Loading

bigkevmcd commented Sep 18, 2024 • edited Loading

matheuscscp commented Sep 18, 2024

bigkevmcd commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigkevmcd commented Sep 17, 2024 •

edited

Loading

matheuscscp commented Sep 18, 2024 •

edited

Loading

bigkevmcd commented Sep 18, 2024 •

edited

Loading