PR for issue #34, use salt api for minion key accept #43

vkhatri · 2017-02-03T21:24:17Z

No description provided.

shortdudey123

Left quite a few comment. Spec testing needs be be greatly expanded to cover all of the changes

shortdudey123 · 2017-02-03T21:35:44Z

.kitchen.yml

@@ -38,11 +38,16 @@ suites:
    includes:
      - centos-7.2
  - name: salt-api
+    data_bags_path: "test/integration/default/data_bags"


integration/default => fixtures

shortdudey123 · 2017-02-03T21:35:50Z

.kitchen.yml

@@ -38,11 +38,16 @@ suites:
    includes:
      - centos-7.2
  - name: salt-api
+    data_bags_path: "test/integration/default/data_bags"
+    encrypted_data_bag_secret_key_path: "test/integration/default/encrypted_data_bag_secret"


shortdudey123 · 2017-02-03T21:38:28Z

attributes/minion.rb

@@ -24,3 +24,17 @@
  'id' => node.name,
  'grains' => {},
 }
+
+default['salt']['minion']['api'] = {


api => master_api? (since minion doesn't have the concept of an api)

yes, it is better

shortdudey123 · 2017-02-03T21:39:18Z

libraries/helpers.rb

+  begin
+    Chef::Log.info("Connecting to host=#{host}, port=#{port}, use_ssl=#{options['use_ssl']}, verify=#{options['verify']}")
+
+    https = Net::HTTP.new(host, port)


any reason to do this yourself and not use the salt-api gem?

thank you for pointing out, will use salt-api gem

shortdudey123 · 2017-02-03T21:40:48Z

recipes/master.rb

@@ -20,6 +20,15 @@
  action :enable
 end

+user node['salt']['master']['api']['user']['name'] do


This should be left up to a wrapper cookbook. People may want to use existing users and adding it here would cause resource cloning (which will break in April)

I am inclined to make the user setup optional to avoid such an issue rather than left it to a wrapper cookbook. If cookbook requires a user to function, user resource should be included in the cookbook itself with option to enable/disable.

Putting the user resource here assumes that someone doesn't already have the needed user. A lot of people, including me, create service users separate with quite different properties listed. Also, if a user named saltapi is already created somewhere else, this will break in Chef13 unless someone explicitly sets the name even if they don't use the resource.

I could see this staying if it was actually put into an if block instead of using an only_if. Then doing the following

node['salt']['master']['api']['user'].tap{ |x| x.delete('enable') }.each do |config, value| send(config.to_sym, value) unless value.nil? end

Agreed to put user resource in a block if default['salt']['master']['api']['user']['enable'] == true in favor of Chef13.

For scenarios where user already exists, it is still possible to change the user name by setting default['salt']['master']['api']['user']['name']. But, it would not matter if wrapped around an if block.

shortdudey123 · 2017-02-03T21:45:12Z

recipes/master.rb

@@ -87,6 +98,28 @@
  end
 end

+# salt-api default user acl
+# TODO: to be replaced by LWRP `external_auth`
+default_user_acl = {


This should be part of node['salt']['master']['config'] and not broken out into a separate file

yeah, i missed this one

shortdudey123 · 2017-02-03T21:45:44Z

recipes/master.rb

+default_user_acl = {
+  'external_auth' => {
+    'pam' => {
+      'saltapi' => [


user should not be hardcoded here

shortdudey123 · 2017-02-03T21:45:50Z

recipes/master.rb

+  },
+}.to_yaml
+
+file 'default-api-user.conf' do


do you mean file[default-api-user.conf user? I will also add a check to remove this file if api is not enabled.

It is not needed since this partially duplicates what is in the main yaml conf

shortdudey123 · 2017-02-03T21:46:38Z

recipes/minion.rb

@@ -75,3 +76,29 @@
 ohai 'salt' do
  action :nothing
 end if defined?(ChefSpec)
+
+api_password = node['salt']['key_accept_method'] == 'api_key_accept' ? Chef::EncryptedDataBagItem.load(node['salt']['minion']['api']['databag']['name'], node['salt']['minion']['api']['databag']['item'])[node['salt']['minion']['api']['databag']['key']] : nil


This should be put into a helper method

shortdudey123 · 2017-02-03T21:48:57Z

recipes/minion.rb

+  block do
+    salt_accept_key(options)
+  end
+  only_if { node['salt']['key_accept_method'] == 'api_key_accept' && Mixlib::ShellOut.new('salt-call test.ping').run_command.exitstatus != 0 }


if the test.ping fails, then a warning should be output saying the api call to the master is being skipped

vkhatri · 2017-02-04T08:14:05Z

@shortdudey123 Sound reasonable. I will give it one more try.

shortdudey123 · 2017-02-24T22:52:22Z

@vkhatri hows it going on your end with the feedback? anything i can do on my end?

vkhatri · 2017-02-25T09:04:32Z

@shortdudey123 will submit a revised PR over the weekend.

shortdudey123 · 2017-02-27T00:13:03Z

Sounds good

vkhatri · 2017-03-09T12:14:18Z

@shortdudey123 Got all the changes in place, just finishing up specs.

shortdudey123 · 2017-03-10T07:43:49Z

Sounds good :)

vkhatri · 2017-03-12T07:28:24Z

Travis is failing because of an issue with yaml configuration file reported in issue #45 .

https://travis-ci.org/shortdudey123/chef-salt/jobs/210111392#L488

   +---
    +external_auth:
    +  pam:
    +    saltapi: !ruby/array:Chef::Node::ImmutableArray
    +      internal:
    +      - "@wheel"
    +      ivars:
    +        :@__path__: []
    +        :@__root__: 
    +        :@__node__: 
    +        :@__precedence__:

It would be great if you could merge this PR.

shortdudey123 · 2017-03-20T20:47:30Z

recipes/master.rb

 include_recipe 'salt::_setup'

+node.default['salt']['master']['config']['external_auth'] = {
+  'pam' => {
+    node['salt']['master']['api']['user']['name'] => [


need to add .to_s at the end of the node attribute, otherwise it stores a Chef::Node::ImmutableArray instead of a String

shortdudey123 · 2017-03-20T20:48:22Z

The new changes look fine in a quick skim over of them. I will review them in more detail this week.
I added a comment to address the travis failure

shortdudey123#34, use salt api for minion key accept

a79869b

shortdudey123 requested changes Feb 3, 2017

View reviewed changes

applied suggested changes on PR

25ca1b2

shortdudey123 reviewed Mar 20, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PR for issue #34, use salt api for minion key accept #43

PR for issue #34, use salt api for minion key accept #43

vkhatri commented Feb 3, 2017

shortdudey123 left a comment

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 4, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 4, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

shortdudey123 Feb 3, 2017

vkhatri Feb 4, 2017

vkhatri commented Feb 4, 2017

shortdudey123 commented Feb 24, 2017

vkhatri commented Feb 25, 2017

shortdudey123 commented Feb 27, 2017

vkhatri commented Mar 9, 2017

shortdudey123 commented Mar 10, 2017

vkhatri commented Mar 12, 2017

shortdudey123 Mar 20, 2017

shortdudey123 commented Mar 20, 2017

PR for issue #34, use salt api for minion key accept #43

Are you sure you want to change the base?

PR for issue #34, use salt api for minion key accept #43

Conversation

vkhatri commented Feb 3, 2017

shortdudey123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkhatri commented Feb 4, 2017

shortdudey123 commented Feb 24, 2017

vkhatri commented Feb 25, 2017

shortdudey123 commented Feb 27, 2017

vkhatri commented Mar 9, 2017

shortdudey123 commented Mar 10, 2017

vkhatri commented Mar 12, 2017

Choose a reason for hiding this comment

shortdudey123 commented Mar 20, 2017