-
Notifications
You must be signed in to change notification settings - Fork 227
Support device modules in apply statements #737
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,8 @@ class Node | |
| class BaseError < Bolt::Error | ||
| attr_reader :issue_code | ||
|
|
||
| def initialize(message, issue_code) | ||
| # TODO can we just drop issue code here? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1 |
||
| def initialize(message, issue_code = nil) | ||
| super(message, kind, nil, issue_code) | ||
| end | ||
|
|
||
|
|
@@ -34,6 +35,12 @@ def kind | |
| end | ||
| end | ||
|
|
||
| class RemoteError < BaseError | ||
| def kind | ||
| 'puppetlabs.tasks/remote-task-error' | ||
| end | ||
| end | ||
|
|
||
| class EnvironmentVarError < BaseError | ||
| def initialize(var, val) | ||
| message = "Could not set environment variable '#{var}' to '#{val}'" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -39,6 +39,7 @@ module Transport | |
| class Base | ||
| STDIN_METHODS = %w[both stdin].freeze | ||
| ENVIRONMENT_METHODS = %w[both environment].freeze | ||
| DEFAULT_INPUT_METHOD = 'both' | ||
|
|
||
| attr_reader :logger | ||
|
|
||
|
|
@@ -68,6 +69,33 @@ def with_events(target, callback) | |
| result | ||
| end | ||
|
|
||
| def provided_features | ||
| [] | ||
| end | ||
|
|
||
| def default_input_method | ||
| 'both' | ||
| end | ||
|
|
||
| def select_implementation(target, task) | ||
| impl =task.select_implementation(target, provided_features) | ||
| impl['input_method'] ||= default_input_method | ||
| impl | ||
| end | ||
|
|
||
| def add_target_param(target, task, params) | ||
| # TODO: How should metadata impact this? | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Metadata seems to be more "controllable" (and visible) to the user, so I think "prioritizing" that makes the most sense: Raising an error if the target is remotable and the task doesn't specify it could be confusing if users don't know where the 'remotable' target is coming from.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on the other hand running a non-remote task against a remote target seems incredibly dangerous.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure I understand what a task be "remotable" is enough to comment. For right now it just seems like "the user said in the metadata it can be run on remote targets", and if they don't specify that they still might be able to run on remote targets. I think we can raise a warning in that case, but limiting the user to only tasks that are remote seems like it makes using remote tasks much harder.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the general statement is that a remoteable task is one that does something meaningful with the IE the service task is not remoteable. If I ran it targeting an aws account it would stop a service on the run-on node rather then do anything to the real device. |
||
| #if target.remote? && !task.remote? | ||
| # raise Bolt::Node::RemoteError.new('Remote targets can only run Remotable tasks') | ||
| #end | ||
| #if !target.remote? && task.remote? | ||
| # raise Bolt::Node::RemoteError.new('Remote tasks can only be run on remote targets') | ||
| #end | ||
|
|
||
| # TODO: wrap sensitive values | ||
| target.remote? ? params.merge('_target' => target.to_h) : params | ||
| end | ||
|
|
||
| def filter_options(target, options) | ||
| if target.options['run-as'] | ||
| options.reject { |k, _v| k == '_run_as' } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| require 'bolt/transport/base' | ||
|
|
||
|
|
||
| module Bolt | ||
| module Transport | ||
| class Remote < Base | ||
| def self.options | ||
| # TODO: We should accept arbitrary options here | ||
| %w[host port user password connnect-timeout device-type run-on] | ||
| end | ||
|
|
||
| def self.validate(options) | ||
| # This will fail when validating global config | ||
| #unless options['device-type'] | ||
| # raise Bolt::ValidationError, 'Must specify device-type for devices' | ||
| #end | ||
| end | ||
|
|
||
| # TODO: this should have access to inventory so target doesn't have to | ||
| def initialize(executor) | ||
| super() | ||
|
|
||
| @executor = executor | ||
| end | ||
|
|
||
| def get_proxy(target) | ||
| # TODO: This needs to have access to the inventory | ||
| inventory = target.instance_variable_get(:@inventory) | ||
| raise "Target was creates without inventory? Not get_targets?" unless inventory | ||
| inventory.get_targets(target.options['run-on'] || 'localhost').first | ||
| end | ||
|
|
||
| # Cannot batch because arugments differ | ||
| def run_task(target, task, arguments, options = {}) | ||
| proxy_target = get_proxy(target) | ||
| transport = @executor.transport(proxy_target.protocol) | ||
| arguments = arguments.merge('_target' => target.to_h.reject {|_, v| v.nil?}) | ||
|
|
||
| # TODO: add support for device-type and feature checking here. | ||
| # * tasks/task_implementations must have the same device-type as the target | ||
| # * Does one implementation need to support multiple device types? | ||
| # * Do we need multiple implementations for the same device based on proxy features? | ||
| # TODO doesn't support transports with only batch exec(orchestrator). update orch | ||
| result = transport.run_task(proxy_target, task, arguments, options) | ||
|
|
||
| Bolt::Result.new(target, value: result.value) | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect to use this for anything other than devices? If not I think 'Device' is a more precise name, since ssh and winrm are also remote transports. But if so this might be the best name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a larger issue(or two closely related issues) I think we need to discuss and/or research.
Are all remote targets devices(ie physical devices or vms with limited control APIs)?
No, remote tasks should be used for cloud resources, notification APIs etc.
Do they use the puppet-device framework?
Usually but not always. I would expect most ruby implementations to use the puppet-device framework but they could very will be written without that especially to enable non-ruby implementations.
My general hypothesis is that module authors especially ruby authors will want to think of the modules they write as "device" modules since they use the device framework and renaming the framework is probably not worth the effort. On the other hand, I think bolt users, and eventually PE GUI admins should not think of these things as devices but rather remote targets that rely on a proxy since it's weird to call an aws account a device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming is hard :) This seems to echo us not knowing how to refer to ephemeral, cloudy | container "nodes".
Definitely worth more discussion.