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.

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

fixes #1809 - Foreman Realm Integration #1061

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
2 participants
@stbenjam
Copy link
Member

commented Nov 27, 2013

This is the realm integration bits for core.

return unless realm?
@realm_api = ProxyAPI::Realm.new :url => realm.smart_proxy.url
rescue => e
failure _("Failed to initialize the Realm proxy: %s") % e

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

Nitpick, lower-case the "Realm" in these errors and log messages throughout this file.

def setRealm
logger.info "Add Realm registration for #{name}"
result = @realm_api.create :fqdn => name
host_parameters << HostParameter.new(:name => "otp_password", :value => result["otp_password"], :nested => false) if result.has_key? "otp_password"

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

I think we could go one better than this, and store it in a column on the host, then make it accessible at @host.otp or realm_otp in the templates.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Nov 28, 2013

Author Member

Ok, that was something I wanted to ask about, whether a parameter or dedicated field was better here.


def queue_realm_destroy
return unless realm? and errors.empty?
queue.create(:name => _("Delete Realm registration for %s") % self, :priority => 50,

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

Nitpick, "registration" versus "entry" above. Neither sounds exactly right to me, but I slightly prefer entry as it's a host/computer object.

@@ -18,6 +18,7 @@ def self.load_defaults
self.set('ssl_ca_file', N_( "SSL CA file that Foreman will use to communicate with its proxies"), ssl_ca_file),
self.set('ssl_priv_key', N_("SSL Private Key file that Foreman will use to communicate with its proxies"), ssl_priv_key),
self.set('manage_puppetca', N_("Should Foreman automate certificate signing upon provisioning new host"), true),
self.set('manage_realm', N_("Should Foreman automatically register new hosts to a configured Realm"), true),

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

I'm unsure if this is needed, since we could just hide the realms on the UI based on whether there are any or not.

def create_models
# If we find no features, self.features is an array and this breaks
if self.features.respond_to? :find_by_name and self.features.find_by_name("Realm")
realm_name = ProxyAPI::Realm.new(:url => self.url).about["name"]

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

I'm think this is too magical, since we normally get users to manage the resources (domains, subnets etc) in Foreman via the UI and API. Among the problems we'll have would be things like permissions and locations/orgs, so it's better to be explicit and add new UIs to manage realms.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Nov 28, 2013

Author Member

As it is now, each Smart Proxy can have one realm -- so when you register the Smart Proxy, I know about the realm because I query it and create the model. Does that design not make sense here?

What fields should a user be able to change?

@@ -29,6 +29,10 @@
{:label => _("Environment"), :onchange => 'update_puppetclasses(this);', :"data-url" => environment_selected_hostgroups_path} %>

<%= puppet_master_fields f %>

<%= select_f f, :realm_id, Realm.all, :id, :to_label, {:include_blank => true},
{:label => _("Default Realm")} if Setting[:manage_realm] %>

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

Maybe add this to the network tab? Why "Default"? Lower-case "R".

This comment has been minimized.

Copy link
@stbenjam

stbenjam Nov 28, 2013

Author Member

At least to me, it looks awkward by itself at the top of the Network tab, and it's not really a component of the primary interface.

But maybe move to a realm tab?

Both FreeIPA and adcli support adding service principals, so perhaps under a realm tab is where you can configure which realm to register to, and which service principals to create.

Good point about Default, it's implied here that these are the hostgroup defaults.

@@ -62,6 +62,10 @@
{:onchange => 'update_puppetclasses(this)', :'data-url' => hostgroup_or_environment_selected_hosts_path,
:'data-host-id' => @host.id, :help_inline => :indicator} %>
<%= puppet_master_fields f %>

<%= select_f f, :realm_id, Realm.all, :id, :to_label, { :include_blank => true },
{:disabled => @host.new_record? ? false : true, :help_inline => :indicator} if Setting[:manage_realm] %>

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

I'm not sure if we should disable this?

This comment has been minimized.

Copy link
@stbenjam

stbenjam Nov 28, 2013

Author Member

If the user changes the realm, then I suppose we can create a new OTP and set it and delete the host from the old realm, but of course, the host isn't going to actually move to the new realm unless the user is doing it with Puppet, rebuilding, or manually.

drop_table :realms
remove_column :hosts, :realm_id
remove_column :hostgroups, :realm_id
remove_column :smart_proxies, :realm_id

This comment has been minimized.

Copy link
@domcleal

domcleal Nov 28, 2013

Contributor

unused?

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Nov 28, 2013

My biggest question about this is implicit versus explicit management of the realms themselves in Foreman, and think we need to have it explicit - i.e. a UI, API etc for managing them.

@stbenjam

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2014

@domcleal This and theforeman/smart-proxy#115 are ready to have another review, when you have time. Now with GUI and no magic.

@stbenjam

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2014

Now with API, tests, and authorization!
The authorization stuff isn't working correctly, and I'm still scratching my head over what I broke. 👎
https://gist.github.com/stbenjam/931e49b02245ff22da51

Any ideas? <-- That was fixed by #1272

@stbenjam

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2014

Authorization works, and tests are passing!

@@ -0,0 +1,22 @@
class CreateRealms < ActiveRecord::Migration
def self.up
Feature.find_or_create_by_name("Realm")

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

This should be in db/seeds.d/11-smart_proxy_features.rb.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 15, 2014

Author Member

This doesn't get run during rake db:migrate, right? How do users get the new feature during an upgrade?

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 17, 2014

Contributor

We run db:seed as well as migrate during upgrades now.


create_table :realms do |t|
t.string :name, :default => "", :null => false
t.integer :realm_proxy_id

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Nitpick, usually the "proxy" word is left out of these column names.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 12, 2014

Author Member

I saw, but there's exceptions like puppetca_proxy_id. In a section of code you might see realm.realm_proxy_id and host.realm_id together, and it seemed clearer to me.

Should I change it to realm_id?

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Ok, that reasoning's fine!


<div class="tab-pane active" id="primary">
<%= text_f f, :name, :help_inline => _("The realm name") %>
<%= select_f f, :realm_proxy_id, Feature.find_by_name("Realm").smart_proxies, :id, :name,

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

The field name should be :realm_proxy so errors are shown correctly.

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Ah, I see now why that doesn't work. Ignore this comment, should be okay if you can remove include_blank as then you can't hit the presence validation error.

<div class="tab-pane active" id="primary">
<%= text_f f, :name, :help_inline => _("The realm name") %>
<%= select_f f, :realm_proxy_id, Feature.find_by_name("Realm").smart_proxies, :id, :name,
{:include_blank => _("None")}, {:label => _("Realm proxy"), :help_inline => _("Realm proxy to use within this realm")}

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

The model enforces presence of a realm proxy, so include_blank should be false (or unset?) here.

# [+key+] : String containing the hostname
# Returns : Boolean status
def delete key
parse(super("#{key}"))

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

super(key) will suffice

@@ -26,6 +26,7 @@
:disabled => !@hostgroup.new_record?,
:help_inline => :indicator } %>
<%= text_f f, :name %>
<%= select_f f, :realm_id, Realm.with_taxonomy_scope_override(@location,@organization).authorized(:view_realms), :id, :to_label, {:include_blank => true}, {:label => _("Realm")} %>

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

a) use blank_or_inherit_f here for include_blank
b) same for the host form, should we move this to the Network tab? It looks overly prominent.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 12, 2014

Author Member

b) I don't disagree, I'll take another look at moving it.

The problem with the network tab is it says "Primary Interface" -- and realm is in the server context, not the interface.

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Hm yes, good point.

I'm just a bit worried by lengthening the form with fields that aren't directly related to each other. Maybe we need subheadings? (Actually, I notice now that compute profile shouldn't be between the (Puppet) environment and puppet proxy fields.)

@abenari any thoughts?

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 15, 2014

Author Member

I like the multiple subheadings:

screen shot 2014-03-15 at 22 30 51

<div class="tab-content">

<div class="tab-pane active" id="primary">
<%= text_f f, :name, :help_inline => _("The realm name") %>

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Perhaps in help_inline give an upper case example ("Realm name, e.g. EXAMPLE.COM"), or suggest that the user might need the upper case name.

%>
</div>

<% if show_location_tab? %>

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

These have been replaced by a helper now, see domains/_form.html.erb.

api :POST, "/realms/", "Create a realm."
param :realm, Hash, :required => true do
param :name, String, :required => false, :desc => "The realm name"
param :realm_proxy_id, :number, :required => false, :allow_nil => true, :desc => "Proxy to use for this realm"

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

This is required by the model.

end

api :POST, "/realms/", "Create a realm."
param :realm, Hash, :required => true do

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Current style is to use def_param_group so this isn't duplicated between the create + update methods.

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Ignore that, it's just in the v2 controller and you already did.

@@ -0,0 +1,55 @@
module Api
module V1
class RealmsController < V1::BaseController

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

It's up to you, but you can leave out the v1 controller (as it wasn't in this version of the API to begin with) and just implement v2.

