From 0659dc868f887bc8a6deb1c62b44a0c5d68e0d24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20F=20Bj=C3=B6rklund?= Date: Thu, 11 Aug 2022 21:26:14 +0200 Subject: [PATCH 1/3] Always use cni unless running with dockershim Since cri-dockerd 0.25, it now defaults to cni. So when using cri (not dockershim), use cni too. --- pkg/minikube/cni/cni.go | 27 ++++++++++++------- .../en/docs/drivers/includes/none_usage.inc | 1 + .../en/docs/drivers/includes/ssh_usage.inc | 1 + 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/pkg/minikube/cni/cni.go b/pkg/minikube/cni/cni.go index 03c9a7988a..00e1c7174f 100644 --- a/pkg/minikube/cni/cni.go +++ b/pkg/minikube/cni/cni.go @@ -87,7 +87,7 @@ func New(cc *config.ClusterConfig) (Manager, error) { var err error switch cc.KubernetesConfig.CNI { case "", "auto": - cnm = chooseDefault(*cc) + cnm, err = chooseDefault(*cc) case "false": cnm = Disabled{cc: *cc} case "kindnet", "true": @@ -117,33 +117,40 @@ func IsDisabled(cc config.ClusterConfig) bool { return true } - if chooseDefault(cc).String() == "Disabled" { + def, err := chooseDefault(cc) + if err == nil && def.String() == "Disabled" { return true } return false } -func chooseDefault(cc config.ClusterConfig) Manager { +func chooseDefault(cc config.ClusterConfig) (Manager, error) { // For backwards compatibility with older profiles using --enable-default-cni if cc.KubernetesConfig.EnableDefaultCNI { klog.Infof("EnableDefaultCNI is true, recommending bridge") - return Bridge{} + return Bridge{}, nil } if len(cc.Nodes) > 1 || cc.MultiNodeRequested { // Enables KindNet CNI in master in multi node cluster, This solves the network problem // inside pod for multi node clusters. See https://github.com/kubernetes/minikube/issues/9838. klog.Infof("%d nodes found, recommending kindnet", len(cc.Nodes)) - return KindNet{cc: cc} + return KindNet{cc: cc}, nil } - if cc.KubernetesConfig.ContainerRuntime != constants.Docker { + version, err := util.ParseKubernetesVersion(cc.KubernetesConfig.KubernetesVersion) + if err != nil { + return nil, err + } + + if cc.KubernetesConfig.ContainerRuntime != constants.Docker || version.GTE(semver.MustParse("1.24.0-alpha.2")) { + // Always use CNI when running with CRI (without dockershim) if driver.IsKIC(cc.Driver) { klog.Infof("%q driver + %q runtime found, recommending kindnet", cc.Driver, cc.KubernetesConfig.ContainerRuntime) - return KindNet{cc: cc} + return KindNet{cc: cc}, nil } klog.Infof("%q driver + %q runtime found, recommending bridge", cc.Driver, cc.KubernetesConfig.ContainerRuntime) - return Bridge{cc: cc} + return Bridge{cc: cc}, nil } // for docker container runtime and k8s v1.24+ where dockershim and kubenet were removed, we fallback to bridge cni for cri-docker(d) @@ -155,11 +162,11 @@ func chooseDefault(cc config.ClusterConfig) Manager { kv, err := util.ParseKubernetesVersion(cc.KubernetesConfig.KubernetesVersion) if err == nil && kv.GTE(semver.MustParse("1.24.0-alpha.2")) { klog.Infof("%q driver + %q container runtime found on kubernetes v1.24+, recommending bridge", cc.Driver, cc.KubernetesConfig.ContainerRuntime) - return Bridge{cc: cc} + return Bridge{cc: cc}, nil } klog.Infof("CNI unnecessary in this configuration, recommending no CNI") - return Disabled{cc: cc} + return Disabled{cc: cc}, nil } // manifestPath returns the path to the CNI manifest diff --git a/site/content/en/docs/drivers/includes/none_usage.inc b/site/content/en/docs/drivers/includes/none_usage.inc index 452612e480..05e7e595f5 100644 --- a/site/content/en/docs/drivers/includes/none_usage.inc +++ b/site/content/en/docs/drivers/includes/none_usage.inc @@ -14,6 +14,7 @@ This VM must also meet the [kubeadm requirements](https://kubernetes.io/docs/set * iptables (in legacy mode) * conntrack * crictl +* cni-plugins * SELinux permissive * cgroups v1 (v2 is not yet supported by Kubernetes) diff --git a/site/content/en/docs/drivers/includes/ssh_usage.inc b/site/content/en/docs/drivers/includes/ssh_usage.inc index 5b2318aadd..c256861515 100644 --- a/site/content/en/docs/drivers/includes/ssh_usage.inc +++ b/site/content/en/docs/drivers/includes/ssh_usage.inc @@ -13,6 +13,7 @@ This VM must also meet the [kubeadm requirements](https://kubernetes.io/docs/set * iptables (in legacy mode) * conntrack * crictl +* cni-plugins * SELinux permissive * cgroups v1 (v2 is not yet supported by Kubernetes) From 425f12997823cb5e01ea448e9de78a6c01b2f76c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20F=20Bj=C3=B6rklund?= Date: Sat, 20 Aug 2022 15:04:56 +0200 Subject: [PATCH 2/3] Replace hardcoded docker check with cni check --- test/integration/net_test.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/test/integration/net_test.go b/test/integration/net_test.go index dae8e1d781..7c06d49524 100644 --- a/test/integration/net_test.go +++ b/test/integration/net_test.go @@ -31,6 +31,7 @@ import ( "github.com/blang/semver/v4" "k8s.io/minikube/pkg/kapi" "k8s.io/minikube/pkg/minikube/config" + "k8s.io/minikube/pkg/minikube/constants" "k8s.io/minikube/pkg/minikube/reason" "k8s.io/minikube/pkg/util" "k8s.io/minikube/pkg/util/retry" @@ -57,7 +58,8 @@ func TestNetworkPlugins(t *testing.T) { namespace string hairpin bool }{ - {"auto", []string{}, "", "", "", true}, + // kindnet CNI is used by default and hairpin is enabled + {"auto", []string{}, "", "", "", usingCNI()}, {"kubenet", []string{"--network-plugin=kubenet"}, "kubenet", "", "", true}, {"bridge", []string{"--cni=bridge"}, "cni", "", "", true}, {"enable-default-cni", []string{"--enable-default-cni=true"}, "cni", "", "", true}, @@ -80,15 +82,14 @@ func TestNetworkPlugins(t *testing.T) { // collect debug logs defer debugLogs(t, profile) - if ContainerRuntime() != "docker" && tc.name == "false" { + if usingCNI() && tc.name == "false" { // CNI is required for current container runtime validateFalseCNI(ctx, t, profile) return } - if ContainerRuntime() != "docker" && tc.name == "kubenet" { + if usingCNI() && tc.name == "kubenet" { // CNI is disabled when --network-plugin=kubenet option is passed. See cni.New(..) function - // But for containerd/crio CNI has to be configured t.Skipf("Skipping the test as %s container runtimes requires CNI", ContainerRuntime()) } @@ -212,6 +213,21 @@ func TestNetworkPlugins(t *testing.T) { }) } +// usingCNI checks if not using dockershim +func usingCNI() bool { + if ContainerRuntime() != "docker" { + return true + } + version, err := util.ParseKubernetesVersion(constants.DefaultKubernetesVersion) + if err != nil { + return false + } + if version.GTE(semver.MustParse("1.24.0-alpha.2")) { + return true + } + return false +} + // validateFalseCNI checks that minikube returns and error // if container runtime is "containerd" or "crio" // and --cni=false From 257642245b3b5135516a99897deca03c0454102e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Anders=20F=20Bj=C3=B6rklund?= Date: Wed, 22 Mar 2023 18:38:27 +0100 Subject: [PATCH 3/3] The CNI warnings are not given for Docker You can still use other network plugins, with it. Just that cni is now used by default, for 1.24 up. --- test/integration/net_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/net_test.go b/test/integration/net_test.go index 7c06d49524..41b0284423 100644 --- a/test/integration/net_test.go +++ b/test/integration/net_test.go @@ -82,13 +82,13 @@ func TestNetworkPlugins(t *testing.T) { // collect debug logs defer debugLogs(t, profile) - if usingCNI() && tc.name == "false" { + if ContainerRuntime() != "docker" && tc.name == "false" { // CNI is required for current container runtime validateFalseCNI(ctx, t, profile) return } - if usingCNI() && tc.name == "kubenet" { + if ContainerRuntime() != "docker" && tc.name == "kubenet" { // CNI is disabled when --network-plugin=kubenet option is passed. See cni.New(..) function t.Skipf("Skipping the test as %s container runtimes requires CNI", ContainerRuntime()) }