Updated: Comment #12

Problem/Motivation

At the moment we allow any site to sync with any site this is causing seriously complex problems, see #2127573: Stop creating node, comment and custom block fields automatically and provide defaults in CMI. There has been a general expectation that installing two Drupal's using the same profile would allow the user to synchronise configuration between these sites. We have explored a number of solutions to try to make this work - for a complete rundown on this read the issue summary of #1613424: Allow a site to be installed from existing configuration. This issue will ensure that if the user has this expectation, and tries to do this, they will not break their site.

Proposed resolution

  • on install write a UUID to system.site that identifies the site.
  • check this UUID matches on config sync.
  • If the UUIDs do not match prevent config synchronisation from occurring.

Remaining tasks

Review implementation

User interface changes

New error message on admin/config/development/configuration when site UUID in staging does not match with the active value.

API changes

  • Adds new method validateSiteUuid() to StorageComparerInterface
  • Throws exception in ConfigImporter::validate() if validateSiteUuid() returns FALSE
Files: 
CommentFileSizeAuthor
#28 interdiff.txt848 bytesmtift
#28 drupal8.config-site-uuid.28.patch8.09 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 60,169 pass(es). View
#23 interdiff.txt636 bytesmtift
#23 drupal8.config-site-uuid.23.patch8.09 KBmtift
FAILED: [[SimpleTest]]: [MySQL] 60,221 pass(es), 25 fail(s), and 14 exception(s). View
#21 interdiff.txt7.13 KBsun
#21 drupal8.config-site-uuid.21.patch8.1 KBsun
PASSED: [[SimpleTest]]: [MySQL] 59,764 pass(es). View
#14 2133325.14.patch8.09 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,753 pass(es). View
#14 10-14-interdiff.txt3 KBalexpott
#10 2133325.10.patch6.61 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,749 pass(es), 9 fail(s), and 0 exception(s). View

Comments

yched’s picture

So yes, #2127573-13: Stop creating node, comment and custom block fields automatically and provide defaults in CMI details some nasty consequences of trying to support "config sync between any and all random drupal installs" (i.e make sure that the install process produces the same UUIDs for the same config entities each and every time).

I've been thinking too that we should only reasonably support "config sync between *copies* of a single initial install" - which is the original promise of CMI: deploy between dev / stage / prod. Then, no more problems of mismatching UUIDs since they have all been created only once in the intial install and then copied:
- no need to hardcode UUIDs in config entities shipped in modules/profiles default config
- no need to go through hoops to forbid programmatic creation of config entities in hook_install()
- no need to go through hoops to prevent runtime code from performing secondary config writes when importing a config entity from the "default config" (i.e "set the isSyncing() flag during the 'default config import' process" - see #2127573-23: Stop creating node, comment and custom block fields automatically and provide defaults in CMI)

Similarly, I've been trying to figure out the drawbacks of this approach too. Here's what I found.

1) 6 months after site launch, you welcome a new dev on the team. The only way to set up a new local dev instance is to copy "code + active config folder + db" from prod - which might be a huge db. No way to start fresh on an empty site with "just" the config, since you cannot run the installer, and so nothing will initialize your local db.

Discussing that with @amateescu last week, we figured that maybe you could export the db *schema* from prod, without the data.
"Empty site" = the code + the config ready in the /active folder + a db with empty tables, here you go ?
If we look at current core, that might be pretty close (problem of users 0 and 1 though), but it's difficult to evaluate whether it will still be true in all of contrib ?

