[22:16:36] Pchelolo: I'm curious why the localRpc variant is needed. I thought MW already treated shellbox as optional and fell back in shell command to local execution as before? I think the change makes sense, I'm just curious as to how come it wasn't needed before and what else I might be missing. [22:20:32] Krinkle: for shellouts, it will fallback to local execution. But for RPC (executing provided PHP code), it currently only supports a remote executor [22:21:25] so far only Wikidata constraint checking (evaluating PCRE regexes) is using the RPC mode [22:22:03] right, yeah, I was wondering about how that would map to the boxed command model. I'm guessing it uses some kkind of pseudo command that maps to a wmf-configured route of sorts? [22:22:10] * Krinkle tries to find wikibase caller [22:22:43] https://codesearch.wmcloud.org/deployed/?q=constraint-regex-checker&i=nope&files=&excludeFiles=&repos= [22:22:52] ah, it calls straight into getClient [22:23:22] * legoktm nods [22:25:40] legoktm: If i understand correctly, 'service' is already optional, that is, there is no hard requirement to set up 'constraint-regex-checker' as long as you have a shellbox server at all, it'll default to the default, but it's a way to allowing config to tune/balance things etc, is that right? [22:26:13] yes [22:26:19] also we explicitly have no default configured in prod [22:26:47] Krinkle: for prod use-cases this local fallback is not really needed, but for third-party or for testing it's gonna be quite useful [22:29:24] Pchelolo: yeah. how do we distinguish between call and shell in the default though? [22:29:43] the wikibase caller doesn't seem to specify an action [22:30:14] these are different methods on Client [22:30:42] for shell, we go via ShellCommandFactory and it decides whether to use shellbox or not. [22:30:54] Ah, I see. Client only has "call()" no "shell()" [22:30:59] for RPC-style PHP shellouts there was no abstraction [22:31:17] higher then the Client. So you could only do RPC remotely, not locally [22:31:26] (specifically the factory decides between a LocalExecutor and a RemoteExecutor) [22:32:59] right, ok. I was thrown off by shell going via sendRequest() called outside Client class [22:39:01] legoktm: if I understand correctly, routeName() is required for remote shell commands, which seems like another layer of protection against essentially allow-listing things that run remotely, and I'm guessing then that anything that uses boxed command, but doesn't have a route, or a route that we don't have configtured, will fail within the MW php process already? [22:39:12] I'm looking at `pagedtiffhandler-metadata` for example, but not seeing any reference to it apart from the caller. [22:39:25] I'm guessing there's a wildcard somewhere [22:40:02] basically there are two ways to route shellbox requests, $wgShellboxUrls, which controls the hostname the request is targeted towards, and routeName(), which adjusts the path used in the query [22:40:24] http://{url...}/shell/{routeName} [22:41:06] currently we only vary on ShellboxUrls, sending each application to a different envoy proxy, which hits a different LVS, leading to that shellbox service [22:41:44] in the future, we intend to use a k8s ingress, which would let us have one inbound URL, and have k8s route requests to each shellbox deployment based on the path (routeName) [22:42:25] the shellbox server will respond to any route name, so they're kind of arbitrary right now [22:51:58] legoktm: hm.. is this where the routeName is shifted/ignored? https://github.com/wikimedia/shellbox/blob/c1ff786f3bb847a5e609b75c021038acc180cfe3/src/Server.php#L100 [22:52:44] after that $components is the routeName, which doesn't get used, so yeah [22:53:38] and then ShellAction uses it to... confrim that the submitted params/json blob contained the same route [22:53:42] but otherwise only reads from params [22:54:07] cool