openstack/puppet-keystone
Code Issues Proposed changes

Refactor of keystone_domain provider.

Browse Source 
This remove the @@default_domain_id from the code.  It is no longer
necessary to have a global class variable sharing this id.  All is now
encapsulated inside the keystone_domain class.

The change come mainly from the new policy regarding default domain[1]
which simplifies the logic, and the code.

[1] https://etherpad.openstack.org/p/keystone_no_domain

Change-Id: I71ab37165db6b0fe8472e7dfc8abcf72e0caac4a
This commit is contained in:
Sofer Athlan-Guyot 2015-11-03 17:27:04 +01:00 committed by Gilles Dubreuil
parent 4c6bd18b6a
commit 5167a2c804
5 changed files with 73 additions and 106 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

15
lib/puppet/provider/keystone.rb
View File

@ -10,7 +10,6 @@ class Puppet::Provider::Keystone < Puppet::Provider::Openstack
INI_FILENAME = '/etc/keystone/keystone.conf'
@@default_domain_id = nil
@@default_domain = nil
def self.admin_endpoint
@ -33,20 +32,6 @@ class Puppet::Provider::Keystone < Puppet::Provider::Openstack
end
end
def self.default_domain_id
if @@default_domain_id
@@default_domain_id
elsif keystone_file and keystone_file['identity'] and keystone_file['identity']['default_domain_id']
keystone_file['identity']['default_domain_id'].strip
else
'default'
end
end
def self.default_domain_id=(id)
@@default_domain_id = id
end
def self.resource_to_name(domain, name, check_for_default=true)
raise Puppet::Error, "Domain cannot be nil for project '#{name}'. " \
'Please report a bug.' if domain.nil?

56
lib/puppet/provider/keystone_domain/openstack.rb
View File