2) At least some CMI files shipped in default config will still contain UUIDs, since most contrib devs will fill the config/ folder of their module by picking and copying from their active config.
What should we do in that case ?
- Treat those as "hardcoded UUIDs" ? (would be really inconsistent, some default config ship with hardcoded UUIDs, some other get UUIDs that will vary site by site...=
- Just overlook the UUID key if present when importing default config ? (would be misleading / confusing ?)
- Die with an exception ? CMI files copied from the active store need to be manually cleaned of their UUID key before they can be shipped in a "default config" folder ?

(side note: Field API "field" and "instance" records might be problematic here, since the instance currently refers to its associated field by a 'field_uuid' property in the CMI file, so shipping those in default config assumes the field has a fixed, known UUID that the instance can point at. The recent cleanups in Field API might let us get rid of that 'field_uuid' entry in instance records, and have them refer to their field just by 'field_name'. Not currently the case though)

3) There is still an issue of race conditions during "default config" import.
Example (same as in #2127573-13: Stop creating node, comment and custom block fields automatically and provide defaults in CMI):
- My foobar.module does things on nodes per node type. It stores the corresponding settings in foobar.settings.[node_type].yml.
Typically, it implements hook_entity_node_type_save() to save a new, default, foobar.settings.[node_type] ConfigEntity whenever a new node type is created.
- I install some barbaz.module, that happens to ship a node.type.barbaz.yml in its config/ folder.
It *may* also know about my foobar.module and ship the corresponding foobar.settings.barbaz.yml as well, with fine-tuned values of its own.

What happens then ? In the other issue I pointed the consequences of setting the isSyncing() flag during "default config import" process, to prevent foobar.module from creating its foobar.settings.barbaz while we're importing barbaz/config/node.type.barbaz.yml: this results in a site with a collection of node types, and only some of them having corresponding foobar.settings.[node_type] records. Foobar module, and all others doing similar stuff, needs to account for that uncertainty by moving away from entity_load() and coming up with their own wrapper APIs with an if(exists) check.

So let's assume here we that don't try to do that: when importing barbaz/config/node.type.barbaz.yml, foobar_entity_node_type_save() runs normally and (tries to) create the foobar.settings.barbaz ConfigEntity. 2 cases, depending on import order (currently based on alphabetical order - meaning both will happen, depending on module names):
a) barbaz/config/foobar.settings.barbaz.yml has already been imported earlier in the process so foobar.settings.barbaz already exists - not sure what happens. I think there is an issue/patch somewhere to raise an exception when saving a new config entity with a name (ID) that already exists.
--> foobar_entity_node_type_save() probably needs to wrap some if (!exists) {create 'foobar.settings.barbaz'} logic.
b) the "default" foobar.settings.barbaz gets created ok, then the import process moves along and tries to import barbaz/config/foobar.settings.barbaz.yml: it already exists, not sure what happens either.

(The desired outcome, of course, is that, whatever the module names and alphabet order, active config ends up with the foobar.settings.barbaz that was present in barbaz/config)

I do think this is solvable, but it needs a bit of thinking ;-)

yched’s picture

xjm’s picture

sun’s picture

A "Site UUID" is essentially the purpose of this existing variable in settings.php:

$drupal_hash_salt = '';

Of course, one could argue whether a system.site:uuid configuration value might be cleaner. But in theory and practice, $drupal_hash_salt is used for the identical purpose, both in core (session handling, access tokens, anonymous Update module site identification for d.o, etc) as well as contributed modules.

I think if we're going to replace $drupal_hash_salt with a system.site:uuid configuration value, then we should do so globally (removing $drupal_hash_salt as a result).

Lastly, @yched's #2 2) and 3) are very interesting thoughts, but appear to be off-topic for this issue? Would you mind to move those considerations to #2121751: [META] Making configuration synchronisation work or some more focused issue? :)

yched’s picture

