Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

100% cnidel UT coverage #71

Merged
merged 8 commits into from
Apr 12, 2019
Merged

100% cnidel UT coverage #71

merged 8 commits into from
Apr 12, 2019

Conversation

Levovar
Copy link
Collaborator

@Levovar Levovar commented Apr 2, 2019

Now, really.

Following bugs were corrected during testing (running list):

  • to make sure external inputs are properly validated, IsDelegationRequired now checks against explicitly lowercased NetworkType value
  • same was done to NetworkType-based fakeipam config creation check
  • conversion of CNI results was faulty, because it was always assumed to be 0.2.0 type. Correct version-based decoding is now applied, so both native 020 and 031 Results are properly managed now

The following functionalities were changed during the review:

  • IP allocation requests resulting in no IPv4, and IPv6 addresses being assigned for dynamic-level backends are considered erroneous moving forward.
    3rd-party CNIs are not equipped to handle this scenario (looking at you macvlan), so it is better to fail the interface creation already in DANM.
    Not everyone can be TelCo ready :) As Intel's SR-IOV CNI supports L2 mode, it was exempt above this rule

@Levovar Levovar force-pushed the cnidel_uts branch 10 times, most recently from c55bc0f to 0fe5b5c Compare April 11, 2019 13:59
Levovar added 8 commits April 12, 2019 17:48
…etworkType parameter against a constant string.

Added easy TCs for two auxiliary cnidel public functions.
Setup related delegate test framework created, with the first negative TC.
…ration check as well.

Empty fakeipam configs are considered erroneous from this day onward.
Reason is that the presence of the "IPAM" field is mandatory according to the CNI spec, and most CNIs can't deal with "none" type IP allocations.
Most CNIs are not DANM ;)
Added TC to test this newly introduced error case.
Created skeleton CNI plugin to be able to test the different delegated config files coming out of cnidel package.
First TC testing static, file-based configuration via Flannel is added to showcase how to use the test tool.
UTs were modified to run in container to avoid trashing the executor's home environment.
Added some extra test cases to cover CNI result conversion code.
It turns out it was faulty, and we were not handling default returned 0.3.1 result well.
Code is corrected, bot 020, and current Results are properly managed now.
Also started checking that the relevant CNI delegation specific environment variables are set to the right value.

Added additional testcases to check 0.2.0 version conversion and parsing works both with IPv4, and IPv6 addresses.
Test environment is mocked by re-using the SR-IOV CNI project's awesome environment setup method.
@Levovar Levovar merged commit 16feefd into master Apr 12, 2019
@Levovar Levovar deleted the cnidel_uts branch April 12, 2019 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant