craftcms / cms

Build bespoke content experiences with Craft.
https://craftcms.com
Other
3.2k stars 617 forks source link

Site Exits from "Offline" Mode when Applying the Project Config #9855

Open josephkerkhof opened 2 years ago

josephkerkhof commented 2 years ago

Description

While performing CMS upgrades using project config, it is often required to put the site into "offline" mode first (using the craft off command) before performing system upgrades. During upgrades, it is also common to apply changes to the CraftCMS Project Config.

In Project Config (project.yaml) there is this value:

system:                                                                                                                                                                                                                                                                                         
  live: true

Then, when running craft project-config/apply, it takes the site out of "offline" mode and turns it back on to "online" mode, possibly before the developers/operations people are ready.

Might it be possible for craft off to temporarily supercede the value in the Project Config and for the site to remain in "offline" mode until the craft on command is issued?

Steps to reproduce

  1. Take any CraftCMS site setup using Project Config.
  2. Run craft off.
  3. Apply the Project Config: craft project-config/apply.
  4. Observe the site is now back in "online" mode.
brandonkelly commented 2 years ago

Yeah this is a known issue – see https://github.com/craftcms/cms/issues/6729#issuecomment-707261113 – however we don’t have a good way of solving it yet. Will keep this open and update if we come up with something.

MoritzLost commented 2 years ago

@brandonkelly Mabye you could just exclude system.live from the project-config? I don't think the on/off state of the system really needs to be part of the persistent configuration, since it's almost always used temporarily for maintenance. Is there a single use-case where I want to store the system.live value as off in the project-config?

Not sure how much work this solution would be from a technical perspective, but from a user perspective this makes perfect sense.

brandonkelly commented 2 years ago

We aren’t just going to stop tracking something like the system live status in project config out of the blue.

thupsi commented 2 years ago

I can relate to both sides of the argument (tracking or not tracking the system live status in project config):

Maybe make it (the tracking) optional so as to accommodate all kinds of workflows? Or, have an option to get the status from an env variable?

EDIT: Somewhat relevant discussion about setting lightswitch values in .env EDIT 2: Hmm, maybe the "env" suggestion would not be helpful in the OP's case if it was the only option. So the ideal situation would be to have two independent options: "on/off/from-env" AND "tracked in project config?" Of course, it is entirely possible I'm overcomplicating things :)

bencresty commented 2 years ago

+1 for leaving this setting out of the project.yaml

Every time we deploy a website to staging or a new production env to test the cms or background tasks via a command controller the site gets unintentially, unwanted (and most of the time even un-knowingly!!) on-line for the world to see, while it was specifically set offline on that hosting for a reason.

thupsi commented 2 years ago

@bencresty As a temporary workaround, maybe you could have system.live set to off on your local dev server and enable the setting Access the site when the system is off for users on that server. I know.. it sounds weird.

bencresty commented 2 years ago

@thupsi thanks for thinking along. Yeah I know and I did that before but it's obviously not a solution to the problem as that would also go to the production server when deployed to prod. I've changed the flow now by leaving it online and added an IP block to all except allowed IP's on the staging server in the .htaccess though

thupsi commented 2 years ago

@bencresty Sure, there are so many different workflows. That's why I think it would be best to have the option to choose what is best for each specific case. Also, have you considered the Knock-Knock plugin? Of course, that's if you are not opposed to installing more plugins than strictly necessary.

nickdunn commented 2 years ago

Could this be solved by setting live = false as an environment variable? I currently set the site name this way, but it doesn't seem to work for the live setting.

system:
  edition: pro
  live: $SITE_LIVE
  name: $SITE_NAME

Edit: I'm using this in my general.php file now which works, so I can switch the site into maintenance mode with an env var before I do a deploy, and switch it back on when I'm happy the deploy has completed:

'isSystemLive' => (getenv('SITE_LIVE') === '1'),

MoritzLost commented 2 years ago

@nickdunn Interesting workaround! But how would you go about changing the environment variable only during deployments? Changing the environment the deployment scripts run in won't have any effect on PHP-FPM … or are you doing that manually? We're using Laravel Forge to deploy automatically whenever a PR is merged into the main branch, so any manual solution won't work, at least for us.

nickdunn commented 2 years ago

I'm using an Azure App Service and I manually change the env var within the Azure portal, which restarts my Docker container. Our deployments are automated in a similar way so we could build this into the deployment script to update the environment variable and cycle the app container before starting the deployment. However my client's own release process forbids continuous deployment through to production, so a manual step works for us.

Does Forge have an API your deployment routine can use to update environment variables and restart apps maybe? Or clear opcache?

MoritzLost commented 2 years ago

I'm using an Azure App Service and I manually change the env var within the Azure portal, which restarts my Docker container. Our deployments are automated in a similar way so we could build this into the deployment script to update the environment variable and cycle the app container before starting the deployment. However my client's own release process forbids continuous deployment through to production, so a manual step works for us.

Yeah, that's understandable … a manual step is not a problem if you deploy manually (to production) anyway. Though it's still something that's easily forgotten.

Does Forge have an API your deployment routine can use to update environment variables and restart apps maybe? Or clear opcache?

Forge is a bit closer to the metal (or rather, the VPS), more of a general server management tool. The environment editor just writes to the .env in the project root directly. The deployment script is just a list of commands that get executed directly on the server. So in theory, you can do anything you want (restarting PHP-FPM to clear the OP Cache is already the last step in our deployment scripts). I guess writing to the .env file directly would be an option – though it would still leave the system offline if any command in the deployment script fails (deployments aren't atomic).

Anyway, thanks for the insight, I'm sure it would be possible to include this workaround in our build script. Though I still think the issue should be addressed in the core (since the current behaviour is unexpected and potentially site-breaking).

nickdunn commented 2 years ago

Agreed. Maybe this thread should be restructured to a feature request, to remove "live" status from project config?

MoritzLost commented 2 years ago

Agreed. Maybe this thread should be restructured to a feature request, to remove "live" status from project config?

Either that or maybe have the off and on commands somehow supersede the project config. Might be easier to implement in a backwards-compatible way than deprecating the system status in the project config.

brandonkelly commented 2 years ago

I’ve just added environment variable support to the System Status setting for the next Craft 3 release – see #10111.

brandonkelly commented 2 years ago

I’ve just added environment variable support to the System Status setting for the next Craft 3 release – see #10111.

Craft 3.7.22 is out now with that change.

JshGrn commented 2 years ago

So when I change that from 'Online' to a env variable then push it up 'off'. I add the new var to the .env on the server but it has not listened to it, when I run php craft off/on on either dev env or prod it changes the project.yaml file?

Am I doing this wrong?

Server -> php craft off -> sync db down to dev -> do development -> push changes up to server -> project/apply -> php craft on

brandonkelly commented 2 years ago

@JshGrn I wouldn’t recommend using on/off coupled with setting it to an environment variable. This is just an alternate way of toggling site status, for environments that have a quick way of modifying environment variables.

MoritzLost commented 2 years ago

Great change regarding environment variable support for the System Status!

Though I would still love to see an update to the on / off commands that allows them to supersede the system status from the project config. The environment variable approach doesn't work well for more "traditional" deployments that aren't atomic, or require additional steps that come with their own downsides as mentioned above. And this behaviour would arguably be what you expect from that command, since it's mostly intended for deployment scripts as far as I can tell.

Right now the deployment script recommended in the Deployment Best Practices article will result in the site being live while migrations are being run, which could break some sites.

brandonkelly commented 2 years ago

Right… I didn’t close this because I don’t think this issue is resolved. Just tangentially related, given @nickdunn’s comment.

JshGrn commented 2 years ago

I think it would be great for the on/off command to edit the env file and ditch it from the project config completely, this would solve the last comment above from @MoritzLost

Edit: posted at the same time as Brandon's comment, is this something likely to happen in 3 or will this be for 4?

brandonkelly commented 2 years ago

It’s not possible for Craft/PHP to modify an environment variable that would affect other requests.

MoritzLost commented 2 years ago

It’s not possible for Craft/PHP to modify an environment variable that would affect other requests.

Not the environment literally, but how about @JshGrn's suggestion to write the value to the .env file? Look for a well-known key like SYSTEM_STATUS in the .env and change it's value accordingly. Similar to the setup commands for the app ID and security key that write to the .env file. Combined with the ability to set the system status to an environment variable, this would allow the deployment script to turn the system off for good, until it's turned on again at the end of the script. The on / off command could even accept an additional parameter to specify the name of the env variable that's used for the SYSTEM_STATUS. This way, this change would be completely backwards compatibility, if the commands retain their old behaviour if the parameter is missing. Something like this:

php craft off --env-variable=SYSTEM_STATUS

Feels like a perfect solution. Or am I missing something here?

nickdunn commented 2 years ago

At risk of overthinking this, maybe an HTTP endpoint that can be called with a token to switch the site on/off would work well here? Deploy scripts could make the request to the site to put it into the desired state before doing a deployment.

This feels like “state” after all so managing it in the database could be an option.

Apps modifying their own environment variables doesn’t feel like a safe pattern to me.

JshGrn commented 2 years ago

I like @MoritzLost's answer because that is what I meant, however I understand @nickdunn's concern on apps modifying their own env variables.

Perhaps a file should be stored in storage in the same way that Laravel handles its up/down? If this file exists then the application is down, if it does not the application is up.

This removes any changes from the project config and additionally removes the database query/store.

The above would resolve this issue completely as far as I am aware.

EDIT: I am not sure how it works on multisite installs but I guess that could be an issue, perhaps a down folder with individual siteID's as the down file in that instance, so by default with one site it would be a file like this: storage/runtime/system_status/1

Perhaps also allow --message and store the message in the file?

brandonkelly commented 2 years ago

UPDATE: This change was reverted – see below.


It’s not possible for Craft/PHP to modify an environment variable that would affect other requests.

Not the environment literally, but how about @JshGrn's suggestion to write the value to the .env file? Look for a well-known key like SYSTEM_STATUS in the .env and change it's value accordingly.

Doh, of course. Alright, I’ve just added a new --env option to both the on and off commands. When passed, the commands will update the environment variable in .env referenced by system.live in the project config, rather than updating the project config directly.

Note that for off --env --retry=x to work, system.retryDuration in the project config will need to reference an environment variable as well, as --env applies to both values.

JshGrn commented 2 years ago

This is great - thanks, but I still think a lock file would be more suitable as I think you are correct in that the env file really shouldn't be handling if the site is up or not. I like the way Laravel handles this, perhaps someone else could also have a thought on this?

MoritzLost commented 2 years ago

@brandonkelly Thanks for the update, that's a great improvement. I'll implement this in all our sites once it's live!

I've updated my answer to the original question on Craft.SE to address this update. I'll try to add step-by-step instructions once this update is live.

This is great - thanks, but I still think a lock file would be more suitable as I think you are correct in that the env file really shouldn't be handling if the site is up or not. I like the way Laravel handles this, perhaps someone else could also have a thought on this?

@JshGrn I don't really see the problem – what's the difference between a separate lock file in a subdirectory and editing the .env file? In theory I agree that apps shouldn't be able to modify their own environment. But Craft uses this approach in a very limited capacity and only exposes that capability through the command line. So an attacker would need to either (a) be able to execute console commands on your server or (b) have write access to your .env file. In which case you have bigger problems. And if an attacker has write access to your server, there's not much difference between the .env file and a separate lock file in a subdirectory.

I'd say if you're worried about Craft editing the .env file in console commands, the issue is more with using an .env file at all. Which you're not forced to do after all, it's only included in the starter project, not in the core. So you could also remove that potential vulnerability by managing the environment in your server management / deployment tool instead of an .env file.

JshGrn commented 2 years ago

I am being more pedantic than anything else I think... I just feel like the .env file itself shouldn't be changing that much except from actual environment changes, but I guess this is okay, its just opinionated at the end of the day. 😄

brandonkelly commented 2 years ago

Now that I think about it a bit more, the --env thing was a bad decision. Changes to a .env file will only affect the current server, but Craft is very commonly used in load-balanced/ephemeral environments where a .env change would only affect whatever server the command was actually run on. So I have reverted that.

I still think a lock file would be more suitable as I think you are correct in that the env file really shouldn't be handling if the site is up or not. I like the way Laravel handles this, perhaps someone else could also have a thought on this?

A lockfile would have the same issue. It really needs to be stored in the database to be safe. (Cache would sortof work too since load-balanced/ephemeral environments should be using a separate cache server like Redis, but it’s not what it was designed for – the cache should never be the source of truth for anything.)

That said, Craft already has:

Adding yet another thing to keep track of system status feels like a step in the wrong direction. We should be simplifying, not making it more complex.

So I am thinking for Craft 4 we will:

JshGrn commented 2 years ago

There is unfortunate circumstances that would argue against the database as far as I am aware such as putting the site into maintenance mode and then pulling down the database, performing updates/changes that may have db changes and pushing it back up to prod you would need to make sure you turn maintenance mode back on prior to restoring the db otherwise you would unintentionally put the site live again.

Laravel's migrations are much, much easier to manage and perform than Yii/Craft unfortunately, so in some instances where I would write a migration to make some changes sometimes its just much, much quicker to do the above to fix something than battle a migration, perhaps its just me and Yii. (I thought I read somewhere you were looking at other frameworks for 4?)

I think the lock file is the best thing moving forward for this, at least for Craft 3. The lock file will present no issues unless using in load balanced environments, even then it can be mitigated easily with less pitfalls than using the database. If I ran into that issue my fix would either be part of the deployment which would ask each balanced server to run the php craft off command or map a shared storage folder to a storage instance which could contain the lock file similar to a database server. This is the usual case for shared assets anyhow that are not using S3 etc? AWS you would use multi-attach EBS volumes as your storage (which would contain the lock).

I like all suggestions for Craft 4, I really can't see many prod environments where isSystemLive can be set (settings disabled in prod) so this shouldn't take precedence IMO. The project config shouldn't contain this information either (as this is the reason the topic is open). Maintenance mode is the best solution here and I really think the lock file is also the best solution coupled with the storage solution above.

brandonkelly commented 2 years ago

I think the lock file is the best thing moving forward for this, at least for Craft 3. The lock file will present no issues unless using in load balanced [and ephemeral] environments, even then it can be mitigated easily with less pitfalls than using the database.

That’s a big “unless”—big enough that I decided to revert the prior .env change. Not going to happen.

We are looking into making it possible to configure Craft with additional “filesystems”, similar to Volumes except not Asset-specific, for Craft 4. Perhaps we could store a lockfile in the runtime filesystem, which defaults to storage/runtime/ but could be configured to use S3 instead, and shared across multiple servers.

JshGrn commented 2 years ago

Maybe a env path to a lock file, defaults to storage when not set. This would resolve those issues?

If on environments which need to lock at the same time then there will be common storage volumes setup so they could set it in the env.

LOCK_FILE=/mnt/shared/lock

Maybe this could accept ‘database’ too for really allowing the user to pick.

brandonkelly commented 2 years ago

If on environments which need to lock at the same time then there will be common storage volumes setup so they could set it in the env.

We’ve been down that road. Most providers don’t allow mounting a file system.

JshGrn commented 2 years ago

AWS do, DO do now and I’ll be very surprised that others don’t. I specifically have used it in both of them providers.

bencresty commented 1 year ago

@brandonkelly

Please, when will this issue get fixed? It's months later and still the system which says 'OFFLINE' keeps being ONLINE. Which is just plain wrong.

When a user hits the switch to put the system OFFLINE (and click save) it should do what the switch says and go OFFLINE.!!!

image

Now, everytime we'd like to put a system offline and be surprised again it just doesn't work we keep needing to go back to this issue thread only to find out it's still not fixed.

Please!!! Fix this. It's just plain weird and frustrating.

Thanks in advance for all future times we'd like to put our system offline!

brandonkelly commented 1 year ago

@bencresty The only issue is that it’s awkward that the on/off commands are competing with the global System Status. I have a plan on how to fully resolve this for Craft 5, but it involves some breaking changes so not possible before then.

JshGrn commented 1 year ago

I don't see why the lock file can't be used rather than a variable that reads from ENV or database... Even the option to use it... That's the best solution IMO and what other frameworks use.

darylknight commented 1 year ago

Is this a common thing to do? I've literally never turned a site off before deploying

MoritzLost commented 7 months ago

@brandonkelly Tiny bump for this issue :)

Is there still a plan to resolve this in Craft 5? I'm running into this issue every time I deploy a change that requires some manual work after deployment. In those cases, I want to take the site offline before the deployment, run the deployment, then do the manual steps and finally take the site online again. However, because the system status is set to on again during the deployment, I have to watch the deployment closely and use php craft off again as soon as the deployment has applied the project config. This is really awkward.

I personally don't need the system status in the project config – I would argue it's a temporal property of the production environment, not of the project. But if it's still needed, the system commands should always supersede the project config.

In particular, this is important for the Entrification. After the deployment has run, we still need to execute the entrify commands on the live server, and before that half the content will be missing. For this scenario, I want to turn the site off until all the content is converted.

brandonkelly commented 7 months ago

@MoritzLost It’s on the list, yeah.