Fixes #12211 - Improve DHCP subnets parsing to get more informations … #330

bagasse · 2015-10-20T09:33:58Z

…from DHCP server

Add ability to parse some subnet options and provides them to the foreman to enhance subnet imports.

This modify the dhcp smart-proxy API (adding options hash).

dmitri-d · 2015-10-20T11:36:28Z

modules/dhcp/providers/server/isc.rb

+            case option
+              when /option\s+routers\s+[\d\.]+/
+                # get the first router match and use it for gateway
+                options[:gateway] = option.scan(/option\s+routers\s+([\d\.]+)/).first.first


why not keep the whole list?

dmitri-d · 2015-10-20T11:56:11Z

@bagasse: cheers for the PR!

I feel that in the data we return we should keep option names and format of their values close to the original ones (if not the same): gateway would become :routers and contain the whole list, :dns_primary and :dns_secondary would become :domain_name_servers and keep all of the servers, etc.

Another issue that I see is that dhcp config file may contain global options applicable to all subnets (and potentially overridden in individual subnet configs). I we should support this if we are going to return these additional options.

bagasse · 2015-10-20T12:27:14Z

@witlessbird

I changed the options names to map names used on the foreman side. I will update the PR to keep names like in the dhcp server config, and keep all values too.
For the global options, i didn't take a look at it because you can have global options, shared networks blocks and group block (with some nested ones) that overrides some of the options. I wanted to keep this simple at this time and just take local subnet options. Parsing dhcpd config with regex with this kind of russian dolls config blocks was out of the scope of this PR (and testing all cases is not trivial), but if it's a requirement for you, i will take a look at it when i will have more time.

dmitri-d · 2015-10-20T12:47:05Z

@bagasse: let's wait with the renaming until we hear something from the foreman side (although I think it makes sense to rename attribute names there and keep them close to original here).

Yeah, I agree that parsing global options makes things more complicated (and possibly warrants a real parser, not regexes). I'm ok with postponing this bit, as long as we can get the functionality in not too distant future.

dLobatog · 2015-11-05T14:43:46Z

[test]

dLobatog · 2015-11-11T07:09:39Z

@bagasse One of the tests you modified (get_root) is failing. see Jenkins or test locally with rake test. Rubocop is also throwing a few warnings, please fix them & update the PR.

bagasse · 2015-11-11T09:51:54Z

@dLobatog: I've fixed rubocop warnings, sorry i forgot to run rubocop before on this PR. For the get_root of dhcp_api_test, all test are green in my dev env (CentOS 7, ruby 1.9.3). I seen in jenkins that this test only fail with ruby 1.8.7. I don't know why, with this ruby version, networks presents in the list are not in the same order than in others ruby versions. I will take a look at this more deeply tomorrow.

dmitri-d · 2015-11-11T10:30:28Z

[test]

dmitri-d · 2015-11-11T10:36:28Z

modules/dhcp/providers/server/isc.rb

@@ -288,8 +298,7 @@ def read_config file, ignore_includes=false
      logger.debug "Reading config file #{file}"
      config = []
      File.readlines(file).each do |line|
-
-        line.strip! # remove left and right whitespace
+        line = line.split('#').first.strip # remove comments, left and right whitespace


