Integration for FreeIPA #91
Conversation
Also added the request payload to the debug output.
Debugging can now be activated by passing an additional boolean to the PalletJack::Ipa::Command function. Disabled by default.
This script will create host accounts for any host missing an account and save the One-Time-Password to the pallet, so that the machine can be enrolled. It will also read a list of host groups from system.ipa.hostgroups and add the new host object to those groups.
This tool does no longer have to know about all possible values of the ipa pallet.
| require 'json' | ||
|
|
||
| class PalletJack | ||
| class Ipa |
There was a problem hiding this comment.
Since PalletJack::Ipa is only used for namespace, it should be a module, not a class.
There was a problem hiding this comment.
Is IPA an acronym, or is Ipa a name?
| @debug = debug | ||
| if @@curl == nil then | ||
| @@curl = Curl::Easy.new | ||
| end |
There was a problem hiding this comment.
The Ruby idiom for initializing something is: @@curl ||= Curl::Easy.new
| if @@curl == nil then | ||
| @@curl = Curl::Easy.new | ||
| end | ||
| if @@ipa_url == nil then |
| self._make_request() | ||
| end | ||
|
|
||
| def _set_server # :nodoc: |
| @@ipa_url = "https://" + target_server + "/ipa" | ||
| end | ||
|
|
||
| def _make_request # :nodoc: |
| resolver.each_resource("_ldap._tcp." + domainname_base, Resolv::DNS::Resource::IN::SRV) do |srv| | ||
| if srv.priority < lowest_prio then | ||
| target_server = srv.target.to_s | ||
| end |
There was a problem hiding this comment.
lowest_prio is never updated.
There was a problem hiding this comment.
#TODO: priority sorted list of servers, for proper redundancy
| @@ipa_url = nil | ||
|
|
||
| # Perform a HTTP request against the JSON-RPC endpoint of a IPA server. | ||
| def initialize(payload, debug = false) |
There was a problem hiding this comment.
The debug feature should be activated by setting a class attribute instead of having it as a visible part of the object creation API.
def self.debug=(maybe)
@@debug = maybe
end
def self.debug
@@debug ||= false
end
private
def debug?
self.class.debug
end
| # Examples: | ||
| # <tt>PalletJack::Ipa::Command.new("host_add", "new_system", { ip_address: "192.168.13.37" })</tt> | ||
| # <tt>PalletJack::Ipa::Command.new("host_find")</tt> | ||
| def initialize(method, name=nil, params={}, debug = false) |
There was a problem hiding this comment.
The debug feature should be activated by setting a class attribute instead of having it as a visible part of the object creation API.
There was a problem hiding this comment.
def initialize(method, *args, **kwargs)
will automatically collect all leftover positional arguments into args, and all keyword arguments into kwargs
| # === Attributes | ||
| # | ||
| # * +method+ - The JSON-RPC Method to call | ||
| # * +name+ - The positional parameters to the method. Can be an array or string. |
There was a problem hiding this comment.
name is a very confusing name for something that should contain positional arguments. Maybe args instead?
| # | ||
| # * +method+ - The JSON-RPC Method to call | ||
| # * +name+ - The positional parameters to the method. Can be an array or string. | ||
| # * +params+ - Named parameters to the method. |
There was a problem hiding this comment.
params is a confusing name for something that should contain keyword arguments. Maybe kwargs instead?
| # if they are registered on the IPA server. The missing hosts will be added. | ||
| # A random password will be generated by IPA and stored in the pallet. | ||
| # | ||
| # If the IPA server is configured as authorative DNS server, records will |
There was a problem hiding this comment.
"authoritative", not "authoratative".
| next unless system['system.role'].include? 'ipa-client' # Only ipa clients | ||
|
|
||
| # Generate a random password | ||
| add_host_params = { 'random' => true } |
There was a problem hiding this comment.
Delete trailing whitespace.
| # Find a interface in the same domain as the host. | ||
| if interface['net.dns.domain'] == system['net.dns.domain'] then | ||
| ip_address = interface['net.ipv4.address'] | ||
| end |
There was a problem hiding this comment.
What does this do if a system has several interfaces in the same domain? Would that be by design? Or is such a warehouse not supported? In any case, it should be documented.
There was a problem hiding this comment.
At the moment only the first IP address of the systems domain will get used. This may be a problem, but on the other hand if we are going to use IPA as authoritative DNS we probably want to create a separate tool to do a more complete sync of DNS records.
|
|
||
| ip_address = nil | ||
| system.children(kind:'ipv4_interface') do |interface| | ||
| # Find a interface in the same domain as the host. |
There was a problem hiding this comment.
"an" interface.
Please choose one consistent style for comments and stick to it. I personally omit the trailing period for comments that are sentence fragments or one sentence that fits on a single line, and include it for longer comments.
| unless add_host.error? then | ||
|
|
||
| # Read | ||
| pallet = system['system.ipa'] |
There was a problem hiding this comment.
Consider, instead:
pallet = system['system.ipa'] || {}which would allow you to drop the if pallet == nil statement.
| host_find = PalletJack::Ipa::Command.new("host_find") | ||
|
|
||
| if host_find.error? then | ||
| puts "ERROR: " + host_find.error_name + " " + host_find.error_message |
There was a problem hiding this comment.
Non-interpolated string literals should be written with single quotes. Someone (probably me) should start a style guide stating such things.
I'm not sure if I prefer embedded interpolations or string additions for building longer strings from fragments. Let's compare:
puts 'ERROR: ' + host_find.error_name + ' ' + host_find.error_messageputs "ERROR: #{host_find.error_name} #{host_find.error_message}"| end | ||
|
|
||
| jack.each(kind: 'system') do |system| | ||
| next if ipa_hosts.include? system['net.dns.fqdn'] # Skip existing hosts |
There was a problem hiding this comment.
Is there never a reason to re-create a host object? Or is that such a rare and invasive procedure that we're OK with the sysadmin having to manually delete it first in that case?
There was a problem hiding this comment.
I think that would depend on how we are going to handle provisioning / unprovisioning in general. Right now there will still be a handful of more-or-less manual steps to setup a new system, mostly running scripts. I imagine that we would want to wrap the tools in some kind of provisioning script, where we could delete a host object and creating a new one prior to re-installation.
Maybe this whole approach is wrong, and we should focus on a more one-shot kind of tool, more like create_system.
| end | ||
| else | ||
| puts " ! ERROR: Adding host " + system['net.dns.name'] | ||
| puts " ! " + add_host.error_name + " " + add_host.error_message |
There was a problem hiding this comment.
We need to sit down and think about what the interaction model for the tools should look like. How and when are they run, should they output anything when successful, where should messages be displayed, should they exit on first error or keep going, etc. etc.
| unless add_host.error? then | ||
|
|
||
| # Read | ||
| pallet = system['system.ipa'] |
There was a problem hiding this comment.
I dislike the name pallet for a variable holding a subtree of the key space in a pallet. Consider ipa instead, since that's the top of the subtree.
| # new(method [,pos1 [,params]]) -> command | ||
| # | ||
| # This class will take a method name, positional parameters and named | ||
| # parameters and make a JSON-RPC request against a IPA server. |
There was a problem hiding this comment.
Having Command#new actually send the commands over the network makes me confused when reading the code. Then again, I'm not sure how a better design would look.
Maybe instead of having the Ipa::Request class implicitly keep global state, have a class Ipa::Session that explicitly keeps global state, and then have a method Ipa::Session#send_command that takes the same arguments as Command#new does currently and returns a Response object?
* Changed some variable naming (removed sense-less _ prefix) * Fixed variable initialization * Changed some if / unless cases to better match the context. * Added global debug handler to the class instead of args to the initailize method. * Reworked explicit if cases and method returns where value is already implicitly correct.
* Resolution now based on system search domain. * Fixed server priority being ignored.
|
I found another existing but fairly new gem for interacting with the FreeIPA API: https://github.com/m4ce/ipa-ruby/ . |
Sounds like a great plan. No need to duplicate efforts. Fork it if we need more functionality, assuming there are no license problems. |
It's 80 lines, including whitespace and comments. I'm not sure how much effort we'd save. But go for it if you think it's worth it.
It's Apache 2.0. Not sure how that interacts with MIT. |
* Removed the PalletJack::Ipa namespace. Will use ruby-ipa gem instead. * The command can now be used to sync a specific system, or every system in the warehouse if not specified. * The --reinstall flag can be used to re-create a host object. * Hostgroups will be synchronized even if the host object already exists. My intention is to implement a better DNS sync for a host, but since that would require upstream improvements of the ruby-ipa gem it will have to wait a while. NOTE: This is currently untested, bugs are expected.
Known issue: Adding systems without a defined IP addres does not work.
| ipa.host_del(hostname: system['net.dns.fqdn'], params { 'update_dns' => true } ) | ||
| del_host = ipa.host_del(hostname: system['net.dns.fqdn'], params: { 'updatedns' => true } ) | ||
| puts " * Deleted host object" | ||
| if del_host['error'] then |
There was a problem hiding this comment.
Is it really true that the host object is deleted regardless of any errors?
Host deletion errors should now be handeled correctly When processing all systems, processing will now continue for the rest of the systems when encountering an error for one system.
Also changed variable naming, ipa.yaml is a box not a pallet.
| def parse_options(opts) | ||
| opts.banner = | ||
| "Usage: #{$PROGRAM_NAME} -w <warehouse> -i <IPA server> [ -s <system> ] [--reinstall] | ||
|
|
There was a problem hiding this comment.
Usage example(s) should make it obvious that the --reinstall option requires the --system option.
| options[:system] = system | ||
| } | ||
| opts.on('-r', '--reinstall', 'Reinstall the system') {|reinstall| | ||
| options[:reinstall] = reinstall |
There was a problem hiding this comment.
Add required_option :system here to make the option parser (helpers) validate all option requirements.
| unless options[:system] then | ||
| abort("--reinstall specified when operating on all systems. This would cause the domain to break and is not allowed.") | ||
| end | ||
| end |
There was a problem hiding this comment.
This should be handled by parse_options, not main processing logic.
State that --system is required when passing the --reinstall option. Updated description and casing of documentation to match other tools.
| raise ArgumentError | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Add required_option :system inside the opts.on block for --reinstall instead of reimplementing required_option here.
|
We have stalled the decision to merge this or not for too long, will need new reviews if we decide to go forward and integrate the FreeIPA support. |
In saab-simc-admin#91 @albgus was working on a tool for adding FreeIPA hosts for Pallet Jack systems. However, he left the project before the tool vould be merged. Import his latest version as a base for future work.
In saab-simc-admin#91 @albgus was working on a tool for adding FreeIPA hosts for Pallet Jack systems. However, he left the project before the tool vould be merged. Import his latest version as a base for future work.
In saab-simc-admin#91 @albgus was working on a tool for adding FreeIPA hosts for Pallet Jack systems. However, he left the project before the tool vould be merged. Import his latest version as a base for future work.
Add an library to simplify talking to the FreeIPA JSON-RPC API.
Also Includes the palletjack2ipahost tool which can pre-create host objects in the domain and currently saves the temporary password in the warehouse awaiting enrolment (subject to change if we figure out a better way to handle this).