Fixes #34205 - External IPAM Integration - #810
Conversation
ekohl
left a comment
ekohl
left a comment
There was a problem hiding this comment.
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.
| end | ||
|
|
||
| def ip_exists(ip, cidr, group_name) | ||
| cidr_key = @ip_cache[group_name.to_sym][cidr.to_sym]&.to_s |
There was a problem hiding this comment.
Why &.to_s? Would it be better to use IPAddr instance and check if the IP is present in the subnet?
There was a problem hiding this comment.
We are checking if the IP is in the cache, not in the subnet
theforeman-bot
added
Needs re-review
Waiting on contributor
and removed
Waiting on contributor
labels
|
Pushed most of these changes, and still working through the last few. I will squash the commits once everything is in alignment |
|
I have no further comments, @ekohl ? |
|
I think this is close to merging, please rebase and we can move forward with this. |
|
@lzap Almost finished with most of the recommended changes. Will try to rebase and push at some point next week |
38a0532 to
60bad6e
Compare
|
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! |
60bad6e to
8172241
Compare
|
@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! |
8172241 to
d60bbbe
Compare
|
@ekohl - Changes are done and tested. Sorry I squashed the commit already but probably should have done that after your review |
|
@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 |
|
@ekohl Where are things regarding the integration of External IPAM? It would be great to finally have this integrated. |
|
@ekohl I ran across this PR while looking to implement a connection to our IPAM with smart proxies. This is pretty old, so I'm not sure if it's mergeable but it would be very nice to have this feature for our proxies. |
| err = [] | ||
| required_params.each do |param| | ||
| unless params[param.to_sym] | ||
| err.push errors[param.to_sym] |
There was a problem hiding this comment.
errors should be ERRORS here — there is no errors method in scope, so this will raise NoMethodError when validation fails on a missing parameter.
| ip_cache.add(group, cidr, new_ip, mac) | ||
| end | ||
|
|
||
| halt 404, { error: "No free addresses found in subnet #{cidr}. Some available ip's may be cached. Try again in #{@ip_cache.cleanup_interval} seconds after cache is cleared." }.to_json unless usable_ip(next_ip, cidr) |
There was a problem hiding this comment.
Should be ip_cache.cleanup_interval — ip_cache is the method parameter here, not an instance variable. This will raise when the usable-IP check fails.
| end | ||
|
|
||
| def get_request_group(params) | ||
| halt 500, { error: errors[:groups_not_supported] }.to_json if params[:group] && !provider.groups_supported? |
There was a problem hiding this comment.
Same as ipam_validator.rb: errors should be ERRORS.
| extend Proxy::Ipam::DependencyInjection | ||
| include ::Proxy::Log | ||
| include ::Proxy::Validations | ||
| helpers ::Proxy::Helpers |
There was a problem hiding this comment.
Other API modules (DHCP, DNS, Realm, etc.) call authorize_with_trusted_hosts and authorize_with_ssl_client here. This module can allocate/reserve/delete IPs in external systems and should do the same.
| halt 400, { error: e.to_s }.to_json | ||
| rescue RuntimeError => e | ||
| logger.exception(ERRORS[:runtime_error], e) | ||
| halt 500, { error: e.to_s }.to_json |
There was a problem hiding this comment.
Returning e.to_s to the client can leak internal/upstream error details. Prefer a generic message in the response and use logger.exception for the real error. Same pattern is repeated on every route in this file.
| rescue RuntimeError => e | ||
| logger.exception(ERRORS[:runtime_error], e) | ||
| halt 500, { error: e.to_s }.to_json | ||
| rescue Errno::ECONNREFUSED, Errno::ECONNRESET |
There was a problem hiding this comment.
e is not bound in this rescue clause — needs rescue Errno::ECONNREFUSED, Errno::ECONNRESET => e. Repeated on every route.
| raise ERRORS[:no_subnet] if subnet.nil? | ||
| response = @api_resource.get("subnets/#{subnet[:id]}/first_free/") | ||
| json_body = JSON.parse(response.body) | ||
| return { error: json_body['message'] } if json_body['message'] |
There was a problem hiding this comment.
Returning { error: ... } here is not handled by the API layer (it only checks for nil), so phpIPAM errors end up as {"data":{"error":"..."}} with HTTP 200. Raise or return nil and let the API map it to an appropriate error response.
| end | ||
|
|
||
| def add_ip_to_subnet(ip, params) | ||
| data = { subnetId: params[:subnet_id], ip: ip, description: 'Address auto added by Foreman' } |
There was a problem hiding this comment.
No hostname is passed when adding an address. Foreman typically has the FQDN available at orchestration time — without it, entries show up unnamed in phpIPAM (see grizzthedj/smart_proxy_ipam#61).
| end | ||
|
|
||
| def authenticated? | ||
| !@token.nil? |
There was a problem hiding this comment.
authenticated? only checks that the token is non-nil; it never validates the token against the Netbox API. A bad token will only fail on the first real request.
| private | ||
|
|
||
| def request(request, uri) | ||
| Net::HTTP.start(uri.hostname, uri.port, use_ssl: uri.scheme == 'https') do |http| |
There was a problem hiding this comment.
No read/open timeout, no TLS options for custom CAs, and no HTTP status check before callers JSON.parse the body. A 401/500 HTML response will surface as a parse error rather than a clear auth/server failure.
| # Class to handle authentication and HTTP transactions with External IPAM providers | ||
| class ApiResource | ||
| include ::Proxy::Log | ||
| include Proxy::Ipam::IpamHelper |
There was a problem hiding this comment.
ApiResource probably does not need IpamHelper — that pulls in Sinatra halt semantics and the heavy externalipam/externalipam require chain into a plain HTTP client class.
| subnet_hash&.any? { |mac, cached_ip| cached_ip[:ip] == ip } | ||
| end | ||
|
|
||
| def ip_expired?(group_name, cidr, ip) |
There was a problem hiding this comment.
ip_expired? calls ip_exists? first, then repeats the same cache scan. Callers always use both together — consider a single lookup or an include_expired flag to avoid the double walk.
| def initialize | ||
| @m = Monitor.new | ||
| @ip_cache = {'': {}} | ||
| start_cleanup_task |
There was a problem hiding this comment.
The cleanup TimerTask starts in initialize via the Singleton, so it runs even when the externalipam module is disabled. Consider starting it lazily on first use, and add a stop hook like Proxy::DHCP::FreeIps.
| @timer_task.execute | ||
| end | ||
|
|
||
| # @ip_cache structure |
There was a problem hiding this comment.
MAC addresses are used as hash keys without normalizing case/format. 00:0A:95:... and 00:0a:95:... would be treated as different cache entries and could get different suggested IPs.
| raise Proxy::Validations::Error, err unless err.empty? | ||
| end | ||
|
|
||
| # def validate_cidr!(address, prefix) |
There was a problem hiding this comment.
Commented-out dead code — please remove before merge.
| @provider ||= | ||
| begin | ||
| unless client.authenticated? | ||
| halt 500, { error: 'Invalid credentials for External IPAM' }.to_json |
There was a problem hiding this comment.
Invalid credentials returns HTTP 500. Foreman often uses 422 for expected client/config errors rather than 500.
| # Test IPAM provider | ||
| class ExternalIpamTestProvider | ||
| def get_next_ip(mac, cidr, group_name) | ||
| { data: "192.0.2.1" } |
There was a problem hiding this comment.
The test provider returns { data: "192.0.2.1" } but the API wraps the provider result again as { data: next_ip }, so the real response is double-wrapped. Mocks should return plain values (e.g. "192.0.2.1") like the real providers do.
| @@ -0,0 +1,113 @@ | |||
| require 'proxy/validations' | |||
| require 'externalipam/externalipam' | |||
There was a problem hiding this comment.
This require pulls in the full plugin chain (externalipam/externalipam → all providers). That creates a circular/heavy dependency for a helper module — consider moving shared constants (e.g. ERRORS) to a smaller file.
|
Hello, I joined the team back recently and I can help to push this forward. This will need a rebase for sure. The biggest problem is that every other Smart Proxy API module uses trusted-host and SSL client verification: helpers ::Proxy::Helpers
authorize_with_trusted_hosts
authorize_with_ssl_clientPlease extend tests for providers, cache, auth, integration and IPv6. Testing is really slim for such a feature. |
7 participants
@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:
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
rkerberosgem.Looking forward to feedback!