Pool 615/bstier+phiggins/restart services on secrets change #1313

blakestier · 2017-06-19T23:54:13Z

chef-server-ctl set-secret --with-restart restarts all enabled services dependent on changed secret.
chef-server-ctl set-secret advises restart of all enabled services dependent on changed secret.

Secrets can also be changed without the chef-server-ctl set-secret command, but that path is not addressed here. POOL-647 opened.

Questions:
This PR does not handle the remove-secret ctl command, either. Should it?
Where should we document this change?

srenatus

Just some minor comments -- this looks good. Thanks 💯 for adding tests

srenatus · 2017-06-20T08:07:19Z

omnibus/files/private-chef-ctl-commands/secrets.rb

@@ -27,13 +29,43 @@
  "opscode-reporting" => ["rabbitmq_password", "sql_password", "sql_ro_password"],
 }

+SERVICES_REQUIRING_RESTART = {


just had to try this, but your flat structure is more space-efficient:

SERVICES_REQUIRING_RESTART = { bookshelf: { access_key_id: ["opscode-erchef", "bookshelf"] secret_access_key: ["opscode-erchef", "bookshelf"], sql_password: ["bookshelf"], }, 'chef-server': { superuser_key: ["opscode-reporting"], webui_key: ["oc_id"], webui_pub_key: ["opscode-erchef", "opscode-reporting"], }, data_collector: { token: ["opscode-erchef", "nginx"], }, ldap: { bind_password: ["opscode-erchef"], }, manage: { secret_key_base: ["chef-manage"], secret_token: ["chef-manage"], }, oc_bifrost: { sql_password: ["oc_bifrost"], superuser_id: ["opscode-erchef", "oc_bifrost", "opscode-chef-mover"], }, oc_id: { secret_key_base: ["oc_id"], sql_password: ["oc_id"], }, 'opscode-reporting': { rabbitmq_password: ["opscode-reporting"], sql_password: ["opscode-reporting"], }, opscode_erchef: { sql_password: ["opscode-erchef", "opscode-chef-mover"], }, 'push-jobs-server': { pushy_priv_key: ["opscode-push-jobs-server"], pushy_pub_key: ["opscode-push-jobs-server"], sql_password: ["opscode-push-jobs-server"], }, rabbitmq: { actions_password: ["opscode-erchef", "opscode-reporting"], management_password: ["opscode-erchef", "rabbitmq"], password: ["opscode-erchef", "opscode-expander", "rabbitmq"], }, redis_lb: { password: ["opscode-chef-mover", "nginx", "redis_lb"], }, saml: { client_id: ["chef-manage"], client_secret: ["chef-manage"], } }

We considered this but thought it was not worth the hassle vs what we ended up with.

srenatus · 2017-06-20T08:11:32Z

omnibus/files/private-chef-ctl-commands/secrets.rb

+  "push-jobs-server.pushy_pub_key" => ["opscode-push-jobs-server"],
+  "push-jobs-server.sql_password" => ["opscode-push-jobs-server"],
+  "rabbitmq.actions_password" => ["opscode-erchef", "opscode-reporting"],
+  "rabbitmq.management_password" => ["opscode-erchef", "rabbitmq"],


Can we remove rabbitmq here? That's actually another edge case ( 🤦‍♀️ , right?): restarting rabbitmq will not make these changes have any effect on the rabbitmq side, because it's the rabbitmq recipe that changes those passwords.

I don't believe we should account for that in any way, just remove it here. (Rationale: If you mess with your RMQ passwords, you better know what you're doing.)

@srenatus We weren't sure if you meant we should remove this line altogether or if we should remove rabbitmq from the list of services to be restarted. We ended up doing the second one, so hopefully that's what you meant!

👍 that's what I've meant, sorry for having been ambiguous.

srenatus · 2017-06-20T08:12:03Z

omnibus/files/private-chef-ctl-commands/secrets.rb

  credentials.add(group, key, value: secret, frozen: true, force: true)
  credentials.save
+  lookup = "#{group}.#{key}"
+  puts "#{lookup} changed."


stray debug output 😃

We actually put that in there as intentional logging-- what do you think?

Oh! 😁 Sorry. I think I'd rather remove it ;)

I would change it to debug mode.

Deleting seems fine to me :)

srenatus · 2017-06-20T08:15:19Z

omnibus/files/private-chef-ctl-commands/secrets.rb

