Fix ifconfig parse logic failing on some tunnels #2635
Conversation
|
Can one of the admins verify this patch? |
There was a problem hiding this comment.
Thanks @Woellchen! In the first commit that demonstrates the failure, can you mark the failing test(s) as pending? And then remove the pending in the second commit. This is important so that each commit passes if we later need to git bisect.
Or you can keep the commits as-is and I can squash them when merging. Let me know which sounds better.
Tunnel interfaces that have a line similar to "tunnel inet 192.0.2.1 --> 192.0.2.2" would fail the network resolver because IPs were extracted through two separate IP + mask patterns. However, in the example above, there is no such thing as a netmask. This fix merges the patterns and extracts the IPs and netmasks together so that no mismatches can occur. It might also fix some cases where only the netmask would be nil and therefore produce an invalid binding that could in turn resolve to the wrong (outer) IP of the tunnel instead of the inner IP.
Woellchen
force-pushed
the
fix/main/fix_ifconfig_tunnel_parsing
branch
from
4c6519c
to
a83e1f9
Compare
|
Hey @joshcooper, thanks for getting back to me! |
| @@ -5,6 +5,10 @@ | |||
|
|
|||
| let(:log_spy) { instance_spy(Facter::Log) } | |||
|
|
|||
| before do | |||
| pending 'contains failing tests to reproduce a bug' | |||
There was a problem hiding this comment.
This will cause all passing tests to fail, because they're "no longer" pending. See https://rspec.info/features/3-12/rspec-core/pending-and-skipped-examples/pending-examples/
You'll want to move the call to pending to the specific tests that aren't working yet (instead of callling raise in the test) like:
it 'returns dhcp server ip as nil' do
pending('not working yet')
expect(networking.resolve(:dhcp)).to be(nil)
endThere was a problem hiding this comment.
Hey @joshcooper! I have put the pending call on purpose in the before clause because all tests are actually failing with the updated ifconfig fixture to demonstrate that it really breaks Facter.
To not put the pending call in each and every test case I have moved it to before, but for that reason I also had to temporarily add the raises in these two tests to make rake spec succeed.
In the following commit I revert the pending and raises. Do you want me to move the pending call to every test case instead?
|
Closing and reopening to trigger the right set of checks |
Tunnel interfaces that have a line similar to
"tunnel inet 192.0.2.1 --> 192.0.2.2" would fail the network resolver
because IPs were extracted through two separate IP + mask patterns.
However, in the example above, there is no such thing as a netmask.
This fix merges the patterns and extracts the IPs and netmasks
together so that no mismatches can occur. It might also fix some
cases where only the netmask would be nil and therefore produce an
invalid binding that could in turn resolve to the wrong (outer) IP of
the tunnel instead of the inner IP.