Fixes #32252 - block ip during host creation

[sbernhard]

No description provided.

[theforeman-bot]

Issues: #32252

[lzap]

Small nitpicks, thank you I like this.

[ekohl]

Interesting idea but is this really the right place to solve it? In the past this has been solved in the Smart Proxy. FreeIps normally marks an IP as allocated internally. This is already a cache of IPs it has returned. These aren't allocated in the actual DHCP backend, just those it has returned to Foreman.

This essentially duplicates that effort which can lead to problems. For example, there could be just a few IPs that are free according to the Smart Proxy but are blocked in Foreman leading to quicker starvation of the pool. It would be my preference to keep that logic in the Smart Proxy and fix any backends that don't implement this.

[ekohl]

Naming wise: I wouldn't say this is blocked. Perhaps pre-reserved?

[sbernhard]

The issue already occurs, if you press create-host multiple times. If you open the create host page 3 times you would currently get 3 times the same IP.

well, from the user perspective the IP is blocked for its usage.

[ekohl]

I still think that's different from blocking it. It feels like a reservation or a lease. However, I won't push on that.

[sbernhard]

opinion @lzap? Its easy to change if you have a proposal? Should we use "reserve"?

[lzap]

Reserve feels a bit better but I do not mind blocking as well. Other synonyms: book, hold, order.

[lzap]

@ekohl smart proxy code for free IP handling is not used for internal IPAMs, only when you select DHCP IPAM then smart proxy performs these lookups and checks.

[ekohl]

It was pointed out on IRC that this doesn't call the Smart Proxy and thus can't use the FreeIps mechanism. Please write this down somewhere. IMHO the commit message is a good place.

[ekohl]

The code itself looks good but I still think it would be good to mention in the commit message that this is only for the DB and does not apply to the DHCP IPAM because there it's solved on the Smart Proxy side.

[sbernhard]

The code itself looks good but I still think it would be good to mention in the commit message that this is only for the DB and does not apply to the DHCP IPAM because there it's solved on the Smart Proxy side.

also done.

No more objections to merging it.

[sbernhard]

Someone has an idea how to handle the failed test?

[lzap]

Isn't that obvious? :-)

      test "should return IPv4 based on MAC if provided" do
        subnet = FactoryBot.build(
          :subnet_ipv4, :name => 'my_subnet',
          :network => '10.0.0.0',
          :mask => '255.0.0.0',
          :ipam => IPAM::MODES[:random_db])
        ipam1 = IPAM::RandomDb.new(:subnet => subnet, :mac => "AA:BB:CC:DD:EE:FF")
        ipam2 = IPAM::RandomDb.new(:subnet => subnet, :mac => "AA:BB:CC:DD:EE:FF")
        assert_equal ipam1.suggest_ip, ipam2.suggest_ip
      end

Two IPAM objects are created, they both share the same Rails cache (which for tests is I think in-memory cache). Both have the same MAC address, that results in the same random generator seed so both are expected to generate the same random pseudo numbers.

You call suggest IP once, that will reserve an IP and return it. Then you do the same therefore the reserved IP is skipped.

My advice is to refactor the constant to initializer argument (with a default value). When zero is passed, then turn off caching completely. By default it will use 30 minutes. Then copy make two tests from this one:

1. When 0 is passed, the old test should pass.
2. When nothing is passed (30 mins by default), a copy of this test will pass if you refute_equal

[sbernhard]

Thanks for your suggestion @lzap

[sbernhard]

Everything is green now @lzap

[lzap]

It could probably be a little cleaner when block_ip would return amount of minutes it will be blocking, therefore there would be no need to expose block_ip_minutes attribute at all. But I am good.

[lzap]

Letting @ekohl to merge since he has a review here. Thank you, this was a long-standing bug that we got asked several times from our customers! Viva la cooperation.

[ekohl]

Thanks!