Better Calico compatibility in a multi-cni configuration

davidjbrady commented 7 years ago

Expected Behaviour

Calico to be extra ;) useful as delegate in a multi cni configuration. E.G only used for pod and service networking in a Kubernetes context.

Current Behavior

Calico CNI is hard coded to be the default gateway https://github.com/projectcalico/cni-plugin/blob/bcd2d3f825002c218bfaa872206f9bf42753c7e3/utils/network.go#L100 It would be great if there was a boolean option exposed in Calico's CNI NetConf. that allowed users to specify if Calico should be the default gateway

Possible Solution

Add an additional field (isDefaultGateway bool) with a default value of true in the NetConf type https://github.com/projectcalico/cni-plugin/blob/28a93c7f17bae5b3ea6347c509b52066f069241b/types/types.go#L59

Wrap a if conf.isDefaultGateway == true {} around the following line https://github.com/projectcalico/cni-plugin/blob/bcd2d3f825002c218bfaa872206f9bf42753c7e3/utils/network.go#L100

Context

I require multiple interfaces exposed to Kubernetes Pods where each pods primary interface is used for Pod and Service networking and a second interface utilised for public internet connectivity. An example use-case for this configuration is independent metering of East/West and North/South traffic that's only possible when multiple interfaces are attached to a pod.

caseydavenport commented 6 years ago

@davidjbrady thanks for raising.

Yes, there are a few bits of the Calico CNI implementation that assume it's the only (or primary) plugin in use.

A flag seems OK, though a little unsatisfying. We'd still need to figure out how to get the traffic out the Calico interface (we'd need to install another route somehow).

caseydavenport commented 6 years ago

One option here I suppose would be for the Calico plugin to support the routes section of the CNI config. If specified, it could use those routes instead of the standard default route programmed by Calico today.

ryanlyy commented 6 years ago

I am trying Calico 3.1 and multus, seems they failed at "Error adding network: Multus: error in invoke Delegate add - "calico": invalid characters detected in the given network name. Only letters a-z, numbers 0-9, and symbols _.- are supported" same with projectcalico/cni-plugin#460.

Checking multis code, it is fired at here when calling calico plugin: result, err := invoke.DelegateAdd(netconf["type"].(string), netconfBytes) if err != nil { return true, fmt.Errorf("Multus: error in invoke Delegate add - %q: %v", netconf["type"].(string), err) }

So Can Calico plugin be used in multus plugin now?

NOTE: I am using CRD instead of TPR and Kubernetes 1.8.

thanks, Ryan

caseydavenport commented 6 years ago

@ryanlyy what is the network name that's being used?

kannanvr commented 6 years ago

Is it possible to use calico with Multus?

shufanhao commented 5 years ago

Any update for this issue ? I also want to add second interface for container.
One question: Could Calico support assign two ip address that come from different IPPool, for example: one is 172.16.0.0/16, another is 192.168.0.0/16。

fasaxc commented 5 years ago

Does this flag do what you need: https://github.com/projectcalico/cni-plugin/blob/master/pkg/types/types.go#L94 If you use that with the host-local IPAM plugin then we support host-local's routes setting. (It is not supported with Calico IPAM)

Jc2k commented 5 years ago

I've been looking into trying to improve this in my env. I've patched my local build of cni-plugin:

To support the routes key for calico-ipam
To detect any existing default routes in the new namespace and skip adding default routes in that case

This is then used with the default-network annotation in multus so that calico is the secondary network.

The aim of this approach is that the majority of my containers get the normal calico network setup (including default route). But the odd few can get an internet facing interface without calico claiming the default route by setting a different cni as the default network.

This seems to work more or less, but the host side routes are wrong. It's picking up the IP from the default network, which is no longer calico. If i manually fix the route it works briefly and something (felix?) flips it back. I don't know felix well enough to understand how the IP is picked up, so any clues would be appreciated.

Jc2k commented 5 years ago

So to recap, one scenario a user might try with mutlus is to make a non-calico CNI the default network. They might do something like this.

I have a 10-calico.conflist:

{
  "name": "calico",
  "cniVersion": "0.3.0",
  "plugins": [
    {
      "type": "calico",
      "log_level": "info",
      "datastore_type": "kubernetes",
      "nodename": "kube1",
      "mtu": 1440,
      "ipam": {
          "type": "calico-ipam",
          "routes": [{"dst": "192.168.0.0/16"}, {"dst": "172.16.0.0/12"}, {"dst": "10.0.0.0/8"}]
      },
      "policy": {
          "type": "k8s"
      },
      "kubernetes": {
          "kubeconfig": "/etc/cni/net.d/calico-kubeconfig"
      }
    },
    {
      "type": "portmap",
      "snat": true,
      "capabilities": {"portMappings": true}
    }
  ]
}