+  lookup = "#{group}.#{key}"
+  puts "#{lookup} changed."
+
+  #TODO: do we need to guard against services being empty? (KNOWN_CREDS being out of sync w/ service list)


The connection betweenSERVICES_REQUIRING_RESTART and KNOWN_CREDS is that every key of SERVICES_REQUIRING_RESTART should be in KNOWN_CREDS; but you're right, there could be a reason for adding something to KNOWN_CREDS that doesn't have a service depend on it.

irb(main):001:0> Array(nil) => []

So, I think what you have there is 👍

srenatus · 2017-06-20T08:17:01Z

omnibus/files/private-chef-ctl-commands/secrets.rb

+  if service == "chef-manage"
+    Mixlib::ShellOut.new("chef-manage-ctl restart").run_command
+  else
+    run_sv_command_for_service("restart", service)


Given this, I believe you could also make this line restart(service)

We're intentionally avoiding calling run_sv_service, since that calls exit! and this code potentially will be called more than once. https://github.com/chef/omnibus-ctl/blob/76347dd8e3281928e69e00cca87721d48b0e0e14/lib/omnibus-ctl.rb#L359

oh right, thanks. 👍

srenatus · 2017-06-20T08:17:22Z

omnibus/files/private-chef-ctl-commands/spec/secrets_spec.rb

@@ -0,0 +1,151 @@
+#
+# Copyright 2016 Chef Software, Inc.


[nit] This file is new isn't it? Let's make this 2017 then

srenatus · 2017-06-20T09:12:41Z

This PR does not handle the remove-secret ctl command, either. Should it?

Hmm what do you think? I'm leaning towards yes, I think... (Feel free to spin this off into another card)

Where should we document this change?

It would be nice to add a line to RELEASE_NOTES.md.

Also, I believe adding a short note here in chef-web-docs would be good.

Signed-off-by: Blake Stier <bstier@chef.io> Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Signed-off-by: Blake Stier <bstier@chef.io> Signed-off-by: Pete Higgins <phiggins@chef.io> Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Signed-off-by: Blake Stier <bstier@chef.io> Signed-off-by: Pete Higgins <phiggins@chef.io> Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

srenatus

Thanks for putting also putting effort into docs ❤️ -- LGTM 👍

afiune · 2017-06-21T15:48:00Z

@blakestier @phiggins About "where do we document this?" we should add a section at: https://docs.chef.io/ctl_chef_server.html#set-secret

blakestier · 2017-06-21T16:55:16Z

Updated doc PR: chef/chef-web-docs#736

Signed-off-by: Blake Stier <bstier@chef.io>

afiune · 2017-06-21T17:24:34Z

omnibus/files/private-chef-ctl-commands/secrets.rb

+  "saml.client_secret" => ["chef-manage"],
+}
+
+MANAGE_SVDIR = "/opt/chef-manage/sv/"


[nit] I wonder if it is a good idea to put this constant at the top of the file? 🤔

afiune · 2017-06-21T17:24:53Z

omnibus/files/private-chef-ctl-commands/secrets.rb

+
+def restart_manage_or_other_service(service)
+  if service == "chef-manage"
+    Mixlib::ShellOut.new("chef-manage-ctl restart", :env => { "SVDIR" => MANAGE_SVDIR }).run_command


Nicely done! 👌

afiune · 2017-06-21T17:27:09Z

omnibus/files/private-chef-ctl-commands/spec/secrets_spec.rb

+    end
+  end
+
+  #this is an unlikely scenario, but worth guarding against confusing messaging about restarting services that are not installed/enabled


Thank you for putting comments!! Though a few needs an space in front of #. Totally a nit!!! 😉

afiune · 2017-06-21T17:28:39Z

omnibus/files/private-chef-ctl-commands/spec/secrets_spec.rb

+  context "when chef-manage depends on the changed secret but is not installed" do
+    before do
+      allow(veil_creds).to receive(:add).with("manage", "secret_key_base", {:value=>"new_key", :frozen=>true, :force=>true})
+      allow(File).to receive(:exist?).with("/opt/chef-manage/sv/").and_return(false)


[nit] Should we make opt/chef-manage/sv also a constant/variable in the tests?

afiune

Pretty nice work!!

A few nitpicks but they don't matter that much.

afiune · 2017-06-21T17:35:53Z

BTW, We might want to run an ad_hoc build to test this in the pipeline.

Signed-off-by: Blake Stier <bstier@chef.io> Specs were breaking with Rspec not found error without this update

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

srenatus · 2017-06-22T10:18:03Z

