Update how agent and handler are configured #345

mfischer-zd · 2016-08-12T07:34:01Z

See commits for information. Closes #328, #341

Some users may wish to assign the api_key and application_key attributes during runtime instead of compile time -- particularly, users who want to obtain the value of the key(s) from an external service. Allow doing so by deferring evaluation of these attributes. Closes DataDog#328

If the user has placed API and/or application keys in the `node.run_state` hash, read them from there instead of from the node's attributes. If run_state is used, implementors can obtain the keys from other data sources (e.g. Hashicorp Vault) and prevent the keys from being inadvertently stored in Chef Server. Closes DataDog#341

The `compile_time` attribute for `chef_gem` has been deprecated in favor of deferring name resolution until convergence time. Fix accordingly. Reference: https://blog.chef.io/2015/03/03/chef-12-1-0-released/

olivielpeau · 2016-08-18T09:10:29Z

Thanks @mfischer-zd, we'll spend some time in the next few weeks to test these changes in details. Being able to prevent storing the unencrypted app/api keys in Chef Server is definitely a very welcome feature.

Can you confirm that your changes on chef_gem's compile_time attribute work well with Chef 11?

mfischer-zd · 2016-08-23T05:48:31Z

Can you confirm that your changes on chef_gem's compile_time attribute work well with Chef 11?

I cannot confirm; we don't use Chef 11. (It should be noted that Chef 12 has been out for 18 months.)

mfischer-zd · 2016-10-12T17:28:12Z

Hello, have you had time to review this? It's been nearly 2 months.

iancward · 2016-12-01T15:30:48Z

I'd recommend leaving the compile_time true in, as even though Chef 12 has been out for some time, I'm sure Chef 11 is still in use in various places, and it's still available for download at https://downloads.chef.io.

haidangwa · 2016-12-01T16:26:11Z

recipes/dd-handler.rb

@@ -32,10 +32,7 @@
 chef_gem 'chef-handler-datadog' do # ~FC009
  action :install
  version node['datadog']['chef_handler_version']
-  # Chef 12 introduced `compile_time` - remove when Chef 11 is EOL.
-  compile_time true if respond_to?(:compile_time)


As the comment indicated, this compile_time should not be removed yet. Chef 11 is still supported by Chef and has not reached EOL. Other than to tidy up the code, the use of #respond_to? effectively negates any technical need to remove this line.

mfischer-zd · 2016-12-02T08:29:33Z

@iancward @haidangwa It wasn't removed to stop supporting Chef 11. It was removed because it's no longer needed with this PR.

haidangwa · 2016-12-02T17:02:59Z

@mfischer-zd If you have tests with chef 11 clients that show it doesn't have any adverse affect, then I'd be ok with that, but removing that will change when the gem is installed. If the difference between getting installed at compile time versus getting installed at convergence doesn't have any negligible affect, then I'm ok. If anything, using compile_time false would suppress warnings in clients pre-chef 12.1.

mfischer-zd · 2016-12-02T20:09:44Z

@haidangwa I'd expect Datadog to run the tests under whatever versions of Chef it plans to support. If the test suite still passes, then I'd expect it to be OK.

olivielpeau · 2016-12-15T23:44:49Z

Quick update on the review of this PR: we plan to test and release these changes for the next minor release of the cookbook (2.8), in the next month or so. Sorry for the delay in getting this merged and thanks again for this big contribution.

Since the cookbook still needs to support Chef 11 the change on compile_time can't be released as-is and will not be merged with the rest of the changes.

Unfortunately we don't have automated tests on Chef 11, we need to improve that.

mfischer-zd · 2016-12-16T01:06:28Z

Since the cookbook still needs to support Chef 11 the change on compile_time can't be released as-is and will not be merged with the rest of the changes.

This part of the change should not impact Chef 11 compatibility.

iancward · 2016-12-16T15:16:02Z

But does the gem need to be installed at compile time to work? If not, then the line can safely be removed. If it does need to be installed at compile time, then the comment should be removed but the code line left. Since Since Chef changed the install time of chef_gem from compile time to run time in Chef 12.

mfischer-zd · 2016-12-16T16:06:36Z

recipes/dd-handler.rb

 end
-require 'chef/handler/datadog'


@iancward Note the move of this line. This is what formerly required the Gem to be installed at compile time; the require here would fail since it would refer to a Gem that hadn't been installed yet.

Took some time to test this, and unfortunately moving the require doesn't change its evaluation time, it's still evaluated at compile time.

This commit still works in Chef 12 because not specifying compile_time on chef_gem still makes the install of chef-handler-datadog happen at compile time (see, in the link you provided: The default behavior has not changed, but every chef_gem resource will now emit out a warning). The default will actually change to compile_time false in Chef 13.

