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.

Sign up for GitHub

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

Jump to bottom

Use resource for CNPs and CCNPs #24509

Merged
merged 3 commits into from Mar 31, 2023
Merged

Use resource for CNPs and CCNPs #24509

merged 3 commits into from Mar 31, 2023

Conversation

pippolo84
Copy link
Member

@pippolo84 pippolo84 commented Mar 21, 2023 •
edited

Add Cilium Network Policies and Cilium Clusterwide Network Policies as shared resources in the proper cell.

With this, the resources events are managed more efficiently thanks to the single informer plus workqueue for each resource type, coupled with the subscriber events queue.

Also, this will make the modularization and testing of all subsystems that depend on CNPs or CCNPs events easier.

@pippolo84 pippolo84 added the release-note/misc This PR makes changes that have no direct user impact. label Mar 21, 2023
@pippolo84 pippolo84 force-pushed the pr/pippolo84/add-cnp-resource branch 2 times, most recently from 4c535fe to 1c4b00e Compare March 22, 2023 10:08
@pippolo84 pippolo84 marked this pull request as ready for review March 22, 2023 10:45
@pippolo84 pippolo84 requested a review from a team as a code owner March 22, 2023 10:45
@pippolo84 pippolo84 added the area/daemon Impacts operation of the Cilium daemon. label Mar 22, 2023
@christarazi christarazi added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Mar 24, 2023
@christarazi christarazi self-requested a review March 24, 2023 15:58
@christarazi
Copy link
Member

/test

@pippolo84 pippolo84 force-pushed the pr/pippolo84/add-cnp-resource branch from 1c4b00e to 6f174b7 Compare March 28, 2023 16:02
@pippolo84
Copy link
Member Author

pippolo84 commented Mar 28, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks E/W loadbalancing (ClusterIP, NodePort from inside cluster, etc) Tests NodePort inside cluster (kube-proxy) with IPSec and externalTrafficPolicy=Local

Failure Output

FAIL: Request from k8s1 to service http://[fd04::11]:31989 failed

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

christarazi
christarazi reviewed Mar 29, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

PR mostly LGTM, just a few comments below.

I was also wondering: could we make a generic version of onUpsert[C]CNP() given that the code inside is pretty much the same?

pkg/k8s/watchers/cilium_network_policy.go Show resolved Hide resolved
pkg/k8s/watchers/cilium_clusterwide_network_policy.go Show resolved Hide resolved
@pippolo84 pippolo84 force-pushed the pr/pippolo84/add-cnp-resource branch from 6f174b7 to 65a8f62 Compare March 29, 2023 10:08
@pippolo84
Copy link
Member Author

pippolo84 commented Mar 29, 2023 •
edited

PR mostly LGTM, just a few comments below.

I was also wondering: could we make a generic version of onUpsert[C]CNP() given that the code inside is pretty much the same?

Actually the underlying type used by both onUpsertCNP and onUpsertCCNP is the same types.SlimCNP, so there's no need to use a type parameter. But I changed onUpsert and onDelete to accept additional parameters for the api group and the related metric label, so now both CNP and CCNP code paths rely on the same callbacks.

@pippolo84 pippolo84 force-pushed the pr/pippolo84/add-cnp-resource branch 2 times, most recently from 0f59845 to dcabd69 Compare March 29, 2023 14:44
christarazi
christarazi approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

🎉

@pippolo84
Copy link
Member Author

kubernetes-e2e failure seems a flake, tracked it here.

@pippolo84
Copy link
Member Author

pippolo84 commented Mar 30, 2023 •
edited by christarazi

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' hit: #24648 (86.19% similarity)

Edit: net-next hit #24573

@maintainer-s-little-helper maintainer-s-little-helper bot mentioned this pull request Mar 30, 2023
Closed
@christarazi
Copy link
Member

/test-1.26-net-next

@christarazi
Copy link
Member

christarazi commented Mar 30, 2023 •
edited by maintainer-s-little-helper bot

/test-1.25-4.19

Job 'Cilium-PR-K8s-1.25-kernel-4.19' hit: #24648 (86.61% similarity)

@pippolo84
k8s: Add shared resources for CNPs and CCNPs
2ed7144 
Add Cilium Network Policies and Cilium Clusterwide Network Policies as
shared resources in the proper cell.

With this, the resources events are managed more efficiently thanks to
the single informer plus workqueue for each resource type coupled with
the subscriber events queue.

Also, this will make the modularization and testing of all subsystems
that depend on CNPs or CCNPs events easier.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
k8s: Use CNP shared resource in k8s watcher
c2011a1 
Refactor the k8s watchers code to rely on the Cilium Network Policy
shared resource instead of starting a new Informer.

Starting a new informer increases the load on api server and without a
workqueue, the handlers will block the events streaming until
completion.  The usage of the CNP shared resource solves this issues.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84
k8s: Use CCNP shared resource in k8s watcher
bba8f78 
Refactor the k8s watchers code to rely on the Cilium Clusterwide Network
Policy shared resource instead of starting a new Informer.

Starting a new informer increases the load on api server and without a
workqueue, the handlers will block the events streaming until
completion.  The usage of the CCNP shared resource solves this issues.

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
@pippolo84 pippolo84 force-pushed the pr/pippolo84/add-cnp-resource branch from dcabd69 to bba8f78 Compare March 30, 2023 22:11
@pippolo84
Copy link
Member Author

pippolo84 commented Mar 30, 2023 •
edited by christarazi

/test

Edit: net-next hit #24573

@christarazi
Copy link
Member

/test-1.26-net-next

2 similar comments
@pippolo84
Copy link
Member Author

/test-1.26-net-next

@pippolo84
Copy link
Member Author

/test-1.26-net-next

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 31, 2023
@christarazi christarazi merged commit d6f5504 into cilium:master Mar 31, 2023
42 checks passed
@pippolo84 pippolo84 mentioned this pull request Apr 11, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

@youngnick youngnick youngnick approved these changes

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@pippolo84 @christarazi @youngnick