Skip to content

Conversation

@grkvlt
Copy link
Member

@grkvlt grkvlt commented Aug 3, 2016

Adds support for HTTP 1.1 and connection upgrades, as well as improving formatting and better use of the StringBuilder

The fix for the NPE when no SSL config is provided has now been merged as #59

@grkvlt grkvlt force-pushed the nginx-connection-upgrade branch from a537e94 to 18ace8c Compare August 4, 2016 04:37
@aledsage
Copy link
Contributor

aledsage commented Aug 8, 2016

@grkvlt what tests should we run to confirm that this hasn't broken previous use-cases, and that it now supports the use-case that motivated you to make this change?

@grkvlt
Copy link
Member Author

grkvlt commented Aug 15, 2016

@aledsage the existing Nginx tests are sufficient to verify there are no regressions, however the original use-case for this code is no longer as urgent. It is still a useful change, since it supports HTTP 1.1 and connection upgrades properly now, and the code is also more readable.

The change to the config file that adds this is the addition of these two groups of lines:

    map $http_upgrade $connection_upgrade {
      default Upgrade;
      ''      close;
    }
    proxy_http_version 1.1;
    proxy_set_header Upgrade $http_upgrade;
    proxy_set_header Connection $connection_upgrade;

@grkvlt grkvlt force-pushed the nginx-connection-upgrade branch from b2b69b7 to afbb285 Compare August 15, 2016 13:18
@neykov
Copy link
Member

neykov commented Feb 3, 2017

@grkvlt the server.conf used with NginxTemplateConfigGenerator follows roughly the config from NginxDefaultConfigGenerator. Can you update it as well.
I think we should just migrate to the template configuration and deprecate NginxDefaultConfigGenerator altogether as its easier to support.

@nakomis
Copy link
Contributor

nakomis commented Nov 27, 2019

@grkvlt Can you take a look at this please to see if it is still relevant, and address the comments above if appropriate

Thanks

@asf-ci
Copy link

asf-ci commented Nov 27, 2019

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/brooklyn-library-pull-requests-pipeline/7/

@nakomis
Copy link
Contributor

nakomis commented Nov 27, 2019

P.S. Can you also rebase if this is still valid

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.

5 participants