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

Copy confd templates from /var/www/drupal #7

Closed
wants to merge 1 commit into from

Conversation

birkland
Copy link

@birkland birkland commented Dec 2, 2020

Overview

This allows confd templates to be defined and maintained as part of drupal development in idc-isle-dc. Any confd config file or template under codebase/rootfs/etc/confd/ (more accurately, present to the drupal container as /var/www/drupal/rootfs/etc/confd/) will be copied to the corresponding place under /etc/confd before rendering templates. This allows developer control over confd templates for Drupal without having to build new buildkit images every time.

To Test

  1. Build the images in this PR via ./gradlew build
  2. Take the as-built tag (e.g. something like upstream-20200824-f8d1e8e-12-gd582c0a), fo into idc-isle-dc development branch and use that value for TAG in .env. Rebuild docker-compose.yml (or make reset), and start up the environment. It should start without error (i.e. this change causes no harm if there is no rootfs directory.
  3. Now stop the stack, create a file such as codebase/rootfs/etc/confd/foo.bar. Start up the stack. Hop into the drupal container interactively and verify that your file is now under /etc/confd.

A subsequent PR to idc-isle-dc will leverage this capability to add confd templates for settings.php.

This allows confd templates to be defined and maintained as part drupal
development in idc-isle-dc.  Any confd config file or template under
codebase/rootfs/etc/confd/ (more accurately, present to the drupal
container as /var/www/drupal/rootfs/etc/confd/) will be copied to
the corresponding place under /etc/confd before rendering templates.
This allows developer control over confd templates for Drupal without
having to build new buildkit images every time.
@birkland birkland requested a review from emetsger December 2, 2020 20:35
@emetsger
Copy link

emetsger commented Dec 2, 2020

@birkland so you have determined to place settings.php under the management of confd and not go with envsubst?

Copy link

@emetsger emetsger left a comment

Choose a reason for hiding this comment

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

@birkland looks OK to me! Can you confirm your approach with settings.php then is to use confd and not envsubst?

@birkland
Copy link
Author

birkland commented Dec 2, 2020

@emetsger Yes, I did a few experiments that demonstrated that when the drupal container starts, the settings.php file is actually not edited by ISLE. That means it's safe to use confd, and we don't immediately have to re-think how config is done.

As you know, there is a utilities.sh that defines a update_settings_php function. Some of what it does involves invoking drush to set settings that will result in writing changes to settings.php. (e.g. the line that does drush -l "${site_url}" islandora:settings:set-flystem-fedora-url "${fedora_url}" will result in setFlysystemFedoraUrl being called, which will ultimately write the result to settings.php.

Anyway, apparently update_settings_php is not called upon normal startup of IDC. I think it was ISLE's intention that it would, but for whatever reason it isn't. The consequence of it not being run is:

  • Drupal actually uses our settings.php as written
  • Using confd to manage settings.php won't conflict with update_settings_php since update_settings_php isn't being called

So we can use confd safely.

This exercise revealed a problem, though. We also have stuff like mysql username/password/db in settings.php. As it stands now, nothing updates these values, so they're used by Drupal as-is. Setting a different mysql password via environment variables actually does nothing. So we have a problem currently in the top of development, and have had it for quite a while. I think, then, we either need to have confd provide values for these as well (in a separate ticket/issue), or enable update_settings.php. If we do the latter, then I think we'd have to throw confd out the window. So I'm thinking we just keep ignoring update_settings_php and manage settings.php with confd for all customization going forward.

@emetsger
Copy link

emetsger commented Dec 3, 2020

@emetsger Yes, I did a few experiments that demonstrated that when the drupal container starts, the settings.php file is actually not edited by ISLE. That means it's safe to use confd, and we don't immediately have to re-think how config is done.

Right, that's correct. IDC's startup does not touch settings.php. Even if it did, confd and drush management aren't at odds, as long as confd isn't processing templates periodically.

The original observations/premise that led you toward using envsubst are still valid.

As you know, there is a utilities.sh that defines a update_settings_php function. Some of what it does involves invoking drush to set settings that will result in writing changes to settings.php. (e.g. the line that does drush -l "${site_url}" islandora:settings:set-flystem-fedora-url "${fedora_url}" will result in setFlysystemFedoraUrl being called, which will ultimately write the result to settings.php.

Anyway, apparently update_settings_php is not called upon normal startup of IDC. I think it was ISLE's intention that it would, but for whatever reason it isn't. The consequence of it not being run is:

  • Drupal actually uses our settings.php as written

Yes. One of the primary reasons is that issuing drush commands slows down startup, and secondarily it makes reasoning about settings.php a lot easier. Finally, we didn't have a real reason for parameterizing it until now, so we haven't been using any approach for managing its content.

  • Using confd to manage settings.php won't conflict with update_settings_php since update_settings_php isn't being called

Correct, but they wouldn't be in conflict anyway, since periodic re-processing of confd templates is disabled, and confd renders templates before anything else happens.

So we can use confd safely.

Right, confd runs at boot, then if we choose, we can use drush, envsubst, or other approaches to manage settings.php.

This exercise revealed a problem, though. We also have stuff like mysql username/password/db in settings.php. As it stands now, nothing updates these values, so they're used by Drupal as-is. Setting a different mysql password via environment variables actually does nothing. So we have a problem currently in the top of development, and have had it for quite a while. I think, then, we either need to have confd provide values for these as well (in a separate ticket/issue), or enable update_settings.php. If we do the latter, then I think we'd have to throw confd out the window. So I'm thinking we just keep ignoring update_settings_php and manage settings.php with confd for all customization going forward.

There are a few options, we can pick our poison.

  • the ISLE way would be to use drush (currently their approach accounts for multi-site drupal installations) but it may be worth asking them if they are planning to continue to manage settings.php via drush, or if they plan to take alternate approaches (I'm not sure they would see the need)
    • if there were ISLE installs that use etcd or consul, the existing mechanism of using drush falls short because settings.php is not rendered by a confd template
  • update settings.php to read in env vars or secrets (that's what was done with the configuration for simplesaml; its settings are pulled from env vars and secrets from a yaml file). Instead of setting a fixed value, issue a getenv or file_get_contents to obtain the value for a setting.
  • just use envsubst

If we were leveraging something like Vault (or even etcd or consul) we'd have a real motivation to manage settings.php with confd. But we aren't, so I'm not sure I see the need to do so. At any rate, we need to do something, so I'd just go with whatever makes you feel good :)

@birkland
Copy link
Author

birkland commented Dec 4, 2020

Working with this a bit, here is where I am with respect to picking poison and/or developer experience

  • confd just feels awkward and nuanced
  • With envsubst, There is some trickery in parsing the file correctly, since PHP variables also start with $. This can be worked around, but ultimately an envsubst-based solution would still need a template file (end settings.php .gitignored) to avoid committing interpolated values instead of the place holders. This is .. fine
  • getenv seems like the most transparent and straightforward option. We get to keep settings.php in our repo, won't get confused and accidentally edit settings.php instead of the template, and it's obvious at a glance what gets interpolated.

Exposing env vars to Drupal gave me pause previously from a standpoint of security worries (everything is in $ENV, and anybody can print that), but apparently my worries were unfounded. It's much more popular to use a .env file with vlucas/phpdotenv module, but it seems as if that pattern is highly popular strictly due to the fact that most drupal installs are on snowflake servers or managed by ansible/puppet et al where docker-style environment variables aren't a thing.

Anyway. I think I'm going to likely close this PR and instead issue one for exposing env vars to the php backends.

@emetsger
Copy link

emetsger commented Dec 4, 2020

Exposing env vars to Drupal gave me pause previously from a standpoint of security worries (everything is in $ENV, and anybody can print that)

The env vars are whitelisted, so you control what's exposed (so one can't see everything).

In order to print out env vars, the attacker has to edit and execute php source somehow. So, if that is the threat model (arbitrary code execution), then what is or is not present in env vars is moot.

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