Skip to content

Conversation

@glennsarti
Copy link
Contributor

Previously Puppet used the V1 API for scheduled tasks however this has been
deprecated in favor of the V2 Win32OLE objects. This commit adds a parent class
which can query and modify tasks via the V2 API and adds a facade class called
Puppet::Util::Windows::TaskScheduler2V1Task which is binary compatible with the
older V1 class Win32::TaskScheduler. This commit also adds basic smoke tests
for the V2 API for CRUD operations.

This commit adds a new non-default provider for the scheduled_task puppet type
which instead uses the V2 API for scheduled tasks. As this is not the default
users will need to opt in to this functionality.

This commit changes the unit tests for the scheduled_task provider by running
the tests against both providers (win32_taskscheduler, the default, and
taskscheduler_api2). This ensures that any behaviors are consistent in both
providers.

The concrete class which services each provider is refactored into the
concrete_klass variable which is then used in the tests themselves.

@glennsarti glennsarti requested a review from Iristyle September 12, 2017 23:34
@glennsarti
Copy link
Contributor Author

Seems I broke posix testing....oh well.

@puppetcla
Copy link

CLA signed by all contributors.

@Iristyle
Copy link
Contributor

The Travis output for bundle exec rake $CHECK is really ... odd:

Processing 42 spec group(s) with 2 worker(s)
error: spec list file 'An' does not exist.
sh: 1: Syntax error: end of file unexpected
error: spec list file 'Failure/Error:' does not exist.
error: spec list file 'if' does not exist.
error: spec list file 'task_provider' does not exist.
error: spec list file 'else' does not exist.
error: spec list file 'task_provider' does not exist.
error: spec list file 'end' does not exist.
sh: 1: Syntax error: "(" unexpected
error: spec list file 'before' does not exist.
error: spec list file 'NameError:' does not exist.
error: spec list file 'uninitialized' does not exist.
sh: 1: Syntax error: "(" unexpected

@glennsarti
Copy link
Contributor Author

I'm pretty sure it's because I broke something when I shell out to run powershell.

@@ -0,0 +1,707 @@
#require 'puppet/util/windows/error'
#require 'win32ole' # TODO should guard this on Windows only
Copy link
Contributor

Choose a reason for hiding this comment

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

removals


# TODO May not need this if no COM
# require 'ffi'
# extend FFI::Library
Copy link
Contributor

Choose a reason for hiding this comment

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

All these bits can get the axe

# extend FFI::Library

# The error class raised if any task scheduler specific calls fail.
class Error < Puppet::Util::Windows::Error; end
Copy link
Contributor

Choose a reason for hiding this comment

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

Confused by this part....

# # TODO needed?
# class << self
# attr_accessor :com_initialized
# end
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove

ROOT_FOLDER = '\\'

# https://msdn.microsoft.com/en-us/library/windows/desktop/aa378137(v=vs.85).aspx
S_OK = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

This is already defined elsewhere FWIW -

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I can use that instead

@task_service = nil
@pITask = nil

# # TODO Do we need COM for OLE?
Copy link
Contributor

Choose a reason for hiding this comment

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

No.. trash this.

@task.max_run_time_as_ms
end

# # Sets the maximum length of time, in milliseconds, that the task can run
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume we can trash this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops....forgot to implement that

raise Error.new(_('No current task scheduler. Schedule.Service is NULL.')) if @task_service.nil?
raise TypeError unless task.is_a?(String)

task_folder = @task_service.GetFolder(get_folder_path_from_task(task))
Copy link
Contributor

Choose a reason for hiding this comment

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

The helpers are unnecessary, though you may have to switch from / to \\:

irb(main):005:0> require 'pathname'
=> true
<(File.expand_path('c:\\windows\\tasks\\file.task'))
=> #<Pathname:c:/windows/tasks/file.task>
irb(main):007:0> f.dirname
=> #<Pathname:c:/windows/tasks>
irb(main):008:0> f.basename
=> #<Pathname:file.task>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not File paths. They are backslash delimited paths