return unless realm?
@realm_api = ProxyAPI::Realm.new :url => realm.realm_proxy.url, :realm_name => realm.name
rescue => e
failure _("Failed to initialize the Realm proxy: %s") % e

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Nitpick, lower case 'R'

def set_realm
logger.info "Add realm entry for #{name}"
options = {:hostname => name}
options[:userclass] = hostgroup.label unless hostgroup.nil?

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

s/label/title/ now

Are userclasses handled with "/" in the name?

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 12, 2014

Author Member

Yup! It's allowed by the LDAP attribute.

options = {:hostname => name}
options[:userclass] = hostgroup.label unless hostgroup.nil?
result = @realm_api.create options
self.otp = result["randompassword"] if result.has_key? "randompassword"

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Should we fail if the OTP wasn't present in the response?

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 12, 2014

Author Member

👍

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2014

A few more general comments:

  • can you add functional and integration tests for realm UI pages so we know they render?
  • add a search definition to app/models/concerns/hostext/search.rb for realm
  • needs a rebase

Otherwise looking really good, thanks! I'm going to do some testing today.

:action => [self, :set_realm])
end

def queue_realm_update; end

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

I think we need this implemented, so if you rebuild a host then the OTP is generated again. Perhaps check if otp.blank? ... queue.create ... end

end

