cms icon indicating copy to clipboard operation
cms copied to clipboard

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

Open josephkerkhof opened this issue 3 years ago • 41 comments

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.

josephkerkhof avatar Sep 20 '21 17:09 josephkerkhof

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.

brandonkelly avatar Sep 20 '21 21:09 brandonkelly

@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.

MoritzLost avatar Sep 21 '21 14:09 MoritzLost

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

brandonkelly avatar Sep 21 '21 22:09 brandonkelly

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

  • If tracking is disabled it could maybe catch some people off-guard when the deployed app's state doesn't match the source of the deployment
  • If tracking is enabled again some people are getting caught off-guard when the live site is enabled, just because it's enabled on local

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 :)

thupsi avatar Sep 22 '21 07:09 thupsi

+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.

bencresty avatar Sep 27 '21 16:09 bencresty

@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.

thupsi avatar Oct 01 '21 08:10 thupsi

@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

bencresty avatar Oct 01 '21 11:10 bencresty

@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.

thupsi avatar Oct 01 '21 11:10 thupsi

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'),

nickdunn avatar Nov 16 '21 09:11 nickdunn

@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.

MoritzLost avatar Nov 16 '21 15:11 MoritzLost

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?

nickdunn avatar Nov 16 '21 16:11 nickdunn

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).

MoritzLost avatar Nov 17 '21 08:11 MoritzLost

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

nickdunn avatar Nov 17 '21 09:11 nickdunn

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.

MoritzLost avatar Nov 17 '21 10:11 MoritzLost

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

brandonkelly avatar Nov 18 '21 07:11 brandonkelly

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.

brandonkelly avatar Nov 23 '21 19:11 brandonkelly

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

JshGrn avatar Nov 25 '21 15:11 JshGrn

@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.

brandonkelly avatar Nov 26 '21 15:11 brandonkelly

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.

MoritzLost avatar Nov 26 '21 16:11 MoritzLost

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

brandonkelly avatar Nov 26 '21 16:11 brandonkelly

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?

JshGrn avatar Nov 26 '21 16:11 JshGrn

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

brandonkelly avatar Nov 26 '21 18:11 brandonkelly

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?

MoritzLost avatar Nov 26 '21 18:11 MoritzLost

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.

nickdunn avatar Nov 26 '21 19:11 nickdunn

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?

JshGrn avatar Nov 26 '21 22:11 JshGrn

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.

brandonkelly avatar Nov 28 '21 20:11 brandonkelly

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 avatar Nov 29 '21 16:11 JshGrn

@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.

MoritzLost avatar Nov 29 '21 16:11 MoritzLost

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. 😄

JshGrn avatar Nov 29 '21 16:11 JshGrn

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:

  • an isSystemLive general config setting (takes precedence if set)
  • the system.live project config setting
  • the concept of “Maintenance Mode”, used to keep track of whether someone is currently performing an update, as a way of preventing conflicts where two people try to update at the same time

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:

  • Deprecate isSystemLive
  • Repurpose Maintenance Mode to be the new primary way of determining whether the system is live, rather than having anything to do with updating (we are going to be dropping the ability to update within the control panel when Dev Mode isn’t enabled anyway, so conflicts aren’t a concern anymore)
  • Drop system.live from the project config

brandonkelly avatar Nov 29 '21 19:11 brandonkelly