BTW, We might want to run an ad_hoc build to test this in the pipeline.

What raises the question if anything in the pipeline uses chef-server-ctl set-secret 🤔 I'm not sure but don't think so.

blakestier requested a review from srenatus June 19, 2017 23:54

srenatus approved these changes Jun 20, 2017

View reviewed changes

Blake Stier and others added 7 commits June 20, 2017 11:35

Services restart/user is prompted to restart on set-secrets

3ec4e79

Signed-off-by: Blake Stier <bstier@chef.io> Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Only prompts restart/restarts enabled services

51cd3d4

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Restarts/prompts restart for chef-manage on secrets change

371a1ee

Signed-off-by: Blake Stier <bstier@chef.io> Signed-off-by: Pete Higgins <phiggins@chef.io> Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Remove rabbitmq

24ba572

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Use chef-manage SVDIR for restart

9975132

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Remove comment and update date

58101d4

Signed-off-by: Blake Stier <bstier@chef.io> Signed-off-by: Pete Higgins <phiggins@chef.io> Signed-off-by: Pete Higgins <pete@peterhiggins.org>

Add note to release notes.

c5853ff

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

phiggins mentioned this pull request Jun 20, 2017

Document new chef-server-ctl set-secret option. chef/chef-web-docs#736

Merged

phiggins force-pushed the POOL-615/bstier+phiggins/restart-services-on-secrets-change branch from 12730e3 to c5853ff Compare June 20, 2017 21:19

srenatus approved these changes Jun 21, 2017

View reviewed changes

No notification on secrets change

053c55b

Signed-off-by: Blake Stier <bstier@chef.io>

afiune reviewed Jun 21, 2017

View reviewed changes

blakestier force-pushed the POOL-615/bstier+phiggins/restart-services-on-secrets-change branch from eb19e2e to 6895b86 Compare June 21, 2017 17:28

afiune reviewed Jun 21, 2017

View reviewed changes

afiune approved these changes Jun 21, 2017

View reviewed changes

Updates readline

cca2972

Signed-off-by: Blake Stier <bstier@chef.io> Specs were breaking with Rspec not found error without this update

blakestier force-pushed the POOL-615/bstier+phiggins/restart-services-on-secrets-change branch from 6895b86 to cca2972 Compare June 21, 2017 17:46

Update some comment formatting.

f3d40b2

Signed-off-by: Pete Higgins <pete@peterhiggins.org>

phiggins force-pushed the POOL-615/bstier+phiggins/restart-services-on-secrets-change branch from 2a86dc0 to f3d40b2 Compare June 21, 2017 20:19

phiggins merged commit 9806d05 into master Jun 22, 2017

phiggins deleted the POOL-615/bstier+phiggins/restart-services-on-secrets-change branch June 22, 2017 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pool 615/bstier+phiggins/restart services on secrets change #1313

Pool 615/bstier+phiggins/restart services on secrets change #1313

blakestier commented Jun 19, 2017

srenatus left a comment

srenatus Jun 20, 2017

phiggins Jun 20, 2017

srenatus Jun 20, 2017

blakestier Jun 20, 2017

phiggins Jun 20, 2017

srenatus Jun 21, 2017

srenatus Jun 20, 2017

blakestier Jun 20, 2017

srenatus Jun 20, 2017

afiune Jun 21, 2017

blakestier Jun 21, 2017

srenatus Jun 20, 2017

srenatus Jun 20, 2017

phiggins Jun 20, 2017

srenatus Jun 20, 2017

srenatus Jun 20, 2017

srenatus commented Jun 20, 2017

srenatus left a comment

afiune commented Jun 21, 2017

blakestier commented Jun 21, 2017

afiune Jun 21, 2017

afiune Jun 21, 2017

afiune Jun 21, 2017

afiune Jun 21, 2017

afiune left a comment •

edited

Loading

afiune commented Jun 21, 2017

srenatus commented Jun 22, 2017

Pool 615/bstier+phiggins/restart services on secrets change #1313

Pool 615/bstier+phiggins/restart services on secrets change #1313

Conversation

blakestier commented Jun 19, 2017

srenatus 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

srenatus commented Jun 20, 2017

srenatus left a comment

Choose a reason for hiding this comment

afiune commented Jun 21, 2017

blakestier commented Jun 21, 2017

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

afiune left a comment • edited Loading

Choose a reason for hiding this comment

afiune commented Jun 21, 2017

srenatus commented Jun 22, 2017

afiune left a comment •

edited

Loading