def queue_realm_create
queue.create(:name => _("Create realm entry for %s") % self, :priority => 50,

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

nitpick, identation

end

def realm?
name.present? and realm.present?

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Nitpick, &&

@@ -14,6 +14,7 @@ class SmartProxy < ActiveRecord::Base
has_many :hostgroups, :foreign_key => 'puppet_proxy_id'
has_many :puppet_ca_hosts, :class_name => 'Host::Managed', :foreign_key => 'puppet_ca_proxy_id'
has_many :puppet_ca_hostgroups, :class_name => 'Hostgroup', :foreign_key => 'puppet_ca_proxy_id'
has_many :realms, :foreign_key => 'realm_proxy_id', :dependent => :destroy

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

I'm not sure dependent => destroy is suitable here, that'd mean the realm would be destroyed when the proxy is deleted.

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2014

app/views/taxonomies/_form.html.erb needs updating to include realms.

before_filter :find_by_name, :only => %w{edit update destroy}

def index
@realms = Realm.search_for(params[:search], :order => params[:order]).paginate(:page => params[:page])

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Style update, s/Realm/resource_base/

@@ -194,6 +194,7 @@ def show_templates
def overview_fields host
fields = [
[_("Domain"), (link_to(host.domain, hosts_path(:search => "domain = #{host.domain}")) if host.domain)],
[_("Realm"), host.realm],

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 12, 2014

Contributor

Once added to the search, you could link this in the same way as domain.

:action => [self, :set_realm])
end

def queue_realm_update; end

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 15, 2014

Author Member

Will implement, but needs some code changes on the proxy side first to support updating in addition to creation.

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 17, 2014

Contributor

It might not be necessary to support "updates" on the proxy, but only updates of hosts on the Foreman side, then skip if there's an OTP set.

I'm thinking mostly of cases where a host has already been built, the OTP gets cleared and then somebody puts the host back into build mode. This won't activate queue_realm_create because this isn't a new record, but will hit queue_realm_update.

So you could just do:

def queue_realm_update
  if self.otp.blank?
    queue.create( ... create realm entry ....)
  end
end

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 17, 2014

Author Member

I'm not sure I follow. queue.create creates an entry in FreeIPA. That entry is still there, even if the host already used up it's OTP. In an update scenario, we need to tell FreeIPA to create an OTP. Not create a new entry + a new OTP.

If we call queue.create, IPA will come back and say host already exists.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 17, 2014

Author Member

(And calling queue.delete/queue.create will make you lose all the HBAC, sudo, etc rules you've attached to the host manually)

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 17, 2014

Contributor

Ah of course, sorry!

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 17, 2014

Author Member

What's the RESTful thing to do?

  • /realm/example.com/update route
  • Call create with a parameter update=1
  • stick with queue.create and have the proxy automatically choose whether to create or just get a new otp

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 17, 2014

Contributor

The RESTful way is usually a PUT to the resource URL (e.g. PUT /api/hosts/foo.example.com) to update or replace it. If you're just going to regenerate an OTP, then a second route could be used I guess, e.g. POST /api/hosts/foo.example.com/generate_otp.

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 20, 2014

Author Member

So in the end, I'm going to use your first suggestion since it's the simplest. Just committed it. I also check if the hostgroup was changed too.

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2014

What do you think about adding a "type" to the realm model, which is an enum of "FreeIPA", "Active Directory"? It might be useful if we could check the realm type from templates if we come to have more proxy backends like adcli, since the join methods will be different.

@stbenjam

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2014

@domcleal That makes sense for the templates but how would the implementation work? A table and foreign keys or just: TYPES = ["FreeIPA", "Active Directory"]?

Ruby doesn't have proper enums, right?

BTW, should there also be a description field for the realm?

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2014

Indeed, no proper enums so just a column, a constant of possible values and then validator on the model to check it's in them (see the lookup_key or notice models). I wouldn't add a realm description if we've got the name, I think domain is the odd one out there.

<%= select_f f, :realm_type, Realm::TYPES, :to_s, :to_s, { },
{:label => _("Realm type"), :help_inline => _("Type of realm, e.g. FreeIPA")}
%>
<%= select_f f, :realm_proxy_id, Feature.find_by_name("Realm").smart_proxies, :id, :name,

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 20, 2014

Contributor

SmartProxy.realm_proxies

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 20, 2014

Author Member

Does it need to be with taxonomy scope & authorize(:view_smart_proxies) or is that automatic?

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 21, 2014

Contributor

The taxonomy scope is there already (it's in the default_scope defined in SmartProxy), but .authorized is an interesting point since other places in the UI don't do this, which I'm going to call part of http://projects.theforeman.org/issues/4477. So just stick with this for now.

@stbenjam

This comment has been minimized.

Copy link
Member Author

commented Mar 21, 2014

Adding that fixture to taxables_taxonomy fixed the integration test that was failing. So it's ready again for another review, I already implemented TYPES and updates.

included do
after_validation :queue_realm
before_destroy :queue_realm_destroy
after_build :clear_otp

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 27, 2014

Contributor

This isn't saving, I think because it runs from after_commit and so fires too late: https://github.com/theforeman/foreman/blob/develop/app/models/host/managed.rb#L29-L36

I think you'll need to add a before_save hook instead, compare if build? changed and then clear.

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 27, 2014

Contributor

Or actually, just add clear_otp to Host::Managed#built.

class CreateRealms < ActiveRecord::Migration
def self.up
create_table :realms do |t|
t.string :name, :default => "", :null => false

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 28, 2014

Contributor

We should add a unique index too

end

add_column :hosts, :otp, :string unless column_exists? :hosts, :otp
add_column :hosts, :realm_id, :integer unless column_exists? :hosts, :realm_id

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 28, 2014

Contributor

Need to add a foreign key on these (db/migrate/20130908170524_add_keys.rb)

create_table :realms do |t|
t.string :name, :default => "", :null => false
t.string :realm_type
t.integer :realm_proxy_id

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 28, 2014

Contributor

Foreign key here too

This comment has been minimized.

Copy link
@stbenjam

stbenjam Mar 29, 2014

Author Member

Done, db stuff below -- foreign keys look right?

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 31, 2014

Contributor

Yep 👍

# If no OTP is set, then this is probably a rebuild
if self.otp.blank?
logger.info "Setting realm for host #{name}"
set_realm :rebuild => true

This comment has been minimized.

Copy link
@domcleal

domcleal Mar 28, 2014

Contributor

You need to add a self.save! after this (or maybe in the controller), as the OTP isn't getting saved on a rebuild.

@domcleal

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2014

Merged as 77f7015 for Foreman 1.5.0. Great work @stbenjam!

@domcleal domcleal closed this Apr 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.