My input, informed by ~8 years of deploying and the most recent problems I experienced with patch application:

1. git am isn't always reliable
2. More automation has the downside of less oversight. This is a problem due to point #1.
3. I'd like to find something better than a bag of .patch files

The biggest recurring inconvenience for deployers is that patches get merged but not removed from /srv/patches so then there are conflicts with the next train branch cut.

This is complicated by the fact that it isn't always obvious that this has happened because the patch that merged might differ from the .patch file in /srv/patches.

A close second for annoying is when patches linger for a long time and the underlying code changes, which also causes the patch to suddenly be in conflict. This is bad but happens somewhat less frequently.

I've argued for years that we shouldn't be cutting a fresh branch every week but that hasn't changed.

In T295925#7602855, @mmodell wrote:

2. More automation has the downside of less oversight. This is a problem due to point #1.

Sure, but if we're checking a few sources of record (be that T276237, a yaml file in production, relevant repo branches, gerrit, etc.) that should provide for more stability and introduce meaningful errors if any of those checks failed. Ideally, these could become a part of https://gerrit.wikimedia.org/r/747544 or whatever other tools Release-Engineering-Team and security patch deployers use.

3. I'd like to find something better than a bag of .patch files

There might be a few solutions for this, but the only one I can think of today is a private gerrit where security patches go through CI and could be more safely pulled/picked to production after they tested well. But since we'll all (theoretically) be on Gitlab sooner than later, perhaps some version of that could be explored there, with CD/k8s MW deployments, and maybe a fallback, less manual process that involved private repos/forks, along the lines of what we've proposed for a Gitlab-based security patch deployment workflow.

5. something else?

I think I have a pretty simple proposal the falls into the "something else" camp.

Let's create a /srv/patches/next and use it to stage any patches for next week's train

Problem

1. Security patch creation /srv/patches/1.39.0-wmf.1/core/01-T1000.patch
2. Security release
   • The patch is still needed for wmf/1.39.0-wmf.1
3. New train copies /srv/patches/1.39.0-wmf.1 ⇒ /srv/patches/1.39.0-wmf.2

😰 Patch already applied

Solution

1. Security patch creation: /srv/patches/1.39.0-wmf.1/core/01-T1000.patch & /srv/patches/next/core/01-T1000.patch
2. Security release
   • ✨Delete /srv/patches/next/core/01-T1000.patch✨
   • The patch is needed for wmf/1.39.0-wmf.1
3. New train copies ✨/srv/patches/next✨ ⇒ /srv/patches/1.39.0-wmf.2

🎉 no patch conflicts

Problems with this

• Security now has to care about 2 (or 3) versions in production

Thoughts?

@thcipriani - so would this be a manual process? Where we look at T276237 (which is hopefully up-to-date) and copy relevant paths/patches over to the next directory? Which releng's tooling then copies to the next proper wmf.x release directory under /srv/patches? Or does /srv/patches/next obviate the need for T276237 and just becomes the canonical source for current security patches? I suppose I'd be fine with either. I'm not certain this changes the Security-Team's current workflow much. Though I guess the latter understanding does as we wouldn't need to update T276237 anymore. I'd still personally prefer 1] some link between Phab and deployment for this information which didn't rely upon a manual process (checking Phab is quicker than hopping on a deployment server, which not everyone has access to do) 2] some smarter code that checks the documented Phab security tasks and/or gerrit for backports when copying over security patches from version to version, though I know this might be a difficult problem to solve.

@sbassett yes, this would be a manual process.

I have no opinions about whether this replaces {T276237} or happens in addition to using that task.

My goal is to decouple managing security patches from the train—make it asynchronous. Currently, the train deployer manages security patches for the next version at the start of the week.

In T295925#7846482, @sbassett wrote:

I'd still personally prefer 1] some link between Phab and deployment for this information which didn't rely upon a manual process (checking Phab is quicker than hopping on a deployment server, which not everyone has access to do) 2] some smarter code that checks the documented Phab security tasks and/or gerrit for backports when copying over security patches from version to version, though I know this might be a difficult problem to solve.

I'm not sure I understand option #1—use phab as the canonical source? Is that what you mean?

For point #2, hrm, that might work: As part of patching we could:

