Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IPv6: dhcp/provisioner: make ipv6 aware #1723

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

matthewoliver
Copy link
Contributor

@matthewoliver matthewoliver commented Dec 5, 2018

If the admin network is IPv6 setup the ISC DHCPD server to configure
and use the IPv6 daemon. For this use a seperate set of ipv6 files
to list hosts and subnets as ipv6 hosts and subnets will fail if
v4 dhcp tries to load them.

Also make sure tftp is listening on both IPv4 and v6.

To make this all happen the Network class in the Barclamp::Inventory
library now tracks the ip_version of the network. Which is used to
make decisions through the DHCP and provisioner cookbooks.

To support backwards compatibilty with ipv4 config naming in the
DHCP barclamp a DhcpHelper was added to return config names in
an IPv4 or IPv6 way. The same pattern was used throughout the
barclamp in the early versions of this patch so it's been
refectered to the helper.

Finally, when dealing with some IPv6 addresses in URIs the address
needs to be wrapped. As such the network barclamp has grown a
NetworkHelper module to start gathering network related helper
method's, it's first is a wrap_ip function. Which will wrap an
address if it happens to an IPv6 address. This makes this available
to us anywhere in crowbar, so long as the network barclamp has been
applied, and as a core barclamp to crowbar, should be always.

file "/etc/dhcp3/hosts.d/host6_list.conf" do
owner "root"
group "root"
mode 0644
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/NumericLiteralPrefix: Use 0o for octal literals. (https://github.com/bbatsov/ruby-style-guide#numeric-literal-prefixes)

file "/etc/dhcp3/subnets.d/subnet6_list.conf" do
owner "root"
group "root"
mode 0644
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/NumericLiteralPrefix: Use 0o for octal literals. (https://github.com/bbatsov/ruby-style-guide#numeric-literal-prefixes)

file "/etc/dhcp3/groups.d/group6_list.conf" do
owner "root"
group "root"
mode 0644
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/NumericLiteralPrefix: Use 0o for octal literals. (https://github.com/bbatsov/ruby-style-guide#numeric-literal-prefixes)

"option domain-name \"#{domain_name}\"",
"option domain-name-servers #{dns_servers.join(", ")}"
]
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kEND

"option domain-name \"#{domain_name}\"",
"option dhcp6.name-servers #{dns_servers.join(", ")}"
]
else
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kELSE

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go home Hound, you're drunk.

crowbar_node = node_search_with_cache("roles:crowbar").first
address = crowbar_node["crowbar"]["network"]["admin"]["address"]
protocol = crowbar_node["crowbar"]["apache"]["ssl"] ? "https" : "http"
server = "#{protocol}://#{address}"
if IPAddr.new(address).ipv6? rescue false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kRESCUE_MOD

action :add do
subnet_template = if IPAddr.new(new_resource.network["subnet"].ipv6? rescue false
"subnet6.conf.erb"
else
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kELSE

action :add do
subnet_template = if IPAddr.new(new_resource.network["subnet"].ipv6? rescue false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kRESCUE_MOD

options: new_resource.options
options: new_resource.options,
prefix: new_resource.prefix,
is_ipv6: IPAddr.new(new_resource.ipaddress).ipv6? rescue false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kRESCUE_MOD

@@ -10,4 +10,16 @@
"option dhcp-client-debug code 226 = unsigned integer 16",
"option dhcp-client-debug 0"
]
default[:dhcp][:options][:v6] = [
"ddns-update-style none",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a sensible comment from hound 😉 It seems we are using 4 spaces everywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it's times like this I wonder should I just fix the hound "error" everywhere in the file to get it to pass or just ignore it. I mean we use 4 spaces everywhere here, so am keeping with the style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have two choices: Ignore Hound, or push a fix-up commit and then base your changes on top of that.

} else if option dhcp6.client-arch-type = 00:09 {
option dhcp6.bootfile-url \"tftp://[#{admin_ip}]/discovery/x86_64/efi/default/boot/bootx64.efi\";
} else if option dhcp6.client-arch-type = 00:0b {
option dhcp6.bootfile-url \"tftp://[#{admin_ip}]/discovery/aarch64/efi/default/boot/bootaa64.efi\";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [106/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

} else if option dhcp6.client-arch-type = 00:07 {
option dhcp6.bootfile-url \"tftp://[#{admin_ip}]/discovery/x86_64/efi/default/boot/bootx64.efi\";
} else if option dhcp6.client-arch-type = 00:09 {
option dhcp6.bootfile-url \"tftp://[#{admin_ip}]/discovery/x86_64/efi/default/boot/bootx64.efi\";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [104/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

"if option dhcp6.client-arch-type = 00:06 {
option dhcp6.bootfile-url \"tftp://[#{admin_ip}]/discovery/ia32/efi/bootia32.efi\";
} else if option dhcp6.client-arch-type = 00:07 {
option dhcp6.bootfile-url \"tftp://[#{admin_ip}]/discovery/x86_64/efi/default/boot/bootx64.efi\";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [104/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

# Always send the PXELINUX options (specified in hexadecimal)
option dhcp-parameter-request-list = concat(option dhcp-parameter-request-list,d0,d1,d2,d3);
}',
"if option dhcp6.client-arch-type = 00:06 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignArray: Align the elements of an array literal if they span more than one line. (https://github.com/bbatsov/ruby-style-guide#align-multiline-arrays)

ipv6_dhcp_opts = ["allow unknown-clients",
"default-lease-time #{lease_time}",
"max-lease-time #{lease_time}",
'if exists dhcp-parameter-request-list {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignArray: Align the elements of an array literal if they span more than one line. (https://github.com/bbatsov/ruby-style-guide#align-multiline-arrays)


ipv6_dhcp_opts = ["allow unknown-clients",
"default-lease-time #{lease_time}",
"max-lease-time #{lease_time}",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignArray: Align the elements of an array literal if they span more than one line. (https://github.com/bbatsov/ruby-style-guide#align-multiline-arrays)

"next-server #{admin_ip}"]

ipv6_dhcp_opts = ["allow unknown-clients",
"default-lease-time #{lease_time}",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignArray: Align the elements of an array literal if they span more than one line. (https://github.com/bbatsov/ruby-style-guide#align-multiline-arrays)

@@ -31,18 +30,53 @@
} else {
filename = "discovery/x86_64/bios/pxelinux.0";
}',
"next-server #{admin_ip}"],
"next-server #{admin_ip}"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignArray: Align the elements of an array literal if they span more than one line. (https://github.com/bbatsov/ruby-style-guide#align-multiline-arrays)

crowbar_node = node_search_with_cache("roles:crowbar").first
address = crowbar_node["crowbar"]["network"]["admin"]["address"]
protocol = crowbar_node["crowbar"]["apache"]["ssl"] ? "https" : "http"
server = "#{protocol}://#{address}"
if IPAddr.new(address).ipv6?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.

file "/etc/dhcp3/subnets.d/subnet_list.conf"
if node[:provisioner][:enable_pxe]
notifies :restart, resources(service: "dhcp3-server"), :delayed
for subnet_list in ["subnet_list.conf", "subnet6_list.conf"] do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file "/etc/dhcp3/hosts.d/host_list.conf"
if node[:provisioner][:enable_pxe]
notifies :restart, resources(service: "dhcp3-server"), :delayed
for host_list in ["host_list.conf", "host6_list.conf"] do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file "/etc/dhcp3/hosts.d/host_list.conf"
if node[:provisioner][:enable_pxe]
notifies :restart, resources(service: "dhcp3-server"), :delayed
for host_list in ["host_list.conf", "host6_list.conf"] do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file "/etc/dhcp3/subnets.d/subnet_list.conf"
if node[:provisioner][:enable_pxe]
notifies :restart, resources(service: "dhcp3-server"), :delayed
for subnet_list in ["subnet_list.conf", "subnet6_list.conf"] do
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -298,12 +301,13 @@
notifies :reload, resources(service: "xinetd")
end
else
ip_addr = admin_ip.ipv6? ? "[#{admin_ip.to_s}]" : admin_ip.to_s
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/StringConversionInInterpolation: Redundant use of Object#to_s in interpolation. (https://github.com/bbatsov/ruby-style-guide#no-to-s)

domain_name = node[:dns].nil? ? node[:domain] : (node[:dns][:domain] || node[:domain])
web_port = node[:provisioner][:web_port]
provisioner_web="http://#{admin_ip}:#{web_port}"
provisioner_web="http://#{admin_ip.to_s}:#{web_port}" if admin_ip.ipv4?
provisioner_web="http://[#{admin_ip.to_s}]:#{web_port}" if admin_ip.ipv6?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceAroundOperators: Surrounding space missing for operator =. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
Lint/StringConversionInInterpolation: Redundant use of Object#to_s in interpolation. (https://github.com/bbatsov/ruby-style-guide#no-to-s)

domain_name = node[:dns].nil? ? node[:domain] : (node[:dns][:domain] || node[:domain])
web_port = node[:provisioner][:web_port]
provisioner_web="http://#{admin_ip}:#{web_port}"
provisioner_web="http://#{admin_ip.to_s}:#{web_port}" if admin_ip.ipv4?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceAroundOperators: Surrounding space missing for operator =. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
Lint/StringConversionInInterpolation: Redundant use of Object#to_s in interpolation. (https://github.com/bbatsov/ruby-style-guide#no-to-s)

@@ -298,12 +301,13 @@
notifies :reload, resources(service: "xinetd")
end
else
ip_addr = admin_ip.ipv6? ? "[#{admin_ip.to_s}]" : admin_ip.to_s
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/StringConversionInInterpolation: Redundant use of Object#to_s in interpolation. (https://github.com/bbatsov/ruby-style-guide#no-to-s)

domain_name = node[:dns].nil? ? node[:domain] : (node[:dns][:domain] || node[:domain])
web_port = node[:provisioner][:web_port]
provisioner_web="http://#{admin_ip}:#{web_port}"
provisioner_web="http://#{admin_ip.to_s}:#{web_port}" if admin_ip.ipv4?
provisioner_web="http://[#{admin_ip.to_s}]:#{web_port}" if admin_ip.ipv6?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceAroundOperators: Surrounding space missing for operator =. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
Lint/StringConversionInInterpolation: Redundant use of Object#to_s in interpolation. (https://github.com/bbatsov/ruby-style-guide#no-to-s)

domain_name = node[:dns].nil? ? node[:domain] : (node[:dns][:domain] || node[:domain])
web_port = node[:provisioner][:web_port]
provisioner_web="http://#{admin_ip}:#{web_port}"
provisioner_web="http://#{admin_ip.to_s}:#{web_port}" if admin_ip.ipv4?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceAroundOperators: Surrounding space missing for operator =. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)
Lint/StringConversionInInterpolation: Redundant use of Object#to_s in interpolation. (https://github.com/bbatsov/ruby-style-guide#no-to-s)

option dhcp6.bootfile-url \"#{admin6_uri}/discovery/ppc64le/bios/\";
} else {
option dhcp6.bootfile-url \"#{admin6_uri}/x86_64/bios/pxelinux.0\";
}"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/MultilineArrayBraceLayout: Closing array brace must be on the line after the last array element when opening brace is on a separate line from the first array element.

"next-server #{admin_ip}"]

ipv6_dhcp_opts = [
"allow unknown-clients",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

@@ -31,18 +32,54 @@
} else {
filename = "discovery/x86_64/bios/pxelinux.0";
}',
"next-server #{admin_ip}"],
"next-server #{admin_ip}"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/MultilineArrayBraceLayout: Closing array brace must be on the line after the last array element when opening brace is on a separate line from the first array element.

pool_opts = {
"dhcp" => ["allow unknown-clients",
ipv4_dhcp_opts = [
"allow unknown-clients",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

option dhcp6.bootfile-url \"#{admin6_uri}/discovery/ppc64le/bios/\";
} else {
option dhcp6.bootfile-url \"#{admin6_uri}/x86_64/bios/pxelinux.0\";
}"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/MultilineArrayBraceLayout: Closing array brace must be on the line after the last array element when opening brace is on a separate line from the first array element.

"next-server #{admin_ip}"]

ipv6_dhcp_opts = [
"allow unknown-clients",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

@@ -31,18 +32,54 @@
} else {
filename = "discovery/x86_64/bios/pxelinux.0";
}',
"next-server #{admin_ip}"],
"next-server #{admin_ip}"]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/MultilineArrayBraceLayout: Closing array brace must be on the line after the last array element when opening brace is on a separate line from the first array element.

