Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch to s3fs so we can have multiple instances #223

Merged
merged 6 commits into from
Apr 23, 2018
Merged

Switch to s3fs so we can have multiple instances #223

merged 6 commits into from
Apr 23, 2018

Conversation

cmc333333
Copy link
Contributor

Drupal writes "optimized" css/js to the "public" file system, which defaults to the local disk. This is problematic when we have multiple instances, as the files will only be written to the disk of one. It'd also be an issue between deployments/restages, except that we clear caches on the first instance.

Unfortunately, flysystem doesn't override the "public" filesystem, so it couldn't be used to store those files in s3. s3fs can, however, with enough configuration. Notably, it requires apache be configured to proxy s3 files, which requires some extra lifting in our system config.

This PR switches to the s3fs module, handles the apache configs, turns on optimized css/js and bumps up our instance count. It's working in cloud.gov, but I'd really appreciate another test of the Docker version (remember to docker-compose build).

This PR's probably easiest to review changeset by changeset, but let me know if a walkthrough'd be better.

CM Lubinski added 4 commits April 16, 2018 15:50
s3fs will allow us to host our "public" files (e.g. compressed CSS/JS) on S3,
a feature flysystem lacks. We need this so that those files are saved between
deployments (and across instances).

This removes S3 from the docker-compose config so that the s3fs config is only
set when in cloud.gov (or mimicking locally).

Generated twig files are no longer stored in the "public" dir (as that'd touch
S3), so we ignore them in a gitignore.
The s3fs module will _store_ generated CSS/JS files in S3, but Drupal will
still generate URLs assuming they're hosted on the same server. s3fs' solution
is to have apache proxy those requests. Unfortunately, we won't know what the
bucket/region is until we're within the cloud.gov environment; this resolves
that by modifying the htaccess file at bootstrap.

This also updates our Dockerfile to include apache's rewrites & proxies (requiring
developers run docker-compose build). Similarly, we tweak the cloud.gov apache
configuration to include the proxy_http module.

Note that we can't use Apache's ProxyPass directive, as our host names are too
long. See
https://unix.stackexchange.com/questions/420615/proxypass-worker-hostname-too-long
Now that S3 is properly configured, we can bump up our number of instances.
@cmc333333 cmc333333 requested a review from fureigh April 16, 2018 20:29
@fureigh fureigh self-assigned this Apr 19, 2018
.gitignore Outdated
@@ -17,6 +17,7 @@ core
vendor

# Ignore local versions of modules.
storage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this has to do with Twig, not a module, I suggest either changing the comment or giving this its own comment and (one-line) section.

#LoadModule version_module modules/mod_version.so
#LoadModule proxy_connect_module modules/mod_proxy_connect.so
#LoadModule proxy_ftp_module modules/mod_proxy_ftp.so
LoadModule proxy_http_module modules/mod_proxy_http.so
Copy link
Contributor

@fureigh fureigh Apr 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this one to the top (with the other enabled modules).

@@ -0,0 +1,114 @@
LoadModule authz_core_module modules/mod_authz_core.so
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment explaining where this file came from.

LoadModule deflate_module modules/mod_deflate.so
LoadModule headers_module modules/mod_headers.so

#LoadModule authn_file_module modules/mod_authn_file.so
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove commented ones and comment that the new one has been turned on.

@@ -13,6 +13,8 @@ RUN apt-get update &&\
rm -rf /var/lib/apt/lists/* &&\
rm -rf /var/cache/apt/*

RUN a2enmod rewrite proxy proxy_http
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mention needing docker-compose build.

Copy link
Contributor

@fureigh fureigh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments, but all looks good! Thanks for doing this!

@fureigh
Copy link
Contributor

fureigh commented Apr 20, 2018

@cmc333333 I'm running into a couple of issues locally. Here's the primary one:

I ran docker-compose build and docker-compose up and got this:

web_1       |  [ERROR] An error occurred while trying to write the config file: "Unable to    
web_1       |          install the <em class="placeholder">s3fs</em> module since it does     
web_1       |          not exist."

So far I've confirmed that s3fs is present in both composer.json and composer.lock. I didn't immediately spot any clues in docker-compose logs but am taking a closer look.

Also, in case this is related: When I ran docker-compose build, the terminal output included the following:

To activate the new configuration, you need to run:
  service apache2 restart

I'm getting a "command not found" error for service; presumably that's a local shell config issue, so I'll proceed accordingly.

@fureigh
Copy link
Contributor

fureigh commented Apr 20, 2018

@cmc333333 noticed that settings.cf.php hadn't come through quite right. chmod a+w web/sites/default and then re-checking-out that file got it running locally! 🎉

I'm still testing locally, but we may end up needing to change (and subsequently revert) some file permissions in bootstrap.sh. For security reasons, we wouldn't want that directory to be writable in any non-local environment.

@fureigh
Copy link
Contributor

fureigh commented Apr 20, 2018

Upon further consideration: because the permissions issue is an artifact of local development (Drupal's install script helpfully removing writability from certain files), I'll leave bootstrap.sh alone and try sorting it out in a Git hook or two instead. No need for that to hold up a merge of this pull request.

In the meantime, I've confirmed that this is working on staging. We'll want to turn on the page cache too (/admin/config/development/performance), but it makes sense to handle that in a separate pull request.

screen shot 2018-04-20 at 3 50 21 pm

Now testing mimicking locally.

@fureigh
Copy link
Contributor

fureigh commented Apr 20, 2018

Beautiful! I was able to access remotely cached CSS and JS files via localhost without any issues. Thanks for all of your work on this, @cmc333333! I say rebase and :shipit:.

@fureigh
Copy link
Contributor

fureigh commented Apr 20, 2018

One more note: because /web/.htaccess does get rewritten to include the actual environment variables, there's a danger of a developer accidentally committing those values to the repo. Worth thinking about how to guard against that (rather than relying on individuals' git hygiene).

@cmc333333
Copy link
Contributor Author

Thanks @fureigh ! Added a note about committing secrets and we can expand if desired.

@cmc333333 cmc333333 merged commit 474f23a into master Apr 23, 2018
@fureigh fureigh deleted the s3fs branch April 23, 2018 17:46
@fureigh fureigh mentioned this pull request Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants