Fixes #12212 - Import addtitionnal informations from DHCP smart proxy

[bagasse]

Add ability to import some subnet options (gateway, dns servers, IP addr range...) from the dhcp smart-proxy.

[dmitri-d]

There's a PR in smart-proxy (theforeman/smart-proxy#330) that introduces support for this in isc provider, but not for the MS windows one. We either need: add similar functionality on Windows, or be able to handle response that doesn't contain these newly introduced fields.

Could you add a test for this?

[dmitri-d]

nm, I see that options hash has been added in the smart-proxy PR. I'd change this end to iterate over all keys available in options and append them to attrs.

[ohadlevy]

but then you would need to remove those which AR does not support, imho a whitelist is a better approch (as a nitpick could use hash slice instead).

[dmitri-d]

Yeah, true. nm, then!

[dmitri-d]

Having said that, on the smart-proxy side I suggested to keep option names and format of values similar to the original ones. This would require explicit handling of each option (renaming, extracting of values, etc) here.

[dLobatog]

[test]

[bagasse]

@witlessbird

I didn't see any test on the import subnet part in the foreman (i used grep, so maybe i've missed something). I wil take a look to test cases with and without the options part.

[dmitri-d]

@bagasse: I was thinking that mapping of the options hash to model attributes is worth a test (esp. so if hash keys will be renamed).

[dLobatog]

Setting this to WoC until a test comes 😄

[dLobatog]

Nitpick - What do you think about extracting this handling of attrs to a separate method?

[dLobatog]

@bagasse Any updates?

[bagasse]

@dLobatog Yes i've refactored options in a separate method, but i didn't see any test on subnet import in the codebase. Any pointers to test output from smartproxy on foreman side ?
Any advices on theforeman/smart-proxy#330 about the namming of attributes ?

[dLobatog]

Typo - parse_dhcp_options, without the exclamation mark

[dLobatog]

@bagasse Alright, there are a few comments inline - it contains a typo basically. I can write a test for this and you can rebase it in your commit if that works for you?

[dmitri-d]

@dLobatog: any thoughts on naming of dhcp options? Atm they are named after db schema in foreman, my suggestion was to use names similar to rfc, and convert them here. This way smart-proxy code will be easier to understand/need fewer comments...

[bagasse]

@dLobatog: oops... done.

[dLobatog]

[test]

[dLobatog]

@witlessbird I agree using the RFC names is more appropriate, we can do the mapping here - I'll set this to WoC until the Smart proxy PR is merged and we have set of options to map to here.

@bagasse The PR is ready IMO - except for the functional tests part, which I can do after theforeman/smart-proxy#330 is merged and we have a stable options hash convention. After it's merged make sure to ping me or comment here to ensure this gets reviewers attention again. Thanks 😄

[bagasse]

@dLobatog, @witlessbird : thank for comments, i will do these changes on both sides.

[bagasse]

@dLobatog @witlessbird, just one point to clarify, RFC2132 don't define standard names but codes for the dhcp options. I can create an hash-table with RFC2132 options codes as index. but it's not really readable, so when you talk about 'using the RFC names', do you mean 'use ISC dhcpd config names' for these options or use RFC2132 codes ?

[dmitri-d]

@bagasse: apologies, I meant isc dhcpd config names.

[dLobatog]

@bagasse Thank you. I wrote two tests to check importing works with and without options. Please add this to your Pull request - 5bf8064

[domcleal]

[test]

[dLobatog]

@bagasse @witlessbird Thanks, I'll leave this PR open until the other side of the equation (theforeman/smart-proxy#330) is merged. It's ready to merge after that.

[dLobatog]

Merged as 302d777, thanks @bagasse!

[bagasse]

@dLobatog @witlessbird, thanks.