They're not really off topic, they're consequences of *this* approach to the "syncing / config entities UUIDs" problem (as opposed to the other approaches that were discussed in #2127573: Stop creating node, comment and custom block fields automatically and provide defaults in CMI and #2138559: Replace UUIDs in configuration entities with a creation ID, which had different consequences). Detailing them was part of the exploration for each approach.

alexpott’s picture

re #5 I'm not sure I 100% agree that drupal_hash_salt and a site uuid are the same thing. drupal_hash_salt is stored in settings.php because if it was in the db then it'd be possible to reverse engineer some of our hashes. So we can not move drupal_hash_salt to configuration and ideally whatever we use to do this would live in config. Also I think there might be use cases where you might not want to disclose your production drupal hash salt to developers.

chx’s picture

Pulling in configuration from another site is called a migration not sync. I suspect it won't be a sync; it will just override crudely but it will essentially work.

Gábor Hojtsy’s picture

I *think* what people intend when they want to "sync" with config of a different site is to get "features" ported over. Packages of config, like a content type with a view, etc. prototyped on a different site to be added to another. If that is indeed the purpose of this wish, then I think that would indeed be solved different from the sync process. Supporting partially reusing components of the site where this "feature" is being imported to (eg. an existing content type) sounds like a possible but twisted use case to support.

alexpott’s picture

Status: Active » Needs review
Related issues: +#1613424: Allow a site to be installed from existing configuration
FileSize
6.61 KB
FAILED: [[SimpleTest]]: [MySQL] 59,749 pass(es), 9 fail(s), and 0 exception(s). View

Implements a uuid key in system.site and validates in during configuration import. This prevents users from syncing sites that are not built from the same install. See #1613424: Allow a site to be installed from existing configuration for a possible solution for how to install a site with a match site UUID without just copying the DB, files and code.

alexpott’s picture

Issue summary: View changes

BTW any suggestions for better text for the error message would be hugely welcome. Current text is The value of the site UUID in the staging directory does not match the current value. Configuration synchronisation requires that sites have the same site uuid so that stuff works.

alexpott’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 10: 2133325.10.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
3 KB
8.09 KB
PASSED: [[SimpleTest]]: [MySQL] 59,753 pass(es). View

Fixing test fails

yched’s picture

Issue summary: View changes
yched’s picture

Note: #1 detailed three caveats with the "only support config sync within a ring of sites that share the same site UUID" appraoch.

Caveat 1. (you can only add a new localdev site to the ring by copying the code + config + full db from prod) is solved by #1613424: Allow a site to be installed from existing configuration.

Caveats 2. (what to do with "default config" files that contain leftover UUIDs)
and 3. (race conditions on "default config" import)
are still up there though.

Do we want to open separate issues ?

yched’s picture

(also, this issue here about the exact scope of the whole "config sync" feature is IMO an important part of what makes #1613424: Allow a site to be installed from existing configuration critical. Shouldn't the issue summary over there point back to this one here ?)

beejeebus’s picture

re #26 caveat 2, i will tackle the default config stuff in #1808248: Add a separate module install/uninstall step to the config import process.

yched’s picture

[edit: removed, I was on crack]

sun’s picture

sun’s picture

FileSize
8.1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,764 pass(es). View
7.13 KB

Polished the code and improved the UI error message:

The staged configuration cannot be imported, because it originates from a different site than this site. You can only synchronize configuration between cloned instances of this site.

Sorry for the unnecessarily large interdiff, but I think the uuid value should really come first in system.site and everywhere else, as it's going to be the most important property.

beejeebus’s picture

i like this patch.

+    // The site UUIDs must match for the import to work.
+    \Drupal::config('system.site')->set('uuid', $this->siteUuid)->save();

the site UUID is not the only thing that is wrong using the 'two drupal installs' method. file paths also break, because they are unique across simpletest methods. that doesn't show up here, because we don't have anything that exercises that yet, but it will.

we don't have to fix that here, just wanted to put it on the radar.

mtift’s picture

Related issues:
FileSize
8.09 KB
FAILED: [[SimpleTest]]: [MySQL] 60,221 pass(es), 25 fail(s), and 14 exception(s). View
636 bytes
mtift’s picture

I like like this patch, too.

Here's a re-roll, plus a minor language tweak.

yched’s picture

Status: Needs review » Reviewed & tested by the community

Patch looks good.

Only uber-nitpicky remark:

  1. +++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportUITest.php
    @@ -116,6 +116,23 @@ function testImportLock() {
    +    // Verify that there are configuration differences to import.
    +    $this->drupalGet('admin/config/development/configuration');
    +    $this->assertText(t('The staged configuration cannot be imported, because it originates from a different site than this site. You can only synchronize configuration between cloned instances of this site.'));
    +    $this->assertNoFieldById('edit-submit', t('Import all'));
    

    Comment looks off ?

Other than that, I just feel that, since this is a fairly important and defining restriction we put on the config sync process here, we miss a place where we actually capture that restriction and a "short" version of the reasoning that led there *in comments in the code*, rather than buried in the issue queue.

Proposal:
The config sync process relies on uniqueness of the config entities' UUIDs. It therefore only supports deployment between site instances that share the same "site UUID", which indicates that they have been created by cloning off from one single initial install (e.g. "dev / stage / prod"). Deploying configuration between any two random drupal installs is not supported.

Just not sure where this would best belong :-)
Also, not a blocker. This is RTBC IMO.

yched’s picture

Side note (not for this issue, of course) :

--- a/core/lib/Drupal/Core/Config/ConfigImporter.php
+++ b/core/lib/Drupal/Core/Config/ConfigImporter.php

In our series about "Config Sync and Default Config Import are in fact two separate and fairly different processes", we'd gain a bit of clarity if the class that does "sync" was not called "importer" ;-)

The last submitted patch, 23: drupal8.config-site-uuid.23.patch, failed testing.

mtift’s picture

Related issues:
FileSize
8.09 KB
PASSED: [[SimpleTest]]: [MySQL] 60,169 pass(es). View
848 bytes
alexpott’s picture

Priority: Normal » Critical
Issue tags: +beta blocker

This should be critical as it is part of #2121751: [META] Making configuration synchronisation work

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Hm. I tried to test this patch with my handy CMI testy script at https://github.com/webchickenator/drupal8-demo/blob/master/cmi.sh but unfortunately something has caused that to seriously b0rk in the past few weeks. :( So I'll just take it on faith that the folks in this issue have tested some deployments manually with this patch and have confirmed that they are working properly.

Committed and pushed to 8.x. Thanks!

mtift’s picture

I updated the "Example" section of the documentation on https://drupal.org/documentation/administer/config to reflect this new requirement, and note the target workflow described in the meta issue.

yched’s picture

Moved the race condition issue described in #2.3 over at #2170579: Race conditions on "default config import"

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

moshe weitzman’s picture

alexpott suggested a couple enhancements which would make this validation less painful:

  1. Add a way to break the site UUID validation and allow the user to import anyway.
  2. or an initial sync process that assumes all matching config id should have the same UUID and does the appropriate thing.