NRI network #57

pfl · 2023-09-22T15:05:55Z

Implementation of NRI network messages. The messages added are:

NetworkConfigurationChanged when the CNI config has been read from disk
PreSetupNetwork before CNI is set up
PostSetupNetwork after successful CNI configuration
Pre- and PostNetworkDeleted once the pod network is removed

pfl · 2023-09-26T15:13:20Z

Fixed forgotten test cases and NetworkDeleted functionality

codecov-commenter · 2023-09-27T07:58:27Z

Codecov Report

Attention: 195 lines in your changes are missing coverage. Please review.

Comparison is base (c68cf49) 64.61% compared to head (40fbd54) 58.27%.

❗ Current head 40fbd54 differs from pull request most recent head ce1e5a1. Consider uploading reports for the commit ce1e5a1 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #57      +/-   ##
==========================================
- Coverage   64.61%   58.27%   -6.34%     
==========================================
  Files           9        9              
  Lines        1834     2030     +196     
==========================================
- Hits         1185     1183       -2     
- Misses        498      696     +198     
  Partials      151      151

Files	Coverage Δ
pkg/adaptation/adaptation.go	`61.14% <0.00%> (-11.55%)`	⬇️
pkg/adaptation/plugin.go	`40.58% <0.00%> (-8.01%)`	⬇️
pkg/adaptation/result.go	`71.02% <1.07%> (-8.82%)`	⬇️

... and 1 file with indirect coverage changes

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

pfl · 2023-10-10T07:28:53Z

Plugin fixed to match expected functions

PreSetupNetwork messages are sent to plugins before CNI is invoked to set up pod networking. The plugins can return a set of additional CNI capabilities and arguments. Once CNI networking has completed, PostSetupNetwork messages containing an equivalent of the CNI Result structure are sent out to interested plugins, which can return the same or a modified version of the received CNI Result structure. Signed-off-by: Patrik Flykt <[email protected]>

NetworkConfigurationChanged messages are sent whenever a CNI network configuation file is updated on disk. One or more CNI configurations can be sent, depending on the implementation in the runtime. Only modifications of existing CNI configurations are supported. Signed-off-by: Patrik Flykt <[email protected]>

When the pod sandbox network is to be removed, NRI plugins are informed of the event in form of PreNetworkDeleted message and a PostNetworkDeleted event. The PreNetworkDeleted request is sent to the plugins while the network namespace is still available and waits until all plugins have responded. Once all plugins have responded to the PreNetworkDeleted message, the namespace is removed, and PostNetworkDeleted events will be sent to plugins subscribed to the event. Signed-off-by: Patrik Flykt <[email protected]>

Signed-off-by: Patrik Flykt <[email protected]>

Log NRI network messages Signed-off-by: Patrik Flykt <[email protected]>

Add example plugin logging network related messages. Signed-off-by: Patrik Flykt <[email protected]>

pfl · 2023-12-05T16:45:16Z

Removed 'WIP' labels as this PR has progressed into something decently useful.

aojea · 2023-12-05T20:49:14Z

/cc

aojea · 2023-12-05T23:32:55Z

README.md

+  - Pod network post-creation after CNI plugins have been called
+  - Pod network removal
+
+### Pod Network Configuration Change


As a person that worked developing and maintaining different CNI plugins, the CNI word evolved from referring to the CNI spec to refer to component that implements the spec (Calico, Cilium, ovn-kubernetes, kindnet, ...)

The signatue of the existing CNI calls in containerd are

err = netPlugin.Status()
err = netPlugin.Remove(ctx, id, path, opts...)
result, err := netPlugin.Setup(ctx, id, path, opts...)

if those 3 methods are added as 3 hooks of the NRI plugin (so it can be actually replaced by the NRI plugin), an existing CNI implementation can move to a more rich and flexible model provided by NRI and move on from the legacy interface that has a lot of constrains (strong typing, shell, json, ...) https://github.com/containernetworking/cni/issues?q=is%3Aissue+is%3Aopen+grpc

These days the "CNI plugins" need to run connected to the apiserver to perform API operations, they can not longer run as standalone binaries, Cilum https://github.com/cilium/cilium/blob/d1128cf948979de7433385b2101a98404d99c64f/pkg/client/client.go#L32-L41
and OVN-kubernets https://github.com/ovn-org/ovn-kubernetes/blob/2916102d231ce9596e23c9880103225db2caa65e/go-controller/pkg/cni/cnishim.go#L37-L49 are alread using client/server model, with a binary that is implementing the CNI interface that uses a socket to connect to the "CNI plugin" , the NRI plugin can make this a native integration

+1 on the native integration.

And I also agree that a "modern" plugin requires knowledge of API server objects (to various extents).

Generalizing this integration would definitely be beneficial.

I'll take a look at those.

The signatue of the existing CNI calls in containerd are

err = netPlugin.Status()
err = netPlugin.Remove(ctx, id, path, opts...)
result, err := netPlugin.Setup(ctx, id, path, opts...)

if those 3 methods are added as 3 hooks of the NRI plugin (so it can be actually replaced by the NRI plugin), an existing CNI implementation can move to a more rich and flexible model provided by NRI and move on from the legacy interface

If these three network methods were to be implemented and only these three new methods were used to set up a network, does that imply the contents of /etc/cni/net.d/ is no longer neither needed nor available to the runtime, as e.g. a netPlugin.GetConfig() equivalent would not exist? Similarly, cniNetConfSyncer also disappears, as the network config is now in the hands of an NRI plugin? Or does the runtime still keep track of files in /etc/cni/net.d/, and keep sending the opts arguments to Setup() and Remove(), as is the case now?

result, err := netPlugin.Setup(ctx, id, path, opts...)

