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.

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

Add http2-host-blacklist config flag #2482

Closed

Conversation

nicknovitski
Copy link
Contributor

What this PR does / why we need it:

A server block is created in nginx.conf for each host in ingresses. Currently, the IC can enable or disable http2 for all server blocks. With this flag, it becomes possible to selectively disable it per host/server.

NGINX v1.13.6 and later send resize notifications for HPACK header compression tables, which breaks a lot of our android traffic. All of this traffic goes to one host, so this feature will let us enable http2 for the majority of our traffic.

Which issue this PR fixes: fixes #2189

Special notes for your reviewer:
I wasn't able to get the e2e tests running on minikube 0.26.1. I couldn't figure out how to get them to work with RBAC, or how to create a cluster without RBAC (kubernetes/minikube#2342). That was a real pain. I'll just use travis for now, but I think the tests should work with RBAC as a default.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2018
@nicknovitski nicknovitski force-pushed the http2-host-blacklist branch 3 times, most recently from 6adcd7b to 5da4a49 Compare May 9, 2018 20:17
@nicknovitski
Copy link
Contributor Author

nicknovitski commented May 10, 2018

It turns out that gorequest doesn't use http/2 if you set TLSClientConfig or anything else which effects the transport (parnurzeal/gorequest#124, golang/go#17051). The solution is supposed to be calling http2.ConfigureTransport to mutate it after construction.

If I don't do that, the connection is http/1.1. But if I do, the response is a 404, I assume due to hitting the default back end, though I don't know why.

e: My mistake, I was just mixing up test results. So my real problem is the blacklist not actually removing http2 from the listen directive. I'll work on that tomorrow.

@nicknovitski nicknovitski force-pushed the http2-host-blacklist branch 11 times, most recently from bd96f59 to 3f27a38 Compare May 10, 2018 21:45
@nicknovitski
Copy link
Contributor Author

Weird. Adding a host to the blacklist definitely removes http2 from the appropriate listen directive in my manual testing, and also apparently in my automated testing (if I'm using WaitForNginxServer correctly), but for some reason the requests after that still use http/2.

@codecov-io
Copy link

Codecov Report

Merging #2482 into master will increase coverage by 0.03%.
The diff coverage is 58.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2482      +/-   ##
==========================================
+ Coverage    41.6%   41.64%   +0.03%     
==========================================
  Files          74       74              
  Lines        5290     5302      +12     
==========================================
+ Hits         2201     2208       +7     
- Misses       2792     2797       +5     
  Partials      297      297
Impacted Files Coverage Δ
internal/file/bindata.go 62.4% <ø> (ø) ⬆️
internal/ingress/controller/template/template.go 64.75% <0%> (-0.7%) ⬇️
internal/ingress/controller/config/config.go 98.24% <100%> (+0.03%) ⬆️
internal/ingress/controller/template/configmap.go 77.57% <100%> (+1.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e82cb31...3f27a38. Read the comment docs.

@nicknovitski nicknovitski force-pushed the http2-host-blacklist branch from 3f27a38 to 34f7a8f Compare June 2, 2018 22:18
@nicknovitski
Copy link
Contributor Author

--vm-driver none failed to install kubelet in travis:

Moving files into cluster...
Downloading kubeadm v1.10.0
Downloading kubelet v1.10.0
Finished Downloading kubelet v1.10.0
Finished Downloading kubeadm v1.10.0
E0602 22:28:58.610170    5553 start.go:234] Error updating cluster:  downloading binaries: transferring kubeadm file: &{BaseAsset:{data:[] reader:0xc420088620 Length:0 AssetName:/home/travis/.minikube/cache/v1.10.0/kubelet TargetDir:/usr/bin TargetName:kubelet Permissions:0641}}: error creating file at /usr/bin/kubelet: open /usr/bin/kubelet: permission denied

@nicknovitski nicknovitski force-pushed the http2-host-blacklist branch 2 times, most recently from ff21dfc to 968a91a Compare June 3, 2018 05:45
@aledbf
Copy link
Member

aledbf commented Jun 17, 2018

@nicknovitski please rebase

@aledbf aledbf self-assigned this Jun 17, 2018
@nicknovitski nicknovitski force-pushed the http2-host-blacklist branch from 968a91a to c1b9add Compare July 14, 2018 02:32
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nicknovitski
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: aledbf

If they are not already assigned, you can assign the PR to them by writing /assign @aledbf in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nicknovitski nicknovitski force-pushed the http2-host-blacklist branch from 31eb297 to cc25d49 Compare July 14, 2018 03:36
@blakebarnett
Copy link

We really need this, is there anything blocking it we can help with?

@aledbf
Copy link
Member

aledbf commented Aug 30, 2018

We really need this, is there anything blocking it we can help with?

Just rebase and that passing CI

@aledbf
Copy link
Member

aledbf commented Aug 30, 2018

NGINX v1.13.6 and later send resize notifications for HPACK header compression tables, which breaks a lot of our android traffic.

Just un case current master uses nginx 1.15.3 that solves some issues with this and old clients

@blakebarnett
Copy link

excellent, I'll see if we can test it out, we're currently running an nginx-ingress fork of 0.9 and I really don't want to try to maintain it!

@aledbf
Copy link
Member

aledbf commented Aug 30, 2018

@blakebarnett I am preparing the PR for 0.19.0. In the PR you will find a test image you can test

@nicknovitski
Copy link
Contributor Author

I'll try out 0.19 and see if it fixes our problem.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 28, 2018
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 28, 2019
@fejta-bot
Copy link

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

@k8s-ci-robot
Copy link
Contributor

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@wzrdtales
Copy link

Was there any progress on that after it lost track?

@nicknovitski nicknovitski deleted the http2-host-blacklist branch May 6, 2019 19:21
@nicknovitski
Copy link
Contributor Author

As I understand it the nginx bug was fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support per-ingress http2 setting through annotation
7 participants