# This is a little nudge to tell multus to look at the 10-calico.conflist 
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: calico
  namespace: networks
spec: {}
---
apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: ens160
  namespace: networks
spec:
  config: '{
                "cniVersion": "0.3.0",
                "plugins": [{
                  "type": "macvlan",
                  "master": "ens160",
                  "ipam": {
                        "type": "static",
                        "addresses": [{"address": "192.168.1.252/24", "gateway": "192.168.1.1"}],
                        "routes": [{"dst": "0.0.0.0/0", "gateway": "192.168.1.1"}]
                  }
                }]
  }'
---
apiVersion: v1
kind: Pod
metadata:
  name: samplepod
  annotations:
    v1.multus-cni.io/default-network: networks/ens160@net2
    k8s.v1.cni.cncf.io/networks: networks/calico@eth0
spec:
  nodeSelector:
    kubernetes.io/hostname: kube1
  containers:
  - name: samplepod
    command: ["/bin/bash", "-c", "trap : TERM INT; sleep infinity & wait"]
    image: dougbtv/centos-network

If working as expected the pod will end up with a net2 that gets its IP directly from a physical uplink and an eth0 that gets its ip from calico. net2 should be the default route, but eth0 should be used for the private LAN addresses. (The intention is that a public IP is assigned to net2, but 192.168.0.0/24 works here because its a more specific route than 192.168.0.0/16).

If networks/ens160 defined a default route (as it does here) then calico would fail as there is no way to surpress its default routes. (In the calico-ipam case it doesn't respect conf.IncludeDefaultRoutes).

To make this work I:

Made cni-plugin only inject default routes if there were no default routes already (patch). Without this the plugin will fail in my example as there is already a default route.
- Made cni-plugin respect the routes section for calico-ipam (patch). Without this in my example i'd end up with no routes for 169.254.1.1.
Hacked libcalico-go so that it ignores Status.PodIP and only uses the calico annotation. Without this felix will stomp on the host side route entry. Specifically in this example it would add 192.168.1.252 as route to the cali* veth instead of the calico assigned 172.20.x.x address.

With these changes in place it seems I am able to bring up calico as a secondary network on some of my containers whilst still having calico as the primary on the rest (although I will be testing this more the coming week). (For other people on this ticket who have asked, not calico twice. Just one calico as a seconary, with a different primary cni).

I don't know enough go (think this is my first attempt at go) or calico to know what is next. In particular, I don't know what the "right" answer is for patch 3 - calico prefers Status.PodIP but i don't know why that is, and as long as it does i cant see how it can be used in a multi-cni configuration.

caseydavenport commented 5 years ago

@Jc2k thanks for your investigation and the detailed write-up! I'll try to take a look at your patches this week to understand in a bit more detail.

I think the next steps are to decide if these patches are the correct path forward. Agree that the libcalico-go change is probably the most suspect, I think we'll need another solution for it (relying solely on the annotation probably won't fly long-term) but it's not obvious to me what the right one is yet (the other two seem sensible at face value)

Jc2k commented 5 years ago

I guess the biggest problem I had moving forward on my own was just knowing what is the reason for having an annotation and using Status.PodIP, especially when are they different and when are they the same.. or when is one missing and the other not.

From an outsiders point of view it seems reasonable for Status.PodIP to not be used, especially when the calico setup is a secondary network setup by multus (and its PodIP is ultimately set by multus, i think?). Are there timing issues where one might be set and another not or some other edge case?

I guess the next thought I had was if we kept the current behaviour and the 2 values are different, how do you know which is "right"? Looking at a couple of containers on a test node right now I see this on the host node:

blackhole 172.20.100.0/26 proto bird 
172.20.100.6 dev calid523f97def9 scope link 
172.20.100.7 dev calic4d776f202d scope link 
172.20.100.26 dev cali3de775f9cb2 scope link

