Introduce command scheduler and runner #1

ethitter · 2017-09-08T22:23:06Z

Introduces a way to create cron events to run whitelisted WP-CLI commands.

Does not yet support any commands that involve files, such as imports or exports.

Command output is directed to error_log().

…the cron runner

``` $ phpenv global 5.3 rbenv: version `5.3' not installed ```

ethitter · 2017-09-12T03:51:23Z

The command validation needs work. Passing a flag right after wp will likely break things right now.

Adopts `\WP_CLI\Configurator\extract_assoc()`

ethitter · 2017-09-12T22:22:07Z

The command validation needs work. Passing a flag right after wp will likely break things right now.

Much better care of 5b60aee, thanks to WP_CLI\Configurator\extract_assoc().

ethitter · 2017-09-13T22:05:22Z

Some sort of kill switch is probably a good idea. Not sure how exactly to implement that for a command that's already running.

ethitter · 2017-09-14T19:04:36Z

If WP-CLI will support it, we should allow some sort of STDIN to be specified, as some commands use it. This might be something to pursue after the basics land, alongside file handling.

nickdaugherty

This looks really cool! Left some feedback here and there.

nickdaugherty · 2017-09-25T18:43:09Z

README.md

+
+Yes, using either the `WP_CLI_CRON_CONTROL_OFFLOAD_COMMAND_BLACKLIST` constant or the `wp_cli_cron_control_offload_command_blacklist` filter. If defined, the constant takes precedence and the filter is ignored.
+
+Regardless of whether the constant or filter is used, either should provide an array of top-level commands to permit:


Are the whitelist / blacklist examples reversed here? Blacklist should be the list of commands to block, and whitelist should be commands to permit.

🤦‍♂️

nickdaugherty · 2017-09-25T18:50:02Z

includes/class-cli.php

+/**
+ * Offload WP-CLI commands to cron
+ */
+class CLI extends WP_CLI_Command {


Just a thought - should this class be called Command instead of CLI? The former seems more descriptive, especially since it extends WP_CLI_Command.

nickdaugherty · 2017-09-25T18:58:10Z

includes/functions.php

+	if ( defined( 'WP_CLI_CRON_CONTROL_OFFLOAD_COMMAND_BLACKLIST' ) && is_array( \WP_CLI_CRON_CONTROL_OFFLOAD_COMMAND_BLACKLIST ) ) {
+		return \WP_CLI_CRON_CONTROL_OFFLOAD_COMMAND_BLACKLIST;
+	}
+


I wonder here - there are probably going to be WP CLI commands that the hosting platform blacklists...then potentially additional commands that the developer of the site wishes to blacklist.

Should we support both? For example, on VIP we could blacklist a few (and those can never be reversed via filter), then clients could add additional commands to blacklist via a filter, and that's only additive.

nickdaugherty · 2017-09-25T19:08:04Z

tests/tests/class-plugin-functions.php

+		$this->assertTrue( is_wp_error( WP_CLI_Cron_Control_Offload\validate_command( 'wp cli info' ) ) );
+		$this->assertTrue( is_wp_error( WP_CLI_Cron_Control_Offload\validate_command( 'cli info' ) ) );
+	}
+


Can we add some tests for improperly constructed commands? I'm thinking things like:

Missing wp (post list),

Trying to sneak additional commands on (wp post list && cat /etc/passwd) (even though this might not actually do anything)

Poorly constructed arguments, like wp do-thing --foo=bar--fubar=yes

Backtick operators

Trying to override --url, perhaps with a different site that's not part of this network

Anything else that isn't a standard wp command subcommand [positional_args] [--assoc_args]

I'd like to be as defensive and paranoid as possible here.

Good call. Pipes and ampersands should also be dropped.

The leading wp is optional, mostly for convenience. WP_CLI::runcommand() doesn't want it, so the parse_command() method strips it. This has the added benefit of normalizing the command to ensure cron's duplicate scheduling kicks in when appropriate.

There's a limit to how much parsing we can do without recreating large portions of WP-CLI (or loading it, in the case of custom commands), but we clearly have room for improvement. That we trust the initial users for this (requires a Go sandbox) helps somewhat.

Checks added in 94d630e, tests in 966d0f3.

Much beyond this is difficult, as I haven't found a way yet to invoke WP-CLI's command validation outside of where WP-CLI uses it.

…s) to blacklists and whitelists set in constants

… system, amongst other "fun."

mjangda

Some suggestions on how to improve the tests.

mjangda · 2017-09-27T04:27:02Z

tests/tests/class-plugin-functions.php

