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

Implement CNI v1.1 support #1021

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

squeed
Copy link
Member

@squeed squeed commented Mar 20, 2024

This implements CNI v1.1 for all plugins.

TODO (will probably shuffle to issues):

  • GC for masquerade rules
  • GC for portmap
  • GC for bandwidth
  • GC for windows

squeed added 3 commits March 19, 2024 11:57
Signed-off-by: Casey Callendrello <c1@caseyc.net>
Signed-off-by: Casey Callendrello <c1@caseyc.net>
cni v1.1 adds new route fields:
- MTU
- AdvMSS
- Priority
- Table
- Scope

Signed-off-by: Casey Callendrello <c1@caseyc.net>
@squeed squeed changed the title Cni version 1.1 Implement CNI v1.1 support Mar 20, 2024
squeed added 16 commits March 20, 2024 21:41
This adds a simple GC implementation for host-local IPAM. It walks the
directory and removes any un-allocated files.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- STATUS and GC implementation
    Still outstanding is GCing iptables rules
- return interface MTU
- tests

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- STATUS and GC implementations
- MTU in result type
- Tests & test fixes

Signed-off-by: Casey Callendrello <c1@caseyc.net>
There are no resources to GC, so just pass through to delegated IPAM.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- report MTU
- delegate STATUS and GC to ipam (if configured)

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- STATUS and GC passthrough
- MTU reporting

Signed-off-by: Casey Callendrello <c1@caseyc.net>
It doesn't implement release, which we will need. It's also abandonware.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
Needed for GC tests.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- STATUS checks to see that the daemon is running
- GC clears stale leases

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- No need for STATUS or GC; the default impls are fine
- return MTU

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- STATUS and GC are just passed through to ipam
- MTU is reported

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- Pass through GC and STATUS to ipam
- Report MTU

Signed-off-by: Casey Callendrello <c1@caseyc.net>
- Pass through STATUS and GC to IPAM
- return MTU

Signed-off-by: Casey Callendrello <c1@caseyc.net>
This passes through STATUS and GC to ipam. It does not clean up any
stale HNS endpoints, nor does it report MTU. These remain as TODO items.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
Signed-off-by: Casey Callendrello <c1@caseyc.net>
This adds a basic CNI v1.1 implementation to the meta plugins.

For the plugins which do have state (firewall, bandwidth, portmap) this
does *not* perform GC. That remains outstanding.

Signed-off-by: Casey Callendrello <c1@caseyc.net>
Copy link

@bleggett bleggett left a comment

Choose a reason for hiding this comment

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

LGTM, ty, lot of little changes.

return err
}

l.IP = net.ParseIP(stringUnMarshal.IP)

Choose a reason for hiding this comment

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

Maybe check if the parse failed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

This whole file is just a copy-paste of an old dependency. It's silly code just for testing; I didn't want to fix it up.

t.pool = t.pool[:(len(t.pool) - 1)]
//Place the Lease At the Begining (This saves us having some sort of counter...)
// Place the Lease At the Beginning (This saves us having some sort of counter...)

Choose a reason for hiding this comment

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

Suggested change
// Place the Lease At the Beginning (This saves us having some sort of counter...)
// Place the Lease at the beginning (This saves us having some sort of counter...)

ultra-nit

Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

  • GC for masquerade rules
  • GC for portmap
  • GC for bandwidth
  • GC for windows

Suppose that we need to mention somewhere else (e.g. issue or TODO file, or each plugin's code comment) to keep track.

var multicastNet string
var ip string // the ip and its full-length prefix

if isV6 {
Copy link
Contributor

Choose a reason for hiding this comment

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

isV6 seems to be used only here. So how about to replace it with ipn.IP.To4() == nil directly?

@@ -0,0 +1,102 @@
package leasepool
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I understand, this code comes from https://github.com/d2g/dhcp4server/, licensed as Mozilla Public License 2.0(MPL2.0) and it includes this code, not vendoring.

Can we copy this file without LICENSE notification? (I mean that dhcp4server is MPL2.0, not APL2.0). Should we add MPL2.0 in LICENSE file?

@@ -114,6 +114,46 @@ func TeardownIPMasq(ipn *net.IPNet, chain string, comment string) error {
return nil
}

func CheckIPMasq(ipn *net.IPNet, chain, comment string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing comments for the function?

@@ -130,6 +131,36 @@ func (d *DHCP) Release(args *skel.CmdArgs, _ *struct{}) error {
return nil
}

func (d *DHCP) Ping(_ *skel.CmdArgs, _ *struct{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ping could be implemented with DHCP discover message. How about to mention in comments as TODO?

ValidAttachments []GCAttachment `json:"cni.dev/valid-attachments,omitempty"`
}

// GCAttachment is the parameters to a GC call -- namely,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use github.com/containernetworking/cni/pkg/types.GCAttachment ?

@@ -625,3 +633,33 @@ func validateCniContainerInterface(intf current.Interface) error {

return nil
}

func cmdStatus(args *skel.CmdArgs) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

We may check the host-device's existence for status. How about to do that (or add comments for TODO?)

@@ -484,3 +491,32 @@ func validateCniContainerInterface(intf current.Interface, modeExpected string)

return nil
}

func cmdStatus(args *skel.CmdArgs) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about to check whether master interface exists or not?

maiqueb added a commit to maiqueb/devconf2024 that referenced this pull request May 29, 2024
We must manually adapt the containernetworking IPAM invoking functions,
since as of today, the PRs adding that support are not merged.

Status is being added in [0], which was extracted from [1], where GC is
added.

[0] - containernetworking/plugins#1050
[1] - containernetworking/plugins#1021

Signed-off-by: Miguel Duarte Barroso <mdbarroso@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants