pkg/ip: Return correct error if container name provided exists, and test cases

[prateekgogia]

If interface name for a container provided by a user is already present,
Veth creation fails with incorrect error.
If os.IsExist error is returned by makeVethPair:

• Check for peer name, if exists generate another random peer name,
• else, IsExist error is due to container interface present, return error.

Fixes #155

[rosenhouse]

@prateekgogia: Thank you for the fix. Could you please update it to follow our contribution guidelines?

Specifically:

• Please add test coverage for your change.
• Please format your commit message(s) according to the preferred style.

For this change, the test case is non-trivial. I'd suggest testing ip.SetupVeth by temporarily replacing rand.Reader in the BeforeEach block. You can replace it with a deterministic io.Reader (e.g. one that always returns 0s). In the It blocks you can then call SetupVeth repeatedly and write expectations on the resulting behavior. But rand.Reader is a process-global variable, so don't forget set it back to the original value (e.g. in the AfterEach() block) so as not to pollute other test cases that use it.

[prateekgogia]

Hi @rosenhouse Thanks for the feedback. I am not familiar with Ginkgo testing framework. I will read about it and add the test cases soon.

[prateekgogia]

Hi @rosenhouse - I have made the changes to commit message and added test cases, can you please review and let me know if anything else is required.

[rosenhouse]

The test suite still passes if I simplify this to

_, err := netlink.LinkByName(name)
if err != nil {
   ...

[rosenhouse]

Hi @prateekgogia,

Thank you so much, this test coverage is a huge improvement.

I noticed a small code simplification and also left some suggestions for improving the readability of the tests. The Ginkgo / Gomega BDD style is most valuable when the Describe, Context and It blocks all read as clear sentences.

I'd ask that you fix these up, and then squash all of your commits together into a single commit that follows the aforementioned style guidelines.

Thanks!

[prateekgogia]

@rosenhouse: updated and squashed all commits in one commit. Thanks for the review :)

[jellonek]

Please remove this empty line.

[prateekgogia]

@rosenhouse @jellonek addressed.

[rosenhouse]

LGTM. Any other @containernetworking/cni-maintainers want to give the second 👍 ?

[jellonek]

LGTM, but i'm not maintainer.

[tomdee]

LGTM - merging.