WIP: Fix default cni config

[woodcockjosh]

fixes #3056
Not sure how to test. Probably I don't know what I'm doing but I get this output when trying to run on Ubuntu 18.04.03

john@john-doe-vm:~/Projects/OpenSource/minikube$ ./out/minikube start --vm-driver kvm2 --network-plugin=cni --extra-config=kubelet.network-plugin=cni --extra-config=kubelet.cni-conf-dir=/etc/cni/net.d --extra-config=kubelet.cni-bin-dir=/opt/cni/bin
😄  minikube v1.4.0 on Ubuntu 18.04
💾  Downloading driver docker-machine-driver-kvm2:
    > docker-machine-driver-kvm2.sha256: 65 B / 65 B [-------] 100.00% ? p/s 0s
    > docker-machine-driver-kvm2: 32.00 MiB / 32.00 MiB  100.00% 7.11 MiB p/s 4
🔥  Creating kvm2 VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🔄  Retriable failure: new host: dial tcp: missing address
🔥  Creating kvm2 VM (CPUs=2, Memory=2000MB, Disk=20000MB) ...
🔄  Retriable failure: new host: dial tcp: missing address
^C

However, even though I can't test it, based on these articles:

https://kubernetes.io/docs/concepts/extend-kubernetes/compute-storage-net/network-plugins/
https://github.com/containernetworking/plugins/blob/master/plugins/meta/portmap/README.md

I think our current configuration is completely invalid.

[k8s-ci-robot]

Hi @woodcockjosh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: woodcockjosh
To complete the pull request process, please assign afbjorklund
You can assign the PR to them by writing /assign @afbjorklund in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[minikube-bot]

Can one of the admins verify this patch?

[woodcockjosh]

/assign @tstromberg

[codecov-io]

Codecov Report

Merging #5526 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5526   +/-   ##
=======================================
  Coverage   36.57%   36.57%           
=======================================
  Files         102      102           
  Lines        7320     7320           
=======================================
  Hits         2677     2677           
  Misses       4292     4292           
  Partials      351      351

[tstromberg]

@minikube-bot OK to test

PR looks good, but I am unfortunately CNI ignorant and also don't know how to review this: our integration tests for CNI don't yet do very much, and that error message is worrisome but perhaps unrelated.

PR does need a better title for release notes.

[medyagh]

could we have an integration test with CNI options you have in the PR description and then this PR should Fix the test ?

[medyagh]

how about an integration tests that starts minikube with the options you specified (the integration test could be only for the driver that supports it) and then you can search the output of minikbue statrt output and expect not the error you mentioned in the PR descrption or any error at all )

[woodcockjosh]

Since that error occurs without the changes, I believe it is unrelated. I am investigating further.

PR does need a better title for release notes.
@tstromberg what do you recommend?

I think an appropriate test for this issue would be that I install a pod with hostPort and I can connect from the host. In terms of validating other things, I think the capabilities should be validated rather than the configuration of those capabilities. We could unknowingly think that the config is valid because the config is indeed valid but unintentionally break a capability through our own ignorance of how cni actually works. It would however, at least save us from a completely invalid config which I think has happened here and could be a good high level test. I don't know how we would check for error messages though. Anything that says "Error" ? :-)

[medyagh]

I don't know how we would check for error messages though. Anything that says "

I think it worth writing an integration test for it, and it will prevent us from breaking it in the future.

would this be helpful ? (we have helpers in the integration tests that returns stdout,stderr and exit code ...
https://github.com/kubernetes/minikube/blob/master/test/integration/helpers.go#L50

type RunResult struct {
	Stdout   *bytes.Buffer
	Stderr   *bytes.Buffer
	ExitCode int
	Args     []string
}

here is an example of starting minikube in integration tests and checking the stdout stderr for a specific output:

minikube/test/integration/functional_test.go

Line 104 in 8f259e7

func validateStartWithProxy(ctx context.Context, t *testing.T, profile string) {

[woodcockjosh]

Before I write a test that fails I'm going to try to get 1.4.0 running with the options in the description and reproduce the problem described in the issue.

[medyagh]

Before I write a test that fails I'm going to try to get 1.4.0 running with the options in the description and reproduce the problem described in the issue.

awesome, that sounds like a good plan !

[tstromberg]

@minikube-bot OK to test

[tstromberg]

Is this still a work in progress?

[woodcockjosh]

Is this still a work in progress?

I'm not able to test it because I cannot get minikube running with the commands supplied in the issue.

[tstromberg]

Closing until we have a way to understand and validate this change.