1: create a git repository in /var/aegir/
2: verify a platform so it shows /var/aegir/ as your repo
3: delete the platform

Everything in /var/aegir/ gets removed.

Thankfully I had a backup, but this seems pretty dangerous.

I believe this module should:
* Not detect git repos above the platform or site root, ever.
* On verify, unset git info from site or platform if the associated .git folder is removed.

Attached are a platform delete log, and a screenshot of the platform showing the git repo aegir detected.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

millenniumtree created an issue. See original summary.

millenniumtree’s picture

millenniumtree’s picture

Issue summary: View changes
millenniumtree’s picture

Further, removing the /var/aegir/.git directory and re-verifying the platforms (even the hostmaster) does NOT remove repo_url, repo_path, deploy_from_git, git_ref from the drushrc alias files.

millenniumtree’s picture

Project: Provision » Aegir Hosting Git

Moving bug report from Provision to Aegir Hosting Git

millenniumtree’s picture

Title: Removing a platform could remove all of /var/aegir » Removing a site or platform could remove all of /var/aegir
Issue summary: View changes
helmo’s picture

I agree that this should not be possible.

Deleting probably happens in drush_provision_git_post_provision_delete()

Finding out the top level of the Git repo is done in provision_git_is_repo(). Since we have the repo_docroot property from #2838489: Support drupal being in a subdirectory of a git repo we might be able to make that code a bit smarter...

PS: I'm not sure if it's practical to keep the whole /var/aegir in Git ... I have /var/aegir/config in Git on some servers via etckeeper.

millenniumtree’s picture

I had hosting_git enabled simply because older versions of aegir (2.x) removed all the .git folders in my platforms unless hosting_git was enabled. That doesn't appear to happen now though, so it turns out I don't actually need hosting_git!

I had a git repo in /var/aegir, like you said, for storing configuration overrides I've made to ~/config/includes/nginx_vhost_common.conf, ~/config/includes/global.inc, and ~/config/server_master/nginx.conf, as well as some drush plugins in ~/.drush

First, I split the repo in two (/var/aegir/config and /var/aegir/.drush) and removed /var/aegir/.git.
Then I disabled hosting_git, and re-verified any platform or site that had received an incorrect git reference. Now I think I can delete old sites and platforms without wiping my entire /var/aegir.
8 servers. :\

We almost wouldn't even need the git repos if there were well-placed includes in nginx_vhost_common.conf, and nginx.conf.

  • helmo committed e8d4fbc on 7.x-3.x
    Issue #2893588 by millenniumtree: Quick fix, remove the delete code
    
helmo’s picture

Version: 7.x-3.11 » 7.x-3.x-dev
Status: Active » Needs work

I created a quick test repo for this https://gitlab.com/aegir/test_drupal_docroot_in_subdir but trying to get that working as it should is already a challenge :(

In the mean time I've disabled the line that's doing the delete.

Jon Pugh’s picture

Priority: Major » Critical

Here's what happened:

In #2544906: Pickup git setting from disk a patch was committed that imported the repo_url and git_ref property by running git commands in the $repo_path directory.

The problem is, it did this, whether or not $repo_path was an actual git repo. So when you verified a site, it ran "git config --local --get remote.origin.url" in the sites/domain.com folder, and it got a good result from /var/aegir/.git or from a platform that was a git clone.

So I'm fixing that over in that issue.

For this problem, we just need to add some extra checks here, to be sure $repo_path is the right repo_path. We could also add a dead-man's switch to prevent us from attempting to delete /var/aegir in the case that repo_root got changed to that somehow.

So if you had /var/aegir/.git, and you've been using hosting_git since that patch has been committed, you should check your site and platform nodes and contexts for incorrect git repo information.

Jon Pugh’s picture

Jon Pugh’s picture

  • Jon Pugh committed 3e4d43d on 7.x-3.x
    Issue #2893588: Removing a site or platform could remove all of /var/...
Jon Pugh’s picture

Status: Needs work » Needs review

I've committed this failsafe, and have addressed the other issue in #2544906: Pickup git setting from disk.

+
+  // Do not allow /var/aegir to be repo_path.
+  // See https://www.drupal.org/node/2893588
+  if ($repo_path == '/var/aegir') {
+    $repo_path = NULL;
+    drush_set_option('repo_path', $repo_path);
+  }
+

This check happens even if your repo path is already set to /var/aegir.

Helmo: Can you review and approve this, and then revert this commit: http://cgit.drupalcode.org/hosting_git/commit/?id=e8d4fbc

Jon Pugh’s picture

Hmm we should add a drush warning there, shouldn't we? This is bad data that we can't automatically fix.

  • Jon Pugh committed fd7e676 on 7.x-3.x
    Issue #2893588: Set error and return if repo_path is aegir home.
    
Jon Pugh’s picture

Oops! turned out I already did. Needed improvement...

Jon Pugh’s picture

Here's the final result:

I decided a hard error was needed here. If /var/aegir is set to repo root, there's a problem that needs to be fixed.

+
+  // Do not allow /var/aegir to be repo_path.
+  // See https://www.drupal.org/node/2893588
+  if ($repo_path == '/var/aegir') {
+    $repo_path = NULL;
+    drush_set_option('repo_path', $repo_path);
+    return drush_set_error('PROVISION_GIT_ERROR', dt('Warning: The "repo_path" for this !type is set to "/var/aegir". This is not allowed. Please manually edit the drush alias file in /var/aegir/.drush/!file.alias.drushrc.php.  Option repo_path has been unset.', array(
+      '!file' => ltrim(d()->name, '@'),
+      '!type' => d()->type,
+    )));
+  }
+
helmo’s picture

Status: Needs review » Fixed

Looks good.

Status: Fixed » Closed (fixed)

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

Jon Pugh’s picture

Status: Closed (fixed) » Needs work

Reopening.

provision-delete on a platform is still reporting:

NOT deleteing See https://www.drupal.org/node/2893588

However, the platform codebase is removed, but the drush alias file is not removed...

helmo’s picture

Status: Needs work » Needs review
FileSize
772 bytes

Looking at your error message d()->repo_path was probably empty ... what about this extra test?

  • helmo committed cc10013 on 7.x-3.x
    Issue #2893588 by helmo: Chweck empty value before issueing a warning.
    
helmo’s picture

Status: Needs review » Fixed

committed.

Status: Fixed » Closed (fixed)

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