Fixes #27067 - IPAM Integration with phpIPAM

[grizzthedj]

NOTE: Submitting a new PR for "Fixes #27067 - IPAM Integration with phpIPAM" using a new branch named: feature/ipam_integration.

There were some issues with the previous branch(plugin/foreman_ipam), that I was not able to recover from. We can use the old PR(which is now closed) as a reference to the code review history.

#6847

===========================================

This PR is a patch to support of the use of plugins for external IPAM use, with opensource provider phpIPAM(https://phpipam.net). The primary use case is to provide the next available IPv4 address from phpIPAM, for a given phpIPAM subnet.

The functionality that Foreman will use for external IPAM is encompassed in the below plugins(which are both new).

foreman_ipam(v0.0.10)
Git: https://github.com/grizzthedj/foreman_ipam
Gem: https://rubygems.org/gems/foreman_ipam

smart_proxy_ipam(v0.0.15)
Git: https://github.com/grizzthedj/smart_proxy_ipam
Gem: https://rubygems.org/gems/smart_proxy_ipam

The foreman_ipam plugin provides a very basic dashboard to display sections and subnets from phpIPAM, and the smart_proxy_ipam plugin communicates with the phpIPAM API.

Features and functionality

• Simple IPAM Dashboard page(Infrastructure => IPAM Dashboard) to view sections and subnets for phpIPAM. More features will be added here based on internal user feedback and from the community.
• When creating a phpIPAM subnet(IPAM type = External IPAM) in Foreman, subnet/mask must exist in phpIPAM, or error will be thrown
• When creating a new host in Foreman, auto suggested IP's from phpIPAM will be persisted back to phpIPAM on Host submit.
• When updating the IP address on a NIC that uses a phpIPAM subnet, the old IP will be deleted from phpIPAM, and new one will be added.
• Deleting a host in Foreman will delete all NIC ip's that used a phpIPAM subnet
• Deleting a NIC from a host will also delete the IP from phpIPAM
• Deleting a subnet in Foreman will NOT delete the associated subnet in phpIPAM. This could easily be added later if it makes sense to the community, however initial thoughts are to not delete it.

Prerequisites for testing:

• Working Foreman environment.
• A phpIPAM instance that at least one Forman Smart Proxy has network connectivity to.
• The API must be enabled in phpIPAM settings, and Prettify Links should also be enabled.
• An API user in phpIPAM must be created. The credentials for this user are to be put in the config/settings.d/externalipam.yml file in the Smart Proxy core

[theforeman-bot]

Can one of the admins verify this patch?

[theforeman-bot]

Can one of the admins verify this patch?

[theforeman-bot]

Can one of the admins verify this patch?

[theforeman-bot]

Issues: #27067

[theforeman-bot]

There were the following issues with the commit message:

• b64804b must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[grizzthedj]

Hi @lzap @ekohl: Sorry for the delay here.

I have refactored based on the recent feedback, added some documentation, and removed the workaround GET method that I was using. I have also refactored the IPAM proxy plugin to accommodate these changes.

There are a couple of points/questions that I need to ask to get further feedback on:

1. IPv6: My intention was to handle this in another PR after the first merge happens. That being said, if you think it makes sense to add this functionality now, I would be happy to do so.

2. Multi-tenant Environments: In phpIPAM, there is a setting called "Require unique subnets". When this setting is enabled, subnets must be unique, and cannot overlap, across the entire phpIPAM instance. The current implementations of the IPAM proxy plugin and Foreman patch handles this scenario nicely.

If however this setting is disabled, that means duplicate subnets and IP addresses can exist in multiple sections(a section is just a logical group of subnets and IP's). For this scenario, the current implementation is hardcoded to always use the first subnet(index 0), so it still needs some work.

We need to store the External IPAM section name on the Foreman side so that we can associate Foreman subnets to phpIPAM sections, so that we can handle the overlap and duplication. I think adding a string column to the subnets model should solve this. Perhaps we can name it like "tenant" and not specific to "section", to keep things generic? Although I haven't really looked into it yet, I suspect other IPAM providers(i.e. Infobox, Netbox etc.) have a similar construct.

In terms of handing this in the Foreman UI/UX, when a user creates an External IPAM subnet in Foreman, it would be assumed that "Require unique subnets" is enabled(as a default). In the case where "Require unique subnets" is disabled, users could edit the Subnet in Foreman, and they would be presented with a drop down list of sections, or tenants, whatever is decided from a naming perspective, and they could associate the section/tenant with the foreman Subnet.

3. Satellite: Once this PR gets merged, would this functionality also be available in Red Hat Satellite? If so, how frequent are the release cycles? When could we expect to see this feature in Satellite?

I also recorded a demo video that shows the Foreman/phpIPAM integration in action, so you can get a sense for how things are currently working:

https://youtu.be/QM5cEXicp9Y

As far as the tests go, looks like they seem to be green now.

Looking forward to your feedback!

Chris

[grizzthedj]

Update: Working on adding support for IPv6 while waiting for feedback

[theforeman-bot]

There were the following issues with the commit message:

• 95e2453 must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[theforeman-bot]

There were the following issues with the commit message:

• a6ee11f must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.

---

This message was auto-generated by Foreman's prprocessor

[Neurozone]

When this will be added to foreman official plugins ?
We need it :)

[grizzthedj]

Update: I have added support for IPv6, and also developed an integration test suite(using cucumber/capybara) to end-to-end test the features.

https://github.com/grizzthedj/foreman_ipam_integration_tests

[grizzthedj]

@lzap @ekohl - A friendly ping.

I have added IPv6, and made the latest round of requested changes from @ekohl. Please let me know if you need anything else for this PR, in addition to answering the questions above. I welcome any further feedback.

[lzap]

Hello, I was on a long PTO so apologies, definitely on my radar. Gonna get to this hopefully next week.

[lzap]

Hey, the code looks good overall, great job. There is however one concern about dual-stack which needs to be solved.

Apologies for late response, if I don't respond in a timely manner (one day) please drop me an email at lzap_at_redhat.com because I have quite backlog on github and notification could get lost. Let's close this one before Christmas!

[grizzthedj]

Force pushed @ekohl's changes. Will forgo the workaround for now.

[ekohl]

I wonder: is this the literal response JSON or a meta-description? I assumed the latter that the body is actually:

{"name":"Awesome Section", "description": "Awesome Section"}

And HTTP status code is 200. Now that I read subnet_exists_in_group again, it uses group['data'].present? and that makes me doubt it. In a REST API I wouldn't expect a wrapper with the status code since at the HTTP level those codes are present.

[lzap]

Yeah I actually missed that too, can you refactor this one more time? This one is pretty straightforward - just extract "code" from body to be HTTP request code both in core and in the plugin.

Normally, I'd merge this and ask you to do in a separate PR, but here we actually define how the API should look like and there was already one community user interested in writing own implementation. Let's get this right from the day one. Tests are green btw the one that fails is a random failure we currently face.

[lzap]

Hello, I know it's been terribly long but please stay with us. I am sure this is the last thing @ekohl has on his mind and we can merge this. It's a great improvement, multiple users were asking for this already.

[grizzthedj]

Sorry @lzap - there have been some fires to put out. Definitely going to get to this --- hopefully tomorrow

[lzap]

It works fine, IPv6 is not returned properly but this is a bug/missing feature in phpIPAM. That's how it returns from their API. Maybe drop a comment on that in our smart proxy plugin README.

Thank you so much.

[lzap]

Merged, we are running Foreman Community Demo two weeks from now. Would you like to present the feature yourself? We also accept pre-recorded demos (I know you have one already we could use that). Alternatively, I can show it if you prefer to. Let me know:

https://community.theforeman.org/t/foreman-community-demo-76/17803

[ekohl]

IMHO we shouldn't have merged this yet. The "REST" API is very inconsistent in its error handling and response codes.

[ares]

@ekohl if you can suggest specific modifications or improvement, there's still time to fix it before next branching. The API is not public yet.

[ekohl]

I had a look at the actual smart proxy implementation and came up with grizzthedj/smart_proxy_ipam#36. It's not done, but the general idea of using actual HTTP status codes is there.