This commit breaks backward compatibility with IP and not a wildcard based vhosts, and since IP based vhosts were used also in 2.x for a long time before we have switched to wildcard by default, this commit will bring down all sites on the instance because it will force wildcard listening on the default vhost and hostmater, while all other vhosts will get overridden (due to the wildcard taking precedence) until all sites are reverified.

Is there any reasoning behind this change/commit, which is, to my best knowledge, not referenced in any issue?

I think it could/should be partially reverted like we did here to bring sites back online after disastrous upgrade which included the aforementioned commit.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

anarcat’s picture

Status: Active » Postponed (maintainer needs more info)

From what I remember about this, IP-based vhosts were only used for SSL. Furthermore, we have a fallback in the backend right now, in config_data(), that initialises the ip_address field properly if it's not sent by the frontend:

  function config_data($config = NULL, $class = NULL) {
    $data = parent::config_data($config, $class);
    $data['http_ssl_port'] = $this->server->http_ssl_port;

    if ($config == 'site' && $this->context->ssl_enabled) {
      foreach ($this->context->ip_addresses as $server => $ip_address) {
        if ($server == $this->server->name || '@' . $server == $this->server->name) {
          $data['ip_address'] = $ip_address;
          break;
        }
      }
      if (!isset($data['ip_address'])) {
        drush_log(dt('No proper IP provided by the frontend for server %servername, using wildcard', array('%servername' => $this->server->name)), 'warning');
        $data['ip_address'] = '*';
      }

Doesn't this work for you?

omega8cc’s picture

Status: Postponed (maintainer needs more info) » Active

No, it didn't help, hence the issue opened here. I didn't test it enough to make sure that the partial revert we had to use doesn't break something for the SSL feature, but I think it should be safe. At least, it is a must have for any old Nginx based install or it will just explode on upgrade with major WTF hard to fix unless you have good understanding on Nginx listen directives precedence and possible conflicts.

The thing is that if the instance is using IP based vhosts, it is not enough to have the update/fix in the hostmaster. You would need to re-verify all sites to convert them to use wildcard listen directive. And then it could cause unexpected issues on multi-IP systems which used IP based vhosts outside of Aegir (like for external SSL proxy etc.)

anarcat’s picture

Can you post a full verify log of such a broken site?

Anytime a site is verified in the backend, config_data() is called to populate the data in the virtualhost template. And the code is pretty clear - if no IP is provided, a wildcard is used and a warning is logged. In no case this problem should happen.

Please provide more information on how to reproduce this issue.

omega8cc’s picture

The only way to reproduce is to install older Aegir 1.x or 2.x version when Nginx still used IP based listen directive in all vhosts and then attempt an upgrade. I think we should have some old enough VM images on the demo server, so we could probably try to reproduce this again in a safe environment. I will post an update once I will have a chance to look into this.

ergonlogic’s picture

Status: Active » Needs review
FileSize
2.03 KB

This appears to be affecting sites without SSL enabled, but the code in #1 will only apply to sites where SSL is enabled. I suspect the solution would be to move that code up to the parent implementation: Provision_Service_http_public::config_data(). Patch attached.

omega8cc’s picture

This should do the trick, I think.

anarcat’s picture

Status: Needs review » Needs work

Hum... that's strange in any case - this code path should never be used in non-SSL vhosts, as the template doesn't use the IP address field.

If it does, that's the bug that should be fixed, not the config_data() call.

ergonlogic’s picture

So, then we should support IP-based vhosts in the templates?

omega8cc’s picture

Yes, we should, for backward compatibility. It is really critical for all older Nginx based installs, since there is no way to convert, upgrade or force them to use wildcard reliably, because of Nginx inherent logic for listen directives precedence and legacy configuration outside of Aegir we know nothing about, which may and will conflict with wildcard listen directive on multi-IP systems. A simple if/else to still support this legacy feature doesn't hurt, I hope.

ergonlogic’s picture

It seems reasonable to me, and a fairly easy fix, I'd think.

@omega8cc gotta patch? :)

anarcat’s picture

I don't think we should support IP-based vhosts for non-SSL hosts. The 2.x upgrade is the perfect time to drop support for such legacy configurations.

In fact, I thought the plan was to completely drop IP-based vhost support from aegir with 3.x, as we discussed re Windows XP. Why would we reintroduce this back into non-SSL vhosts?

omega8cc’s picture

Well, for me, no problem, we can (and have to) continue to patch Aegir to make it backward compatible for BOA users. It will be interesting to see if others using vanilla old Aegir with Nginx will complain about it after upgrade to Aegir 2.

ergonlogic’s picture

Is there an update hook we should introduce then? Had you suggested that we'd need to re-verify all sites?

omega8cc’s picture

Yeah, forcing re-verify for all sites is another option. We should just mention in the release notes that if someone is using old, IP based listen directives in any vhost outside of Aegir on the same system, he must modify them to also use wildcard instead of IP after the upgrade to 2.x, or weird things may happen, even if we will force re-verify for all sites. Just let's make sure to not force the re-verify on systems already using wildcard.

butler360’s picture

Just upgraded from a Debian non-package install 1.x to aegir2 Debian package and boy was that a challenge. Luckily I found this issue here, otherwise I would have been lost. At first all sites were redirecting to the first site alphabetically (so going to z.com sent me to a.com) and no sites were up. I had to use sed to change all instances of my IP address in each vhost to an asterisk. I also had static sites unrelated to Aegir that had to be changed because then all Drupal sites redirected to the first static site alphabetically.

It seems like if this isn't going to be fixed it should at least be highlighted on the upgrade and/or install notes (unless it's already there and I somehow missed it).

omega8cc’s picture

Right, now you have the proof that this is a critical problem to fix.

helmo’s picture

Status: Needs work » Closed (outdated)

The 6.x-2.x branch will go EOL along with Drupal this week. So I'm closing
this issue. If it remains a confirmed issue in 7.x-3.x, feel free to re-open,
or better yet, create a new issue referencing this one.

PS: The IP assignment code has seen refactoring since...