+		$this->assertTrue( is_wp_error( WP_CLI_Cron_Control_Offload\validate_command( 'post list 2> /tmp/nope' ) ) );
+		$this->assertTrue( is_wp_error( WP_CLI_Cron_Control_Offload\validate_command( 'post list 1>&2 /tmp/nope' ) ) );
+		$this->assertTrue( is_wp_error( WP_CLI_Cron_Control_Offload\validate_command( 'post list 2>&1 /tmp/nope' ) ) );
+		$this->assertTrue( is_wp_error( WP_CLI_Cron_Control_Offload\validate_command( 'post list &> /tmp/nope' ) ) );


For tests like this when we are running through a list of inputs/outputs, it's better to use PHPUnit's data providers: https://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.data-providers -- each input gets executed as a separate test and we can label them so we know which ones fail.

Same thing applies to test_blocked_event_scheduling and a bunch of other ones above where the main action and assertion between each test is the same.

Finally, it's nice to use this pattern for tests: http://wiki.c2.com/?ArrangeActAssert -- i.e. instead of squeezing everything into one line, opt for readability instead:

$input = 'post list & date'; $output = WP_CLI_Cron_Control_Offload\validate_command( $input ); $this->assertWpError( $output );

mjangda · 2017-09-27T04:33:59Z

tests/tests/class-plugin-functions.php

