fix: fixing the CIDR format of getNetworkName() on Windows #1589
Conversation
cni/network/network_windows.go
Outdated
| ipAddrNet := ipamAddResult.ipv4Result.IPs[0].Address | ||
| _, ipNet, err := net.ParseCIDR(ipAddrNet.String()) | ||
| if err != nil { | ||
| log.Printf("Error parsing network CIDR: %v.", err) |
There was a problem hiding this comment.
log ipAddrNet.String() as well in error case
tamilmani1989
changed the title
behzad-mir
force-pushed
the
behzadm-networkWindows-Multitenancy
branch
from
9ade267 to
fc7545f
Compare
I'll merge it after the multi-tenancy swift test is done |
Let also e2e and lint succeed |
behzad-mir
force-pushed
the
behzadm-networkWindows-Multitenancy
branch
2 times, most recently
from
ecddde8 to
7c6dec2
Compare
behzad-mir
force-pushed
the
behzadm-networkWindows-Multitenancy
branch
from
1d87d75 to
f18ffc8
Compare
behzad-mir
force-pushed
the
behzadm-networkWindows-Multitenancy
branch
from
f18ffc8 to
42ff151
Compare
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| networkName, err := tt.plugin.getNetworkName(tt.netNs, tt.ipamAddResult, tt.nwCfg) |
There was a problem hiding this comment.
Do we want to cover the error case scenario as well ? Since this test case is checking the happy path right now. Want to understand the code coverage aspect we take into consideration while writing these tests.
There was a problem hiding this comment.
At this point we only wanted to make sure that we're producing correct CIDR when the CNS response is valid. Seems that the failure path is not that critical for unit test.
There was a problem hiding this comment.
Checking the failure cases is absolutely critical in unit tests, come on. You should at least be testing that the input validation that you added correctly returns the error that you added when the input is invalid. How else do you know it rejects bad inputs?
There was a problem hiding this comment.
I added the unit test that was asked for this particular case. Yes, in general unit test is critical for non-happy paths. I didn't consider it critical here, since I was informed malformed CIDR is not reaching to this function. Nevertheless, I will add a test case for malformed CIDR since it is the only error check that I have added.
behzad-mir
force-pushed
the
behzadm-networkWindows-Multitenancy
branch
from
42ff151 to
26ce90f
Compare
| for _, tt := range tests { | ||
| tt := tt | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| networkName, err := tt.plugin.getNetworkName(tt.netNs, tt.ipamAddResult, tt.nwCfg) |
There was a problem hiding this comment.
Checking the failure cases is absolutely critical in unit tests, come on. You should at least be testing that the input validation that you added correctly returns the error that you added when the input is invalid. How else do you know it rejects bad inputs?
…g a unti test for the function
Testcases for failure paths has been added.
behzad-mir
force-pushed
the
behzadm-networkWindows-Multitenancy
branch
from
26ce90f to
190ba34
Compare
The issue is that right now with an IP "172.168.1.1/24" networkName "azure" and multitenancy id "1", we expect the function to return "azure-vlan1-172.168.1-0_24" but instead it returns "azure-vlan1-172.168.1-1_24"
This PR is fixing the issue by parsing the CIDR correctly.