Add Configurable HTTP/2 Support #254
Conversation
- nginx can not serve both HTTP/1 and HTTP/2 on the same port, if the port does not use TLS. See https://trac.nginx.org/nginx/ticket/816. - Implementation mirrors force_https to make it user-configurable - new staticfile option of `enable_http2` and use of environment variable `ENABLE_HTTP2` to configure nginx - Route HTTP/2 configuration is available via manifest when using cf cli v7 or with the map-route command when using cf cli v8 - since cutlass only supports cf cli v6, testing bypasses cutlass with "cf curl" commands. These can be replaced with proper cf cli commands when upgrading the cf cli version - integration tests only runs on api 2.172.0 or higher [cloudfoundry/routing-release#200] Co-authored-by: Michael Oleske <moleske@vmware.com>
|
We have created an issue in Pivotal Tracker to manage this: https://www.pivotaltracker.com/story/show/179788221 The labels on this github issue will be updated when the story is started. |
| Expect(err).To(BeNil()) | ||
| apiVersion, err := semver.Make(apiVersionString) | ||
| Expect(err).To(BeNil()) | ||
| apiHasHttp2, err := semver.ParseRange("> 2.172.0") |
There was a problem hiding this comment.
Do all v3 endpoints support HTTP/2 ? This semver will pass all v3 endpoints.
However, answering/fixing this is probably not required for this PR as we don't really check this in other parts of the test
There was a problem hiding this comment.
We need capi 1.118.0 which has api version 2.172.0 and 3.107.0. We only need to check 2.x.x version as capi releases always bump api version. We could switch it to 3.107.0 if that makes it less confusing
There was a problem hiding this comment.
No, that makes sense. Thanks for explaining.
I didn't make the connection that you don't care about v2 vs v3 but instead you care about the underlying CAPI functionality. I also didn't make the connection that ensuring the api version 2.172.0 also guarantees the specific v3 endpoints you require.
There was a problem hiding this comment.
Oh, but shouldn't the > read >=?
There was a problem hiding this comment.
Yes you're right. I shall update that
|
This looks good, and the integration tests pass. There are some stylistic changes I'd like to see (e.g. |
- Implementation PR: cloudfoundry/staticfile-buildpack#254 [cloudfoundry/routing-release#200] Authored-by: Greg Cobb <gcobb@pivotal.io>
port does not use TLS. See https://trac.nginx.org/nginx/ticket/816.
force_httpsto make it user-configurableenable_http2and use of environmentvariable
ENABLE_HTTP2to configure nginxv7 or with the map-route command when using cf cli v8
"cf curl" commands. These can be replaced with proper cf cli commands
when upgrading the cf cli version
[cloudfoundry/routing-release#200]
Co-authored-by: Michael Oleske moleske@vmware.com
Thanks for contributing to the buildpack. To speed up the process of reviewing your pull request please provide us with:
A short explanation of the proposed change:
An explanation of the use cases your change solves
I have viewed signed and have submitted the Contributor License Agreement
I have made this pull request to the
developbranchI have added an integration test