libcni: add config caching [v2]

[mccv1r0]

Continuation of #661 that fixes up conflicts in the existing PR.

[squeed]

Need to escape all these variables - there's no guarantee that they are filename-safe.

[mccv1r0]

Ensuring these variables will be finename-safe will be address via #679

[dcbw]

@mccv1r0 also Casey pointed out in the other review that maybe we should have some versioning in the cache file. See #661 (comment)

[mccv1r0]

I agree that variables should be sanitized on plugins level. For this version, I escaped the path. Versioning added as well.

[jellonek]

The name C:\Users\appveyor\AppData\Local\Temp\1\cni_cachedir523654598\config\"apitest-some-container-id-some-eth0" does not look to be healthy.

Btw. please remove the merge master branch commit from history and use instead git rebase -i origin/master on your branch.

[dcbw]

RE escaping see #679 (comment)

[jellonek]

@mccv1r0 so there is still a question why you are calling sprintf on sum of strings.

[mccv1r0]

@mccv1r0 so there is still a question why you are calling sprintf on sum of strings.

As it stands now, each of the strings, ContainerID, networkName and ifName might need escaping.

Would you prefer just ContainerID? ifName won't need it, networkName "probably" won't need it but I don't want to chance it.

[jellonek]

@mccv1r0 so there is still a question why you are calling sprintf on sum of strings.

As it stands now, each of the strings, ContainerID, networkName and ifName might need escaping.

Would you prefer just ContainerID? ifName won't need it, networkName "probably" won't need it but I don't want to chance it.

Ok, maybe my question was not fully clear - you are taking the sum of strings, which is a string, then you are passing that to sprintf as a format string, but without any format arguments. So the question is not about what should be inside constructed string (IMO this set of fields is fine), but why fully built string is used as a lone parameter for sprintf. As @dcwb noted - you should use sum, or sprintf.

[squeed]

It would be good to see a test case that covered reading an "old" result-only file.

[dcbw]

It would be good to see a test case that covered reading an "old" result-only file.

@mccv1r0 I didn't see a testcase for this yet, but may have missed it.

[mccv1r0]

It would be good to see a test case that covered reading an "old" result-only file.

@mccv1r0 I didn't see a testcase for this yet, but may have missed it.

See line 517

				Context("no cached config just result", func() {
					It("passes a prevResult to the plugin", func() {
						err := ioutil.WriteFile(cacheFile, []byte(`{
							"cniVersion": "0.3.1",
							"ips": [{"version": "4", "address": "10.1.2.3/24"}],
							"dns": {}
						}`), 0600)
						Expect(err).NotTo(HaveOccurred())

						err = cniConfig.CheckNetwork(ctx, netConfig, runtimeConfig)
						Expect(err).NotTo(HaveOccurred())
						debug, err := noop_debug.ReadDebug(debugFilePath)
						Expect(err).NotTo(HaveOccurred())
						Expect(string(debug.CmdArgs.StdinData)).To(ContainSubstring("\"prevResult\":"))
						Expect(string(debug.CmdArgs.StdinData)).NotTo(ContainSubstring("\"config\":"))
						Expect(string(debug.CmdArgs.StdinData)).NotTo(ContainSubstring("\"kind\":"))
					})
				})

Notice the tests to ensure kind and config are not in the file.

[dcbw]

@mccv1r0 since all our cache stuff is combined now, I suggest:

setCachedResult -> cacheAdd
getResultCacheFilePath -> getCacheFilePath
delCachedResult -> cacheDel

otherwise LGTM

[mccv1r0]

changed

[dcbw]

@mccv1r0 one last thing: cachedDel -> cacheDel

[dcbw]

/lgtm

[mars1024]

LGTM, thanks! @mccv1r0

[jellonek]

LGTM