Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unique export file for security policy resource #2350

Merged
merged 2 commits into from Nov 29, 2017

Conversation

jquick
Copy link
Contributor

@jquick jquick commented Nov 29, 2017

The SecurityPolicy resource has a potential race condition when exporting the policy. If two runners were both working on the SecurityPolicy resource one could delete the policy export file before the other finished the export. This would cause the policy test to fail on the second runner.

This change is to create a SecureRandom hex and append it to the policy export file. This resolves the issue as each instance will have its own unique file.

Signed-off-by: Jared Quick jquick@chef.io

Signed-off-by: Jared Quick <jquick@chef.io>
@jquick jquick requested a review from adamleff November 29, 2017 00:03
@jquick jquick requested a review from a team as a code owner November 29, 2017 00:03
return skip_resource "Can't read security policy" if cmd.exit_status.to_i != 0
@content = cmd.stdout

if @content.empty? && !file.empty?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was removed as it seems to be legacy code. We don't have any local file variable. In the future we could look into using the Train::File to capture the content of the policy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should remove the content check completely. If the content is empty, we should not expect that to be okay, so it is probably a failure. Can we add a test that ensures the parsing is not crashing if we have an empty file?

Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix, @jquick.

@adamleff adamleff added the Type: Bug Feature not working as expected label Nov 29, 2017
Signed-off-by: Jared Quick <jquick@chef.io>
Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @jquick

@chris-rock chris-rock merged commit 3f14e46 into master Nov 29, 2017
@chris-rock chris-rock deleted the jq/security_policy_unique_export branch November 29, 2017 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants