Fixes #38051 - Fix built request to accept ipv6 hosts #10392

ShimShtein · 2024-12-01T15:20:20Z

Built request should be able to accept built requests sent from IPv6 only machine. This pr makes sure ip6 field is updated accordingly.

ekohl

We have some related code in

foreman/app/models/host/base.rb

Lines 378 to 399 in 9b8f759

    
           def parse_ip_address(address, ignore_link_local: true) 
        
             return nil unless address 
        
             begin 
        
               addr = IPAddr.new(address) 
        
             rescue IPAddr::InvalidAddressError 
        
               # https://tickets.puppetlabs.com/browse/FACT-1935 facter can return an 
        
               # address with the link local identifier. Ruby can't parse because of 
        
               # https://bugs.ruby-lang.org/issues/8464 so we manually strip it off 
        
               # if the interface identifier if present 
        
               if address.is_a?(String) && address.include?('%') 
        
                 addr = IPAddr.new(address.split('%').first) 
        
               else 
        
                 logger.warn "Ignoring invalid IP address #{address}" 
        
                 return nil 
        
               end 
        
             end 
        
             return if ignore_link_local && addr.link_local? 
        
             addr.to_s 
        
           end

which has some handling around IPv6 link local addresses. I don't know if you can hit the same thing here, but I'd appreciate it if you can take a look.

ekohl · 2024-12-04T15:43:22Z

app/controllers/unattended_controller.rb

+    end
+
+    if address.ipv6?


Suggested change

end

if address.ipv6?

elsif address.ipv6?

ekohl · 2024-12-04T16:04:38Z

test/controllers/unattended_controller_test.rb

@@ -373,6 +373,16 @@ class UnattendedControllerTest < ActionController::TestCase
      refute nic.build
    end

+    test "should accept built notifications over ipv6 and store the address" do
+      nic6 = FactoryBot.build(:nic_managed, :with_ipv6)
+      Setting[:update_ip_from_built_request] = true


Does this revert the change after the test? We have a helper for SETTINGS:

foreman/test/test_helper.rb

Lines 204 to 214 in 9b8f759

def with_temporary_settings(**kwargs)

old_settings = SETTINGS.dup

begin

SETTINGS.update(kwargs)

yield

ensure

SETTINGS.replace(old_settings)

end

end

end

But I'm not sure we need to a similar thing for Setting.

It does not, but from looking at the code, we don't revert any of the settings. I suppose if the test relies on a specific setting, it should be explicitly set in the setup block.

ekohl · 2024-12-04T16:06:12Z

test/controllers/unattended_controller_test.rb

+    test "should accept built notifications over ipv6 and store the address" do
+      nic6 = FactoryBot.build(:nic_managed, :with_ipv6)
+      Setting[:update_ip_from_built_request] = true
+      @request.env["REMOTE_ADDR"] = nic6.ip6


Isn't it easier to just use some specific IP instead of using the NIC factory?

I didn't want to copy the IP generation code, so having FactoryBot actually generating one was an easy option for me.

You don't need a random IP, a static IP from the 2001:db8::/32 range is IMHO better. That way you can assert for it. Your current code could have a bug where both values are set to nil and you wouldn't notice it.

ekohl · 2024-12-19T15:09:34Z

app/controllers/unattended_controller.rb

@@ -217,8 +217,14 @@ def update_ip
    logger.debug "Built notice from #{ip}, current host ip is #{@host.ip}, updating" if @host.ip != ip


This isn't taking IPv6 into account and needs to be moved into the if block's branches.

ekohl · 2024-12-19T15:10:02Z

app/controllers/unattended_controller.rb

+    if address.ipv4?
+      old_ip = @host.ip
+      @host.ip = old_ip unless @host.update({'ip' => ip})
+    elsif address.ipv6?


Please filter out link local addresses

Suggested change

elsif address.ipv6?

elsif address.ipv6? && !address.link_local?

You perhaps you want to log a message if it's ignored, but I wouldn't feel too strongly about it.

ShimShtein · 2024-12-24T11:22:12Z

Ready for another round

pr-processor bot added the Not yet reviewed label Dec 1, 2024

ShimShtein force-pushed the add_ipv6_to_built branch from 30e77d0 to 9ebe07b Compare December 1, 2024 18:53

ekohl reviewed Dec 4, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Dec 4, 2024

stejskalleos assigned nofaralfasi Dec 6, 2024

ShimShtein mentioned this pull request Dec 9, 2024

Ipv6 combined #10400

Draft

4 tasks

ShimShtein force-pushed the add_ipv6_to_built branch from 9ebe07b to 4b8860c Compare December 19, 2024 11:38

ekohl requested changes Dec 19, 2024

View reviewed changes

pr-processor bot added the Waiting on contributor label Dec 19, 2024

ShimShtein force-pushed the add_ipv6_to_built branch from 4b8860c to 7899652 Compare December 23, 2024 10:15

pr-processor bot added Needs re-review and removed Waiting on contributor labels Dec 23, 2024

Fixes #38051 - Fix built request to accept ipv6 hosts

13cb803

ShimShtein force-pushed the add_ipv6_to_built branch from 7899652 to 13cb803 Compare December 23, 2024 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #38051 - Fix built request to accept ipv6 hosts #10392

Fixes #38051 - Fix built request to accept ipv6 hosts #10392

ShimShtein commented Dec 1, 2024

ekohl left a comment

ekohl Dec 4, 2024

ekohl Dec 4, 2024

ShimShtein Dec 19, 2024

ekohl Dec 4, 2024

ShimShtein Dec 19, 2024

ekohl Dec 19, 2024

ekohl Dec 19, 2024

ekohl Dec 19, 2024

ShimShtein commented Dec 24, 2024

	def parse_ip_address(address, ignore_link_local: true)
	return nil unless address

	begin
	addr = IPAddr.new(address)
	rescue IPAddr::InvalidAddressError
	# https://tickets.puppetlabs.com/browse/FACT-1935 facter can return an
	# address with the link local identifier. Ruby can't parse because of
	# https://bugs.ruby-lang.org/issues/8464 so we manually strip it off
	# if the interface identifier if present
	if address.is_a?(String) && address.include?('%')
	addr = IPAddr.new(address.split('%').first)
	else
	logger.warn "Ignoring invalid IP address #{address}"
	return nil
	end
	end

	return if ignore_link_local && addr.link_local?

	addr.to_s
	end

	def with_temporary_settings(**kwargs)
	old_settings = SETTINGS.dup
	begin
	SETTINGS.update(kwargs)

	yield
	ensure
	SETTINGS.replace(old_settings)
	end
	end
	end

		@@ -217,8 +217,14 @@ def update_ip
		logger.debug "Built notice from #{ip}, current host ip is #{@host.ip}, updating" if @host.ip != ip

	elsif address.ipv6?
	elsif address.ipv6? && !address.link_local?

Fixes #38051 - Fix built request to accept ipv6 hosts #10392

Are you sure you want to change the base?

Fixes #38051 - Fix built request to accept ipv6 hosts #10392

Conversation

ShimShtein commented Dec 1, 2024

ekohl left a comment

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

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

Choose a reason for hiding this comment

ShimShtein commented Dec 24, 2024