Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
We need to decide on what preflight checks we want to run for automatic updates.
Let's categorize these into 2 flavors:
1) runtime
2) module enablement/configuration
Proposed resolution
Checks to include
Read only file system- Files writeable (maybe not owned) by web user.
- Patches applied to core / hacked core, See #3050804: Modified code checker
Sufficient disk space- Are there pending updates (update.php needs to be run), See #3053804: Checker: Are there pending updates (update.php needs to be run)
- On the current core version (or 1 less than the security release), See #3053795: On the current core version (or 1 less than the security release)
- Check for environment Variable
DRUPAL_AUTOUPDATE_NOT_SUPPORTED_HELP_URL
and if it is existing show it with a message and the link. How frequently is cron running? When was it run last?Add a check on page build for admin pages to check when the preflight was run last and warn if it is over X hours. Related to cron check above.- Warn loudly if a PSA exists and preflight is failing. See #3053796: PSA exists and preflight is failing
- PHP version is lower than supported version See #3053798: Checker: PHP version is lower than supported version
Button to run the preflight check manually- Site support for HTTPS downloading See #3053799: Site support for HTTPS downloading
- Site support for signature validation See #3053801: Checker: Site support for signature validation
- Is site on current release version of Drupal (where current is either supported minor i.e. 8.5.current or 8.6.current) See above and #3053795: On the current core version (or 1 less than the security release)
- #3054005: Checker: warn for projects that don't have packing info added
Potential channels for warning site owners
Warning on admin pages if it hasn't been run recently- Error on admin pages if it hasn't been run and there's a PSA or available sec update, See above and #3053796: PSA exists and preflight is failing
Ditto if it failed (warning and error in the respective categories)Status report entryIncorporate into update status reportEmail to site owners (which would eventually replace the "update available" nagging)- Other stuff?
Remaining tasks
- Decisions, decisions, decisions
- Implementation
- UX review
- Need a way to disable preflight so it doesn’t annoy on platforms that it wouldn’t work. i.e. pantheon, acquia, platform, etc.
- Wouldn't these platforms just have auto-updates off and wire up their own update/deployment instead?
Steps to test:
To test readiness checks and see failures
- Read only - change permissions on
core
folder tochmod 555 core
or vendor:chmod 555 vendor/composer
- Diskspace - change
DiskSpace::MINIMUM_DISK_SPACE = 999999999999999999999999999
- Then visit
admin/config/automatic_updates
and manually run readiness checks.
Comment | File | Size | Author |
---|---|---|---|
#69 | image (2).png | 10 KB | heddn |
#69 | image (1).png | 26.35 KB | heddn |
#69 | image.png | 36.87 KB | heddn |
#69 | automatic_updates.gif | 3.58 MB | heddn |
#54 | 3043521-54.patch | 43.66 KB | heddn |
Comments
Comment #2
mbayntonWould it be worth arranging for modules to be able to run possibly lengthy/batchable operations and add to the report? Seems like a great idea at first, although I'm a bit curious if there's many valid use cases.
Comment #3
catchShould probably also have a check that there's no pending updates to be run - i.e. ensure people run update.php before updating the code base again.
Comment #4
heddn#3 make sense. Added to the list.
For a pre-flight check of can we run auto update... what would the batch system do? Your description doesn't make sense to me. Is it for something else?
Comment #5
mbayntonMy train of thought there was based on the assumption that some of the core checks would be batched, like inspecting each file for patches. The assumption might be wrong; really I was just wondering if we should provide ways for contrib to participate with the same set of tools core has.
What does this refer to?
Comment #6
heddn#5 The points when we can check are with hook_requirements, cron job, site status page and at run time.
2) is more of the hook_requirements phase.
From what I'm seeing, we want a method for contrib to alter/insert their own checks into this preflight system? Give some examples when this would be necessary? I assume it is needed, but want to see some examples.
Comment #7
catchIs PHP version worth adding here as well? #3039287: Implement changes required to remove support for PHP 5.5 and 5.6 would allow us to detect if someone's running lower than the minimum supported PHP for the version they're currently on. Needs composer or similar to check requirements for the release to update to of course.
Comment #8
mbaynton@catch The "pre-flight check" as it was conceived of at MWDS would be available to site owners to perform well ahead of the new release, which makes taking into account properties of the new release (like its PHP version requirements) a little tricky. The use case of folks in timezones 12 hours offset from US who want to go to bed was brought up, for example. Maybe just PHP version could be checked as a special case that wouldn't require evaluating the new release's composer.json?
The other thing with warning users that their environment is about to be insufficient though is that, unlike these auto update preflight checks that are thinking about who can write to what files etc, PHP version will apply to audiences not necessarily using the auto update pre-flight check too. Making it a dynamic element of PSAs or something seems better to me.
@heddn let's forget I said anything about allowing contrib to influence the preflight checks. I have been unable to think of very compelling use cases either; best I've got is perhaps one would want to write a module that performed additional checks particular to a given popular hosting platform.
So with cron jobs being able to put stuff on the report, there won't be a "re-check now" button necessarily?
Comment #9
pwolanin CreditAttribution: pwolanin at SciShield commentedComment #10
Warped CreditAttribution: Warped commentedWould it make sense, as part of security also, to have a module that flags changed core files? Daily do a compare to the hashed value. You would know then of any added/changed/missing files. Could even have attributes that allowed certain files to be modified (i.e. .htacess, etc) that could be patched, rather than replaced? Does the directory structure as it stands allow some directories that could be prepared in advance, and then just swapped? (Move \core to \backup, move \newcore to \core)? That would allow things to be done in stages, minimizing update time. If that module also zipped the directory any time a change was detected, at update time, a simple zip and compare hashes would verify that no changes had been made in the mean time.
Comment #11
xjmRe: #7, PHP version should be included, yes.
Comment #12
larowlansufficient memory limit to unzip a tarball - assuming that is tarball are involved
Comment #13
heddnComment #14
heddnComment #15
heddnComment #16
heddnI'm not suggesting we do all the preflight checks here in this issue. This is more of a decision / policy issue here. But this implements a read only check and shows some chosen code libraries and gives us an idea of how we could adopt the rest of the checks.
What I want feedback on is the file system library and drupal finder. And any other high-level architecture type things. I'm not looking for code nits or feedback so much on the actual code in question here. It is more of PoC at this time.
Comment #17
Schnitzel CreditAttribution: Schnitzel at amazee.io commentedDuring the DrupalCon Seattle Sprints I discussed Preflight checks for hosting infrastructures that would like to provide a link to some documentation which explains why autoupdate is not supported by the infrastructure.
For this we would like to add to the proposed solution:
- Check for environment Variable
DRUPAL_AUTOUPDATE_NOT_SUPPORTED_HELP_URL
and if it is existing show it with a message and the linkComment #18
Schnitzel CreditAttribution: Schnitzel at amazee.io commentedComment #19
xjmSince the "pre-flight check" metaphor has confused a lot of people, retitling to describe what the feature is actually checking: Your site's update readiness.
From our discussion at DrupalCon Seattle: In addition to checks about the site's state, there's a second type of check we need to do, and that's whether the updates themselves are compatible with auto-update. In our initial MVP, these might exclude updates that:
.htaccess
,robots.txt
,settings.php
, etc.This might be a boolean metadata item on the release node that's in the d.o feed of release information, rather than something we try to calculate in core, since there could be a lot of reasons a release is not auto-updateable. I think this is actually a separate issue from the update readiness check, because it would be checked as part of the update process rather than prior to the release. However, we will probably want to use similar reporting channels and UX to the update readiness check and might want to take both into account for the architecture, which is why I'm summarizing here at @heddn's suggestion.
Edit: If we have a security update that is not auto-updatable, even if it's not highly critical, that might also be something we'll want to issue a PSA about. Most of the time the sec team will know in advance if such an update is going to happen. In the rarer situation where we don't, we could issue the PSA at the same time as the advisory.
Comment #20
xjmComment #21
eiriksmJust wanted to throw this in there as well. What if we made the preflight check a plugin? Could make it easier to get to core, we could do the plugin itself in contrib?
Comment #22
eiriksmforgot the interdiff. I basically just renamed the service to a plugin, and moved the interface. And added the plugin manager.
Comment #23
heddnI like the idea of plugins. More flexibility.
Comment #24
heddnFrom a next steps point of view, we can always easily add to the list of preflight checks. And with all the discussions on this at drupalcon seattle, I think we got pretty decent attention on this issue. I think we're ready to start doing things now.
Let's start doing stuff. I'd like to spin up the rest of the checks into their own issue. And keep the existing file system check. That way we keep things easily reviewable and even open this up for a larger community to contribute. Each of the checks is an easy preflight check. Pratically speaking, let's add a drupal_set_message and entry on the site status page. And flesh out a full file system check. And finish this.
Comment #25
heddnSo, here's the rest of the things I consider as part of the MVP for a preflight. We'll need to add more features and more checks. But is the first pass for an initial implementation.
Comment #26
heddnHmm, we probably don't want to actually run the readiness checks on every page load. I'm going to move that to cron. And add a button to manually run the process. And cache the results. For now, I'm _not_ going to run it via batch API. If needed, we can add that later.
Comment #27
heddnI'll post an interdiff later. Here's the latest.
Comment #28
heddnComment #29
catchIs it worth using 'update readiness'/'update readiness check' for the plugin code as well instead of 'preflight'? Don't have a very strong opinion but it'd make the code match the ui (and some code comments) more. I've never seen preflight used outside an air travel situation, the analogy is fine but not sure it's self-explanatory.
Also I'm not sure about using plugins here. We're never expecting people to configure which plugins run, it's more a way of allowing each check to be provided independently - for that tagged services might be a better option (see https://www.drupal.org/docs/8/api/plugin-api/why-plugins).
Comment #30
heddnSwitches to using tagged services. I'm not sure how helpful the interdiff will be, as a lot of things moved around.
Comment #31
heddnMoved around some magic numbers and add a link from the state report page.
Comment #32
heddnHere we add a disk space checker.
Comment #33
eiriksmJust a couple of remarks from looking at it:
Should we add a flag for enabling/disabling readiness checks as well? Maybe someone wants to enable PSA but not enable readiness checks? In that case I think it makes sense to move this code out of the actual hook and do the same with the logic for PSAs
Can we assign this to a variable and typehint this for easier navigation? Just a preference.
ControllerBase already uses StringTranslationTrait. Also, my personal preference is to pass it to the constructor and assign it to $this->stringTranslation
Interesting pattern. I have not seen it like this before. Are there any advantages to not passing these to the constructor? I feel like the convention is to pass it to the constructor, but I am curious and open to be wrong :)
Also, we probably need a test for the disk space checker :)
Comment #34
heddnHere we add more tests and response to UX feedback from the UX meeting yesterday.
Comment #40
heddnAdding issue credit for ux reviews.
Comment #41
heddnHere we respond to #34.
re 34.4: this is something I read about from alexpott and seems to be something that will probably start to happen more to insulate against upstream change to constructors. But it was implemented wrong, so I've fixed it here.
Comment #42
mbayntonI can't be the only one that gets
"You have requested a non-existent service "automatic_updates.readiness_checker"."
if I try to enable patch 41 on a fresh site, or uninstall early versions of the module and then re-enable after applying 41?Comment #43
mbayntonWhat about something like this to prevent double warnings when you have drupal root and vendor on the same filesystem?
I'm not having success getting the module as a whole to work still.
Comment #44
heddnre #42: I tested this morning with the module uninstalled, applied the patch then installed the module. No errors/issues.
Also updated IS with steps to test.
Comment #45
heddnI like the idea for #43. I've added some test coverage too. Interdiff is against 41.
Comment #46
heddnDiscussed the current state of things in here with @catch. Since some of this work is core-bound, we have to be careful what composer dependencies we use. With that in mind:
Over in #3043235: [Policy, No patch] How to provide automated, in place security updates of Drupal core, we've basically decided to use a "patch" approach for updating.
The main reason for using the external file api (#1 above) was for caching. Since we've basically decided not to hash all the files at run time application of updates (only the files that have changed since the last update will get hashed), the caching argument looses merit.
The reason for 2 levels of readiness checks is that we need to give levels of assurance for automatic updates. We can't say 100%, a priori, that an update will or will not apply if there are some patches against core. But we can warn that the filesystem is patched and therefore, depending on the contents of the update, it might or might not get update. If there is no patching, then we can say 100% the site is update ready. But we also have some things, like disk space or old version of core or any number of things that will 100% make the site not eligible for an upgrade. These are errors. So two levels of readiness.
Comment #47
heddnThis does 46.2. Next up is 46.1.
Comment #48
heddnThis addresses 46.1. I also checked about webflo/drupal-finder dependency. Per catch, that's probably ok to keep. If we need to for core we could either use that project or move something akin to its logic into core.
Comment #49
heddnThis needs a re-roll now that #3046855: Notify site administrators when new PSA is posted has landed.
Comment #50
heddnAnd a re-roll.
Comment #51
heddnWe'll also need to address the issues uncovered with #3050876: PSA UX follow-up: blocking update.php. But this is pretty close now I think. Everything but that is ready for review.
Comment #52
mbayntonWell, my mistake was trying to run it on the current stable core, not 8.7.x-dev. Now that I can actually do stuff:
Otherwise I do like the principle of changing the contents of the the directory as the test of writability. FWIW the current implementation in the core_update module renames the old file to somewhere else to facilitate rollbacks and then writes out a brand new file so if that or a model like it is used, this is the most accurate test.
Comment #53
heddnJust the type of feedback I was hoping for. Very helpful. Thank you for putting time into reviewing the work here.
I think this addresses all the feedback in #52.
disk_free_space
and didn't see any complaints about its accuracy. I'm in favour of landing this check and changing it if we discover it is troublesome.I envision we'll do this:
But for checking read only, copy/delete is sufficient.
Comment #54
heddnReroll after #3050876: PSA UX follow-up: blocking update.php landed.
Comment #55
catchCron isn't necessarily run with the web-user, so should we be somehow doing these checks on cron only, then showing the results on the site based on the behaviour from cron? Otherwise feels like we could get false negatives here.
Comment #56
mbayntonRe: #55, point well made. I didn't catch this because I've been imagining the updates being applied at the end of a page request. This is likely on account of being the guy that wrote the module that applies attended core updates, and that model is necessarily as part of http requests.
If we are going to do it in cron, which kind of makes sense because a) we're already dependent on cron to discover the new version and b) for the few who do wire up cron to run as a different user, we don't want to tell them "you still have to make your files webserver writable", then per @mlhess's suggestion at the Seattle BoF, we should also add a readiness warning that you are succeeding in running cron once an hour or better. I see there's a long list of desired checks in the IS, but maybe this one is easy enough to add now?
Comment #57
mbayntonNever mind on the hourly cron check for now; this is premature because checking for new versions currently happens at most once a day. That really needs to be addressed, but probably not here.
Comment #58
heddnWe can't tell who or how cron is run. It can be run via drush (any system user) or via poorman's cron (web system user). Perhaps we need to add some more documentation to the handbook page that discusses this? But as far as readiness checks, I struggle to think we can really do much about false failures/positives. Unless you we have the checkers run via a drush command outside of drupal cron. Hmm, it wouldn't work in core but it would in contrib.
Comment #59
heddn#56 errors on frequency of cron:
We already add an error when cron or something isn't running the checks often enough.
Comment #60
mbaynton#59, that's fair, but that's not what the message
value
says :PIf I saw that myself I'd most probably resolve it by manually running the readiness checks, which wouldn't say anything about my site actually getting updated in a timely manner when it got to be time to. I still think there's a point in explicitly and separately checking that *cron* is running in a timely fashion & I'll submit a patch for it, unless I'm missing something.
#58, my read of @catch's suggestion is essentially execute the ReadOnlyFilesystem check inside drupal cron and have the online readiness checker just report on its results. That seems doable in core or contrib? Or is your concern that since the check itself wouldn't occur if you manually re-ran the readiness check, users tinkering with permissions could be frustrated/confused by the delay in clearing the readiness error?
Comment #61
mbayntonActually, perhaps the cron running frequently and writable files check should be combined or work in parallel - drupal cron might get run as the webserver user and as another system user, we want to report filesystem writability problems only if none of the cron runs in the last hour were running in such a way that writing was possible.
Comment #62
heddnCan we address the question about cron in a follow-up? It is a tricky issue. Some folks run it via drush, some via an external cron calling the cron url, others use ultimate cron, others don't run it at all.
I've had it where unix cron is /usually/ where cron is run, but to force something it gets run manually via drush. And they are running it as a non-web user. And someone else on the team might login as themselves and run it too. Then unix cron kicks in and runs it as the web user. Lots of variables at play. Let's address in a follow-up and land what we have.
Comment #63
heddnI've opened #3052541: Checker: File owner and php script user are different as a follow-up.
Comment #64
heddnSo we can keep things moving in follow-ups, I'm going to land this. We can 1) roll it back or 2) fix it in a follow-up if there are thing we need to change. Because the size of this patch is making it hard to start working on the smaller bits that depend on the plumbing provided by this patch.
Comment #66
heddnComment #67
heddnI've updated this issue with the current linked child pages. And also re-opened it as a META so we can track progress.
There are two items in the IS that don't have follow-ups opened.
DRUPAL_AUTOUPDATE_NOT_SUPPORTED_HELP_URL
and if it is existing show it with a message and the link.Comment #68
heddnComment #69
heddnI need reviews. Lots of them. That is what is currently blocking commits on several checkers. Starting next week on Tuesday, I'm going to take the least controversial of the checkers and committing them after doing my own self-reviews. So please help out and give reviews so I don't have to resort to that.
The lone exception to Tues is the modified files checker. I'd really, really like a review on that one. But even it, if I don't get a review by EOW I'll be looking to land too.
I'm also attaching a visual of what these checkers look like below. So you can get a feel for what things look like.
All of the patches and existing checkers applied (except modified files checker):
Modified files checker:
Status report page (with a mix of errors and warnings):
Status report page (with only warnings):
Comment #70
ressa CreditAttribution: ressa at Ardea commentedGreat work so far @heddn.
I was able to trigger the disk space warning, by upping
MINIMUM_DISK_SPACE
in src/ReadinessChecker/DiskSpace.php:Logical disk "/app/web" has insufficient space. There must be at least 1.0E+47 megabytes free.
I get the message
Drupal PSA endpoint http://localhost/automatic_updates/test-json is unreachable.
on theadmin/reports/status
page.I also see this in
automatic_updates.settings.yml
, so perhaps the PSA function is simply not ready yet?I am using Lando, and it seems like it auto-corrects permissions after I
chmod 555
the core folder, so I can't test the writability check.Comment #71
heddnThanks for everyone's help here. All the base checkers are in now for D8. And we have #3063125: [META] backport readiness checkers to Drupal 7 opened for backporting them to D7.
Comment #73
tedbowWe are now porting this portion to core #3162655: Create Automatic Updates Readiness Checks in new experimental module if anyone wants to help.