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.

Sign up for GitHub

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

Jump to bottom

if probe url includes a https we set the appropriate header, that mag… #1352

Open
wants to merge 1 commit into
base: devel
Choose a base branch
from
Open

if probe url includes a https we set the appropriate header, that mag… #1352

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions app/code/community/Nexcessnet/Turpentine/Model/Varnish/Configurator/Abstract.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -746,13 +746,18 @@ protected function _vcl_get_probe($probeUrl) {
.probe = {
.request =
"GET {{probe_path}} HTTP/1.1"
"Host: {{probe_host}}"
"Host: {{probe_host}}"{{probe_https}}
Copy link
Contributor

Choose a reason for hiding this comment

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

How will this impact users not using https? Do you foresee any backward compatibility issue?

Copy link
Author

Choose a reason for hiding this comment

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

it only triggers when the probe url includes https as protocol, see additional lines below. if this is not the case, the variable will be empty and therefore it will produce the same result like before.

Copy link
Contributor

@miguelbalparda miguelbalparda Feb 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

So you mean having Host: {{probe_host}} with a value for {{probe_host}} will be the same as having Host: {{probe_host}} where {{probe_host}} will be rendered into an empty string? I'd like to have a cleaner approach to take both http an https into account and not return an empty string instead of the host returned by parse_url.

Copy link
Author

Choose a reason for hiding this comment

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

i didn't change anything on {{probe_host}}. this contains only the host part of the url, e.g. www.website.com. For clarification, i add two sample results, one with and one without https configured for the probe:

Probe url set to http://www.website.com/test.php results in:

            .probe = {
                .request =
                    "GET /test.php HTTP/1.1"
                    "Host: www.website.com"
                    "Connection: close";
            }

Probe url set to https://www.website.com/test.php results in:

            .probe = {
                .request =
                    "GET /test.php HTTP/1.1"
                    "Host: www.website.com"
                    "X-Forwarded-Proto: https"
                    "Connection: close";
            }

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I misinterpreted the preview of the files changed. I thought you deleted {{probe_host}} in favor of {{probe_https}} but that wasn't the case.

Copy link
Author

Choose a reason for hiding this comment

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

not sure where, but this would be good to be written down somewhere in the docs if it gets merged.

Copy link
Author

Choose a reason for hiding this comment

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

@miguelbalparda is this PR likely to be merged? If not, I need to overwrite this for my current project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't test this in detail, if it gets merged it will be first in the devel branch and then moved to a stable version after some time, allowing others to test it first.

"Connection: close";
}
EOS;
$additionalHeader='';
if ($urlParts['scheme']==='https'){
$additionalHeader="\n \"X-Forwarded-Proto: https\"";
}
$vars = array(
'probe_host' => $urlParts['host'],
'probe_path' => $urlParts['path']
'probe_host' => $urlParts['host'],
'probe_path' => $urlParts['path'],
'probe_https' => $additionalHeader
);
return $this->_formatTemplate($tpl, $vars);
}
Expand Down Expand Up @@ -910,7 +915,7 @@ protected function _vcl_sub_https_redirect_fix() {
$baseUrl = Mage::getBaseUrl(Mage_Core_Model_Store::URL_TYPE_WEB);
$baseUrl = str_replace(array('http://', 'https://'), '', $baseUrl);
$baseUrl = rtrim($baseUrl, '/');

switch (Mage::getStoreConfig('turpentine_varnish/servers/version')) {
case 4.0:
case 4.1:
Expand Down Expand Up @@ -1085,7 +1090,7 @@ protected function _getTemplateVars() {
// set the vcl_error from Magento database
$vars['vcl_synth'] = $this->_vcl_sub_synth();
}

if (Mage::getStoreConfig('turpentine_varnish/general/https_redirect_fix')) {
$vars['https_redirect'] = $this->_vcl_sub_https_redirect_fix();
if (Mage::getStoreConfig('turpentine_varnish/servers/version') == '4.0' || Mage::getStoreConfig('turpentine_varnish/servers/version') == '4.1') {
Expand Down