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.

Sign up for GitHub

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

Jump to bottom

(PE-15036) Fix Windows permission inheritance #112

Merged
merged 1 commit into from
Apr 20, 2016
Merged

(PE-15036) Fix Windows permission inheritance #112

merged 1 commit into from
Apr 20, 2016

Conversation

highb
Copy link
Contributor

@highb highb commented Apr 20, 2016

Prior to this commit there was an issue where the inherit permission
value on the client_datadir folder was being lost, causing the folder
to be inaccessible to the MCO/PCP daemons.

This commit ensures that directory has the proper inherit permission,
if it appears that it does not have it. For more details: PE-15036

@highb
Copy link
Contributor Author

highb commented Apr 20, 2016

I've tested this with r10k_puppet_agent_fork=highb r10k_puppet_agent_ref=fix/master/pe-15037_fix_inhieritable_permissions_windows ./frankenbuilder -v 2015.3.2 --upgrade_from 2015.2.3 --tests /s/pe_acceptance_tests/setup/install.rb,/s/pe_acceptance_tests/setup/agent_upgrade.rb --vmpooler and it passed!

      Test Suite: tests @ 2016-04-19 20:59:18 -0700

      - Host Configuration Summary -


              - Test Case Summary for suite 'tests' -
       Total Suite Time: 2193.59 seconds
      Average Test Time: 1096.79 seconds
              Attempted: 2
                 Passed: 2
                 Failed: 0
                Errored: 0
                Skipped: 0
                Pending: 0
                  Total: 2

      - Specific Test Case Status -

Failed Tests Cases:
Errored Tests Cases:
Skipped Tests Cases:
Pending Tests Cases:

This should resolve the failing windows puppet_agent tests in CI.

@highb highb force-pushed the fix/master/pe-15037_fix_inhieritable_permissions_windows branch from 36ea4bf to 86369e1 Compare April 20, 2016 06:08
Iristyle
Iristyle reviewed Apr 20, 2016
View reviewed changes
manifests/windows/install.pp
@@ -56,4 +56,11 @@
command => "${::system32}\\cmd.exe /c start /b ${_cmd_location} /c \"${_installbat}\" ${::puppet_agent_pid}",
path => $::path,
}

# PE-15037 Cache dir loses inheritable SYSTEM perms
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also note the original PUP ticket here for ref - https://tickets.puppetlabs.com/browse/PUP-5480

@Iristyle
Copy link
Contributor

Iristyle commented Apr 20, 2016 •
edited
Loading

I can't really comment on the tests, but everything looks good to me aside from the 2 minor issues I added - and it passed - hurray!

(PE-15036) Fix Windows permission inheritance
5b11210 
Prior to this commit there was an issue where the inherit permission
value on the `client_datadir` folder was being lost, causing the folder
to be inaccessible to the MCO/PCP daemons.

This commit ensures that directory has the proper inherit permission,
if it appears that it does not have it. For more details: PE-15036
@highb highb force-pushed the fix/master/pe-15037_fix_inhieritable_permissions_windows branch from 86369e1 to 5b11210 Compare April 20, 2016 16:29
@highb
Copy link
Contributor Author

highb commented Apr 20, 2016

@Iristyle Updated with the PUP ticket and quoted the paths. Thanks again for helping so much on this! :)

MikaelSmith
MikaelSmith reviewed Apr 20, 2016
View reviewed changes
manifests/windows/install.pp
exec { 'fix inheritable SYSTEM perms':
command => "${::system32}\\icacls.exe \"${::puppet_client_datadir}\" /grant \"SYSTEM:(OI)(CI)(F)\"",
unless => "${::system32}\\icacls.exe \"${::puppet_client_datadir}\" | findstr \"SYSTEM:(OI)(CI)(F)\"",
require => Exec['install_puppet.bat'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure why this requires install_puppet, but it shouldn't hurt because install_puppet won't do anything until Puppet exits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was my thinking; we don't want it to run before the upgrade or during it, so I did this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I follow... this will always run before the upgrade, because install_puppet.bat just waits until the current Puppet run finishes before triggering the MSI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, right. So really this is sort of a race condition. :/

Copy link
Contributor

@MikaelSmith MikaelSmith Apr 20, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

As in, the current version of Puppet may break the ACL settings on client_datadir again before we exit? I don't think that will happen, as we should only be managing permissions on client_datadir during settings application, at the beginning of the run.

@MikaelSmith
Copy link
Contributor

👍

@MikaelSmith MikaelSmith merged commit 7580884 into puppetlabs:master Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@highb @Iristyle @MikaelSmith