Calico: Exposing the IPIP tunnel value in the ConfigMap instead hardcoding in the DaemonSet

[igoratencompass]

This is a feature request.

Description

I have noticed in calico.yml file that the Calico MTU is an option in theConfigMap:

kind: ConfigMap
apiVersion: v1
metadata:
  name: calico-config
  namespace: kube-system
data:
  cni_network_config: |-
    {
        "name": "k8s-pod-network",
        "cniVersion": "0.1.0",
        "type": "calico",
        ...
        "mtu": 1500,
        ...
    }

but then in the DaemonSet there is another environment value for the ipip tunnel that I think should be closely coupled with the above one:

kind: DaemonSet
apiVersion: extensions/v1beta1
...
      containers:
        # Runs calico/node container on each Kubernetes node.  This
        # container programs network policy and routes on each
        # host.
        - name: calico-node
          env:
          ...
            # Set MTU for tunnel device used if ipip is enabled
            - name: FELIX_IPINIPMTU
              value: "1440"
          ...

and maybe worth moving this variable into the ConfigMap too. The reasoning behind this is the users in AWS for example would for sure want the MTU bumped to 9000 after which the FELIX_IPINIPMTU would/should follow with 8940 so they don't miss on improved tunnel performance.

Also maybe worth mentioning this fact in the documentation since as it is now it can easily happen that users would modify the mtu value but easily oversee the FELIX_IPINIPMTU one.

[igoratencompass]

/cc @caseydavenport

[caseydavenport]

Moving to the configmap seems fine to me.

We still fundamentally have two controls - one for the CNI plugin and another for the tunl0 interface. Ideally, you'd run without the tunl0 interface anyway for best results :)

[ketkulka]

@caseydavenport i can help with this.

One option would be having:
ipinip_mtu key-value in confimap data.

and keep FELIX_IPINIPMTU same but get its valueFrom configMapKeyref ipinip_mtu

Still user will have to configure ipinip_mtu and mtu in confimap but at least it would at the same place.

Let me know what you think.

Thanks
Ketan

[caseydavenport]

FWIW the documentation here seems to explain the two options fairly well: https://docs.projectcalico.org/v3.0/usage/configuration/mtu

I'm questioning the value of moving into the configmap without first considering if we could simplify even further somehow. For example, is there ever a situation where users want the CNI MTU and the IPIP MTU to be different?

If not, perhaps we could streamline a bit more and end up with a single config option.

[ketkulka]

hmm thanks for the doc pointer.
If I may take a shot at simplifying it; let me know how this proposal look like -

1. have a single MTU var in configmap. Remove existing mtu: from cni_network_config
2. modify install-cni.sh in such way that cni config file that it generates includes mtu from new MTU var in configmap (possible using sed)
3. remove FELIX_IPINIPMTU from daemonset and have MTU var in daemonset config and inherit its value from configmap MTU
4. modify felix/run to export FELIX_IPINIPMTU from the MTU var less 20 bytes.

if you think its worth pursuing; i can work on it accordingly.

[caseydavenport]

@ketkulka I think that sounds sensible to me. We can add a new replacement variable to install-cni, like __CNI_MTU__ which gets replaced with the value from the CNI_MTU env var, and defaults to 1500.

Then both the install-cni and calico-node containers can pull in their respective MTU env vars from the config map, allowing for a single option which controls both.

The one thing I'm not sure about is this part:

modify felix/run to export FELIX_IPINIPMTU from the MTU var less 20 bytes.

I think that is likely to confuse users who expect that the value they set in the config to be the value used. I think let's not modify the value and just pass through the info from the configmap.

[ketkulka]

@caseydavenport thanks for considering the proposal and reply. I have one question though; with today's configuration options; user gets to choose the MTU and IPINIP_MTU. This may be useful (?) for the cases mentioned in documentation specifically - VxLAN and Tunnels.
with the proposed single configurable variable; we may miss out giving this freedom.

can you please share your view on this?

i can open the PR for the same.

[caseydavenport]

@ketkulka yep exactly, we'd still have the same individual controls that we've got today, but they would by default be configured from the same field in the config map.

If some users wanted different values, they could still configure the individual options as they do today.

[ketkulka]

Open two PRs

1. for install-cni
   https://github.com/projectcalico/cni-plugin/pull/491
2. for calico-node
   https://github.com/projectcalico/calico/pull/1770

[ketkulka]

friendly reminder @caseydavenport

[caseydavenport]

@ketkulka thanks, I hope to look at this week, I've just been super busy. Keep poking me if I move too slowly!