@taavi also mentioned the behavior to apply a manual security patch should probably be integrated with scap, I agree:

jnuche: and I wonder if that script should be integrated in scap entirely.. having 'run this script you just downloaded' as the official workflow isn't ideal especially for security stuff

In T328667#8582143, @jnuche wrote:

@taavi also mentioned the behavior to apply a manual security patch should probably be integrated with scap, I agree:

IME, there are lots of random things that can go wrong with security patches that aren't caught by niceties like CI, public code review, etc. which is why security deployments have remained a mostly manual process for as long as they have, with the deploy_security.py being kind of an unofficial way to make the process easier at times.

Seems like this happened and broke the script: T329602: deploy_security.py hangs after scap

dancy updated https://gitlab.wikimedia.org/repos/releng/release/-/merge_requests/13

T329602: Revert "deploy_security: Add --pause-after-testserver-sync to scap sync-file"

dancy merged https://gitlab.wikimedia.org/repos/releng/release/-/merge_requests/13

T329602: Revert "deploy_security: Add --pause-after-testserver-sync to scap sync-file"

@dancy @kostajh et al - Can we resolve this for now? The issue was addressed but I guess technically the problem wasn't solved, IIUC? So not sure if we want to leave this open or stalled or something.

It was reverted so either the task should be declined or someone needs to figure out the correct way to re-add it, I think.

(At a guess, subprocess.run only forwards the output of the subpocess when that flushes its output buffer, and scap isn't doing that. Popen allows fine-tuning output buffer controls so maybe that should be used instead.)

In T328667#8677203, @sbassett wrote:

@dancy @kostajh et al - Can we resolve this for now? The issue was addressed but I guess technically the problem wasn't solved, IIUC? So not sure if we want to leave this open or stalled or something.

I'm just here because I handled the revert of the change made to deploy_security.py. @Ladsgroup is who attempted the change. I have no opinion on the disposition of this ticket. Seems like T329602 can be marked resolved though.

In T328667#8677438, @Tgr wrote:

(At a guess, subprocess.run only forwards the output of the subpocess when that flushes its output buffer, and scap isn't doing that. Popen allows fine-tuning output buffer controls so maybe that should be used instead.)

The change made in https://gitlab.wikimedia.org/repos/releng/release/-/merge_requests/11 added --pause-after-testserver-sync to the call to scap sync-file made by deploy_security.py. --pause-after-testserver-sync by design wants to interact with the user on stdin/stdout. However, scap sync-file is being executed with stdout/stderr redirected to a pipe, so the user never sees the prompts.

The next attempt to make this work needs to be fully tested in the train-dev environment before merging.

In T328667#8677811, @dancy wrote:

However, scap sync-file is being executed with stdout/stderr redirected to a pipe, so the user never sees the prompts.

deploy_security.py does redirect stdout/stderr, it's just apparently not working.

In T328667#8678423, @Tgr wrote:

In T328667#8677811, @dancy wrote:

However, scap sync-file is being executed with stdout/stderr redirected to a pipe, so the user never sees the prompts.

deploy_security.py does redirect stdout/stderr, it's just apparently not working.

The redirection is the problem. It hides the "do you want to proceed with further deployment" prompt that --pause-after-testserver-sync adds. To deal with that I recommend disabling stdout/stderr redirection for the run('scap sync-file ...) (and any other place run() is used but its return value is not).

Oh gosh, I messed it up. I wrote both the function that calls subprocess (the whole file actually) and the change that makes it wait for stdin making everything collapse. I should have known better. In my defense, I have none. I will take care of it once I'm properly back. So far the revert should be good for now (which is done). Sorry for this.

A rather simple solution is to maybe run scap that just lands it into mwdebug and exists (I don't know if it's possible) and let deploy_security ask the user and handle the response.