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.

Sign up for GitHub

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

Jump to bottom

Fixes #34205 - External IPAM Integration #810

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Fixes #34205 - External IPAM Integration #810

wants to merge 1 commit into from

Conversation

grizzthedj
Copy link
Member

@lzap

This long overdue PR is for merging the External IPAM features from the smart_proxy_ipam plugin (https://github.com/grizzthedj/smart_proxy_ipam) into Smart Proxy Core.

This currently adds support for the below IPAM providers:

  1. phpIPAM
  2. Netbox

Tests for the External IPAM module have been written and are green. Please note that I am unable to run the entire test suite due to some mac install issues with the rkerberos gem.

Looking forward to feedback!

lzap
lzap reviewed Jan 4, 2022
View reviewed changes
config/settings.d/externalipam.yml.example
# 1. phpIPAM: externalipam_phpipam
# 2. Netbox: externalipam_netbox

:use_provider: externalipam_netbox
Copy link
Member

Choose a reason for hiding this comment

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

Note for myself / @ekohl - I am fine with bringing this feature into core, after all External IPAM is already in Foreman core, however, I would suggest to ship providers as RPM/DEB subpackages so products owners can decide what to ship and what not. Opinion?

Copy link
Member

Choose a reason for hiding this comment

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

It does not introduce additional dependencies and the Smart Proxy is already quite smart in not loading the module if it's inactive so I'd just ship it in the same RPM/deb. Subpackages only make sense to reduce additional dependencies or reducing runtime overhead at the cost of more complex installation. In this case I think it's not worth it but I appreciate the consideration.

Copy link
Member

Choose a reason for hiding this comment

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

Okay let me rephrase it this way - I do not want to support this in Red Hat Satellite at this moment. Perhaps we can disable it via the installer scenario?

Copy link
Member

Choose a reason for hiding this comment

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

Currently we don't have good facilities for that. It would need some thought.

Copy link
Member

Choose a reason for hiding this comment

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

Ok this is an internal discussion, I do not want block this effort.

ekohl
ekohl requested changes Jan 4, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I went through it, but there's a bunch of repeated patterns. It may feel a bit like an inconsistent review and I think we may have a long back and forth on this for a while. That said, I am in favor of getting this into the Smart Proxy so please don't take it as being against it. Quite the opposite: Foreman itself has the functionality in core so the Smart Proxy should too.

modules/externalipam/phpipam/phpipam_client.rb Outdated Show resolved Hide resolved
modules/externalipam/api_resource.rb Outdated Show resolved Hide resolved
modules/externalipam/ip_cache.rb Outdated Show resolved Hide resolved
modules/externalipam/ip_cache.rb Outdated
end

def ip_exists(ip, cidr, group_name)
cidr_key = @ip_cache[group_name.to_sym][cidr.to_sym]&.to_s
Copy link
Member

Choose a reason for hiding this comment

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

Why &.to_s? Would it be better to use IPAddr instance and check if the IP is present in the subnet?

Copy link
Member Author

Choose a reason for hiding this comment

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

We are checking if the IP is in the cache, not in the subnet

modules/externalipam/ip_cache.rb Show resolved Hide resolved
modules/externalipam/netbox/netbox_client.rb Show resolved Hide resolved
modules/externalipam/netbox/netbox_client.rb Outdated Show resolved Hide resolved
modules/externalipam/api_resource.rb Outdated Show resolved Hide resolved
modules/externalipam/netbox/netbox_client.rb Outdated Show resolved Hide resolved
modules/externalipam/netbox/netbox_client.rb Outdated Show resolved Hide resolved
@grizzthedj
Copy link
Member Author

Pushed most of these changes, and still working through the last few. I will squash the commits once everything is in alignment

@lzap
Copy link
Member

lzap commented Mar 2, 2022

I have no further comments, @ekohl ?

ekohl
ekohl requested changes Mar 3, 2022
View reviewed changes
config/settings.d/externalipam.yml.example Outdated Show resolved Hide resolved
config/settings.d/externalipam_netbox.yml.example Outdated Show resolved Hide resolved
config/settings.d/externalipam_phpipam.yml.example Outdated Show resolved Hide resolved
modules/dhcp_common/free_ips.rb Outdated Show resolved Hide resolved
modules/externalipam/ipam_validator.rb Outdated Show resolved Hide resolved
modules/externalipam/ip_cache.rb Outdated Show resolved Hide resolved
modules/externalipam/ip_cache.rb Outdated Show resolved Hide resolved
modules/externalipam/ip_cache.rb Outdated Show resolved Hide resolved
modules/externalipam/ipam_api.rb Outdated Show resolved Hide resolved
modules/externalipam/ipam_api.rb Outdated Show resolved Hide resolved
@lzap
Copy link
Member

lzap commented Mar 15, 2022

I think this is close to merging, please rebase and we can move forward with this.

@grizzthedj
Copy link
Member Author

@lzap Almost finished with most of the recommended changes. Will try to rebase and push at some point next week

@grizzthedj
Copy link
Member Author

grizzthedj commented Mar 28, 2022 •
edited

@lzap @ekohl Sorry for the delay. Had to rebuild my local Foreman dev env. Changes are done and rebased. Let me know if anything else is needed here.

Here is the Foreman PR that is also needed for this integration to work

theforeman/foreman#9174

@grizzthedj grizzthedj mentioned this pull request Apr 3, 2022
Merged
@lzap
Copy link
Member

lzap commented Apr 8, 2022

Overall I am okay with the patch, Ewoud had more comments than I anyway I will let him to finish the review. Thanks for this feature!

@grizzthedj
Copy link
Member Author

@ekohl Just did a fresh rebase and also wanted to follow-up on this PR. Changes are done and tested. Let me know if you need anything else. Will be great to have this as part of the Foreman/Proxy core!

@lzap lzap requested a review from ekohl July 20, 2022 08:13
ekohl
ekohl reviewed Jul 20, 2022
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think this is looking pretty good. Some minor comments inline.

lib/proxy/validations.rb Outdated Show resolved Hide resolved
modules/dhcp_common/free_ips.rb Outdated Show resolved Hide resolved
modules/externalipam/api_resource.rb Show resolved Hide resolved
modules/externalipam/ip_cache.rb Outdated Show resolved Hide resolved
modules/externalipam/ip_cache.rb Outdated Show resolved Hide resolved
modules/externalipam/ipam_api.rb Outdated Show resolved Hide resolved
modules/externalipam/ipam_api.rb Outdated Show resolved Hide resolved
@grizzthedj
Copy link
Member Author

grizzthedj commented Jul 22, 2022 •
edited

@ekohl - Changes are done and tested. Sorry I squashed the commit already but probably should have done that after your review

@grizzthedj
Copy link
Member Author

@ekohl Just a friendly ping on this. I hoping the plan is still to merge this into core. I have completed the requested changes - Let me know if there is anything else needed to move this forward

@dmgeurts
Copy link

@ekohl Where are things regarding the integration of External IPAM? It would be great to finally have this integrated.

@ekohl ekohl mentioned this pull request Feb 19, 2024
Merged
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lzap lzap lzap left review comments

@ekohl ekohl ekohl requested changes

Assignees
No one assigned
Labels
Needs re-review Needs testing
Projects
None yet
Milestone
No milestone
5 participants
@grizzthedj @lzap @dmgeurts @ekohl @theforeman-bot