fix overwriting of OverlappingRanges by mergo

[toelke]

What this PR does / why we need it:

When configuring enable_overlapping_ranges in the pod-configuration (e.g. by using a multus NetworkAttachmentDefinition, the parsing of the configuration makes an error. This was already noticed in #164 but I did not manage to get that solution to work.

What happens is that parsing the flatfile (where enable_overlapping_ranges is not set) results in an in-memory representation of the configuration where OverlappingRanges is set to true; this will overwrite the pod-specific configuration of false.

[maiqueb]

Looks good.

We'd need a unit test to verify it does what we want it to though.

[toelke]

I have expanded an existing test. It indeed fails before the fix and is successful afterwards.

[maiqueb]

I have expanded an existing test. It indeed fails before the fix and is successful afterwards.

I would still prefer to see a new unit test, focused explicitly on this - that is easier to follow, and will work as documentation of what we expect around this flag.

Modifying an existing test does the job, but it does not work for well for documenting the behavior.
Since we're doing BDD on (most of) the tests, I think that should be the focus.

I would add something like:

        var globalConf string

        BeforeEach(func() {
        		globalConf = `{
      "datastore": "kubernetes",
      "kubernetes": {
        "kubeconfig": "/etc/cni/net.d/whereabouts.d/whereabouts.kubeconfig"
      },
      "log_file": "/tmp/whereabouts.log",
      "log_level": "debug",
      "gateway": "192.168.5.5"
    }`

		Expect(ioutil.WriteFile("/tmp/whereabouts.conf", []byte(globalconf), 0755)).To(Succeed())
        })
        
	It("overlapping range can be set", func() {
		ipamconfig, _, err := LoadIPAMConfig([]byte(generateIPAMConfWithOverlappingRanges()), "")
		Expect(err).NotTo(HaveOccurred())
		
		Expect(ipamconfig.OverlappingRanges).To(BeTrue())
	})
	
	It("overlapping range can be disabled", func() {
		ipamconfig, _, err := LoadIPAMConfig([]byte(generateIPAMConfWithoutOverlappingRanges()), "")
		Expect(err).NotTo(HaveOccurred())
		
		Expect(ipamconfig.OverlappingRanges).To(BeFalse())
	})

func generateIPAMConfWithOverlappingRanges() string {
	return `{
      "cniVersion": "0.3.1",
      "name": "mynet",
      "type": "ipvlan",
      "master": "foo0",
      "ipam": {
        "configuration_path": "/tmp/whereabouts.conf",
        "type": "whereabouts",
        "range": "192.168.2.230/24",
        "range_start": "192.168.2.223",
        "gateway": "192.168.10.1",
        "leader_lease_duration": 3000,
        "leader_renew_deadline": 2000,
        "leader_retry_period": 1000
        "leader_retry_period": 1000,
        "enable_overlapping_ranges": true
      }
      }`
}

func generateIPAMConfWithoutOverlappingRanges() string {
	return `{
      "cniVersion": "0.3.1",
      "name": "mynet",
      "type": "ipvlan",
      "master": "foo0",
      "ipam": {
        "configuration_path": "/tmp/whereabouts.conf",
        "type": "whereabouts",
        "range": "192.168.2.230/24",
        "range_start": "192.168.2.223",
        "gateway": "192.168.10.1",
        "leader_lease_duration": 3000,
        "leader_renew_deadline": 2000,
        "leader_retry_period": 1000
        "leader_retry_period": 1000,
        "enable_overlapping_ranges": false
      }
      }`
}

You could even use a table for these 2 tests, thus avoiding the duplicate test code (pass the configuration and the matcher as inputs to the func, for instance).

[toelke]

You are correct. I also changed your proposed test to actually overwrite the flatfile in both directions -- the original code does not allow overwriting a true with a false. The fixed code allows both directions.

[coveralls]

Pull Request Test Coverage Report for Build 2891521153

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 65.15%

---

Totals
Change from base Build 2889580130:	0.1%
Covered Lines:	931
Relevant Lines:	1429

---

💛 - Coveralls

[maiqueb]

Seems to me we're leaking this file after the execution, but I've noticed that also happens for other tests.

I'll handle that in a follow-up.

[maiqueb]

You should rebase latest master, to give the e2e tests a chance to work - with the current state of your tree, it'll fail trying to install the CNI plugins in the k8s nodes.

[toelke]

Done

[maiqueb]

Had forgotten to hit the approve button - sorry for that :)

[coveralls]

Pull Request Test Coverage Report for Build 2888908291

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.1%) to 65.15%

---

Totals
Change from base Build 2882834733:	0.1%
Covered Lines:	931
Relevant Lines:	1429

---

💛 - Coveralls