Does the 'opts' argument here turn into json marshalled protobuf map<string, bytes> and unmarshalled in the other end as string, interface{} mappings in Go, or did you have some other format in mind?

as e.g. a netPlugin.GetConfig() equivalent would not exist? Similarly, cniNetConfSyncer also disappears, as the network config is now in the hands of an NRI plugin? Or does the runtime still keep track of files in /etc/cni/net.d/, and keep sending the opts arguments to Setup() and Remove(), as is the case now?

heh, that is the 1M dollar question

I think CNI solves a problem and works well solving that problem, but is hard to extend. If we think about this as CNI 2.0 , that is very similar per the issues linked https://github.com/containernetworking/cni/issues?q=is%3Aissue+is%3Aopen+grpc , it will logically to think about it as exclusive, so people should choose between v1 or v2, but never both at same time, I do not know if that is even possible though

aojea · 2023-12-05T23:44:01Z

README.md

+arguments to PreSetupNetwork includes an array of strings containing all JSON
+formatted CNI configuration files associated with the pod.
+
+The reply data consists of CNI capabilities and 'args' arguments. Any returned


"CNI capabilities" is not a great API, mainly because is a convention https://github.com/containernetworking/cni/blob/main/CONVENTIONS.md#current-conventions

Looking at the above comments, we need plain 'opts' transported back and forth here?

pfl · 2024-05-23T07:37:15Z

Let's revisit this PR once KNI has taken shape. At that point we will learn whether any CNI specific plugins will be needed or if plugins interested in network activity listen to KNI instead.

MikeZappa87 · 2024-05-30T15:35:38Z

@pfl want to reopen this?

pfl · 2024-05-31T16:36:19Z

Once KNI messaging gets specified, NRI plugins may perhaps hook into KNI directly. If needed, a non-KNI NRI based setup should be as close to a KNI one as possible in order to avoid excessive coding and added architecture. Right now I'm unfortunately (de)allocated to some DRA work until mid Q3. Thus the pause in this PR.

samuelkarp marked this pull request as draft September 22, 2023 23:01

pfl force-pushed the nri_network branch from 27c3e99 to 1977ab1 Compare September 26, 2023 15:12

pfl force-pushed the nri_network branch from 1977ab1 to c3952b1 Compare September 27, 2023 07:56

pfl force-pushed the nri_network branch from c3952b1 to 0926ef0 Compare September 27, 2023 14:52

pfl force-pushed the nri_network branch from 0926ef0 to ce1e5a1 Compare October 10, 2023 07:27

pfl force-pushed the nri_network branch from ce1e5a1 to d14b6a1 Compare October 20, 2023 14:35

pfl force-pushed the nri_network branch from d14b6a1 to 6dff50f Compare November 17, 2023 14:26

pfl force-pushed the nri_network branch from 6dff50f to 479e20e Compare December 1, 2023 16:06

pfl added 2 commits December 5, 2023 17:48

pfl force-pushed the nri_network branch from 479e20e to 53cbe21 Compare December 5, 2023 16:41

pfl added 4 commits December 5, 2023 18:41

Add NRI network message description and figures

a55ace4

Signed-off-by: Patrik Flykt <[email protected]>

plugins/logger: Add NRI network messages to logger plugin

29bf43e

Log NRI network messages Signed-off-by: Patrik Flykt <[email protected]>

Add plugin for NRI network messages

53cbe21

Add example plugin logging network related messages. Signed-off-by: Patrik Flykt <[email protected]>

pfl changed the title ~~WIP: Nri network~~ NRI network Dec 5, 2023

aojea reviewed Dec 5, 2023

View reviewed changes

pfl mentioned this pull request Dec 15, 2023

WIP: NRI CNI network policy support containerd/containerd#9550

Closed

pfl closed this May 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NRI network #57

NRI network #57

pfl commented Sep 22, 2023 •

edited

Loading

pfl commented Sep 26, 2023

codecov-commenter commented Sep 27, 2023 •

edited

Loading

pfl commented Oct 10, 2023

pfl commented Dec 5, 2023

aojea commented Dec 5, 2023

aojea Dec 5, 2023

aojea Dec 5, 2023

maiqueb Dec 11, 2023

pfl Dec 14, 2023

pfl Jan 8, 2024 •

edited

Loading

pfl Jan 8, 2024

aojea Jan 9, 2024

aojea Dec 5, 2023 •

edited

Loading

pfl Jan 8, 2024

pfl commented May 23, 2024

MikeZappa87 commented May 30, 2024

pfl commented May 31, 2024

NRI network #57

NRI network #57

Conversation

pfl commented Sep 22, 2023 • edited Loading

pfl commented Sep 26, 2023

codecov-commenter commented Sep 27, 2023 • edited Loading

Codecov Report

pfl commented Oct 10, 2023

pfl commented Dec 5, 2023

aojea commented Dec 5, 2023

aojea Dec 5, 2023

Choose a reason for hiding this comment

aojea Dec 5, 2023

Choose a reason for hiding this comment

maiqueb Dec 11, 2023

Choose a reason for hiding this comment

pfl Dec 14, 2023

Choose a reason for hiding this comment

pfl Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

pfl Jan 8, 2024

Choose a reason for hiding this comment

aojea Jan 9, 2024

Choose a reason for hiding this comment

aojea Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

pfl Jan 8, 2024

Choose a reason for hiding this comment

pfl commented May 23, 2024

MikeZappa87 commented May 30, 2024

pfl commented May 31, 2024

pfl commented Sep 22, 2023 •

edited

Loading

codecov-commenter commented Sep 27, 2023 •

edited

Loading

pfl Jan 8, 2024 •

edited

Loading

aojea Dec 5, 2023 •

edited

Loading