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

APIServerSource with selector to target namespaces #6665

Merged
merged 16 commits into from
Jan 25, 2023

Conversation

gab-satchi
Copy link
Contributor

Fixes #6644

Proposed Changes

  • 🎁 ApiServerSource can specify a selector to target one or more namespaces. If the selector is missing, it will default to targeting the namespace in which the source resides

API changes (additive):

  • spec.namespaceSelector: A label selector to identify namespaces to track
  • status.namespaces: A list of namespaces currently being tracked by the source

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

📄 ApiServerSource can specify a selector to target one or more namespaces. If the selector is missing, it will default to targeting the namespace in which the source resides

Docs

Docs to follow once the behaviour is finalized and approved.

@knative-prow knative-prow bot requested review from aliok and tayarani January 5, 2023 15:55
@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 5, 2023
@codecov
Copy link

codecov bot commented Jan 5, 2023

Codecov Report

Base: 80.62% // Head: 80.49% // Decreases project coverage by -0.14% ⚠️

Coverage data is based on head (2775d57) compared to base (466d123).
Patch coverage: 80.58% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6665      +/-   ##
==========================================
- Coverage   80.62%   80.49%   -0.14%     
==========================================
  Files         236      236              
  Lines       12018    12071      +53     
==========================================
+ Hits         9690     9717      +27     
- Misses       1847     1867      +20     
- Partials      481      487       +6     
Impacted Files Coverage Δ
pkg/apis/sources/v1/apiserver_types.go 100.00% <ø> (ø)
pkg/reconciler/apiserversource/controller.go 74.35% <46.15%> (-15.30%) ⬇️
pkg/reconciler/apiserversource/apiserversource.go 80.75% <82.43%> (-1.44%) ⬇️
pkg/adapter/apiserver/adapter.go 68.18% <100.00%> (+1.12%) ⬆️
...ciler/apiserversource/resources/receive_adapter.go 92.24% <100.00%> (+0.06%) ⬆️
pkg/broker/filter/filter_handler.go 75.51% <0.00%> (-2.63%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Can we add an E2E tests with rekt?

@gab-satchi
Copy link
Contributor Author

Thanks for the review @pierDipi, I've addressed the comments. Working on adding an e2e test now

@evankanderson
Copy link
Member

Question: do we want to do per-namespace watches, or do we want to leverage an all-namespace watch (different path, single watch rather than one call per namespace)?

@gab-satchi
Copy link
Contributor Author

gab-satchi commented Jan 11, 2023

An all namespaces watch is useful. I can simplify the namespaces in the status to just say ALL as well for readability. The authorization check would still need to be per namespace I believe.

I left the status as is. It provides more information than just saying ALL

@knative-prow 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 Jan 11, 2023
@knative-prow knative-prow bot added the area/test-and-release Test infrastructure, tests or release label Jan 11, 2023
@gab-satchi gab-satchi force-pushed the api-server-source-mt branch 2 times, most recently from 2acd166 to 2439161 Compare January 11, 2023 23:25
@gab-satchi
Copy link
Contributor Author

/retest

@gab-satchi gab-satchi requested review from pierDipi and removed request for aliok and tayarani January 17, 2023 15:09
test/rekt/resources/pod/pod.yaml Outdated Show resolved Hide resolved
pkg/reconciler/apiserversource/apiserversource.go Outdated Show resolved Hide resolved
@gab-satchi gab-satchi force-pushed the api-server-source-mt branch from 2439161 to c42ef0d Compare January 19, 2023 18:08
@gab-satchi
Copy link
Contributor Author

/hold until the reconciler-test changes come in

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 19, 2023
@evankanderson
Copy link
Member

/retest

@evankanderson
Copy link
Member

@gab-satchi
Do we expect reconciler-test changes to have come in and fixed the problem? If not, we'll need to re-cut the reconciler-test branch with another cherrypick...

/retest

Though I doubt it will help.

@gab-satchi gab-satchi force-pushed the api-server-source-mt branch from c4163c3 to 2775d57 Compare January 24, 2023 21:56
@gab-satchi
Copy link
Contributor Author

/hold cancel

changes from reconciler-test have been merged

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2023
@evankanderson
Copy link
Member

/retest

@gab-satchi gab-satchi requested a review from pierDipi January 24, 2023 23:03
@evankanderson
Copy link
Member

(it looks like ./test/experimental/... and ./test/rekt/... may be failing for cause (I attempted a re-run)

Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 25, 2023
@knative-prow
Copy link

knative-prow bot commented Jan 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gab-satchi, pierDipi

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:

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

@matzew
Copy link
Member

matzew commented Feb 6, 2023

Unit Test (eventing-kafka-broker, knative-sandbox) was broken by this (see in the GA actions)

@gab-satchi
Copy link
Contributor Author

I did a rebase once the reconciler-test changes were in. Before that, I believe the unit tests were passing like in this run

I'm also seeing the same failure with these changes

I'll look into it some more but my familiarity with kafka is pretty limited.

@pierDipi
Copy link
Member

pierDipi commented Feb 6, 2023

eventing kafka broker is fixed but it wasn't related to this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test-and-release Test infrastructure, tests or release lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend APIServerSource to watch multiple namespaces
4 participants