Add SubscriptionsAPI filters to APIServerSource #7799

rh-hemartin · 2024-03-19T10:40:47Z

This MR introduces the filters key in the APIServerSource Spec. This new field allows users to filter which messages are sent from the APIServerSource to the specified sink. The filter language is the new SubscriptionsAPI, that allows for powerful filtering.

I changed a function to be exported in the broker/filter package. The other option I considered was to move the whole thing to a new package but it looked more disruptive.
I followed the same approach that Triggers in Implement CE Subscriptions filters #5715 to introduce the field into the CRD (allow for everything).
This is my first contribution here, so bear with me 🤣

Fixes #7791

Proposed Changes

🎁 Add new feature

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

The filters field in APIServerSource is now beta and enabled by default

Docs

Docs #5916

linux-foundation-easycla · 2024-03-19T10:40:56Z

The committers listed above are authorized under a signed CLA.

✅ login: rh-hemartin / name: Hector Martinez (52966bc)

knative-prow · 2024-03-19T10:40:57Z

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

knative-prow · 2024-03-19T10:40:58Z

Hi @rh-hemartin. 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.

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/test-infra repository.

pkg/apis/sources/v1/apiserver_validation.go

pkg/adapter/apiserver/delegate.go

pierDipi

Can we create an e2e test similar to TestMTChannelBrokerNewTriggerFilters

eventing/test/rekt/new_trigger_filters_test.go

Line 34 in 2aa8985

