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

NRI network #57

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ PLUGINS := \
$(BIN_PATH)/differ \
$(BIN_PATH)/ulimit-adjuster \
$(BIN_PATH)/v010-adapter \
$(BIN_PATH)/setupnetwork \
$(BIN_PATH)/template


Expand Down Expand Up @@ -118,6 +119,10 @@ $(BIN_PATH)/template: $(wildcard plugins/template/*.go)
$(Q)echo "Building $@..."; \
cd $(dir $<) && $(GO_BUILD) -o $@ .

$(BIN_PATH)/setupnetwork: $(wildcard plugins/setupnetwork/*.go)
$(Q)echo "Building $@..."; \
cd $(dir $<) && $(GO_BUILD) -o $@ .

#
# test targets
#
Expand Down
78 changes: 78 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,84 @@ The following pieces of container metadata are available to plugins in NRI:
Apart from data identifying the container, these pieces of information
represent the corresponding data in the container's OCI Spec.

## Pod Network Lifecycle Events

NRI plugins can subscribe to the following pod network events:

- Pod network configuration change
- Pod network pre-creation before CNI plugins are called
- Pod network post-creation after CNI plugins have been called
- Pod network removal

### Pod Network Configuration Change
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

I'll take a look at those.

Copy link
Author

@pfl pfl Jan 8, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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


<details>
<summary>NRI pod network configuration change event</summary>
<p align="center">
<img src="./docs/nri-cni-conf.svg" title="NRI Pod Network Configuration Change Event">
</p>
</details>

A NetworkConfigurationChanged event is sent each time a change in CNI
configuration is detected. It is left as an implementation detail for the
runtime whether a NetworkConfigurationChanged evet contains one or more
changed CNI configurations should a simultaneous modification of more than
one configuration be detected at the same time.

NRI plugins are able to alter only those CNI configuraions that are sent by the
runtime, no additions are allowed. An NRI plugin can return either a modified
version of one or more CNI configurations received, unmodified versions or an
empty value or nil. Returned values will later be used by the runtime when a
pod network is created.

### Pod Network Creation

<details>
<summary>NRI pod network setup events</summary>
<p align="center">
<img src="./docs/nri-cni-setup.svg" title="NRI Pod Network Setup Events">
</p>
</details>

### PreSetupNetwork

The PreSetupNetwork event is sent immediately before CNI is invoked to set
up the pod network in its namespace. In addition to the PodSandbox object,
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
Copy link
Contributor

@aojea aojea Dec 5, 2023

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

capabilities and arguments will be added to the ones already set for the pod
network.

### PostSetupNetwork

The PostSetupNetwork event is sent immediately after a successful CNI
invocation. In addtion to the PodSandbox objedct, the message carries a
structure containing the output CNI Result Type data received from CNI, which
is equivalent of the output of the last CNI plugn processed.

NRI is allowed to modify and return the modified contents of the Result Type.
NRI may reshuffle for example the order of the interface information thus
changing the default interface or otherwise keep track of CNI parameters
when implementing its policy.

### Pod Network Deletion

<details>
<summary>NRI pod network deletion events</summary>
<p align="center">
<img src="./docs/nri-cni-delete.svg" title="NRI Pod Network Delete Event">
</p>
</details>

A PreNetworkDeleted method call is sent on pod network deletion before the
network namespace is removed. As an empty return message is expected, NRI has
time to undo any modifications that relate to the network namespace. Once all
plugins have processed the event, the namespace is removed, and a
PostNetworkDeleted event is sent signalling the finalization of the network
removal.

### Container Adjustment

During container creation plugins can request changes to the following
Expand Down
1 change: 1 addition & 0 deletions docs/nri-cni-conf.svg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Loading