RequestReply: Add CESQL function by d3akhtar · Pull Request #8360 · knative/eventing

d3akhtar · 2024-12-01T19:54:48Z

Proposed Changes

Added correlation ID filter that creates a user-defined CESQL function, then returns a CESQL filter
Added tests for correlation ID filter

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

Added CESQL user function to validate correlation ID

Docs

linux-foundation-easycla · 2024-12-01T19:54:52Z

The committers listed above are authorized under a signed CLA.

✅ login: d3akhtar (1396df0, 16053ce, 3a4c452, 5725c03, 52c8fd2, d3ad11a, fefc79a, 3f90088)

knative-prow · 2024-12-01T19:54:57Z

Welcome @d3akhtar! It looks like this is your first PR to knative/eventing 🎉

knative-prow · 2024-12-01T19:54:57Z

Hi @d3akhtar. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Cali0707 · 2024-12-01T20:02:17Z

/ok-to-test

Cali0707

Great start @d3akhtar ! I left a few initial review comments, but since we worked on it together hopefully @pierDipi or @creydr can take a closer look as well

/cc @pierDipi @creydr

Cali0707 · 2024-12-01T20:06:09Z

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter.go

+				return false, errors.New("correlationId Format: <original id>:<base64/hex encoded encrypted id>")
+			}
+
+			slice := strings.Split(correlationId, ":")


For the split here, do we need to consider that : may be a part of the id itself? Is there a need for this to be configurable? cc @pierDipi @creydr

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter.go

Cali0707 · 2024-12-01T20:08:35Z

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter.go

+		},
+	)
+
+	ceruntime.AddFunction(correlationIdFilterFunction)


I might add this to the runtime somewhere else, since this will be called to make a new CESQL expression as well. The function is doing a bit too much. There should be 2 functions:

Make and add the CESQL function

Parse the CESQL expression into a filter

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter.go

Cali0707 · 2024-12-01T20:10:55Z

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter.go

+	if err != nil {
+		return nil, err
+	}
+	secrets, err := clientset.CoreV1().Secrets("default").List(context.Background(), metav1.ListOptions{})


Probably need to configure:

which namespace to look up secrets in

which secret names to get

d3akhtar · 2025-01-28T01:59:11Z

@pierDipi How can I use the injected fake kubeclient to create secrets to test my code on? Right now, if I try the tests, I get this error.

panic: Unable to fetch k8s.io/client-go/kubernetes.Interface from context. This context is not the application context (which is typically given to constructors via sharedmain).

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter_test.go

…yption algorithm

codecov · 2025-02-10T21:15:45Z

Codecov Report

Attention: Patch coverage is 75.17730% with 35 lines in your changes missing coverage. Please review.

Project coverage is 62.75%. Comparing base (162575c) to head (1396df0).
Report is 84 commits behind head on main.

Files with missing lines	Patch %	Lines
...ter/subscriptionsapi/cesql_correlationid_filter.go	75.00%	23 Missing and 12 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8360      +/-   ##
==========================================
- Coverage   64.09%   62.75%   -1.34%     
==========================================
  Files         388      396       +8     
  Lines       23365    24158     +793     
==========================================
+ Hits        14975    15160     +185     
- Misses       7593     8183     +590     
- Partials      797      815      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Cali0707

One small nit, otherwise this looks good to me

/lgtm

/cc @creydr @pierDipi

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter.go

…QLCorrelationIdFilter

knative-prow · 2025-02-14T14:32:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: d3akhtar
Once this PR has been reviewed and has the lgtm label, please ask for approval from cali0707. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Cali0707 · 2025-03-10T20:05:21Z

/lgtm

@pierDipi @matzew @creydr could one of you provide a review as well here?

github-actions · 2025-06-09T01:29:48Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

RequestReply: Add CESQL function (knative#8318)

16053ce

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 1, 2024

knative-prow bot requested review from creydr and matzew December 1, 2024 19:54

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 1, 2024

Cali0707 reviewed Dec 1, 2024

View reviewed changes

knative-prow bot requested a review from pierDipi December 1, 2024 20:12

d3akhtar added 2 commits January 27, 2025 14:33

Moved some code to functions and used kubeclient

3a4c452

Merge branch 'main' into correlation-id-cesql-user-function

5725c03

pierDipi reviewed Feb 4, 2025

View reviewed changes

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter_test.go Outdated Show resolved Hide resolved

d3akhtar added 2 commits February 8, 2025 20:31

Changed tests to use fake kubeclient and organized them based on encr…

3f90088

…yption algorithm

Merge branch 'main' into correlation-id-cesql-user-function

52c8fd2

knative-prow bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 9, 2025

d3akhtar added 2 commits February 10, 2025 18:11

Changed NewCESQLCorrelationIdFilter() to only add function to runtime

d3ad11a

Merge branch 'main' into correlation-id-cesql-user-function

fefc79a

Cali0707 reviewed Feb 14, 2025

View reviewed changes

pkg/eventfilter/subscriptionsapi/cesql_correlationid_filter.go Outdated Show resolved Hide resolved

knative-prow bot requested a review from pierDipi February 14, 2025 14:17

knative-prow bot assigned Cali0707 Feb 14, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2025

Changed function name from NewCESQLCorrelationIdFilter to RegisterCES…

1396df0

…QLCorrelationIdFilter

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 14, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 10, 2025

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 9, 2025

github-actions bot closed this Jul 10, 2025

Conversation

d3akhtar commented Dec 1, 2024

Proposed Changes

Pre-review Checklist

Uh oh!

linux-foundation-easycla bot commented Dec 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow bot commented Dec 1, 2024

Uh oh!

knative-prow bot commented Dec 1, 2024

Uh oh!

Cali0707 commented Dec 1, 2024

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

Cali0707 Dec 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cali0707 Dec 1, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cali0707 Dec 1, 2024

Choose a reason for hiding this comment

Uh oh!

d3akhtar commented Jan 28, 2025

Uh oh!

Uh oh!

codecov bot commented Feb 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

knative-prow bot commented Feb 14, 2025

Uh oh!

Cali0707 commented Mar 10, 2025

Uh oh!

github-actions bot commented Jun 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

linux-foundation-easycla bot commented Dec 1, 2024 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading