make salt-api arg/kwarg handling the same as cli for all clients

[mattp-]

What does this PR do?

both cherrypy and saltnado now pass all args through parse_input to
allow for behavior equivalent to salt cli yaml based arg parsing. This
will allow for pepper to behave more like salt cli, or direct api users.

also included is a minor feature to saltnado/cherrypy, a new X-Salt-Version
header for usage in pepper to determine if this feature among others
is available to use.

What issues does this PR fix or reference?

will fix saltstack/pepper#99 when equivalent pepper changes are made.

Previous Behavior

yaml based foo=bar args are not passed through parse_input

New Behavior

args are passed through parse_input

Tests written?

yes

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

[mattp-]

@rallytime I notice you disabled one of the other saltnado tests for being too flaky, the one I've added is essentially the same. It's not clear to me what race condition was the problem, but we can turn this test off for now as well if you like.

[gtmanfred]

looks like we just need some tests expected results updated for this change, and also pylint has one error, otherwise this looks perfect.

Thanks!
Daniel

[mchugh19]

I'd suggest throwing something into the release notes as well. Modifying the args/kwargs handling of salt-api and the version header is a pretty large (and positive) change.

[mattp-]

@gtmanfred ok, should have the tests/lint fixed up. will let jenkins chew on it for a bit
@mchugh19 seems reasonable, but i think generally the release note management is done by the release team when they cut releases, not in this PR :)

[gtmanfred]

Nope, we do it in the PRs when we add features, it would be great to include this in the Neon release notes.

[mattp-]

ah gotcha, added salt-api release notes to neon.rst

[rallytime]

@whiteinge Is this something you'd like to review?

[whiteinge]

Thanks for the ping, @rallytime. This functionality should already be baked into Salt and I did not include this functionality in salt-api on purpose for the reasons described below. That said, I'll happily defer to the wishes of the core team and any contributors that want to put in the actual work. Please consider or disregard as you see fit.

Parsing the arg=val strings into kwargs is a central workflow in Salt and thus should live in a central place and work consistently regardless if you are calling Salt from the CLI, from a REST API, from a Python wrapper, HTML form fields, a Slack bot, and many, many other examples we've seen. The best central place to do that is Salt's Python API (LocalClient, RunnerClient, WheelClient, etc). That is what the above-linked Pepper issue is discussing. All those client interfaces should run the arg (singular) array values through the utils function we have that splits arg=val strings apart and parses the right-hand side of that as YAML. I know for certain this has worked correctly in the past for LocalClient and RunnerClient (I don't recall offhand where WheelClient stood), because I have used it many, many times and even wrote tests for some of it. However this functionality has also regressed more than once (again the Pepper discussion) and I can't speak to the current state of things.

salt/tests/integration/client/test_runner.py

Lines 108 to 135 in 23e0d40

	def test_cmd_sync_arg_kwarg_parsing(self):
	low = {
	'client': 'runner',
	'fun': 'test.arg',
	'arg': [
	'foo',
	'bar=off',
	'baz={qux: 123}'
	],
	'kwarg': {
	'quux': 'Quux',
	},
	'quuz': 'on',
	}
	low.update(self.eauth_creds)
	ret = self.runner.cmd_sync(low)
	self.assertEqual(ret, {
	'args': ['foo'],
	'kwargs': {
	'bar': False,
	'baz': {
	'qux': 123,
	},
	'quux': 'Quux',
	'quuz': 'on',
	},
	})

IMO the best fix is to figure out how to prevent future regressions, to keep this functionality in the *client interfaces and to not to bake this into salt-api.

A concrete example of why this is best done in Salt instead of salt-api is this discussion about the Slack engine. A Slack bot would be accepting arg=val strings because people are typing them into a chat interface -- just like the CLI, just like webhooks, just like Pepper, just like HTML forms, etc. It would be ideal for any module type that goes through the Python API (not just the REST API which of course uses the Python API, just like all the other module types) to not have to reinvent this over and over.

It is easier to put this into salt-api, for sure, and fixing this (and avoiding future regressions) in core may be quite difficult indeed (#39472). But I do feel that is the better solution.

My $0.02. I sincerely wish whoever takes either path good luck and godspeed. 😄

[mattp-]

@whiteinge thanks for chiming in, appreciate your input. I actually did start off with a separate PR to put parse_input in localclient directly (#49886), however after some chat with @cachedout / @terminalmage we were hesitant to put a change in localclient directly that manhandles args since its such a sensitive codepath (some tests surfaced expected behavior in the contrary). however if 'arg={yaml: foo}' is a documented behavior of localclient at the api layer, I think the original change makes sense, and we can change those tests.
That said, I don't call the shots on what goes into salt either :) I lean towards your proposal since runnerclient already works that way. Anyone else have any thoughts/decisions?

[rallytime]

I agree with @whiteinge on this one. I wasn't aware of that history. Thanks for filling that in!

I am definitely curious what @cachedout and @terminalmage may have to say, given @whiteinge's explanation above.

[terminalmage]

I need to re-evaluate this, I did not see all the conversation here.

[mattp-]

per slack convo with @terminalmage im going to close this in favor of #49886 once that is fixed up and ready for merge.

[mchugh19]

WIth #49886 being merged, the release notes and X-Salt-Version header from this PR would still be very helpful and resolve part of #42491