# TODO SHOULD BE tested
def get_folder_path_from_task(task_name)
path = task_name.rpartition('\\')[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Pathname can do this already, unless you think the helpers are better suited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are not File paths. They are backslash delimited paths

raise Error.new(_('No current task scheduler. Schedule.Service is NULL.')) if @task_service.nil?
raise TypeError unless folder_path.is_a?(String)

options = {} if options.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not assign options = {} as the default value in the method signature?

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...I'll fix that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also a note about using the Ruby way of merging options hashs..

task_folder.GetTasks(TASK_ENUM_HIDDEN).each do |task|
included = true

unless !included || options[:include_compatibility].empty?
Copy link
Contributor

Choose a reason for hiding this comment

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

unless + ! makes for a confusing check...

Copy link
Contributor

Choose a reason for hiding this comment

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

Can probably clean up the logic here inside the loop to make it a bit clearer...

#
class Puppet::Util::Windows::TaskScheduler2V1Task
# The error class raised if any task scheduler specific calls fail.
class Error < Puppet::Util::Windows::Error; end
Copy link
Contributor

Choose a reason for hiding this comment

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

Defined here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Spec tests were failing as it was expecting these definitions.


array << task.Path if included
end
return array unless options[:include_child_folders]
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 I'd break apart this method a little bit / refactor... I can show you what I mean later.

@glennsarti glennsarti force-pushed the ticket/master/pup-7894-create-tasksched2-util branch from b1e7539 to c3ff155 Compare September 14, 2017 20:35
@glennsarti
Copy link
Contributor Author

Fixed the travis issue. It was parallel:spec just completely blowing up on a simple bug in the spec file.

@MosesMendoza
Copy link
Contributor

@glennsarti is this PR still actively in progress?

@glennsarti
Copy link
Contributor Author

@MosesMendoza Yes. PuppetConf took precedence.


# DONE
def activate(task)
raise Error.new(_('No current task scheduler. Schedule.Service is NULL.')) if @task_service.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

@task_service doesn't need nil checks in here anywhere given its assigned in the initializer and never reset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know if task has to have a leading \, or can it be a bare string?

end

# DONE
def deactivate()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is being used currently by the shim taskscheduler2_v1task.rb ... but perhaps activate should take an optional parameter specifying the requested compatibility level?


full_taskname = Puppet::Util::Windows::TaskScheduler2::ROOT_FOLDER + validate_task_name(task_name)

result = @task.activate(full_taskname)
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps activate should take an optional parameter specifying the requested compatibility level? Then no extra clean up is necessary and deactivate method can disappear.

end

# DONE
def definition()
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

I think we believe that we will never ask for a task without also asking for its definition? Therefore, we can probably either remove this method, or change it so that it simply returns @pITaskDefinition (in the event its a public method used by other classes).

That would mean that activate is also responsible for creating the definition AND that all the below nil checks on the definition are no longer necessary.

result = task_folder.DeleteTask(get_task_name_from_task(task),0)
rescue WIN32OLERuntimeError => e
# TODO win32ole errors are horrible. Assume the task doesn't exist so deletion is successful
return true
Copy link
Contributor

Choose a reason for hiding this comment

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

For all the methods here, not sure that we should eat the WIN32OLERuntimeError. In the prior implementation, nearly all COM errors end up calling raise. The com.rb file has a helper that automatically turns failed HRESULTs into exceptions.

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/com.rb#L140-L143

In the original v1, there are a few cases where well known HRESULTs are ignored during certain circumstances... whether they should be here or not remains to be seen.

# Saves the current task. Tasks must be saved before they can be activated.
#
# DONE
def save()
Copy link
Contributor

Choose a reason for hiding this comment

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

Something to think about for a future commit / iteration of the API - do we want to pass in the definition and/or the task?

def set_principal(user, password)
raise Error.new(_('No currently active task. ITask is NULL.')) if definition.nil?

if (user.nil? || user == "") && (password.nil? || password == "")
Copy link
Contributor

Choose a reason for hiding this comment

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

user.empty? instead of user == ""

definition.Principal.RunLevel = TASK_RUNLEVEL_HIGHEST
return true
else
# TODO!!!
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like some work to do here.. wonder if it uses SCHED_E_ACCOUNT_INFORMATION_NOT_SET the same way as set_account_information does in the old version?

# Sets the compatibility with the task.
# https://msdn.microsoft.com/en-us/library/windows/desktop/aa381846(v=vs.85).aspx
#
def compatibility=(value)
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

Might not need this just yet? Also seems like an unsafe operation?

Could this be used by Puppet users to migrate their v1 managed tasks to v2?

# https://msdn.microsoft.com/en-us/library/windows/desktop/aa383070(v=vs.85).aspx
#
# DONE
def priority
Copy link
Contributor

Choose a reason for hiding this comment

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

@glennsarti says move this to the shim

value
end

def new_task_defintion(task_name)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing an i in definition


action = nil

begin
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to look at behavior of puppet resource to see how these come out?

May need to hide this for the moment as we build out compatibility layer? Not sure just yet.. need to do some more homework.

Copy link
Contributor

Choose a reason for hiding this comment

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

Unlike trigger, we're returning a COM instance of an Action object - do we want that, given this is a leaky interface?

end

# DONE
def append_trigger(trigger_hash)
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

Do we want to assign a default value here as documentation that this is a hash? Or does a {} not really make sense anyhow?

Mostly just concerned with making sure it's understood that it's a Ruby hash and not a Trigger COM object.

time = @pITask.NextRunTime

# The API will still return a time WAAAY in the past if there is no schedule.
# As this is looking forward, if the next execution is 'scheduled' in the 1900s assume
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is the minimum OLE datetime value? https://msdn.microsoft.com/en-us/library/system.datetime.tooadate(v=vs.110).aspx

An OLE Automation date is implemented as a floating-point number whose integral component is the number of days before or after midnight, 30 December 1899, and whose fractional component represents the time on that day divided by 24. For example, midnight, 31 December 1899 is represented by 1.0; 6 A.M., 1 January 1900 is represented by 2.25; midnight, 29 December 1899 is represented by -1.0; and 6 A.M., 29 December 1899 is represented by -1.25.
The base OLE Automation Date is midnight, 30 December 1899. The minimum OLE Automation date is midnight, 1 January 0100. The maximum OLE Automation Date is the same as DateTime.MaxValue, the last moment of 31 December 9999.

# and nS is the number of seconds (for example, PT5M specifies 5 minutes and P1M4DT2H5M specifies one month,
# four days, two hours, and five minutes)
def time_limit_to_hash(time_limit)
regex = /^P((?'year'\d+)Y)?((?'month'\d+)M)?((?'day'\d+)D)?T((?'hour'\d+)H)?((?'minute'\d+)M)?((?'second'\d+)S)?$/
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is ISO8601 duration format - see the standard definition at https://en.wikipedia.org/wiki/ISO_8601#Time_intervals

Ruby has some support for ISO8601, but not sure about durations https://ruby-doc.org/stdlib-2.1.1/libdoc/time/rdoc/Time.html#method-c-iso8601

# will run before terminating.
#
# DONE
def max_run_time_as_ms
Copy link
Contributor

Choose a reason for hiding this comment

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

@glennsarti says this is unused in the v1 world

# Sets the maximum length of time, in milliseconds, that the task can run
# before terminating. Returns the value you specified if successful.
#
def max_run_time=(max_run_time)
Copy link
Contributor

Choose a reason for hiding this comment

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

No equivalent in v1

def win32ole_to_hash(win32_obj)
hash = {}

win32_obj.ole_get_methods.each do |method|
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

Need to answer the question around whether we want to do this dynamically like this... or just use the names of actual properties we care about.

Given that there are a number of arbitrary trigger types that have varying properties - see https://msdn.microsoft.com/en-us/library/windows/desktop/aa383868(v=vs.85).aspx ... this might be the best way to extract information into a Ruby hash. The only other alternative seems to be looking at the XML definition of the trigger somehow.


hash = win32ole_to_hash(task_trigger)

hash['type_name'] = task_trigger.ole_type.name
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

So type_name ends up being a hint for future deserialization / hydration of an appropriate COM trigger class name.... Looking at this further, I think we only need the trigger type enum value and can remove the type_name as its the name of the actual COM class, and given we never use it to create specific instance types.

Exposed as https://msdn.microsoft.com/en-us/library/windows/desktop/aa383978(v=vs.85).aspx

# constructor is the same as calling TaskScheduler.new plus
# TaskScheduler#new_work_item.
#
def initialize(work_item = nil, trigger = nil)
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

Should trigger accept an array?

Looks like original implementation in taskscheduler.rb did not... and we only found one call to Win32::TaskScheduler.new that took the trigger parameter in create and its a dummy value... so no compelling reason to change this for the moment.


def enum()
array = []
@task.enum_task_names(Puppet::Util::Windows::TaskScheduler2::ROOT_FOLDER,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can use map

end

def save(file = nil)
raise NotImplementedError unless file.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the existing provider implementation doesn't pass a file anyhow in flush at https://github.com/puppetlabs/puppet/blob/master/lib/puppet/provider/scheduled_task/win32_taskscheduler.rb#L236

This is making us wonder if pIPersistFile.Save is being called with a NULL string?? https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/windows/taskscheduler.rb#L291-L294

end

# As this class is emulating the older V1 API we only support execution actions (not email etc.)
return nil unless action.Type == Puppet::Util::Windows::TaskScheduler2::TASK_ACTION_EXEC
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

Could this method be cleaned up a bit logically?

Are v1 tasks even allowed to have a list of actions?

# job should run.
#
def new_work_item(task_name, task_trigger)
raise TypeError unless task_trigger.is_a?(Hash)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we create a default exec action at the same we're creating the named task and its trigger? Might be the sensible happy-path use case?

#
# Note - This method name is a mis-nomer. It's actually appending a newly created trigger to the trigger collection.
def trigger=(v1trigger)
append_trigger(v1trigger)
Copy link
Contributor

Choose a reason for hiding this comment

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

Fortunately, this method is much better at describing what is actually happening.

However, the original API of .trigger = as something that adds new triggers to the end of a running list of triggers is totally bonkers 👎 . We should fix both of those, but we can ticket that / do it later.

https://github.com/puppetlabs/puppet/blob/master/lib/puppet/provider/scheduled_task/win32_taskscheduler.rb#L192-L196


# Adds a trigger at the specified index.
#
# Note - This method name is a mis-nomer. It's actually setting a trigger at the specified index
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

Roll this up in the same ticket as fixing trigger =

Maybe just remove add_trigger as it's currently unused?


# Generate the V1 Flags integer from the task definition
# flags list - https://msdn.microsoft.com/en-us/library/windows/desktop/aa381283%28v=vs.85%29.aspx
# TODO Need to implement the rest of the flags
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, these should be implemented

def transform_and_validate(hash)
new_hash = {}

hash.each{ |key, value|
Copy link
Contributor

@Iristyle Iristyle Nov 20, 2017

Choose a reason for hiding this comment

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

probably want to use do / end for multi-line instead of { and }

Since this looks to be a verbatim copy of Win32::TaskScheduler code, might make sense to leave it as-is ... and / or extract to shared helper. Of course, if taskscheduler.rb is being deleted, then no shared helper is necessary.

@glennsarti glennsarti force-pushed the ticket/master/pup-7894-create-tasksched2-util branch from 3453070 to e0c544d Compare November 21, 2017 00:28
Previously Puppet used the V1 API for scheduled tasks however this has been
deprecated in favor of the V2 Win32OLE objects.  This commit adds a parent class
which can query and modify tasks via the V2 API and adds a facade class called
Puppet::Util::Windows::TaskScheduler2V1Task which is binary compatible with the
older V1 class Win32::TaskScheduler.  This commit also adds basic smoke tests
for the V2 API for CRUD operations.
This commit adds a new non-default provider for the scheduled_task puppet type
which instead uses the V2 API for scheduled tasks.  As this is not the default
users will need to opt in to this functionality.
This commit changes the unit tests for the scheduled_task provider by running
the tests against both providers (win32_taskscheduler, the default, and
taskscheduler_api2). This ensures that any behaviors are consistent in both
providers.

The concrete class which services each provider is refactored into the
concrete_klass variable which is then used in the tests themselves.
@glennsarti glennsarti force-pushed the ticket/master/pup-7894-create-tasksched2-util branch from e0c544d to 11b78a9 Compare November 21, 2017 00:30
@Iristyle
Copy link
Contributor

@Iristyle
Copy link
Contributor

This has shipped in a different form in puppetlabs/puppetlabs-scheduled_task#6

@Iristyle Iristyle closed this Jan 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants