From d2458a5c9dfa828e10a1f9d10e7f74fcffbd8da6 Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Thu, 26 Apr 2018 16:58:55 +0200 Subject: [PATCH 1/2] network: Ensure data returned by allocate_ip is correct When allocate_ip is called, it returns some data representing the related network information. However, as network attributes can be overridden by having attributes on a node (to override the conduit for a specific network on that node, for instance), which is something that was not reflected here, resulting in incorrect data. --- crowbar_framework/app/models/network_service.rb | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/crowbar_framework/app/models/network_service.rb b/crowbar_framework/app/models/network_service.rb index f27da159eb..e278886b7e 100644 --- a/crowbar_framework/app/models/network_service.rb +++ b/crowbar_framework/app/models/network_service.rb @@ -54,6 +54,7 @@ def allocate_ip_by_type(bc_instance, network, range, object, type, suggestion = return [404, "No node found"] if node.nil? name = node.name.to_s else + node = nil name = object.to_s end @@ -70,7 +71,7 @@ def allocate_ip_by_type(bc_instance, network, range, object, type, suggestion = begin lock = acquire_ip_lock db = Chef::DataBag.load("crowbar/#{network}_network") rescue nil - net_info = build_net_info(role, network) + net_info = build_net_info(role, network, node) # Did we already allocate this, but the node lost it? if db["allocated_by_name"].key?(name) @@ -428,13 +429,13 @@ def enable_interface(bc_instance, network, name) node.save Rails.logger.info("Network enable_interface: Assigned: #{name} #{network}") - [200, build_net_info(role, network)] + [200, build_net_info(role, network, nil)] end - def build_net_info(role, network) - net_info = {} - role.default_attributes["network"]["networks"][network].each do |k, v| - net_info[k] = v unless v.nil? + def build_net_info(role, network, node) + net_info = role.default_attributes["network"]["networks"][network].to_hash + unless node.nil? || node.crowbar.nil? + net_info.merge!(node["crowbar"]["network"][network] || {}) end net_info end From d5bb352ec4d416074b0a5fa5845d087f39c8c16c Mon Sep 17 00:00:00 2001 From: Vincent Untz Date: Thu, 26 Apr 2018 18:36:34 +0200 Subject: [PATCH 2/2] crowbar: Fix Node.get_network_by_type for overridden attributes We were looking for attributes overriding the network proposal only on the node role, but this is not consistent with what the cookbook does (it looks at attributes on the node itself), and customer using this feature are more likely to edit the node directly. So reflect that by looking at the node attributes also. This is slightly tricky, as the merge of node attributes and node role attributes occur on a chef run, and in the rails app, this can be out of sync. So we do our own merge to always see the real values. --- .../app/models/network_service.rb | 4 +-- crowbar_framework/app/models/node.rb | 35 +++++++++++++++++-- 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/crowbar_framework/app/models/network_service.rb b/crowbar_framework/app/models/network_service.rb index e278886b7e..48f57b68c4 100644 --- a/crowbar_framework/app/models/network_service.rb +++ b/crowbar_framework/app/models/network_service.rb @@ -434,8 +434,8 @@ def enable_interface(bc_instance, network, name) def build_net_info(role, network, node) net_info = role.default_attributes["network"]["networks"][network].to_hash - unless node.nil? || node.crowbar.nil? - net_info.merge!(node["crowbar"]["network"][network] || {}) + unless node.nil? + net_info.merge!(node.crowbar_network[network] || {}) end net_info end diff --git a/crowbar_framework/app/models/node.rb b/crowbar_framework/app/models/node.rb index d63959f7c7..e4d44b84e7 100644 --- a/crowbar_framework/app/models/node.rb +++ b/crowbar_framework/app/models/node.rb @@ -634,9 +634,38 @@ def destroy Rails.logger.debug("Done with removal of node: #{@node.name} - #{crowbar_revision}") end + def crowbar_network + # This is slightly annoying: we store data in the node role, but since we + # allow overriding the network data on a per-node basis, users may also put + # some data in the node directly. + # Within a chef cookbook, it's fine because it's all merged and accessible + # directly through the attribute, but we can't rely on this here as the + # merge of the node attributes and the node role attributes only occurs on + # a chef run. That means that if the rails app is changing the node role + # attribute and then trying to read the data before a chef run, it cannot + # rely solely on the the node attributes. + # Therefore we manually merge here the two sets of attributes... + + role_attrs = crowbar["crowbar"]["network"].to_hash + + # We don't take default attributes, as we would also include the node role + # attributes as they exists on the last chef run, therefore always + # overriding the possibly different node role attributes that exist now. As + # a result, the attribute path may not exist and we need some care when + # accessing what we look for. + node_normal_crowbar = @node.normal_attrs["crowbar"] + node_attrs = if node_normal_crowbar.nil? || node_normal_crowbar["network"].nil? + {} + else + @node.normal_attrs["crowbar"]["network"].to_hash + end + + role_attrs.merge(node_attrs) + end + def networks networks = {} - crowbar["crowbar"]["network"].each do |name, data| + crowbar_network.each do |name, data| # note that node might not be part of network proposal yet (for instance: # if discovered, and IP got allocated by user) next if @node["network"]["networks"].nil? || !@node["network"]["networks"].key?(name) @@ -647,11 +676,11 @@ def networks def get_network_by_type(type) return nil if @role.nil? - return nil unless crowbar["crowbar"]["network"].key?(type) + return nil unless crowbar_network.key?(type) # note that node might not be part of network proposal yet (for instance: # if discovered, and IP got allocated by user) return nil if @node["network"]["networks"].nil? || !@node["network"]["networks"].key?(type) - @node["network"]["networks"][type].to_hash.merge(crowbar["crowbar"]["network"][type].to_hash) + @node["network"]["networks"][type].to_hash.merge(crowbar_network[type].to_hash) end def set_network_attribute(network, attribute, value)