Fix for s3 secret not getting copied on target vm #428
Conversation
| @@ -689,6 +689,8 @@ def bootstrap_common_params(bootstrap) | |||
| bootstrap.config[:first_boot_attributes_from_file] = locate_config_value(:first_boot_attributes_from_file) | |||
| bootstrap.config[:encrypted_data_bag_secret] = s3_secret || locate_config_value(:secret) | |||
| bootstrap.config[:encrypted_data_bag_secret_file] = locate_config_value(:secret_file) | |||
| #cl_secret needs to be set as chef checks for this key | |||
| Chef::Config[:knife][:cl_secret] = s3_secret if locate_config_value(:s3_secret) | |||
There was a problem hiding this comment.
This code is a little confusing. I'm not sure that it might need a little refactor. At least might consider something like this:
# retrieving the secret from S3 is unique to knife-ec2, so we need to set "command line secret" to the value fetched from S3
Chef::Knife::DataBagSecretOptions.set_cl_secret(s3_secret)
We should add a test either way.
There was a problem hiding this comment.
@dheerajd-msys please improve the comment here. can you answer "why does chef need this key?" in the comment?
There was a problem hiding this comment.
@btm,
yes, I will be improving the comment and code as per your suggestion and have added tests also.
When secret option is passed for Linux vm, instead of knife-windows, chef module is imported.
In chef code mentioned here, when secret option is passed, its proc function sets the value using set_cl_secret method in Chef::Config[:knife][:cl_secret].
And later the chef code instead of checking if value is present in bootstrap.config also, it checks for value in Chef::Config[:knife][:cl_secret] code here
Hence only setting the s3 secret value in bootstrap.config[:secret] doesn't work, as only upon verification that cl_secret is set, the code uses bootstrap.config[:secret] value.
dheerajd-msys
force-pushed
the
dh/fix_for_s3_secret_issue
branch
from
3a60749
to
1988aa7
Compare
| @@ -486,6 +486,30 @@ | |||
| end | |||
| end | |||
|
|
|||
| description 'S3 secret test cases' do | |||
There was a problem hiding this comment.
this is causing travis to fail. I think you mean describe
|
please rebase, fix the tests, and improve the comments around why we need to set cl_secret. |
dheerajd-msys
force-pushed
the
dh/fix_for_s3_secret_issue
branch
from
1988aa7
to
cc0c68f
Compare
dheerajd-msys
force-pushed
the
dh/fix_for_s3_secret_issue
branch
from
cc0c68f
to
f7088b3
Compare
|
@btm, |
| expect(Chef::Config[:knife][:cl_secret]).to eql(@secret_content) | ||
| end | ||
|
|
||
| it 'should set the cl_secret key to nil' do |
There was a problem hiding this comment.
The description of the spec is not complete. When should the cl_secret be set to nil? Please add context here.
Also avoid using should and should not in the beginning of it block. Please refer https:/reachlocal/rspec-style-guide#should-it-or-should-not-in-it-statements
|
👍 |
NimishaS
force-pushed
the
dh/fix_for_s3_secret_issue
branch
from
d884943
to
7307c76
Compare
No description provided.