@ -9,6 +9,7 @@ Puppet::Type.type(:keystone_domain).provide(
desc 'Provider that manages keystone domains'
@credentials = Puppet::Provider::Openstack::CredentialsV3.new
@current_default_domain_id = nil
def initialize(value={})
super(value)
@ -46,6 +47,8 @@ Puppet::Type.type(:keystone_domain).provide(
@property_hash.clear
end
mk_resource_methods
def enabled=(value)
@property_flush[:enabled] = value
end
@ -58,14 +61,6 @@ Puppet::Type.type(:keystone_domain).provide(
@property_flush[:description] = value
end
def description
@property_hash[:description]
end
def id
@property_hash[:id]
end
def is_default
bool_to_sym(@property_hash[:is_default])
end
@ -75,33 +70,20 @@ Puppet::Type.type(:keystone_domain).provide(
end
def ensure_default_domain(create, destroy=false, value=nil)
if !self.class.keystone_file
return
end
changed = false
curid = self.class.default_domain_id
newid = id
curid = self.class.current_default_domain_id
default = (is_default == :true)
entry = keystone_conf_default_domain_id_entry(id)
if (default && create) || (!default && (value == :true))
# new default domain, or making existing domain the default domain
if curid != newid
self.class.keystone_file['identity']['default_domain_id'] = newid
changed = true
if curid != id
entry.create
end
elsif (default && destroy) || (default && (value == :false))
# removing default domain, or making this domain not the default
if curid == newid
# can't delete from inifile, so just reset to default 'default'
self.class.keystone_file['identity']['default_domain_id'] = 'default'
changed = true
newid = 'default'
if curid == id
entry.destroy
end
end
if changed
self.class.keystone_file.store
self.class.default_domain_id = newid
debug("The default_domain_id was changed from #{curid} to #{newid}")
end
end
def self.instances
@ -112,7 +94,7 @@ Puppet::Type.type(:keystone_domain).provide(
:enabled => domain[:enabled].downcase.chomp == 'true' ? true : false,
:description => domain[:description],
:id => domain[:id],
:is_default => domain[:id] == default_domain_id
:is_default => domain[:id] == current_default_domain_id
)
end
end
@ -141,4 +123,22 @@ Puppet::Type.type(:keystone_domain).provide(
@property_flush.clear
end
end
private
def self.current_default_domain_id
return @current_default_domain_id unless @current_default_domain_id.nil?
current = Puppet::Resource.indirection
.find('Keystone_config/identity/default_domain_id')[:value]
current = nil if current == :absent
@current_default_domain_id = current
end
def keystone_conf_default_domain_id_entry(newid)
conf = Puppet::Type::Keystone_config
.new(:title => 'identity/default_domain_id', :value => newid)
entry = Puppet::Type.type(:keystone_config).provider(:ini_setting)
.new(conf)
entry
end
end

1
spec/spec_helper.rb
View File

@ -18,7 +18,6 @@ def setup_provider_tests
@tenant_hash = nil
@admin_token = nil
@keystone_file = nil
Puppet::Provider::Keystone.default_domain_id = nil
Puppet::Provider::Keystone.default_domain = nil
@domain_hash = nil
end

105
spec/unit/provider/keystone_domain/openstack_spec.rb
View File

@ -52,13 +52,10 @@ describe provider_class do
describe '#create' do
it 'creates a domain' do
# keystone.conf
File.expects(:exists?).returns(true)
kcmock = {
'identity' => {'default_domain_id' => ' default'}
}
Puppet::Util::IniConfig::File.expects(:new).returns(kcmock)
kcmock.expects(:read).with('/etc/keystone/keystone.conf')
provider_class.expects(:current_default_domain_id).returns('default')
entry = mock
provider.expects(:keystone_conf_default_domain_id_entry).returns(entry)
provider.class.expects(:openstack)
.with('domain', 'create', '--format', 'shell', ['foo', '--enable', '--description', 'foo'])
.returns('id="1cb05cfed7c24279be884ba4f6520262"
@ -74,17 +71,13 @@ enabled=True
describe '#destroy' do
it 'destroys a domain' do
provider.instance_variable_get('@property_hash')[:id] = 'my-domainid'
# keystone.conf
File.expects(:exists?).returns(true)
kcmock = {
'identity' => {'default_domain_id' => ' default'}
}
Puppet::Util::IniConfig::File.expects(:new).returns(kcmock)
kcmock.expects(:read).with('/etc/keystone/keystone.conf')
provider.class.expects(:openstack)
provider_class.expects(:current_default_domain_id).returns('default')
entry = mock
provider.expects(:keystone_conf_default_domain_id_entry).returns(entry)
provider_class.expects(:openstack)
.with('domain', 'set', ['foo', '--disable'])
provider.class.expects(:openstack)
provider_class.expects(:openstack)
.with('domain', 'delete', 'foo')
provider.destroy
expect(provider.exists?).to be_falsey
@ -94,11 +87,12 @@ enabled=True
describe '#instances' do
it 'finds every domain' do
provider.class.expects(:openstack)
provider_class.expects(:openstack)
.with('domain', 'list', '--quiet', '--format', 'csv', [])
.returns('"ID","Name","Description","Enabled"
"1cb05cfed7c24279be884ba4f6520262","foo","foo",True
')
provider_class.expects(:current_default_domain_id).returns('default')
instances = provider_class.instances
expect(instances.count).to eq(1)
end
@ -107,60 +101,50 @@ enabled=True
describe '#create default' do
let(:domain_attrs) do
{
:name => 'foo',
:description => 'foo',
:name => 'new_default',
:description => 'New default domain.',
:ensure => 'present',
:enabled => 'True',
:is_default => 'True',
:is_default => 'True'
}
end
it 'creates a default domain' do
File.expects(:exists?).twice.returns(true)
mock = {
'identity' => {'default_domain_id' => ' default'}
}
Puppet::Util::IniConfig::File.expects(:new).twice.returns(mock)
mock.expects(:read).twice.with('/etc/keystone/keystone.conf')
mock.expects(:store)
provider.class.expects(:openstack)
.with('domain', 'create', '--format', 'shell', ['foo', '--enable', '--description', 'foo'])
.returns('id="1cb05cfed7c24279be884ba4f6520262"
context 'default_domain_id defined in keystone.conf' do
it 'creates a default domain' do
provider_class.expects(:openstack)
.with('domain', 'create', '--format', 'shell',
['new_default', '--enable', '--description', 'New default domain.'])
.returns('id="1cb05cfed7c24279be884ba4f6520262"
name="foo"
description="foo"
enabled=True
')
expect(provider.class.default_domain_id).to eq('default')
expect(another_class.default_domain_id).to eq('default')
provider.create
expect(provider.exists?).to be_truthy
expect(mock['identity']['default_domain_id']).to eq('1cb05cfed7c24279be884ba4f6520262')
expect(provider.class.default_domain_id).to eq('1cb05cfed7c24279be884ba4f6520262')
expect(another_class.default_domain_id).to eq('1cb05cfed7c24279be884ba4f6520262')
provider_class.expects(:current_default_domain_id).returns('default')
entry = mock
provider.expects(:keystone_conf_default_domain_id_entry).returns(entry)
entry.expects(:create).returns(nil)
provider.create
expect(provider.exists?).to be_truthy
end
end
end
describe '#destroy default' do
it 'destroys a default domain' do
provider.instance_variable_get('@property_hash')[:is_default] = true
provider.instance_variable_get('@property_hash')[:id] = 'my-domainid'
# keystone.conf
File.expects(:exists?).returns(true)
kcmock = {
'identity' => {'default_domain_id' => ' my-domainid'}
}
Puppet::Util::IniConfig::File.expects(:new).returns(kcmock)
kcmock.expects(:read).with('/etc/keystone/keystone.conf')
kcmock.expects(:store)
provider_class.expects(:current_default_domain_id).returns('my-domainid')
entry = mock
provider.expects(:keystone_conf_default_domain_id_entry).returns(entry)
provider.expects(:is_default).returns(:true)
provider.expects(:id).twice.returns('my-domainid')
provider.class.expects(:openstack)
.with('domain', 'set', ['foo', '--disable'])
provider.class.expects(:openstack)
.with('domain', 'delete', 'foo')
entry.expects(:destroy)
provider.destroy
expect(provider.exists?).to be_falsey
expect(kcmock['identity']['default_domain_id']).to eq('default')
expect(provider.class.default_domain_id).to eq('default')
expect(another_class.default_domain_id).to eq('default')
end
end
@ -171,7 +155,7 @@ enabled=True
:description => 'new description',
:ensure => 'present',
:enabled => 'True',
:is_default => 'True',
:is_default => 'False'
}
end
@ -183,14 +167,13 @@ enabled=True
end
it 'changes is_default' do
# keystone.conf
File.expects(:exists?).returns(true)
kcmock = {
'identity' => {'default_domain_id' => ' my-domainid'}
}
Puppet::Util::IniConfig::File.expects(:new).returns(kcmock)
kcmock.expects(:read).with('/etc/keystone/keystone.conf')
provider.is_default=(true)
provider_class.expects(:current_default_domain_id).returns('previous_default_domain-id')
entry = mock
provider.expects(:keystone_conf_default_domain_id_entry).returns(entry)
provider.expects(:id).twice.returns('current_default_domain')
entry.expects(:create)
provider.is_default=(:true)
provider.flush
end
end

2
spec/unit/provider/keystone_user/openstack_spec.rb
View File

@ -76,7 +76,7 @@ username="user1"
describe '#exists' do
context 'when user does not exist' do
subject(:response) do
response = provider.exists?
provider.exists?
end
it { is_expected.to be_falsey }