func TestMTChannelBrokerNewTriggerFilters(t *testing.T) {

pierDipi · 2024-03-25T09:16:58Z

Thanks @rh-hemartin, this is a really good PR

rh-hemartin · 2024-03-27T07:56:14Z

Can we create an e2e test similar to TestMTChannelBrokerNewTriggerFilters

eventing/test/rekt/new_trigger_filters_test.go

Line 34 in 2aa8985

func TestMTChannelBrokerNewTriggerFilters(t *testing.T) {

I can't reply to your standalone comments, so I'm quoting you...

I'm having trouble with the eventshub image when it comes to the e2e tests. The eventshub image is not getting replaced in the installation process. To reproduce this I got:

1. Clone
1. `kind create cluster`
1. `export KO_DOCKER_REPO="kind.local"`
1. `./hack/install.sh`
1. `./hack/e2e-debug.sh TestApiServerSourceDataPlane_EventModes ./test/rekt`
1. Get errors because "message": "Back-off pulling image \"knative.dev/reconciler-test/cmd/eventshub\"""
1. `kind delete cluster`

How can I get the reconciler-test image uploaded and used? I tried with ko build vendor/knative.dev/reconciler-test/cmd/eventshub but it has some problems too:

Error: failed to publish images: importpath "ko://vendor/knative.dev/reconciler-test/cmd/eventshub/" is not supported: importpath is not `package main`

Cali0707 · 2024-03-27T12:55:55Z

How can I get the reconciler-test image uploaded and used? I tried with ko build vendor/knative.dev/reconciler-test/cmd/eventshub but it has some problems too:

@rh-hemartin can you try running ./test/upload-test-images.sh ?

rh-hemartin · 2024-03-27T13:23:36Z

How can I get the reconciler-test image uploaded and used? I tried with ko build vendor/knative.dev/reconciler-test/cmd/eventshub but it has some problems too:

@rh-hemartin can you try running ./test/upload-test-images.sh ?

That is executed within ./hack/install.sh, so nothing different. Also the logs do not show any eventshub image being uploaded. It makes sense, because the eventshub is not on test/test_images, but on the vendor folder.

pierDipi · 2024-03-28T13:14:37Z

@rh-hemartin can you paste the full logs you're getting? On which OS are you on?

pierDipi · 2024-03-28T13:18:45Z

For this step:

./hack/e2e-debug.sh TestApiServerSourceDataPlane_EventModes ./test/rekt`

can you instead run:

SYSTEM_NAMESPACE=knative-eventing go test -tags=e2e -v -run TestApiServerSourceDataPlane_EventModes ./test/rekt

rh-hemartin · 2024-03-28T13:46:51Z

@rh-hemartin can you paste the full logs you're getting? On which OS are you on?

Hey! Getting all the logs together I think I found the problem, so thanks!

logger.go:146: 2024-03-28T14:40:14.795+0100	DEBUG	ko/cmd.go:55		github.com/google/[email protected] indirectly requires github.com/mitchellh/[email protected]: github.com/mitchellh/[email protected]: invalid version: git ls-remote -q origin in /home/hemartin/go/pkg/mod/cache/vcs/94ed57c5b21c953d93b47487113db43a5c9b69fd990329ec70dc77348c4dd443: exit status 128:	{"test": "TestApiServerSourceDataPlane_EventModes"}
    logger.go:146: 2024-03-28T14:40:14.795+0100	DEBUG	ko/cmd.go:55		remote: Repository not found.	{"test": "TestApiServerSourceDataPlane_EventModes"}
    logger.go:146: 2024-03-28T14:40:14.795+0100	DEBUG	ko/cmd.go:55		fatal: repository 'https://github.com/mitchellh/osext/' not found	{"test": "TestApiServerSourceDataPlane_EventModes"}
    logger.go:146: 2024-03-28T14:40:14.796+0100	WARN	environment/images.go:100	Ko publish failed, using image directly	{"test": "TestApiServerSourceDataPlane_EventModes", "error": "ko publish failed: exit status 1 -- command: [\"go\" \"run\" \"github.com/google/[email protected]\" \"publish\" \"-B\" \"knative.dev/reconciler-test/cmd/eventshub\"]", "image": "knative.dev/reconciler-test/cmd/eventshub"}

And I attached the full thing. I'm investigating this error and see if I can solve it...

full.log

Edit: Specifying GOOGLE_KO_VERSION=v0.15.2 for the test command solved the problem. Looks like the version specified in vendor/knative.dev/reconciler-test/pkg/images/ko/publish.go is broken (v0.11.2).

rh-hemartin · 2024-03-28T15:33:40Z

Hey, I will be away from my computer for three weeks, I will continue with this then. I'm now progressing on the e2e rekt test. I pushed the last changes, that included the run of ./hack/update-codegen.sh.

pierDipi · 2024-04-23T08:23:18Z

Edit: Specifying GOOGLE_KO_VERSION=v0.15.2 for the test command solved the problem. Looks like the version specified in vendor/knative.dev/reconciler-test/pkg/images/ko/publish.go is broken (v0.11.2).

I'm bumping the default version here knative-extensions/reconciler-test#708

rh-hemartin · 2024-04-24T10:09:12Z

Can we create an e2e test similar to TestMTChannelBrokerNewTriggerFilters

eventing/test/rekt/new_trigger_filters_test.go

Line 34 in 2aa8985

func TestMTChannelBrokerNewTriggerFilters(t *testing.T) {

I created a rekt test similar to that, I had to introduce changes to some places, as the apiserversource template and new methods to convert filters to strings, extracted from the trigger test implementation.

pierDipi · 2024-04-24T12:29:22Z

/ok-to-test

Cali0707 · 2024-04-29T13:43:03Z

/test reconciler-tests

codecov · 2024-04-30T11:01:31Z

Codecov Report

Attention: Patch coverage is 92.59259% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 69.26%. Comparing base (7e1c082) to head (52966bc).
Report is 23 commits behind head on main.

Files	Patch %	Lines
pkg/broker/filter/filter_handler.go	50.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7799      +/-   ##
==========================================
+ Coverage   69.22%   69.26%   +0.03%     
==========================================
  Files         339      341       +2     
  Lines       19494    15846    -3648     
==========================================
- Hits        13494    10975    -2519     
+ Misses       5337     4199    -1138     
- Partials      663      672       +9

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

pkg/adapter/apiserver/delegate.go

Cali0707

This is looking great @rh-hemartin - just one small nit from my end

config/core/configmaps/features.yaml

pierDipi · 2024-04-30T14:59:29Z

it looks like a flake

/test reconciler-tests

pkg/apis/sources/v1/apiserver_validation.go

pierDipi · 2024-05-02T07:59:11Z

@rh-hemartin with the switch to disabled by default the tests are now failing, we need to add a new config-features configmap for tests with the feature enabled here https://github.com/knative/eventing/tree/main/test/config

rh-hemartin · 2024-05-02T09:58:23Z

@rh-hemartin with the switch to disabled by default the tests are now failing, we need to add a new config-features configmap for tests with the feature enabled here https://github.com/knative/eventing/tree/main/test/config

I did a copy of the features configmap from the production folder. That may impact more things than expected. That will need to be sync with the production configmap.

Would it be better to create the configmap but just include this feature flag? Then the default values will come from the code instead, maybe it will be better from the sync perspective. What do you think?

rh-hemartin · 2024-05-02T11:41:03Z

/test reconciler-tests

This MR introduces the `filters` key in the APIServerSource Spec. This new field allows users to filter which messages are sent from the APIServerSource to the specified sink. The filter language is the new SubscriptionsAPI, that allows for powerful filtering. Signed-off-by: Hector Martinez <[email protected]>

pierDipi · 2024-05-03T11:55:25Z

@rh-hemartin with the switch to disabled by default the tests are now failing, we need to add a new config-features configmap for tests with the feature enabled here https://github.com/knative/eventing/tree/main/test/config

I did a copy of the features configmap from the production folder. That may impact more things than expected. That will need to be sync with the production configmap.

Would it be better to create the configmap but just include this feature flag? Then the default values will come from the code instead, maybe it will be better from the sync perspective. What do you think?

Let's start with this, I think this flag disabled by default will be soon (3 months until the next release) become enabled by default as the underlying library is already part of a beta feature, so we will remove the CM soon as well

pierDipi

/lgtm
/approve

Thanks for the contribution @rh-hemartin !

knative-prow · 2024-05-03T11:55:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pierDipi, rh-hemartin

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pierDipi]

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

knative-prow bot requested review from aliok and lionelvillard March 19, 2024 10:40

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 19, 2024

rh-hemartin mentioned this pull request Mar 19, 2024

Add SubscriptionsAPI filters to APIServerSource knative/docs#5916

Merged

rh-hemartin force-pushed the apiserversource-filters branch from ce970e9 to 0353280 Compare March 19, 2024 15:29

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 19, 2024

Cali0707 self-requested a review March 19, 2024 19:33

pierDipi reviewed Mar 25, 2024

View reviewed changes

pkg/apis/sources/v1/apiserver_validation.go Outdated Show resolved Hide resolved

pierDipi reviewed Mar 25, 2024

View reviewed changes

pkg/adapter/apiserver/delegate.go Outdated Show resolved Hide resolved

pierDipi reviewed Mar 25, 2024

View reviewed changes

rh-hemartin closed this Mar 27, 2024

rh-hemartin reopened this Mar 27, 2024

rh-hemartin force-pushed the apiserversource-filters branch from 0353280 to 107a20d Compare March 27, 2024 08:29

rh-hemartin force-pushed the apiserversource-filters branch from 107a20d to 8216e79 Compare March 28, 2024 15:33

pierDipi mentioned this pull request Apr 23, 2024

Bump default Ko version knative-extensions/reconciler-test#708

Merged

rh-hemartin force-pushed the apiserversource-filters branch from 8216e79 to a699889 Compare April 24, 2024 08:41

knative-prow bot added the area/test-and-release Test infrastructure, tests or release label Apr 24, 2024

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 Apr 24, 2024

rh-hemartin force-pushed the apiserversource-filters branch from a699889 to 3e296c0 Compare April 29, 2024 06:52

pierDipi reviewed Apr 30, 2024

View reviewed changes

pkg/adapter/apiserver/delegate.go Outdated Show resolved Hide resolved

rh-hemartin force-pushed the apiserversource-filters branch from 3e296c0 to df002e2 Compare April 30, 2024 13:43

Cali0707 reviewed Apr 30, 2024

View reviewed changes

config/core/configmaps/features.yaml Outdated Show resolved Hide resolved

rh-hemartin force-pushed the apiserversource-filters branch 2 times, most recently from 4fdb996 to ce06ea2 Compare May 2, 2024 06:38

rh-hemartin commented May 2, 2024

View reviewed changes

pkg/apis/sources/v1/apiserver_validation.go Show resolved Hide resolved

rh-hemartin force-pushed the apiserversource-filters branch from ce06ea2 to 2663ce6 Compare May 2, 2024 09:56

rh-hemartin force-pushed the apiserversource-filters branch from 2663ce6 to 52966bc Compare May 2, 2024 12:48

pierDipi approved these changes May 3, 2024

View reviewed changes

knative-prow bot assigned pierDipi May 3, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2024

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 3, 2024

knative-prow bot merged commit 3dfe973 into knative:main May 3, 2024
32 of 38 checks passed

rh-hemartin deleted the apiserversource-filters branch May 3, 2024 12:37

creydr mentioned this pull request May 10, 2024

Move new-apiserversource-filters test to experimental suite #7914

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SubscriptionsAPI filters to APIServerSource #7799

Add SubscriptionsAPI filters to APIServerSource #7799

rh-hemartin commented Mar 19, 2024 •

edited

Loading

linux-foundation-easycla bot commented Mar 19, 2024 •

edited

Loading

knative-prow bot commented Mar 19, 2024

knative-prow bot commented Mar 19, 2024

pierDipi left a comment

pierDipi commented Mar 25, 2024

rh-hemartin commented Mar 27, 2024 •

edited

Loading

Cali0707 commented Mar 27, 2024

rh-hemartin commented Mar 27, 2024

pierDipi commented Mar 28, 2024

pierDipi commented Mar 28, 2024

rh-hemartin commented Mar 28, 2024 •

edited

Loading

rh-hemartin commented Mar 28, 2024 •

edited

Loading

pierDipi commented Apr 23, 2024

rh-hemartin commented Apr 24, 2024

pierDipi commented Apr 24, 2024

Cali0707 commented Apr 29, 2024

codecov bot commented Apr 30, 2024 •

edited

Loading

Cali0707 left a comment

pierDipi commented Apr 30, 2024

pierDipi commented May 2, 2024 •

edited

Loading

rh-hemartin commented May 2, 2024

rh-hemartin commented May 2, 2024

pierDipi commented May 3, 2024

pierDipi left a comment

knative-prow bot commented May 3, 2024

Add SubscriptionsAPI filters to APIServerSource #7799

Add SubscriptionsAPI filters to APIServerSource #7799

Conversation

rh-hemartin commented Mar 19, 2024 • edited Loading

Proposed Changes

Pre-review Checklist

linux-foundation-easycla bot commented Mar 19, 2024 • edited Loading

knative-prow bot commented Mar 19, 2024

knative-prow bot commented Mar 19, 2024

pierDipi left a comment

Choose a reason for hiding this comment

pierDipi commented Mar 25, 2024

rh-hemartin commented Mar 27, 2024 • edited Loading

Cali0707 commented Mar 27, 2024

rh-hemartin commented Mar 27, 2024

pierDipi commented Mar 28, 2024

pierDipi commented Mar 28, 2024

rh-hemartin commented Mar 28, 2024 • edited Loading

rh-hemartin commented Mar 28, 2024 • edited Loading

pierDipi commented Apr 23, 2024

rh-hemartin commented Apr 24, 2024

pierDipi commented Apr 24, 2024

Cali0707 commented Apr 29, 2024

codecov bot commented Apr 30, 2024 • edited Loading

Codecov Report

Cali0707 left a comment

Choose a reason for hiding this comment

pierDipi commented Apr 30, 2024

pierDipi commented May 2, 2024 • edited Loading

rh-hemartin commented May 2, 2024

rh-hemartin commented May 2, 2024

pierDipi commented May 3, 2024

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented May 3, 2024

rh-hemartin commented Mar 19, 2024 •

edited

Loading

linux-foundation-easycla bot commented Mar 19, 2024 •

edited

Loading

rh-hemartin commented Mar 27, 2024 •

edited

Loading

rh-hemartin commented Mar 28, 2024 •

edited

Loading

rh-hemartin commented Mar 28, 2024 •

edited

Loading

codecov bot commented Apr 30, 2024 •

edited

Loading

pierDipi commented May 2, 2024 •

edited

Loading