Skip to content

Add opt-in VirtualService informer label filtering#1504

Open
anencore94 wants to merge 2 commits into
knative-extensions:mainfrom
anencore94:vs-informer-label-filtering
Open

Add opt-in VirtualService informer label filtering#1504
anencore94 wants to merge 2 commits into
knative-extensions:mainfrom
anencore94:vs-informer-label-filtering

Conversation

@anencore94
Copy link
Copy Markdown

@anencore94 anencore94 commented Feb 4, 2026 •
edited
Loading

Summary

  • add opt-in VirtualService informer filtering by networking.knative.dev/ingress label
  • wire filtered VS informers into ingress and serverlessservice controllers
  • label SKS-created VirtualServices for filtering
  • fallback to direct API get when lister misses an existing VS (e.g. filtered cache)
  • add unit tests for VS filtering util and AlreadyExists fallback

Upgrade Notes

  • when filtering is enabled, preexisting unlabeled VS may be hidden from cache; the reconcile path now falls back to API Get on AlreadyExists, updates labels, and brings them into the filtered cache on subsequent resyncs

Testing

  • go test ./...

@knative-prow
Copy link
Copy Markdown

knative-prow Bot commented Feb 4, 2026

Welcome @anencore94! It looks like this is your first PR to knative-extensions/net-istio 🎉

@knative-prow 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 Feb 4, 2026
@knative-prow
Copy link
Copy Markdown

knative-prow Bot commented Feb 4, 2026

Hi @anencore94. Thanks for your PR.

I'm waiting for a knative-extensions 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.

@knative-prow
Copy link
Copy Markdown

knative-prow Bot commented Feb 4, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anencore94
Once this PR has been reviewed and has the lgtm label, please assign skonto for approval. 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:

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

@knative-prow knative-prow Bot requested review from dprotaso, linkvt and skonto February 4, 2026 03:41
linkvt
linkvt reviewed Feb 4, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@linkvt linkvt left a comment

Choose a reason for hiding this comment

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

Hi @anencore94 ,

thanks for your PR, it looks really good to me but I'm also not a maintainer of the net-istio project so I can't give you an approval or so as I don't have enough background infos on the project.

I left a few comment, I think the use of networking.IngressLabelKey is the most important one as it works I guess but not sure if there is another recommended label to use.

/ok-to-test.

Comment thread pkg/reconciler/informerfiltering/util.go Outdated
Comment thread pkg/reconciler/informerfiltering/util_test.go Outdated
Comment thread pkg/reconciler/serverlessservice/resources/virtualservice.go
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: sks.Namespace,
Labels: map[string]string{networking.IngressLabelKey: sks.Name},
Copy link
Copy Markdown
Member

@linkvt linkvt Feb 4, 2026

Choose a reason for hiding this comment

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

Using this label looks good at first but are there other net- implementations that also use this label? Or do they use different labels?

Copy link
Copy Markdown
Author

@anencore94 anencore94 Feb 7, 2026

Choose a reason for hiding this comment

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

Sure, there could be a better label or we could make another. I need another comments from other contributors :)

Copy link
Copy Markdown
Author

@anencore94 anencore94 Feb 7, 2026

Choose a reason for hiding this comment

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

Good question. I kept networking.IngressLabelKey intentionally for consistency with existing net-istio ownership/lookup paths: ingress-created VS already use this label, and ingress cleanup/list paths already rely on it (e.g. selectors in ingress reconciler). Using the same key for SKS VS lets informer-level filtering include both ingress-owned and SKS-owned VS without introducing a second ownership label dimension.

@linkvt
Copy link
Copy Markdown
Member

linkvt commented Feb 4, 2026

/ok-to-test

@knative-prow 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 Feb 4, 2026
@linkvt
Copy link
Copy Markdown
Member

linkvt commented Feb 11, 2026

/lgtm

What do you think @dprotaso , any opinion on this? Seems quite straight forward.

@knative-prow knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Feb 11, 2026
@dprotaso
Copy link
Copy Markdown
Contributor

dprotaso commented Feb 12, 2026

/hold see: #1503 (comment)

I think if the istio client has trouble listing then there's something amiss as that shouldn't happen.

@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 Feb 12, 2026
dprotaso
dprotaso reviewed Mar 5, 2026
View reviewed changes
Comment thread pkg/reconciler/accessor/istio/virtualservice.go
if apierrs.IsNotFound(err) {
vs, err = vsAccessor.GetIstioClient().NetworkingV1beta1().VirtualServices(ns).Create(ctx, desired, metav1.CreateOptions{})
if err != nil {
if apierrs.IsAlreadyExists(err) {
Copy link
Copy Markdown
Contributor

@dprotaso dprotaso Mar 5, 2026

Choose a reason for hiding this comment

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

Can we drop the changes in this file and it's test directory?

If an error occurs the reconciliation will just happen again - I don't think we should 'recover' here by 'fetching' from the API server.

Or at a minimum pull these changes into a separate PR so we can discuss them separately

Comment thread pkg/reconciler/ingress/controller.go
}

func getVirtualServiceInformer(ctx context.Context) istioinformer.VirtualServiceInformer {
if informerfiltering.ShouldFilterVSByLabel() {
Copy link
Copy Markdown
Contributor

@dprotaso dprotaso Mar 5, 2026

Choose a reason for hiding this comment

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

out of curiosity - why is this guarded and getSecretInformer is not?

@dprotaso
Copy link
Copy Markdown
Contributor

dprotaso commented Mar 5, 2026

I'm good with a labelling/filtering change like this (excluding the original intent - cause that's a separate issue lol)

@dprotaso
Copy link
Copy Markdown
Contributor

dprotaso commented Mar 5, 2026

Added some review comments

@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 5, 2026

Codecov Report

❌ Patch coverage is 61.11111% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.95%. Comparing base (3b01e15) to head (4cd8e20).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
pkg/reconciler/serverlessservice/controller.go 0.00% 6 Missing ⚠️
pkg/reconciler/accessor/istio/virtualservice.go 50.00% 1 Missing and 3 partials ⚠️
pkg/reconciler/ingress/controller.go 50.00% 2 Missing and 1 partial ⚠️
cmd/controller/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #1504      +/-   ##
==========================================
- Coverage   81.96%   81.95%   -0.01%     
==========================================
  Files          23       23              
  Lines        1519     1546      +27     
==========================================
+ Hits         1245     1267      +22     
- Misses        180      183       +3     
- Partials       94       96       +2

☔ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dprotaso dprotaso dprotaso left review comments

@skonto skonto Awaiting requested review from skonto

+1 more reviewer

@linkvt linkvt linkvt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@linkvt linkvt

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@anencore94 @linkvt @dprotaso