So we should keep the compile_time true line until we can get rid of the require altogether (and I think we'll be able to do that once we bump the requirement on chef_handler to ~> 1.3, see related #338 (comment) ). Let's keep this discussion in a separate issue/PR though.

OK with me.

mfischer-zd · 2016-12-16T16:07:47Z

recipes/dd-handler.rb

+    config
+  end
+
+  require 'chef/handler/datadog'


@iancward Note the move of the require line to here. Since it's now declared in a resource provider block, it isn't evaluated until runtime. Therefore, the Gem no longer needs to be installed at compile time. As long as it's declared before this handler resource is declared, things will work properly even if the Gem is installed at runtime.

olivielpeau · 2016-12-20T00:43:54Z

Thanks @mfischer-zd for clarifying your changes on compile_time, makes sense to me now. Haven't tested it thoroughly but this change looks good.

alq666 · 2017-01-19T13:36:19Z

@olivielpeau we need an update on this.

olivielpeau · 2017-01-19T23:50:02Z

Just opened:

Defer evaluation of api and app keys + read from run_state #395, a rebased version of this PR on top of the current master branch
[dd-handler] Bump requirement on chef_handler #396 to remove the require statement in dd-handler

alq666 · 2017-01-24T17:26:17Z

@olivielpeau thanks, keep me posted.

olivielpeau · 2017-01-24T23:35:20Z

Going to close this PR as I've just merged #395. Will be released with 2.8.0 in the next couple of days.

Thanks again @mfischer-zd!

olivielpeau · 2017-01-25T21:58:53Z

Update: 2.8.0 is out https://github.com/DataDog/chef-datadog/releases/tag/v2.8.0 :)

mfischer-zd added 3 commits August 12, 2016 15:31

Don't install chef-handler-datadog at compile time

29fa562

The `compile_time` attribute for `chef_gem` has been deprecated in favor of deferring name resolution until convergence time. Fix accordingly. Reference: https://blog.chef.io/2015/03/03/chef-12-1-0-released/

mfischer-zd force-pushed the defer_attribute_lookup branch from 981327b to 29fa562 Compare August 12, 2016 07:44

mfischer-zd mentioned this pull request Aug 12, 2016

Defer evaluation of api key attribute #329

Closed

olivielpeau added this to the Triage milestone Aug 18, 2016

olivielpeau added the feature label Aug 18, 2016

olivielpeau modified the milestones: 2.8.0, Triage Nov 18, 2016

haidangwa reviewed Dec 1, 2016

View reviewed changes

mfischer-zd commented Dec 16, 2016

View reviewed changes

olivielpeau self-assigned this Dec 20, 2016

olivielpeau mentioned this pull request Jan 19, 2017

Defer evaluation of api and app keys + read from run_state #395

Merged

olivielpeau mentioned this pull request Jan 20, 2017

Add support for retrieving secrets from chef-vault #377

Open

olivielpeau closed this Jan 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update how agent and handler are configured #345

Update how agent and handler are configured #345

mfischer-zd commented Aug 12, 2016

olivielpeau commented Aug 18, 2016

mfischer-zd commented Aug 23, 2016

mfischer-zd commented Oct 12, 2016

iancward commented Dec 1, 2016

haidangwa Dec 1, 2016 •

edited

Loading

mfischer-zd commented Dec 2, 2016

haidangwa commented Dec 2, 2016

mfischer-zd commented Dec 2, 2016

olivielpeau commented Dec 15, 2016

mfischer-zd commented Dec 16, 2016

iancward commented Dec 16, 2016 •

edited

Loading

mfischer-zd Dec 16, 2016 •

edited

Loading

olivielpeau Jan 19, 2017

mfischer-zd Jan 19, 2017

mfischer-zd Dec 16, 2016

olivielpeau commented Dec 20, 2016

alq666 commented Jan 19, 2017

olivielpeau commented Jan 19, 2017

alq666 commented Jan 24, 2017

olivielpeau commented Jan 24, 2017

olivielpeau commented Jan 25, 2017

Update how agent and handler are configured #345

Update how agent and handler are configured #345

Conversation

mfischer-zd commented Aug 12, 2016

olivielpeau commented Aug 18, 2016

mfischer-zd commented Aug 23, 2016

mfischer-zd commented Oct 12, 2016

iancward commented Dec 1, 2016

haidangwa Dec 1, 2016 • edited Loading

Choose a reason for hiding this comment

mfischer-zd commented Dec 2, 2016

haidangwa commented Dec 2, 2016

mfischer-zd commented Dec 2, 2016

olivielpeau commented Dec 15, 2016

mfischer-zd commented Dec 16, 2016

iancward commented Dec 16, 2016 • edited Loading

mfischer-zd Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

olivielpeau Jan 19, 2017

Choose a reason for hiding this comment

mfischer-zd Jan 19, 2017

Choose a reason for hiding this comment

mfischer-zd Dec 16, 2016

Choose a reason for hiding this comment

olivielpeau commented Dec 20, 2016

alq666 commented Jan 19, 2017

olivielpeau commented Jan 19, 2017

alq666 commented Jan 24, 2017

olivielpeau commented Jan 24, 2017

olivielpeau commented Jan 25, 2017

haidangwa Dec 1, 2016 •

edited

Loading

iancward commented Dec 16, 2016 •

edited

Loading

mfischer-zd Dec 16, 2016 •

edited

Loading