So that blackhole route means there is an IPAM block is assigned to this node. If Pod.Status.PodIP was in that /26 then we should add a route. Otherwise if the annotation is in that /26 then we should add a route to that IP. (I'm not suggesting we rely on the presence of the blackhole route, just that its presence shows we have the information we need to do a test somewhere. How efficient would it be to look up the ips in the store?). Finally, we know that 192.168.1.222 is not one of the blocks calico routes to this box, so why would we add a route for it to the cali* end of one of our veths? With this test we'd never add a random non calico ip to the route table like we do now.

caseydavenport commented 5 years ago

Are there timing issues where one might be set and another not or some other edge case?

This is exactly right. The annotation is just meant to close a window where the Status.PodIP is not set by Kubernetes.

The Status.PodIP is always given precedence when set when Calico is choosing with IP to use. The annotation is only used during a small window during pod initialization.

logan2211 commented 3 years ago

Thanks for the work you did on this @Jc2k. I have a similar situation where I need to place multiple IPs in a pod, ideally all provided by Calico.

I don't care if the IPs are on separate veths or all bound on a single veth, and ended up getting the latter setup working. Using the two patches linked below, I can successfully launch a pod with multiple IPs bound on a single veth when providing ipAddrs annotations with the pod.

Patch projectcalico/cni-plugin#1 - cni-plugin: Allow the CNI plugin to accept multiple addresses in the ipAddrs annotation from the same address family. Patch projectcalico/cni-plugin#2 - libcalico-go: When converting Kubernetes objects to WorkloadEndpoints, prefer the calico podIPs annotation when collecting the pod IPs rather than the Kubernetes PodIP/PodIPs attributes which only support one IP address per address family. Note that PodIP and PodIPs are still used as a fallback in case the calico annotation is not present, however only the first container IP from each address family will be used in that case.

Example pod:

apiVersion: v1
kind: Pod
metadata:
  name: samplepod
  namespace: test
  annotations:
    cni.projectcalico.org/ipAddrs: "[\"10.150.0.1\", \"10.150.0.2\"]"
spec:
  containers:
  - name: samplepod
    command: ["/bin/sh", "-c", "trap : TERM INT; sleep infinity & wait"]
    image: ubuntu:focal

Output from container:

# kubectl -n test exec samplepod -- ip addr list dev eth0
4: eth0@if352: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1480 qdisc noqueue state UP group default
    link/ether 52:c2:a4:b0:ed:a5 brd ff:ff:ff:ff:ff:ff link-netnsid 0
    inet 10.150.0.1/32 brd 10.150.0.1 scope global eth0
       valid_lft forever preferred_lft forever
    inet 10.150.0.2/32 brd 10.150.0.2 scope global eth0
       valid_lft forever preferred_lft forever
    inet6 fe80::50c2:a4ff:feb0:eda5/64 scope link
       valid_lft forever preferred_lft forever

Host routes present:

# ip route | grep 10.150.
10.150.0.1 dev cali03a994187ba scope link
10.150.0.2 dev cali03a994187ba scope link

I think it would be great if Calico will consider adding this feature. I understand there are concerns about preferring the podIPs annotation over the PodIP/PodIPs attributes, however the annotation seems to be present on every pod I have in my env, and there is a fallback to the k8s PodIP/PodIPs in case the annotation is missing. Maybe this is a feature that could be enabled via feature flag if the libcalico change is not desirable to have enabled by default?

Jc2k commented 3 years ago

It's about 2 years since I looked at this, so I should give an update. Right now i've gone with a hack. I have a local CNI plugin that has the power to clobber calico generated routes. It's used via multus with:

apiVersion: "k8s.cni.cncf.io/v1"
kind: NetworkAttachmentDefinition
metadata:
  name: eth0.3
  namespace: networks
spec:
  config: '{
    "cniVersion": "0.3.0",
    "plugins": [
      {
        "type": "macvlan",
        "master": "eth0.3",
        "ipam": {
          "type": "static",
          "addresses": [{"address": "a.b.c.2/24", "gateway": "a.b.c.1"}]
          }
      },
      {
        "type": "local-routes",
        "routes": [
          {"dst": "10.0.0.0/8", "gateway": "169.254.1.1"},
          {"dst": "172.16.0.0/12", "gateway": "169.254.1.1"},
          {"dst": "192.168.0.0/16", "gateway": "169.254.1.1"},
          {"dst": "0.0.0.0/0", "gateway": "a.b.c.1"}
        ]
      },
      {
        "type": "sbr"
      }
    ]
  }'

This is invoked by multus and the k8s.v1.cni.cncf.io/networks annotation.

I think this is a little different to the setup I originally described. Here calico is still the primary network. So I don't hit the pod annotation problem at all. And by having this net-attach-def be a secondary network, the ordering is right for my CNI plugin to clobber the routes after calico has set them up.

This has been working for me in production since I contributed to this ticket, though i'd still prefer to not need a custom CNI that has to undo work done by calico.

After re-reading my notes above it looks like my first 2 patches would be enough on their own as long as calico was the primary network provider. I only needed the 3rd patch to make calico be the secondary network provider, and actually there is no need to do that for quite a few multus use cases.

Those patches were:

Made cni-plugin only inject default routes if there were no default routes already (patch). Without this the plugin will fail in my example as there is already a default route.
Made cni-plugin respect the routes section for calico-ipam (patch). Without this in my example i'd end up with no routes for 169.254.1.1.

Though I imagine they'll need rebasing a bit 😆 @caseydavenport do you think it is worth rebasing them and having another look, and we just ignore the 3rd for now. As it turns out it's not needed for most use cases after all.