Why this change? The next line used to skip over comments (and isn't needed with this change).

In my understanding, the next line remove lines that starts with comment, this remove comments at the end of lines. But if you want i can remove this.

once you call String#split all instances of the symbol you used to split the string will be removed, which makes https://github.com/bagasse/smart-proxy/blob/Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server/modules/dhcp/providers/server/isc.rb#L302 redundant.

I'm ok with this change if you remove code on line 302.

line 302 removed.

bagasse · 2015-11-11T16:41:07Z

@witlessbird: changed, arrays to sets, thanks for pointing this out to me.

dLobatog · 2015-11-11T17:01:58Z

[test]

dmitri-d · 2015-11-11T18:22:06Z

test/dhcp/server_isc_test.rb

@@ -82,8 +82,24 @@ def test_poap_quirks
  def test_loadSubnets_loads_managed_subnets
    subnets = @dhcp.loadSubnets


I know that this was here before, but it wasn't a very good test: loadSubnets returns subnets that were processed, not the data that was stored.

Another issue is that this tests too many things; which will make it harder to pinpoint an issue should code change. Could you break this up to test specific cases, for example handling of domain_name_server option when it's not present, when only one server is present, when both are present, etc.

It is probably easier to work directly with subnet_service (see @subnet_service in setup -- you can query it for relevant subnets, as opposed to parsing the hash returned by @dhcp.subnets.

In my understanding, loadSubnets implementation of Proxy::DHCP::Server::ISC load subnets only from config files and Proxy::DHCP::SubnetService is only here to store loaded data, so i don't see the problem to use loadSubnets here. For the other point, i've separated tests in several methods.

Nods,that's correct. However, currently these tests verify the behaviour of parse_config_for_subnets method: so either calls to loadSubnetsshould be replaced with calls to parse_config_for_subnets, or the assertions should be made against Proxy::DHCP::SubnetService (the main side-effect of loadSubnets is initialization of SubnetService).

dmitri-d · 2015-11-11T18:29:50Z

@bagasse: Looks pretty good, only needs some changes in tests around added functionality.

domcleal · 2015-11-12T08:30:58Z

Thanks for your PRs and work on this feature @bagasse, and welcome to our GitHub organisation. If you can set your membership to "public" on this page then Jenkins will automatically test any updates you make.

domcleal · 2015-11-12T08:35:49Z

[test] one last time :)

bagasse · 2015-11-12T08:38:20Z

@domcleal Glad to join and thank you for the invitation :)

dLobatog · 2015-11-12T09:16:17Z

modules/dhcp/providers/server/isc.rb

-        ret_val << Proxy::DHCP::Subnet.new(match[0], match[1])
+      @config.scan(/subnet\s+([\d\.]+)\s+netmask\s+([\d\.]+)\s*{([\w|;|\.|,|\-|\s]*)}/) do |match|
+        options = {}
+        if match[2]


Mind to extract this logic to a method or something similar? Maybe write a comment / save match[0] match[1] match[2] to named variables? It's a bit difficult to understand without being well acquainted with the format of the conf file.

dLobatog · 2015-11-12T09:23:08Z

@bagasse I only have 2 comments about the regexes & maintainability, but overall great job 👍. IMO if you could fix that that should be it. In any case @domcleal and @witlessbird feel free to merge if you think it's fine already, you are the ones that work on this code the most.

bagasse · 2015-11-12T09:39:31Z

@dLobatog you are right, it's not really readable, i've extracted range regexp too and used named vars.

dmitri-d · 2015-11-12T10:19:28Z

modules/dhcp/providers/server/isc.rb

@@ -409,4 +421,14 @@ def poap_options_statements(options)
      statements
    end
  end
+
+  # Get all IPv4 addresses provided by the ISC DHCP config line following the pattern "my-config-option-or-directive IPv4_ADDR[, IPv4_ADDR] [...];" and return an array.
+  def get_ip_list_from_config_line(option_line)


Could you add a test for this method?

It was already there, but i've added a test for get_range_from_config_line that i've extracted too. I changed loadSubnets calls to parse_config_for_subnets as suggested.

…from DHCP server

dmitri-d · 2015-11-12T11:22:26Z

[test]

dmitri-d · 2015-11-12T11:44:37Z

Waiting for tests to pass before merging.

dmitri-d · 2015-11-12T12:41:27Z

merged in at e2a21ec. Thanks, @bagasse!

bagasse · 2015-11-12T13:18:32Z

@witlessbird @dLobatog, thank you for pointers and re (re re re)-reviews :)

theforeman-bot added Needs testing Not yet reviewed labels Oct 20, 2015

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from 5a59cdb to 5839bd7 Compare October 20, 2015 09:41

dmitri-d mentioned this pull request Oct 20, 2015

Fixes #12212 - Import addtitionnal informations from DHCP smart proxy theforeman/foreman#2846

Closed

dmitri-d reviewed Oct 20, 2015
View reviewed changes

dmitri-d added Waiting on contributor and removed Not yet reviewed labels Oct 20, 2015

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from 5839bd7 to e19101a Compare November 4, 2015 15:37

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 4, 2015

dLobatog added Waiting on contributor and removed Needs re-review labels Nov 11, 2015

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from e19101a to 0977f1c Compare November 11, 2015 09:43

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 11, 2015

dmitri-d reviewed Nov 11, 2015
View reviewed changes

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from 0977f1c to c28d12b Compare November 11, 2015 16:25

dmitri-d reviewed Nov 11, 2015
View reviewed changes

dmitri-d added Waiting on contributor and removed Needs re-review labels Nov 11, 2015

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from c28d12b to ec1d84d Compare November 12, 2015 08:31

theforeman-bot added Needs re-review Needs testing and removed Waiting on contributor labels Nov 12, 2015

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from ec1d84d to d2938d9 Compare November 12, 2015 09:01

dLobatog reviewed Nov 12, 2015
View reviewed changes

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from d2938d9 to 424b1b6 Compare November 12, 2015 09:37

dmitri-d reviewed Nov 12, 2015
View reviewed changes

Fixes #12211 - Improve DHCP subnets parsing to get more informations …

3761282

…from DHCP server

bagasse force-pushed the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch from 424b1b6 to 3761282 Compare November 12, 2015 10:39

dmitri-d removed Needs testing Needs re-review labels Nov 12, 2015

dmitri-d closed this Nov 12, 2015

bagasse deleted the Fixes_12211_-_Improve_DHCP_subnets_parsing_to_get_more_informations_from_DHCP_server branch June 1, 2016 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #12211 - Improve DHCP subnets parsing to get more informations … #330

Fixes #12211 - Improve DHCP subnets parsing to get more informations … #330

bagasse commented Oct 20, 2015

dmitri-d Oct 20, 2015

dmitri-d commented Oct 20, 2015

bagasse commented Oct 20, 2015

dmitri-d commented Oct 20, 2015

dLobatog commented Nov 5, 2015

dLobatog commented Nov 11, 2015

bagasse commented Nov 11, 2015

dmitri-d commented Nov 11, 2015

dmitri-d Nov 11, 2015

bagasse Nov 11, 2015

dmitri-d Nov 11, 2015

bagasse Nov 12, 2015

bagasse commented Nov 11, 2015

dLobatog commented Nov 11, 2015

dmitri-d Nov 11, 2015

bagasse Nov 12, 2015

dmitri-d Nov 12, 2015

dmitri-d commented Nov 11, 2015

domcleal commented Nov 12, 2015

domcleal commented Nov 12, 2015

bagasse commented Nov 12, 2015

dLobatog Nov 12, 2015

dLobatog commented Nov 12, 2015

bagasse commented Nov 12, 2015

dmitri-d Nov 12, 2015

bagasse Nov 12, 2015

dmitri-d commented Nov 12, 2015

dmitri-d commented Nov 12, 2015

dmitri-d commented Nov 12, 2015

bagasse commented Nov 12, 2015

		@@ -82,8 +82,24 @@ def test_poap_quirks
		def test_loadSubnets_loads_managed_subnets
		subnets = @dhcp.loadSubnets

Fixes #12211 - Improve DHCP subnets parsing to get more informations … #330

Fixes #12211 - Improve DHCP subnets parsing to get more informations … #330

Conversation

bagasse commented Oct 20, 2015

Choose a reason for hiding this comment

dmitri-d commented Oct 20, 2015

bagasse commented Oct 20, 2015

dmitri-d commented Oct 20, 2015

dLobatog commented Nov 5, 2015

dLobatog commented Nov 11, 2015

bagasse commented Nov 11, 2015

dmitri-d commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bagasse commented Nov 11, 2015

dLobatog commented Nov 11, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Nov 11, 2015

domcleal commented Nov 12, 2015

domcleal commented Nov 12, 2015

bagasse commented Nov 12, 2015

Choose a reason for hiding this comment

dLobatog commented Nov 12, 2015

bagasse commented Nov 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Nov 12, 2015

dmitri-d commented Nov 12, 2015

dmitri-d commented Nov 12, 2015

bagasse commented Nov 12, 2015