• Crawl /srv/patches/<version - 1>; for each patch:
  • Search gerrit for patches matching the bug number (e.g., curl -sLn https://gerrit.wikimedia.org/r/changes/?q=bug:TXXX&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=MESSAGES&o=CURRENT_FILES&o=DETAILED_ACCOUNTS')
  • Do some basic checks of those patches against patches on disk
  • If we find a matching patch in gerrit, don't copy it over

🤔 What do you think? Would that be reliable (are there always Bug: TXXX trailers)? Are there security implications you'd worry about (this would depend on the checks, but we could check the commit message, the files, the author without toooooo much effort, I think)?

In T295925#7858369, @thcipriani wrote:

I have no opinions about whether this replaces {T276237} or happens in addition to using that task.

I think that task has come in handy at times, but I have no way to gauge how worthwhile it would be to keep it around. I believe it's really only referenced and edited by a few security team and release engineering folks. I see its usefulness primarily as:

1. A convenient place to check the status of security patches if one isn't always on a deployment server.
2. A convenient place to check the status of security patches if one does not have access to a deployment server.
3. A convenient place to discuss problems related to security patches as they surface.

My goal is to decouple managing security patches from the train—make it asynchronous. Currently, the train deployer manages security patches for the next version at the start of the week.

I guess I'm just confused as to what creating a new staging directory accomplishes versus copying over the patches from /srv/patches/wmf.previous to /srv/patches/wmf.new and attempting to apply those. If a new staging directory somehow makes it easier for release engineering to organize and apply current security patches, that's fine. Would that primarily be the onus of your team? Or would there be an expectation security team folks hop on each Monday and provide a thumbs-up for what's in /srv/patches/next?

I'm not sure I understand option #1—use phab as the canonical source? Is that what you mean?

Yes, but that's a bad idea for at least a couple of reasons, even if I personally kind of like it :) The table within T276237 (or lack thereof) might just be another source to check for option #2. Or it might just overcomplicate things.

For point #2, hrm, that might work: As part of patching we could:

• Crawl /srv/patches/<version - 1>; for each patch:
  • Search gerrit for patches matching the bug number (e.g., curl -sLn https://gerrit.wikimedia.org/r/changes/?q=bug:TXXX&o=CURRENT_REVISION&o=CURRENT_COMMIT&o=MESSAGES&o=CURRENT_FILES&o=DETAILED_ACCOUNTS')
  • Do some basic checks of those patches against patches on disk
  • If we find a matching patch in gerrit, don't copy it over

🤔 What do you think? Would that be reliable (are there always Bug: TXXX trailers)? Are there security implications you'd worry about (this would depend on the checks, but we could check the commit message, the files, the author without toooooo much effort, I think)?

Yes, this would just be another way to guard against bad/outdated patches and forgetting still-relevant patches, similar to some things that are already within the deploy_security.py script AIUI. And I envision this as more of a best effort scenario where, if anything was found within various gerrit data, it could be reported upon and potentially thwart some problems. But if a patch doesn't satisfy common conventions (Bug: , etc.) then it obviously can't report any potentially useful information.

Circling back to this since we've had three trains fail in a row with security patch issues.

What if we agreed on the logic for when scap could drop a security patch and automated it?

If scap apply-patchesand deploy_security.py agree on valid security patches, that eliminates manual steps (at least the ones that need no human intervention).

Proposal for when to skip applying a security patch:

1. No Bug trailer
2. Bug: trailer exists, but points to a closed task

Anything else we should check? What do you all think about this idea?

In T295925#9021230, @thcipriani wrote:

Proposal for when to skip applying a security patch:

1. No Bug trailer
2. Bug: trailer exists, but points to a closed task

Sadly, I don't know if these would work consistently :/ I think it'd likely need to look more like:

1. Check gerrit (or gitlab) for backports to master/main or the most recent one or two wmf release branches. Not sure what the best way to automate this would be as Bug: in the commit msg and SECURITY: in the patch subject are used most of the time, but probably not 100% of the time. It might have to be something that looks for recent patch sets/MRs for the affected repo, searches for affected files and then diffs the code? That sounds like it could be complicated though.
2. Have scap apply-patches always try to do a --3way if the first apply --check fails. I feel like this works half the time.
3. Check for public phab tasks based upon Bug:, if it exists, but probably only use this as secondary corroboration for something like (1)
4. Super wish-listy: have scap attempt to update T276237 or a similar report somewhere.

I'd also suggest that we retire deploy_security.py so that we do not have two kinda-similar scripts hanging around that would likely diverge. I was under the impression that scap apply-patches already does most or all of what deploy_security.py was initially designed to do?

In T295925#9021335, @sbassett wrote:

1. Have scap apply-patches always try to do a --3way if the first apply --check fails. I feel like this works half the time.

I can confirm that scap apply-patches uses git am --3way.

I'd like to try to revive this change to the workflow that @thcipriani proposed:

1. Security patch creation: /srv/patches/1.39.0-wmf.1/core/01-T1000.patch & /srv/patches/next/core/01-T1000.patch
2. Security release
   • ✨Delete /srv/patches/next/core/01-T1000.patch✨
   • The patch is needed for wmf/1.39.0-wmf.1
3. New train copies ✨/srv/patches/next✨ ⇒ /srv/patches/1.39.0-wmf.2

We currently have a nightly Jenkins job that pre-tests the patches ahead of the next train. The job runs against the master/mainline branches but uses the patches from the most recent branch dir at /srv/patches (e.g. /srv/patches/1.41.0-wmf.26).

Recently, the job failed which helped us detect a patch that was still applying cleanly for the most recent MW branch, but had a merge conflict with the latest changes in master. The patch was rebased against master, but at that point there was no directory in /srv/patches where to put the rebased version. Creating a dir for the next branch manually solved the problem and allowed the nightly to recover.

Adopting Tyler's proposed change to the workflow would have the following advantages:

• As mentioned by him, it would fix the merge conflicts that sometimes happen when a new security releases is created and the public patch differs from the one in /srv/patches
• If the creator of a patch is aware of a merge conflict with master ahead of time, they can already create a rebase version and put it in /srv/patches/next immediately. Nipping the problem in the bud even before the nightly job runs
• If the nightly job detects a patch failure, /srv/patches/next is the canonical location for the fix against master. No need to create a new dir for the patch manually, this can save significant time if the person who reports the problem is not familiar with the process and is not aware of the simple fix

@sbassett from the point of view of the Security-Team , the only change in the workflow would be to remember to add new patches to /srv/patches/next.

In T295925#9160334, @jnuche wrote:

@sbassett from the point of view of the Security-Team , the only change in the workflow would be to remember to add new patches to /srv/patches/next.

I think some of my concerns from above might still stand, unless you're only talking about creating a /srv/patches/next directory for upcoming patches. That, I suppose, would be a simple enough change, though I'm not sure the Security-Team (or other security deployers) would ever use it? Our workflow is typically:

1. Once a security patch is CR'd and ready to deploy, we scp/rsync it up to a deployment host.
2. We move the patch into the relevant /srv/patches/{wmf-prod-version} directory and git commit. FWI, there should always be a relevant, current wmf prod version (sometimes 2 if mid-week).
3. Check the patch applies in /srv/mediawiki-staging/{wmf-prod-version} and perform any other quick QA tests, if feasible.
4. Sometimes pull to an mw-debug host and prod-test with the mw-debug browser extension.
5. Deploy the patch.

I'm all for making Releng's lives easier, so if having /srv/patches/next helps, great, I just don't think the Security-Team would pay much attention to it unless something went wrong.

dancy opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/888

utils.py: Make select_latest_patches treat /srv/patches/next as latest

dancy merged https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/888

utils.py: Make select_latest_patches treat /srv/patches/next as latest

dancy opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/906

Add scap update-next-patches command

dancy opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/909

apply-patches: Skip .dropped patches

thcipriani merged https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/906

Add scap update-next-patches command

dancy opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/910

scap update-next-patches: Add --dry-run flag

dancy merged https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/909

apply-patches: Skip .dropped patches

dancy opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/912

Finalize "next" patches when prepping a new train branch

thcipriani merged https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/910

scap update-next-patches: Add --dry-run flag

It looks like the message from the patch bot when patches fail to apply for master will need to be updated: https://gitlab.wikimedia.org/repos/releng/scap/-/blob/028d1f88e2d48143c952e2e3bbbb7d7f01a5e001/scap/patches.py#L292. And the associated commands as well.

@dancy I can try to at least put up a draft with those changes before I go on vacation next week, but I could use a quick rundown on your changes to make sure I got the intention right

thcipriani merged https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/912

Finalize "next" patches when prepping a new train branch

jnuche opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/919

Draft: patches.py: Update PatchBot-related code

dancy opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/920

patches.py: Update PatchBot-related code

dancy closed https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/920

patches.py: Update PatchBot-related code

/srv/patches/next is now live on the deploy server!

What does this mean for you?

For the typical case where you want the patches in /srv/patches/<this-week’s-train> to be the source of patches for next week’s train, you don’t need to do or think about anything differently! Patches in /srv/patches/next are automatically kept up-to-date with patches in /srv/patches/<this-week’s-train> as needed.

However, the availability of /srv/patches/next allows us to handle some situations that we couldn’t handle gracefully before.

Scenario 1: A security patch is still needed for this week’s train but that patch should NOT be carried over to next week’s train
Now you can add a /srv/patches/next/…/blah.patch.dropped file to indicate that the corresponding /srv/patches/this-week’s-train/.../blah.patch file should not be carried over when next week’s train is prepped. We’re working on a scap subcommand to make it easy to mark a patch as “dropped”.

Scenario 2: A security patch for this week’s train needs to be different for next week’s train
Now you can add a /srv/patches/next/…./blah.patch.updated file to indicate that the corresponding /srv/patches/this-week’s-train/…/blah.patch file is different and should not be automatically copied into /srv/patches/next. We’re working on a scap subcommand to make it easy to install an “.updated” next patch. When next week’s train is prepped, the .updated suffix will automatically be removed from the patch in /srv/patches/next before the patches are copied to /srv/patches/<new-train>.

Please contact Release-Engineering-Team if you have any questions, concerns, or problems with the changes. Thanks!

dancy opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/978

patches.py: Add "next" handling to update-patch/remove-patch

jnuche opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/979

patches: update automated Phab notification from PatchBot

dancy merged https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/978

patches.py: Add "next" handling to update-patch/remove-patch

jnuche merged https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/979

patches: update automated Phab notification from PatchBot

jnuche closed https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/919

patches.py: Update PatchBot-related code