+	 * Test each blocked bash operator
+	 */
+	function test_for_invalid_bash_operators() {
+		$this->assertTrue( is_wp_error( WP_CLI_Cron_Control_Offload\validate_command( 'post list & date' ) ) );


There's a assertWPError method which might be nicer than doing is_wp_error + assertTrue (https://make.wordpress.org/core/handbook/testing/automated-testing/writing-phpunit-tests/#assertions)

mjangda

Noted a few things that should probably be changed. I think the biggest thing is probably the parse/implode stuff (I think longer term maintenance is going to be challenging).

The other thought I had: what's the benefit of using this vs. just executing the command via vipd + host action?

mjangda · 2017-09-27T04:38:59Z

includes/class-command.php

+	 */
+	public function create( $args, $assoc_args ) {
+		$command = WP_CLI\Utils\get_flag_value( $assoc_args, 'command', '' );
+		$command = validate_command( $command );


This is redundant, since schedule_cli_command runs it as well.

mjangda · 2017-09-27T04:48:09Z

includes/functions.php

+ * @param string $filter_tag String for list filter.
+ * @return array
+ */
+function _filter_list_allow_only_additions( $constant, $filter_tag ) {


We should definitely add some tests for this.

mjangda · 2017-09-27T04:49:53Z

includes/functions.php

+ * @return array
+ */
+function get_command_blacklist() {
+	if ( defined( 'WP_CLI_CRON_CONTROL_OFFLOAD_COMMAND_BLACKLIST' ) && is_array( \WP_CLI_CRON_CONTROL_OFFLOAD_COMMAND_BLACKLIST ) ) {


Should we be blocking cli-cron-offload / CLI_NAMESPACE by default to avoid weird infinite loops?

Yes indeed. I thought I'd included that, but I can't find it. Adding to the is_command_allowed() function.

mjangda · 2017-09-27T04:51:27Z

includes/functions.php

+
+	// If there's a whitelist, default to it.
+	if ( ! empty( get_command_whitelist() ) ) {
+		add_filter( 'wp_cli_cron_control_offload_is_command_allowed', __NAMESPACE__ . '\command_is_whitelisted', 9, 2 );


Why does this need to be a filter? Why can't we evaulate the in_array and pass the value into apply_filters below?

I used this approach to allow for no whitelist, if someone wanted to be dangerous. That said, they could hook __return_true late, so there's no need for it.

mjangda · 2017-09-27T05:03:27Z

includes/functions.php

+ */
+function validate_command( $command ) {
+	$command = trim( $command );
+	$command = parse_command( $command );


I think the parse / implode feel a bit dangerous and maybe even unnecessary. Looking through, there are some things we want to do:

find out which command is being called (so we can check against the whitelist/blacklist).

prevent bad positionals (&, '|', etc.)

I think we could do both those things without the full complexity of breaking out the command and then rebuilding it.

The other option would be to change how the command is executed so that instead of accepting a freeform string, we ask for a explicit structure (i.e. the command needs to be formatted as {command} {subcommand} {flags}; anything else fails validation).

Most of the logic comes from WP-CLI, and arose from wanting to accomodate WP_CLI::runcommand(). It doesn't expect the leading wp (trivial), but also passes along many of the global associative arguments, so there's a bit of need to clean the command. WP-CLI also doesn't expose its validation methods for reuse.

I like your idea to change how the command is accepted. That'll remove a lot of the magic.

I'm leaning towards breaking command, subcommand and flags into their own associative arguments, removing all of the magic from parsing the command.

ethitter · 2017-09-29T01:36:27Z

what's the benefit of using this vs. just executing the command via vipd + host action?

This came originally from the idea of eliminating future-dated hosted actions, as well as being able to run commands on the CLI containers rather than our sandboxes (sitemaps, for example). It was also somewhat inspired by a client already doing essentially this--they've built a UI of preset commands, and can run them via cron as needed. While I see this as initially meant for those with a Go sandbox, I could see a UI and reviewed commands permitted via the whitelist.

If someone wants to run without a whitelist, hook `__return_true`.

philipjohn · 2018-02-10T10:34:26Z

Is this likely to get worked on at all? We've had a few examples recently where we've wanted to offload a CLI command to the CLI containers, and @david-binda came up with his own PHP-based workaround recently but it'd be good to something more permanent.

mjangda · 2018-02-11T05:16:19Z

@philipjohn We're going to stick with @david-binda's workaround for now until we get proper wp-cli support baked in (i.e. can be run by VIPs, provides real-time-ish output, etc.).

We may revisit this if that ends up being pushed out in terms of timelines.

ethitter added 23 commits September 7, 2017 14:57

More scaffolding

f0fa8b1

Match filename to namespace

5aa1177

Further scaffolding

f7c9097

Provide way to limit commands that can be scheduled

5287b0e

More renaming

2bbbfba

Better naming, most of the plugin will live here to support later UI

c6c452a

Whitelist WP-CLI's native commands

9d25b5d

Primative scaffolding to schedule a CLI command

f2808dd

Rough approach to run events and log results

b7a1650

Time event execution

eca8ecc

When the spawned command fails, return its error rather than killing …

058a267

…the cron runner

Note more TODOs

a15a433

Last of package renaming

fbc6427

Grunt build

d87e7c1

Improve message format and support translation

c28c37b

Reogranize to match use and priority

009d038

Blacklist takes priority

4cdfcde

Allow future-dated execution

27b2abe

Fix variable naming to reflect what it actually is

3658a3c

Bug fixes for 3658a3c

83d9a3b

CLI command to offload a CLI command

97baa5b

Oops

3c7644a

Use timestamp after bothering to return it in 97baa5b

ace4421

ethitter self-assigned this Sep 8, 2017

ethitter added 4 commits September 8, 2017 15:29

Fix filter, since we're whitelisting by top-level WP-CLI command

20ccbcb

Remove placeholder text

5366706

Grunt build

35052c9

Travis dropped 5.3

2a5dedb

``` $ phpenv global 5.3 rbenv: version `5.3' not installed ```

ethitter added the WIP label Sep 8, 2017

PHPCS fixes

c808127

ethitter added 2 commits September 12, 2017 15:18

Improve command parsing by borrowing WP-CLI's approach

5b60aee

Adopts `\WP_CLI\Configurator\extract_assoc()`

Don't just note source in commit message

8631cab

ethitter added 4 commits September 12, 2017 15:47

Respect global versus local assoc args

5e01506

Assuaging my neurosis

ba5d9d6

grunt build

523bcd7

Update readme for latest changes

a5ad987

Use actions so users can decide how to handle "output"

626c3f7

nickdaugherty reviewed Sep 25, 2017

View reviewed changes

nickdaugherty removed the needs review label Sep 25, 2017

ethitter added 5 commits September 25, 2017 13:16

First round of review feedback

1c14761

Review feedback: extend filters to allow additions (and only addition…

eafe83e

…s) to blacklists and whitelists set in constants

PHPCS fix

919f8e3

Check for basic bash operators that would permit wandering around the…

94d630e

… system, amongst other "fun."

Tests for 94d630e

966d0f3

ethitter added the needs review label Sep 26, 2017

Translation updates for 94d630e

35c2831

mjangda reviewed Sep 27, 2017

View reviewed changes

mjangda requested changes Sep 27, 2017

View reviewed changes

ethitter added 4 commits September 28, 2017 19:18

Better tests

7e7fe4c

Review feedback

797e802

Block recursive scheduling, no one really likes infinite loops

7fc1f2c

Simplify how the whitelist is applied

43883c8

If someone wants to run without a whitelist, hook `__return_true`.

ethitter removed their assignment Dec 29, 2019


		Yes, using either the `WP_CLI_CRON_CONTROL_OFFLOAD_COMMAND_BLACKLIST` constant or the `wp_cli_cron_control_offload_command_blacklist` filter. If defined, the constant takes precedence and the filter is ignored.

		Regardless of whether the constant or filter is used, either should provide an array of top-level commands to permit:

Introduce command scheduler and runner #1

Are you sure you want to change the base?

Introduce command scheduler and runner #1

Uh oh!

Conversation

ethitter commented Sep 8, 2017

Uh oh!

ethitter commented Sep 12, 2017

Uh oh!

ethitter commented Sep 12, 2017

Uh oh!

ethitter commented Sep 13, 2017

Uh oh!

ethitter commented Sep 14, 2017

Uh oh!

nickdaugherty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjangda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjangda left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethitter commented Sep 29, 2017

Uh oh!

philipjohn commented Feb 10, 2018

Uh oh!

mjangda commented Feb 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants