Fix windows default paths #326

florindragos · 2018-09-10T14:45:46Z

read windows system paths from custom facts

davejrt · 2018-09-10T23:34:23Z

@glennsarti would be quite keen to get your input here

glennsarti

A bunch of things to change;

Excessive use of PATH changes seems un-needed
Using top level scope variables for facts instead of $facts
Dangerous path changes using TEMP directories
Non-idempotent resources

glennsarti · 2018-09-11T00:48:58Z

manifests/compose.pp

@@ -55,26 +55,24 @@
      $docker_download_command = "if (Invoke-WebRequest ${docker_compose_url} ${proxy_opt} -UseBasicParsing -OutFile \"${docker_compose_location_versioned}\") { exit 0 } else { exit 1}"

      exec { 'Enable TLS 1.2 in powershell':


This whole exec probably isn't required. IIRC this only affects the current process and you may as well just add this to the PS1 template files instead of a resource in of itself. This also will have no affect on older powershell module (1.x)

glennsarti · 2018-09-11T00:51:06Z

manifests/compose.pp

      exec { "Install Docker Compose ${version}":
-        path     => ['c:/Windows/Temp/', 'C:/Program Files/Docker/'],
-        command  => "& ${script_path}",
+        path     => ["${::systemroot}/Temp/", "${::program_files_path}/Docker/"],


Path is not required here as the template file itself uses absolute paths.

side note - Also "${::systemroot}/Temp/" is not what you want. That is the TEMP environment for the SYSTEM account. If puppet runs under a different account (which is common enough) then the TEMP path is different.

glennsarti · 2018-09-11T00:51:48Z

manifests/compose.pp

        provider => powershell,
        creates  => $docker_compose_location_versioned,
      }
+
+      file { $docker_compose_location:


nit: Can't say I'm a fan of symlinks as it's not Windows-y but I guess it will do.

glennsarti · 2018-09-11T00:52:43Z

manifests/compose.pp

@@ -55,26 +55,24 @@
      $docker_download_command = "if (Invoke-WebRequest ${docker_compose_url} ${proxy_opt} -UseBasicParsing -OutFile \"${docker_compose_location_versioned}\") { exit 0 } else { exit 1}"

      exec { 'Enable TLS 1.2 in powershell':
-        path     => ['c:/Windows/Temp/', 'C:/Program Files/Docker/'],
+        path     => ["${::systemroot}/Temp/", "${::program_files_path}/Docker/"],
        command  => '[Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12',
        provider => powershell,
        creates  => $docker_compose_location_versioned,


If you keep this resource in, this is not idempotent. this resource can never create the $docker_compose_location_versioned file.

glennsarti · 2018-09-11T00:59:19Z

manifests/exec.pp

    $exec_timeout = 3000
-    $exec_path = ['c:/Windows/Temp/', 'C:/Program Files/Docker/']
+    $exec_path = ["${::systemroot}/Temp/", "${::program_files_path}/Docker/"]


I don't think any of this is required.

See above. Systemroot\temp is not what you want.

In fact there is no way you want %TEMP% to be first in the executable path. That is a security risk.

$docker::params::docker_command should contain the absolute path, not a relative one. As you're using absolute paths everywhere else.

glennsarti · 2018-09-11T01:00:07Z

manifests/image.pp

@@ -38,11 +38,11 @@

  if $::osfamily == 'windows' {
    $update_docker_image_template = 'docker/windows/update_docker_image.ps1.erb'
-    $update_docker_image_path = 'C:/Windows/Temp/update_docker_image.ps1'
-    $exec_environment = 'PATH=C:/Program Files/Docker/'
+    $update_docker_image_path = "${::systemroot}/Temp/update_docker_image.ps1"


See above. Systemroot\temp is not what you want.

glennsarti · 2018-09-11T01:00:54Z

manifests/params.pp

-    $tls_cacert                        = 'C:/ProgramData/docker/certs.d/ca.pem'
-    $tls_cert                          = 'C:/ProgramData/docker/certs.d/server-cert.pem'
-    $tls_key                           = 'C:/ProgramData/docker/certs.d/server-key.pem'
+    $tls_cacert                        = "${::program_data_path}/docker/certs.d/ca.pem"


This is configurable for the Docker daemon. Should this path be parameterised?

glennsarti · 2018-09-11T01:04:01Z

manifests/registry.pp

@@ -48,9 +48,9 @@
  $docker_command = $docker::params::docker_command

  if $::osfamily == 'windows' {
-    $exec_environment = ['PATH=C:/Program Files/Docker/']
+    $exec_environment = ["PATH=${::program_files_path}/Docker/"]


Setting PATH and exec_path is bad practice

https://puppet.com/docs/puppet/5.3/types/exec.html#exec-attribute-environment

glennsarti · 2018-09-11T01:07:17Z

lib/facter/docker.rb

@@ -67,3 +67,24 @@ def interfaces
    end
  end
 end
+
+Facter.add(:systemroot) do


I'm not keen on these fact names. It's very possible they will collide with other modules that need to do same thing.

Perhaps prefix with docker_ or something?

Also it's generally considered better practice for one file per fact. Although I see prior art says otherwise.

glennsarti · 2018-09-11T01:12:48Z

manifests/run.pp

    $exec_provider = 'powershell'
-    $cidfile = "c:/Windows/Temp/${service_prefix}${sanitised_title}.cid"
+    $cidfile = "${::systemroot}/Temp/${service_prefix}${sanitised_title}.cid"


See above. Systemroot\temp is not what you want.

glennsarti · 2018-09-11T01:13:34Z

manifests/services.pp

    $exec_timeout = 3000
-    $exec_path = ['c:/Windows/Temp/', 'C:/Program Files/Docker/']
+    $exec_path = ["${::systemroot}/Temp/", "${::program_files_path}/Docker/"]


See above about PATH= and exec_path

glennsarti · 2018-09-11T01:16:16Z

templates/windows/update_docker_image.ps1.erb

+(
+    [string]$DockerImage
+)
+$docker_cmd = "${Env:ProgramFiles}\docker\docker.exe"


Given docker_cmd is part of the params.pp surely we should be using that?

glennsarti · 2018-09-11T01:16:51Z

spec/defines/image_windows_spec.rb

    end

    context 'with ensure => present and image_tag => nanoserver' do
        let(:params) { { 'ensure' => 'present', 'image_tag' => 'nanoserver' } }
-        it { should contain_exec('& C:/Windows/Temp/update_docker_image.ps1 base:nanoserver') }
+        it { should contain_exec('& C:/Windows/Temp/update_docker_image.ps1 -DockerImage base:nanoserver') }


These tests seem very brittle for the resource name. Perhaps a better resource name would make sense?

florindragos · 2018-09-19T12:24:17Z

@glennsarti i've fixed some of the problems.
I'm not sure if we should use full paths for the docker command everywhere. It's the same way on linux and on windows we might run into problems with path separators since the acceptance tests are run using beaker that does ssh into cygwin and then cmd /c
Getting the users temp directory from an the TMP env var was also problematic because of cygwin so I just went with LOCALAPPDATA\Temp

glennsarti · 2018-09-25T00:33:54Z

@florindragos So you're running your Windows tests under cygwin?

florindragos · 2018-09-25T07:23:38Z

@glennsarti beaker uses ssh to connect to the hosts running on vmpooler. on windows, cygwin is used for the ssh server

glennsarti · 2018-09-25T08:03:01Z

Seems very odd given a lot of other modules use env vars in acceptance. Have you got some debug logs etc. to show the issue?

florindragos · 2018-09-25T09:28:46Z

I'm seeing really strange behaviour using TMP env var for the custom fact.
Listing the fact on the VM (beaker does cmd.exe /c):

bash-4.3$ cmd.exe /c puppet facts | grep docker_user_temp_path
    "docker_user_temp_path": "C:\\cygwin64\\tmp",

but running the tests:
Warning: Unknown variable: '::docker_user_temp_path'.

even if we got C:\cygwin64\tmp, this is not the value we are expecting...

glennsarti · 2018-09-25T10:30:28Z

On a real windows machine using PowerShell

Note - Ignore the Get http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.38/version: errors. My docker VM wasn't running at the time.

Using "#{ENV['LOCALAPPDATA']}\\Temp"

C:\Source\tmp\puppetlabs-docker-pr326 [master ↑2 +0 ~1 -0 !]> & bundle exec puppet facts --modulepath .\spec\fixtures\modules\ | sls docker_user_temp_path
Failed to load feature test for cfpropertylist: cannot load such file -- CFPropertyList
error during connect: Get http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.38/version: open //./pipe/docker_engine: The system cannot find the file specified. In the default daemon configuration on Windows, the docker client must be run elevated to connect. This error may also indicate that the docker daemon is not running.
error during connect: Get http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.38/info: open //./pipe/docker_engine: The system cannot find the file specified. In the default daemon configuration on Windows, the docker client must be run elevated to connect. This error may also indicate that the docker daemon is not running.
Warning: Facter: Fact resolution fact='puppet_classes', resolution='<anonymous>' resolved to an invalid value: Expected all to be one of [Integer, Float, TrueClass, FalseClass, NilClass, String, Array, Hash], but was Symbol

    "docker_user_temp_path": "C:\\Users\\glenn.sarti\\AppData\\Local\\Temp",

Using ENV['TMP']

C:\Source\tmp\puppetlabs-docker-pr326 [master ↑2 +0 ~1 -0 !]> & bundle exec puppet facts --modulepath .\spec\fixtures\modules\ | sls docker_user_temp_path
Failed to load feature test for cfpropertylist: cannot load such file -- CFPropertyList
error during connect: Get http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.38/version: open //./pipe/docker_engine: The system cannot find the file specified. In the default daemon configuration on Windows, the docker client must be run elevated to connect. This error may also indicate that the docker daemon is not running.
error during connect: Get http://%2F%2F.%2Fpipe%2Fdocker_engine/v1.38/info: open //./pipe/docker_engine: The system cannot find the file specified. In the default daemon configuration on Windows, the docker client must be run elevated to connect. This error may also indicate that the docker daemon is not running.
Warning: Facter: Fact resolution fact='puppet_classes', resolution='<anonymous>' resolved to an invalid value: Expected all to be one of [Integer, Float, TrueClass, FalseClass, NilClass, String, Array, Hash], but was Symbol

    "docker_user_temp_path": "C:\\Users\\GLENN~1.SAR\\AppData\\Local\\Temp",

Note that these give different values

So given that;

You get different behaviour between manual and beaker testing
99.999999% of windows users do not use cygwin SSH shells
Works fine on real Windows
There is a difference between TMP and LOCALAPPDAT (8.3 filenames), although this is minor

Changing your code because of a flaky test or test environment is bad idea. We know that the correct env var is TMP, and the fact you're working around this is a "bad smell" in the testing framework.

florindragos · 2018-09-26T12:35:06Z

@glennsarti you're right. making assumptions for the temp folder location in the module just so the tests are working is wrong. The temp folder could also be C:\Users\Administrator\AppData\Local\Temp\2
I changed the custom fact to ENV['TEMP'] and explicitly setting the TEMP variable in the spec_helper_acceptance.ro when running the acceptance tests seems to work just fine.

glennsarti · 2018-10-08T07:44:36Z

spec/spec_helper_acceptance.rb

          docker_path = "/cygdrive/c/Program Files/Docker"
          host.add_env_var('PATH', docker_path)
+          host.add_env_var('TEMP', 'C:\Users\Administrator\AppData\Local\Temp')


This still seems very wrong. The host should already have a correctly setup TEMP directory.

Even though the cygwin environment says \tmp, external processes created from within cygwin use C:\cygwin64\tmp

Windows 2012R2 Pooler VM

Administrator@adx7v3xdcx8nwfn ~ $ export ... declare -x TEMP="/tmp" declare -x TERM="xterm-256color" declare -x TMP="/tmp" ... Administrator@adx7v3xdcx8nwfn ~ $ cmd.exe /c set temp TEMP=C:\cygwin64\tmp Administrator@adx7v3xdcx8nwfn ~ $ powershell.exe -Command "Write-Host \$ENV:TEMP" C:\cygwin64\tmp

That is the way it should work. Even though facter shows the fact pointing to C:\cygwin64\tmp,
when running tests, the fact does not seem to be initialized:
Warning: Unknown variable: '::docker_user_temp_path'
This happens if I don't set the TEMP env var from within the spec_helper_acceptance...
It works fine for the other 2 facts that are being read from the environment.

It's difficult to understand how overriding a system set env variable is the right way to go, especially since it's being hard coded to a specific user's temp path that may or may not be the correct user for a particular run.

Once this variable is overwritten in this way, it's hard to tell what might be broken down the road. Doesn't it seem like this issue you are seeing might be down to misbehavior on the part of beaker or cygwin, that should be worked around more formally?

Unfortunately, I have not found a way to work around this.
After doing some more digging around, it looks like the user profile is not being loaded when beaker runs the commands.
The commands are being run remote through ssh: ssh Administrator@host -i key cmd /c puppet apply /tmp/manifest

Logging in with ssh will load your user profile and give you the TMP env var but running it remotely will not:

interactive:

ssh [email protected] -i .ssh/id_rsa-acceptance -bash-4.3$ cmd /c set TMP TMP=C:\cygwin64\tmp

vs non-interactive:

ssh [email protected] -i .ssh/id_rsa-acceptance cmd /c set TMP Environment variable TMP not defined

Adding the variable in .ssh/environment would probably fix this, but we still need to enter the value manually.

I agree, it's not right to hardcode the username but since we are running on the vmpooler VMs, we can assume it is running under Administrator.

Iristyle · 2018-10-11T15:28:50Z

lib/facter/docker.rb

+Facter.add(:docker_user_temp_path) do
+  confine :osfamily => :windows
+  setcode do
+    ENV['TEMP']


For the Puppet service this is probably OK. For end users with Unicode characters in their usernames running puppet apply, there's a chance that this corrupts the directory name due to Ruby bugs.

If you must expose the TEMP directory as a fact (which I would generally advise against if you can help it), you'll want to use the built-in Puppet helpers Puppet::Util.get_env - see https://github.com/puppetlabs/puppet/blob/24ead48f617cd3912491fe419ac7b67cda53a320/lib/puppet/util.rb#L40-L52

Thanks for the tip. I'll update the code with Puppet::Util.get_env.
Is there a reason why we should not use expose the TEMP directory as a custom fact and is there a better alternative to getting to the temporary directory from within a manifest?

florindragos force-pushed the defaults branch from 164d2c2 to c95cb9c Compare September 10, 2018 14:52

davejrt requested a review from glennsarti September 10, 2018 23:34

glennsarti suggested changes Sep 11, 2018

View reviewed changes

florindragos force-pushed the defaults branch from c95cb9c to 61748fc Compare September 18, 2018 12:17

florindragos force-pushed the defaults branch from 61748fc to 73c53ab Compare September 26, 2018 12:29

glennsarti reviewed Oct 8, 2018

View reviewed changes

Iristyle reviewed Oct 11, 2018

View reviewed changes

florindragos force-pushed the defaults branch 2 times, most recently from a7531ab to 49d485b Compare November 26, 2018 15:28

Fix windows default paths

9c9f3f1

florindragos force-pushed the defaults branch from 49d485b to 9c9f3f1 Compare November 27, 2018 12:46

davejrt merged commit 1896eca into puppetlabs:master Dec 5, 2018

Ramesh7 added the bugfix label Jul 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix windows default paths #326

Fix windows default paths #326

florindragos commented Sep 10, 2018

davejrt commented Sep 10, 2018

glennsarti left a comment

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

glennsarti Sep 11, 2018

florindragos commented Sep 19, 2018

glennsarti commented Sep 25, 2018

florindragos commented Sep 25, 2018

glennsarti commented Sep 25, 2018

florindragos commented Sep 25, 2018 •

edited

Loading

glennsarti commented Sep 25, 2018 •

edited

Loading

florindragos commented Sep 26, 2018

glennsarti Oct 8, 2018

florindragos Oct 9, 2018 •

edited

Loading

RandomNoun7 Oct 11, 2018

florindragos Oct 15, 2018 •

edited

Loading

Iristyle Oct 11, 2018

florindragos Oct 16, 2018

		@@ -55,26 +55,24 @@
		$docker_download_command = "if (Invoke-WebRequest ${docker_compose_url} ${proxy_opt} -UseBasicParsing -OutFile \"${docker_compose_location_versioned}\") { exit 0 } else { exit 1}"

		exec { 'Enable TLS 1.2 in powershell':

Fix windows default paths #326

Fix windows default paths #326

Conversation

florindragos commented Sep 10, 2018

davejrt commented Sep 10, 2018

glennsarti 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

florindragos commented Sep 19, 2018

glennsarti commented Sep 25, 2018

florindragos commented Sep 25, 2018

glennsarti commented Sep 25, 2018

florindragos commented Sep 25, 2018 • edited Loading

glennsarti commented Sep 25, 2018 • edited Loading

florindragos commented Sep 26, 2018

Choose a reason for hiding this comment

florindragos Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florindragos Oct 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florindragos commented Sep 25, 2018 •

edited

Loading

glennsarti commented Sep 25, 2018 •

edited

Loading

florindragos Oct 9, 2018 •

edited

Loading

florindragos Oct 15, 2018 •

edited

Loading