pool_opts = {
"dhcp" => ["allow unknown-clients",
ipv4_dhcp_opts = [
"allow unknown-clients",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentArray: Use 2 spaces for indentation in an array, relative to the start of the line where the left square bracket is.

@@ -31,18 +32,56 @@
} else {
filename = "discovery/x86_64/bios/pxelinux.0";
}',
"next-server #{admin_ip}"],
"next-server #{admin_ip}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignArray: Align the elements of an array literal if they span more than one line. (https://github.com/bbatsov/ruby-style-guide#align-multiline-arrays)

@@ -31,18 +32,56 @@
} else {
filename = "discovery/x86_64/bios/pxelinux.0";
}',
"next-server #{admin_ip}"],
"next-server #{admin_ip}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/AlignArray: Align the elements of an array literal if they span more than one line. (https://github.com/bbatsov/ruby-style-guide#align-multiline-arrays)

nicolasbock
nicolasbock previously approved these changes Dec 18, 2018
Copy link
Contributor

@nicolasbock nicolasbock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

@@ -10,4 +10,16 @@
"option dhcp-client-debug code 226 = unsigned integer 16",
"option dhcp-client-debug 0"
]
default[:dhcp][:options][:v6] = [
"ddns-update-style none",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a sensible comment from hound 😉 It seems we are using 4 spaces everywhere else.

action :add do
if IPAddr.new(new_resource.network["subnet"]).ipv6?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this sort of thing in a number of places. I wonder if it might be cleaner to use an ip_version variable to store 4 vs. 6, and then you can simply use ip_version as a suffix and do away with some if/else blocks and make any file names explicit (ie subnet4.conf.erb vs. subnet.conf.erb)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way that crowbar updates and removes subnets from the list does make we worry about it from an upgrade perspective. I.e an existing env with subnet_list.conf with a bunch of subnets and we suddently change it. Will only "new" subnet source config lines be added.

I'll re-roll 1/2 way between. Leave ipv4 and subnet_list.conf and use subnet6_list.conf. But change to just adding a 6 or not. It does look much cleaner. Though ip_version = '' on ipv4 but. still cleaner then it was.

require "ipaddr"
admin_addr = IPAddr.new(address)

if admin_addr.ipv4?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my previous comment about maybe referencing an ip_version variable. I also wonder if this precludes us from doing dual-stack, meaning we would have to come back to this and refactor it again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I have a feeling there might be a bunch or refactoring required for true dual stack. Currently crowbar only expects 1 network of each type. So I think a bunch of changes maybe be required for it to have more then 1 network for each type. I think once we're gone through single stack, we'll have a very good idea of how much of crowbar we need to touch.

Unfortunately if I was more experienced with Crowbar, maybe I'd be able to tell you outright. But this is a new journey into the crux of crowbar for me too ;)

@nicolasbock
Copy link
Contributor

Rebased

@@ -525,7 +530,7 @@
owner "root"
group "root"
source "crowbar_register.erb"
variables(admin_ip: admin_ip,
variables(admin_ip: admin_ip.to_s,
admin_broadcast: admin_net.broadcast,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What meaning does admin_net.broadcast have when dealing with IPv6? Is this something that should be set conditionally?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'll be an empty string in IPv6 I believe.. which might make the crowbar_register script fail to find the correct interface (from a very quick look at the script) when attempting to register a new node with crowbar.

So I have a feeling we'll need to modify the register script some more. But that comes into play when were creating and registering nodes. So in the next few mkcloud steps after this one. But a good candidate for a new card in our backlog:

Test and modify crowbar_register to work with IPv6 addresses, as this script has comes reliance on broadcast addresses to find interfaces.

@@ -1373,7 +1408,7 @@ def upgrade_all_compute_nodes
upgrade_compute_nodes type
end

def parallel_upgrade_compute_nodes(compute_nodes)
def parallel_upgrade_compute_nodes(compute_nodes, controller = nil)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/AbcSize: Assignment Branch Condition size for parallel_upgrade_compute_nodes is too high. [58.29/30] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricsabcsize, http://c2.com/cgi/wiki?AbcMetric)
Metrics/CyclomaticComplexity: Cyclomatic complexity for parallel_upgrade_compute_nodes is too high. [11/6]
Metrics/MethodLength: Method has too many lines. [62/50] (https://github.com/bbatsov/ruby-style-guide#short-methods)
Metrics/PerceivedComplexity: Perceived complexity for parallel_upgrade_compute_nodes is too high. [11/7]

upgrade_nodes.each do |node|
node.set_pre_upgrade_attribute
if node.roles.include?("cinder-controller") &&
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/Next: Use next to skip iteration. (https://github.com/bbatsov/ruby-style-guide#no-nested-conditionals)

ip_version = if IPAddr.new(new_resource.network["subnet"]).ipv6?
'6'
else
''
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

action :add do
ip_version = if IPAddr.new(new_resource.network["subnet"]).ipv6?
'6'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

@@ -22,7 +22,8 @@
admin_ip = admin_net.address
domain_name = node[:dns].nil? ? node[:domain] : (node[:dns][:domain] || node[:domain])
web_port = node[:provisioner][:web_port]
provisioner_web="http://#{admin_ip}:#{web_port}"
provisioner_web="http://#{admin_ip}:#{web_port}" unless admin_net.ip_version == "6"
provisioner_web="http://[#{admin_ip}]:#{web_port}" if admin_net.ip_version == "6"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceAroundOperators: Surrounding space missing for operator =. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)

@@ -22,7 +22,8 @@
admin_ip = admin_net.address
domain_name = node[:dns].nil? ? node[:domain] : (node[:dns][:domain] || node[:domain])
web_port = node[:provisioner][:web_port]
provisioner_web="http://#{admin_ip}:#{web_port}"
provisioner_web="http://#{admin_ip}:#{web_port}" unless admin_net.ip_version == "6"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceAroundOperators: Surrounding space missing for operator =. (https://github.com/bbatsov/ruby-style-guide#spaces-operators)

address = Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin").address
admin_network = Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin")
intfs = [admin_network.interface]
address = admin_network.address
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/UselessAssignment: Useless assignment to variable - address. (https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars)

@@ -60,21 +75,24 @@
end

# This needs to be evaled.
intfs = [Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin").interface]
address = Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin").address
admin_network = Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/TrailingWhitespace: Trailing whitespace detected. (https://github.com/bbatsov/ruby-style-guide#no-trailing-whitespace)

@@ -14,10 +14,14 @@
#

action :add do
ip_version = new_resource.ip_version
if ip_version == "4"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||. (https://github.com/bbatsov/ruby-style-guide#if-as-a-modifier)

Copy link
Contributor

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love the direction this is going, with some cleanup, this should be awesome.

file "/etc/dhcp3/hosts.d/host6_list.conf" do
owner "root"
group "root"
mode 0644
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to only write these out if ip_version is 6?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, when originally writing this I was thinking about an environment that was dual stack. But your right, I'm already making assumptions based on the admin network and what version it is in order to create the correct dhcpd[6] config. I'll make the change to only write out for the ip_version of the admin net.

intfs = [Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin").interface]
address = Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin").address
admin_network = Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin")
intfs = [admin_network.interface]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned you've dropped address, is that really unused below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is unused.. but the comment above say. "# this needs to be evaled." so maybe it's where to make sure an address exists? All I know if hound complained because it was an unused variable. Will add it back as it was there before.

@@ -14,10 +14,13 @@
#

action :add do
ip_version = new_resource.ip_version
ip_version = "" if ip_version == "4"
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pattern keeps getting repeated. Perhaps a filename helper function in the barclamp library:

get_network_service_filename("subnet", new_resource) -> returns subnet.conf.erb or subnet6.conf.erb

include "/etc/dhcp3/groups.d/group6_list.conf";
include "/etc/dhcp3/subnets.d/subnet6_list.conf";
include "/etc/dhcp3/hosts.d/host6_list.conf";
<% else -%>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are most of the included options commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are the default values from the "sample" config, there commented out there too. Really there there because maybe there something we may or may not what to tweak and left them here seeing as I haven't really testing the actual running dhcpd6 yet.. other then it starts.

Also it was WIP so figured doesn't hurt. I'll remove them for now.

server = "#{protocol}://#{address}"
server = if IPAddr.new(address).ipv6?
"#{protocol}://[#{address}]"
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sort of agree with the terrible dog here, surely there's something better here. Didn't we already add a function that will wrap only IPv6 addresses, meaning this logic can live in that function and we only call it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did we? I don't remember it. But doesn't mean there isn't... I do have young girls who don't sleep.. which means I don't :P

If we don't I'll add an address_wrapped to the network class.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We did -- as a shell function for mkcloud. However, I think it's a nice idea, and it will certainly be needed for future barclamps (think bind addresses), so we may as well add it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added it to the barclamp Network class. We seem to use those, at least where I've seen em.
I could've added a NetworkHelper in the network barclamp which "would" be more obvious maybe.. but seeing as we already encapsulate the networks in the Barclamp network class I put it there.

"option domain-name \"#{domain_name}\"",
"option dhcp6.name-servers #{dns_servers.join(", ")}"
]
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Go home Hound, you're drunk.

@@ -168,7 +174,7 @@

service "dhcp3-server" do
if %w(suse rhel).include?(node[:platform_family])
service_name "dhcpd"
service_name "#{DhcpHelper.config_filename("dhcpd", admin_network.ip_version, "")}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/UnneededInterpolation: Prefer to_s over string interpolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah, I guess that makes more sense, thanks hound :)

file "/etc/dhcp3/groups.d/group_list.conf" do
# This needs to be evaled.
admin_network = Chef::Recipe::Barclamp::Inventory.get_network_by_type(node, "admin")
address = admin_network.address
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/UselessAssignment: Useless assignment to variable - address. (https://github.com/bbatsov/ruby-style-guide#underscore-unused-vars)

@@ -33,7 +34,7 @@
end
utils_line "include \"#{filename}\";" do
action :add
file "/etc/dhcp3/subnets.d/subnet_list.conf"
file "/etc/dhcp3/subnets.d/#{DhcpHelper.config_filename("subnet_list", new_resource.ip_version)}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/LineLength: Line is too long. [101/100] (https://github.com/SUSE/style-guides/blob/master/Ruby.md#metricslinelength)

filename = "/etc/dhcp3/subnets.d/#{new_resource.subnet}.conf"
template filename do
cookbook "dhcp"
source "subnet.conf.erb"
source "#{subnet_template}"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/UnneededInterpolation: Prefer to_s over string interpolation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, yeah, it's already a variable.

@@ -0,0 +1,9 @@
module DhcpHelper
def self.config_filename(base, ip_version, extension=".conf")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/SpaceAroundEqualsInParameterDefault: Surrounding space missing in default value assignment. (https://github.com/bbatsov/ruby-style-guide#spaces-around-equals)

address.to_s
end
end
end
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kEND

require "ipaddr"
if IPAddr.new(address).ipv6? rescue false
"[#{address}]"
else
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: else without rescue is useless

module NetworkHelper
def self.wrap_ip(address)
require "ipaddr"
if IPAddr.new(address).ipv6? rescue false
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint/Syntax: unexpected token kRESCUE_MOD

@matthewoliver matthewoliver force-pushed the ipv6-dhcpd branch 2 times, most recently from 7a59c21 to df9ea11 Compare February 5, 2019 06:00
@matthewoliver
Copy link
Contributor Author

This time forgot to remove the address_wrapped from the network class, which isn't required now that we have the NetworkHelper.

Copy link
Contributor

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is preceding in a fantastic direction, I have some concerns about the crowbar_join templates, but this is looking like amazing work.

@@ -114,7 +114,11 @@ wait_for_hostname() {
wait_for_admin_server() {
# wait for admin server to become pingable
tries_left=120
while ! ping -q -c1 $IP > /dev/null; do
local ping="ping"
if (( $IPV6 > 0 )); then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is that defined?

<% if @ip_version == "6" -%>
IPV6=1
<% else -%>
IPV6=0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Below where it is first used? I'm so confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah it is confusing, but really the code here is run first because it's outside of functions. Then it's used in functions. So was just following the pattern that was previously used for the $IP. As I figured once this is templated out into a bash script I wanted it to look like the old version as how knows in anyone ever goes in an hacks it. I guess they shouldn't.

I was lazy, so figured following an existing pattern and minimalise the change to the crowbar_join rather then refactor and probably cause or introduce bugs.

ADMIN_IP_WRAPPED="[ADMIN_IP]"
else
ADMIN_IP_WRAPPED="ADMIN_IP"
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we collapse these into one if block? They will effectively do the same thing anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, good point. I guess I shouldn't try and write back code that allows someone to easy hack on a machine (turn IPV6 to 0 or 1) cause it's not really making it any better.
DONE

"#{base}#{extension}"
else
"#{base}#{ip_version}#{extension}"
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be fair I had envisioned this as always returning "#{base}#{extra}#{extension}" where extra gets set to 6 if ip_version is 6, but this is very much a nit.

@@ -10,4 +10,16 @@
"option dhcp-client-debug code 226 = unsigned integer 16",
"option dhcp-client-debug 0"
]
default[:dhcp][:options][:v6] = [
"ddns-update-style none",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have two choices: Ignore Hound, or push a fix-up commit and then base your changes on top of that.

@@ -163,9 +167,9 @@ do_setup() {
# Make sure that the client knows how to talk to the server.
local cfg=/etc/chef/client.rb
if ! [ -f $cfg ] || \
! grep -q "^\s*chef_server_url\s*[\"\']http://$IP:4000[\"\']" $cfg; then
! grep -q "^\s*chef_server_url\s*[\"\']http://$IP_WRAPPED:4000[\"\']" $cfg; then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this? Because below is ADMIN_IP_WRAPPED, not IP_WRAPPED ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh, it's all IP_WRAPPED in this script. Unless I'm missing something.. which might be possible :P

@@ -0,0 +1,10 @@
module DhcpHelper
def self.config_filename(base, ip_version, extension = ".conf")
if ip_version == "4"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.

@@ -0,0 +1,10 @@
module DhcpHelper
def self.config_filename(base, ip_version, extension = ".conf")
if ip_version == "4"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.

@@ -0,0 +1,10 @@
module DhcpHelper
def self.config_filename(base, ip_version, extension = ".conf")
if ip_version == "4"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.

} else {
option dhcp6.bootfile-url \"#{admin6_uri}/x86_64/efi/#{boot_ip_hex}/boot/bootx64.efi\";
}"
]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/IndentArray: Indent the right bracket the same as the start of the line where the left bracket is.

ipaddress admin_ip_address
options [
"if exists dhcp-parameter-request-list {
if admin_data_net.ip_version == '6'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

s-t-e-v-e-n-k
s-t-e-v-e-n-k previously approved these changes May 27, 2019
Copy link
Contributor

@s-t-e-v-e-n-k s-t-e-v-e-n-k left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With CI passing, we can think about landing this. I have only comment remaining, I think.

ipaddress admin_ip_address
options [
"if exists dhcp-parameter-request-list {
if admin_data_net.ip_version == "6"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks to be very much duplicating the next block. I'm not going to block on it, but can comment that it could be refactored to be IP version agnostic.

if @ip_version == "6"
IPAddr.new('ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff').mask(cidr).to_s
else
IPAddr.new('255.255.255.255').mask(cidr).to_s
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)


if cidr
if @ip_version == "6"
IPAddr.new('ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff').mask(cidr).to_s
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

range = 32
end

cidr = Integer(@netmask) rescue nil
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/RescueModifier: Avoid using rescue in its modifier form. (https://github.com/bbatsov/ruby-style-guide#no-rescue-modifiers)

@@ -124,6 +132,30 @@ def interface_list
@interface_list
end

def cidr_to_netmask
if @ip_version == "6"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/ConditionalAssignment: Use the return of the conditional for variable assignment and comparison.

@@ -124,6 +132,30 @@ def interface_list
@interface_list
end

def cidr_to_netmask
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Metrics/CyclomaticComplexity: Cyclomatic complexity for cidr_to_netmask is too high. [7/6]
Metrics/PerceivedComplexity: Perceived complexity for cidr_to_netmask is too high. [10/7]

@@ -79,7 +79,7 @@
variables(ntp_servers: ntp_servers,
admin_interface: local_admin_address,
admin_subnet: admin_net.subnet,
admin_netmask: admin_net.netmask,
admin_netmask: admin_net.cidr_to_netmask(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/MethodCallWithoutArgsParentheses: Do not use parentheses for method calls with no arguments. (https://github.com/bbatsov/ruby-style-guide#method-invocation-parens)

@matthewoliver
Copy link
Contributor Author

Added to more commits to this PR. First adds the adminip to the kernel commandline so the new version of the sleshammer script can pick it up. And the second fixes the ntp configuration.
I've only roughly tested on my laptop. Tomorrow I'll work through hounds grips and clean up a little :)

@nicolasbock
Copy link
Contributor

Rebased

If the admin network is IPv6 setup the ISC DHCPD server to configure
and use the IPv6 daemon. For this use a seperate set of ipv6 files
to list hosts and subnets as ipv6 hosts and subnets will fail if
v4 dhcp tries to load them.

Also make sure tftp is listening on both IPv4 and v6.

To make this all happen the Network class in the Barclamp::Inventory
library now tracks the ip_version of the network. Which is used to
make decisions through the DHCP and provisioner cookbooks.

To support backwards compatibilty with ipv4 config naming in the
DHCP barclamp a DhcpHelper was added to return config names in
an IPv4 or IPv6 way. The same pattern was used throughout the
barclamp in the early versions of this patch so it's been
refectered to the helper.

Finally, when dealing with some IPv6 addresses in URIs the address
needs to be wrapped. As such the network barclamp has grown a
NetworkHelper module to start gathering network related helper
method's, it's first is a wrap_ip function. Which will wrap an
address if it happens to an IPv6 address. This makes this available
to us anywhere in crowbar, so long as the network barclamp has been
applied, and as a core barclamp to crowbar, should be always.
When the sleshammer discovery image boots on nodes in an IPv6
environment there is no sane way of determining the admin ip.

In IPv4 we can grab it from the SERVERID produced by wicked's
dhcpd4 binary. Except in IPv6 the serverid seems to the be DHCP
DUID.

This patch adds an 'adminip=..' option to the commandline for
the discovery image. The start-up.sh script and sleshammer image
has been modified seperately.
The ntp.conf used in crowbar wont work with IPv6 as the netmask
in the ntp conf file need to be in the full form not CIDR.

This patch adds a new method, cidr_to_subnet to the Barclamp library's
Network class. Which is then used to send the correct string to the
ntp.conf.erb template.

Allowing the crowbar nodes being booted via the sleshammer discovery
image to sync with the admin node.
@@ -152,6 +152,10 @@ def make_zone(zone)

zonefile_entries
end
def address_version(address)
'ip6addr' if IPAddr.new(address).ipv6?
'ip4addr' if IPAddr.new(address).ipv4?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

@@ -152,6 +152,10 @@ def make_zone(zone)

zonefile_entries
end
def address_version(address)
'ip6addr' if IPAddr.new(address).ipv6?
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. (https://github.com/SUSE/style-guides/blob/master/Ruby.md#stylestringliterals)

@@ -152,6 +152,10 @@ def make_zone(zone)

zonefile_entries
end
def address_version(address)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Layout/EmptyLineBetweenDefs: Use empty lines between method definitions. (https://github.com/bbatsov/ruby-style-guide#empty-lines-between-methods)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants