Feature/get interface ip nonforwardable network adapters handling #38
Conversation
The interfaces are filtered by the `forwardable` flag as described in the equivalent sockaddr command. However, the user might rely on the name/description assuming that all network interfaces are considered without exclusions. Signed-off-by: Alessandro De Blasis <[email protected]>
…nterfaceFlags GetInterfaceIP currently implicitly filters the interfaces excluding the ones with a non-forwardable address. This commit clarifies the behaviour and also adds a new command that can be used to retrieve the IP of an interface even if it's not forwardable such as: 169.254.0.0/16 Signed-off-by: Alessandro De Blasis <[email protected]>
ifaddr_private_test.go
Outdated
| //Test_getInterfaceIPByFlags is here to facilitate testing of the private function getInterfaceIPByFlags | ||
| func Test_getInterfaceIPByFlags(t *testing.T) { |
There was a problem hiding this comment.
my preference (and I may be wrong here) is to test the interface. IOW, let's go thru the exported/ "public" funcs and test all the test cases below? What do you think?
There was a problem hiding this comment.
I totally agree with you, I would like to test reliably the outer layer too but we cannot control what's returned by GetAllInterfaces (called by GetInterfaceIP and GetInterfaceIPWithoutInterfaceFlags) and consequently, we can only test around the test runner host's interfaces. Unless I am missing something 🤔
I went this way in order to have a quick and dirty way to stub IfAddrs for the various use cases I tested.
Also, I wanted to have a discussion with you guys about the correct way to go about this 😊
|
This overall LGTM! Thanks for the contribution here! -- Left a few nits, in the meantime I will ping another team mate for an extra pair of 👀 |
Signed-off-by: Alessandro De Blasis <[email protected]>
|
Hi @FFMMM ! You are very welcome. Awesome! In the meantime, I fixed the easy ones and now I am looking forward to some guidance regarding how to properly test this, I left a comment with my 2cents. |
this commit introduces a way of controlling what's returned by `sockaddr.GetAllInterfaces()` in tests. Useful in tests that can now stub real world setups. Signed-off-by: Alessandro De Blasis <[email protected]>
Signed-off-by: Alessandro De Blasis <[email protected]>
|
Hi there! I have learnt something while working on another project where I found myself in a similar situation and I thought about this PR. I have also removed the private func tests with a separate commit cb0bec0 so it would be easy to revert in case you'd like to keep it. |
ifaddr_test.go
Outdated
| realGetAllInterfaces := sockaddr.GetAllInterfaces | ||
| defer func() { sockaddr.GetAllInterfaces = realGetAllInterfaces }() |
There was a problem hiding this comment.
Hm, I think there's nothing wrong with mocking the GetAllInterfaces func here but curious if we could mock a something "external" to the system.
Let me comment below what I mean.
| // available IP addresses on each interface and converts them to | ||
| // sockaddr.IPAddrs, and returning the result as an array of IfAddr. | ||
| func GetAllInterfaces() (IfAddrs, error) { | ||
| func getAllInterfaces() (IfAddrs, error) { | ||
| ifs, err := net.Interfaces() |
There was a problem hiding this comment.
to aid the set up of the "fake environment", we were curious if we could try to do some dependency injection type thing here and mock out subsequent calls to net.Interfaces().
Does that make sense? Happy to chat more as needed
There was a problem hiding this comment.
Hi @FFMMM, yes it makes sense, I hope I fully understood what you meant, I would have asked for clarification but given the holidays I gave it a shot by introducing NetworkInterfacesProvider (naming is hard), its default implementation called OSNetProvider (that I thought made sense to move into a separate file but I may be wrong) and I also attempted in improving the way the "fake environment" in tests is configured.
One thing I don't particularly like is the fact that sockaddr.NetProvider is public, I would fix that by using a separate file with a public accessor and a build flag that would make it available only when testing.
If I missed the point, please clarify how you would like to inject the dependency that takes care of enumerating the net interfaces and I will adjust accordingly.
|
hey @deblasis thanks for your patience -- I had some folks on the team look over the PR and it definitely looks good! We had one small call out regarding mocking calls that are external to the system! (See inline comments) |
|
Hi @FFMMM! Sorry for not getting back to you earlier but I found myself very busy with a project I am working on. I already wrote some code in my spare time, I think I should be able to push an update on Sunday or next week at the latest. In the meantime, I wish you happy holidays!! 🎅 |
default implementation (OSNetProvider) and overridability in tests Signed-off-by: Alessandro De Blasis <[email protected]>
os_net_provider.go
Outdated
| // available IP addresses on each interface and converts them to | ||
| // sockaddr.IPAddrs, and returning the result as an array of IfAddr. | ||
| func (n *OSNetProvider) GetAllInterfaces() (IfAddrs, error) { | ||
| ifs, err := net.Interfaces() |
There was a problem hiding this comment.
hey @deblasis thanks for following up on the team's latest -- we really appreciate your efforts here and receptiveness to feedback.
I may not have been super clear in a previous comment but the line we want to stub out is this one!
We probably want all of the code here to be executed during the tests, except for this line.
This line should look something like
ifs, err := netProvider.Interfaces()
and in the testing we can set up a FakeNetProvider. What do you think?
There was a problem hiding this comment.
Actually, I'm not sure if we can easily inject a netProvider-like dependency here since the exported functions are func(string) (string, error).
It will probably be easiest to declare a global function variable
packages sockaddr
var netInterfaces = net.Interfacesand stub out global variable sockaddr.netInterfaces in tests
There was a problem hiding this comment.
Hi there, sorry for the delayed reply, I have been very busy freelancing.
I had a quick look and I believe we are oversimplifying. I'll try to explain what I mean:
We can sure stub out net.Interfaces but that means that we are stubbing out the hardware information about our interfaces, what about the software part?
We are forgetting the calls to intf.Addrs() that look like they are returning network protocol and IP Addresses for the NICs by querying the RIB (Routing Information Base) which returns the address table.
that internally call net.interfaceAddrTable
and net.addrTable
As far as I understand, in order to be able to stub out all the system calls that are involved in computing the return value of our sockaddr.GetAllInterfaces(), I think we should also look into stubbing these other system calls as well...
Please correct me if I am wrong and let me know how to proceed.
There was a problem hiding this comment.
That's a great point @deblasis, I agree that the entire GetAllInterfaces function should be mocked.
I think exporting new interfaces and implementation structs to mock it are overkill; could we perhaps simply do
var GetAllInterfaces func() (IfAddrs, error) = getAllInterfaces
func getAllInterfaces() (IfAddrs, error) {
// implementation ...
}
// ifaddr_test.go
func TestGetInterfaceIP(t *testing.T) {
orig := sockaddr.GetAllInterfaces
sockaddr.GetAllInterfaces = mockFunc()
t.Cleanup(func(){ sockaddr.GetAllInterfaces = orig })
// tests ...
}There was a problem hiding this comment.
Hi @kisunji!
I apologize for the delay.
Sounds good, I have updated the PR accordingly.
|
|
Signed-off-by: Alessandro De Blasis <[email protected]>
Hi there, this PR addresses what was discovered in #31 quite a while ago and that I found by looking at the Consul issue hashicorp/consul#5371.
Basically it simply clarifies
GetInterfaceIP's behaviour and also addsGetInterfaceIPWithoutInterfaceFlags(not sure about the naming of this one since we are only preventing the exclusion offorwardable=falseinterfaces).