@logan2211 I'm glad this ticket was able to help! I wonder if your use case is different enough to be it's own ticket. Looking back at what i've done to make mutli-cni work for me, none of it really helps you. And I think 2 of my patches are still valid, but also wouldn't help your use case. We both hit the annotation vs status thing, but for quite different use cases. I needed to a way to find calico provided ip addresses when constructing host side routes and not confuse them with multus provided non-cni routes (i.e when calico is not the primary CNI). But your patch is to support multiple addresses from the same CNI.

duylong commented 2 years ago

Hi,

I am facing the same problem.

I would like not to use Calico's default route but the default route defined by Multus. Without this functionality, requests are sent with the internal IP of the container and not the public IP provided on the interface created by Multus.

In summary, it would be nice to be able to disable Calico's default route and keep the routes for internal communication in Kubernetes. The default route will be added by Multus (which is already possible with the "isDefaultGateway" parameter).

Jc2k commented 2 years ago

Do you know how the multus isDefaultGateway feature is implemented? Does multus set the default route for us, or does it ask the delegate cni provider to set the default route?

If multus sets the default route for us then we can use the 2nd of my patches (https://github.com/projectcalico/cni-plugin/commit/72dcc1618db4cbfc79190b4be2bf432649094aa3) still.

Then you'd have a cni config like this:

{
  "name": "calico",
  "cniVersion": "0.3.0",
  "plugins": [
    {
      "type": "calico",
      "log_level": "info",
      "datastore_type": "kubernetes",
      "nodename": "kube1",
      "mtu": 1440,
      "include_default_routes": true,
      "ipam": {
          "type": "calico-ipam",
          "routes": [{"dst": "192.168.0.0/16"}, {"dst": "172.16.0.0/12"}, {"dst": "10.0.0.0/8"}]
      },
      "policy": {
          "type": "k8s"
      },
      "kubernetes": {
          "kubeconfig": "/etc/cni/net.d/calico-kubeconfig"
      }
    },
    {
      "type": "portmap",
      "snat": true,
      "capabilities": {"portMappings": true}
    }
  ]
}

If i understand the code correctly with the 2nd patch I posted, that would be enough. The presence of routes in the CNI config should stop calico from injecting its default routes. But without my patch, calico won't respect the routes key so it will fall back and include its default routes still.

However this only works when calico is the primary multus delegate. If it is a secondary then calico will generate routes for the primary multus delegate and not for the calico endpoint.

duylong commented 2 years ago

So I expressed it a bit quickly, I don't know if my default route is created by Multus or by IPv6 autoconfiguration, so you would have to test on your side if isDefaultGateway brings some things.

If it works, It's a good solution that I have to test too, I would also add the IPv6 addresses to the list of routes.

Effectively without any patch, I have 2 routes on my containers and it is impossible for me to delete the route created by Calico:

default via fe80::ecee:eeff:feee:eeee dev eth0  metric 1024                <- Calico
default via fe80::a5b:abc:defb:b7b6 dev net1  metric 1024  expires 0sec    <- RA or Multus ???? I think RA

duylong commented 2 years ago

Hi,

Any news for this issue? I see that the ticket has the "kind/enhancement" label, but in the list, it does not appear. It would be great to manage our own default route and create the routes for Calico ourselves.

maxpain commented 2 years ago

Any updates?

toelke commented 1 year ago

This has been working for me in production since I contributed to this ticket, though i'd still prefer to not need a custom CNI that has to undo work done by calico.

@Jc2k Do you happen to have the code for that custom CNI still lying around?

Nevermind, for my use-case it was easy enough:

#!/usr/bin/env bash

### Remove calicos default route and add a specific route for inter-pod communication

set -eEuo pipefail
shopt -s inherit_errexit

inputData=$(cat)

cniVersion=$(echo "$inputData" | jq -r .cniVersion)
if [[ $cniVersion != "0.3.0" ]] && [[ $cniVersion != "0.3.1" ]]
then
    exit 1
fi

case $CNI_COMMAND in
    VERSION)
        echo "{\"cniVersion\": \"$cniVersion\", \"supportedVersions\": [\"0.3.0\", \"0.3.1\"]}"
        exit 0
        ;;
    ADD)
        netns=$(echo "$inputData" | jq -r '.prevResult.interfaces[0].sandbox')
        nsenter --net="$netns" bash -euxc "ip route del default; ip route add 169.254.0.0/16 via 169.254.1.1"
        # Pass through previous result
        echo "$inputData" | jq -r .prevResult
        exit 0
        ;;
    DEL)
        exit 0
        ;;
    *)
        exit 4
        ;;
esac

projectcalico / calico