HTTP2 and gRPC support by tanzeeb · Pull Request #2539 · knative/serving

tanzeeb · 2018-11-24T00:58:59Z

This PR adds support for HTTP/2 and gRPC services:

For each revision, the container port name is used to determine the appropriate protocol, as described in the runtime contract. The default is HTTP/1.1.
For each revision, the port name of the k8s service is determined by the revision's protocol. For h2c, http2 and for http1, http. The default is http.
The activator is served on two separate ports, one for each protocol.
When a revision is scaled-to-zero, the ClusterIngress will route to the appropriate activator port based on the revision's protocol.

Fixes #813
Fixes #706
Fixes #707

Release Note

* Support gRPC and HTTP/2 requests

Edit Added implementation details.

knative-prow-robot

@tanzeeb: 1 warning.

Details

In response to this:

This PR adds support for HTTP/2 and gRPC services.

Fixes #813
Fixes #706
Fixes #707

Release Note
* Support gRPC and HTTP/2 requests

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.

mattmoor · 2018-11-24T15:02:02Z

I think we definitely want an e2e test for this.

markusthoemmes

Thanks a lot @tanzeeb for having separately reviewable commits with meaningful commit messages. That helped a lot while stepping through this change! 🙏

The change overall looks good to me, I left a few comments throughout but nothing major at all. Great job!

tanzeeb · 2018-11-26T18:41:31Z

/test pull-knative-serving-upgrade-tests
/test pull-knative-serving-integration-tests

tanzeeb · 2018-11-27T00:19:57Z

/hold

Thanks for reviewing @mattmoor @markusthoemmes

Will fix the broken tests and add some e2e tests.

tanzeeb · 2018-12-05T18:10:37Z

Update:

TL;DR: This PR breaks for all applications that only support HTTP 1.1. Turns out I did a really bad job of 1) testing this feature, and 2) bisecting the test failure (sorry revision.timeoutSeconds, it wasn't your fault 😞).

Background:

Istio uses the Kubernetes Service port name of the upstream service (in our case, the Revision) to determine the request protocol. Changing the Kubernetes Service port name from http to http2 instructs the Envoy IngressGateway to make HTTP 2 requests to the service:

        // ServicePortName = "http"
	ServicePortName = "http2"

This will convert all requests, including HTTP 1.1 requests, into HTTP 2. This works very well for apps that support HTTP 2, but breaks all other apps. If we keep the port name as http, all requests, including HTTP 2 requests, will get converted to HTTP1.

The full compatibility matrix looks like this:

Port name: http2 / grpc:

	HTTP 1.1 request	HTTP 2 request	Unary gRPC request	Streaming gRPC request
HTTP 1.1 server	❌	❌	❌	❌
HTTP 2 server	✅	✅	✅	✅
gRPC server	❌	❌	✅	✅

Port name http:

	HTTP 1.1 request	HTTP 2 request	Unary gRPC request	Streaming gRPC request
HTTP 1.1 server	✅	✅	❌	❌
HTTP 2 server	✅	✅	❌	❌
gRPC server	❌	❌	❌	❌

Problem

In this PR, I was hoping to use http2 to support all http-ish protocols. This won't work. Istio will always convert the request to the upstream service protocol. Envoy has a feature to use the client protocol instead of the upstream protocol, but this breaks Istio so it is not an option.

Potential Solution?

We have to dynamically select http, http2 or grpc for the K8s Service port name.

In a previous PR there was hesitation around specifying the protocol in the revision spec. I can't think of a way around it...

Update from @evankanderson:

See the runtime spec for the current way to do this, using the container.ports[0].name field.

Edit: Corrected the charts

evankanderson · 2018-12-06T00:18:54Z

Thanks for the in-depth investigation. It looks like istio/istio#6158 is not very conclusive -- istio/istio#6611 reverts the behavior, but I'm wondering whether unspecified port name should be equivalent to envoy's USE_DOWNSTREAM_PROTOCOL. Unfortunately, it looks like this would require adding an enum value to the supported Pilot Protocols.

In particular, it's possible to have a docker container which answers HTTP1, HTTP via h2c, and gRPC via h2c -- there's no particular reason why the container would need to support HTTP1, but it might be convenient for testing (distro curl may not support the -2 flag, for example). It would be nice to be able to request "attempt http2 but fall back to http" in the Istio configuration, which is different than USE_DOWNSTREAM_PROTOCOL (more of a USE_BEST_PROTOCOL).

tanzeeb · 2018-12-06T20:44:09Z

It would be nice to be able to request "attempt http2 but fall back to http" in the Istio configuration, which is different than USE_DOWNSTREAM_PROTOCOL (more of a USE_BEST_PROTOCOL).

This would solve all of our problems 😃

evankanderson · 2018-12-07T23:34:13Z

Is there a feature request upstream for this? USE_BEST_PROTOCOL is best option. :-D

…

On Thu, Dec 6, 2018 at 12:44 PM Tanzeeb Khalili ***@***.***> wrote: It would be nice to be able to request "attempt http2 but fall back to http" in the Istio configuration, which is different than USE_DOWNSTREAM_PROTOCOL (more of a USE_BEST_PROTOCOL). This would solve all of our problems 😃 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2539 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHlyN4ZBphnKT4JF13Zz1O5IADBJQYDaks5u2YGcgaJpZM4YxNw5> .

-- Evan Anderson <argent@google.com>

Changing it to http2 for all services breaks services which only support http1. Support for http2 will require selectively setting the port name to http2 only for services that explicitly support it.

…targets

… the revision protocol

knative-metrics-robot · 2019-01-30T21:39:15Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/activator/activator.go	Do not exist	100.0%
pkg/activator/util/io.go	Do not exist	100.0%
pkg/activator/util/rewinder.go	92.9%	100.0%	7.1
pkg/apis/serving/v1alpha1/revision_types.go	87.0%	87.9%	0.9

evankanderson · 2019-01-30T22:37:35Z

I'm comfortable with a quick followup e2e test, but note that until there is an e2e test for this functionality, it's likely to backslide and be broken by accident.

tanzeeb · 2019-01-30T23:13:35Z

Hi folks,

Thanks for the feedback so far. I'm happy to tackle the e2e tests next, but in the meantime, here's a manual test plan:

Test Plan

Pre-requisites

Get the app go get github.com/evankanderson/sia
Create a Service

apiVersion: serving.knative.dev/v1alpha1
kind: Service
metadata:
  name: sia
  namespace: default
spec:
  runLatest:
    configuration:
      revisionTemplate:
        spec:
          container:
            image: github.com/evankanderson/sia
            ports:
              - name: h2c   # set this to `http1` to test http1
                containerPort: 8080

Install grpcurl and Curl with HTTP/2 support (--http2-prior-knowledge)

Test HTTP 1.1

ko apply -f sia.yaml with port name http1
curl -H 'Host: sia.default.example.com' http://${ip}:80 --http1.1
Check the logs (kubectl logs <sia pod> user-container), should say "Got GET on 1 with ..."
Repeat steps 2-3 after scale-to-zero

Test HTTP 2.0

ko apply -f sia.yaml with port name h2c
curl -H 'Host: sia.default.example.com' http://${ip}:80 --http2-prior-knowledge
Check the logs (kubectl logs <sia pod> user-container), should say "Got GET on 2 with ..."
Repeat steps 2-3 after scale-to-zero

Test Unary gRPC

ko apply -f sia.yaml with port name h2c
echo '{"thing":"SOMETHING"}' | grpcurl -plaintext -proto doer/doer.proto -authority sia.default.example.com -format json -d @ ${ip}:80 doer.Doer/DoIt
Should see { "words": "Did: SOMETHING" }
Check the logs (kubectl logs <sia pod> user-container), should say "Got POST on 2 with application/grpc"
Repeat steps 2-4 after scale-to-zero

Test Streaming gRPC

ko apply -f sia.yaml with port name h2c
echo '{"thing":"SOMETHING"}{"thing":"SOMETHING ELSE"}' | grpcurl -plaintext -proto doer/doer.proto -authority sia.default.example.com -d @ ${ip}:80 doer.Doer/KeepDoing
Should see { "words": "Did: SOMETHING" }{ "words": "Did: SOMETHING ELSE" }
Check the logs (kubectl logs <sia pod> user-container), should say "Got POST on 2 with application/grpc"
yes '{"thing":"SOMETHING"}' | grpcurl -plaintext -proto doer/doer.proto -authority sia.default.example.com -d @ ${ip}:80 doer.Doer/KeepDoing
Should see endless stream of { "words": "Did: SOMETHING" }
Repeat steps 2-6 after scale-to-zero

tanzeeb · 2019-01-30T23:37:08Z

I'm comfortable with a quick followup e2e test, but note that until there is an e2e test for this functionality, it's likely to backslide and be broken by accident.

Understood. It's happened a few times while I was working on the PR, so e2e tests are definitely a top priority.

I'd still like to get this PR in before that, so that folks have a chance to play with the functionality. There's been a lot of theoretical discussion on how this will impact other areas, such as autoscaling (eg. #2916), and it'd be helpful for those discussions to be informed by an actual implementation.

evankanderson · 2019-01-30T23:56:22Z

/approve

knative-prow-robot · 2019-01-30T23:56:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: evankanderson, tanzeeb

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [evankanderson]

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

evankanderson · 2019-01-30T23:56:52Z

/lgtm

* Use http.DefaultTransport dialer settings in h2c.DefaultTransport * Support streaming in activator/util.Rewinder * Use custom timeout handler in queue-proxy The http.TimeoutHandler will buffer the response body in memory until either the request completes or the request times out. This works well for HTTP, but is a problem for HTTP2 and gRPC streaming requests, where responses should be written as each sub-request is processed. This commit enforces the timeout by processing the request in a separate goroutine and panicing with http.ErrAbortHandler if the timeout is reached. The http.ErrAborthandler is a canary error used by the net/http and x/net/http2 packages to gracefully end the connection without dumping the stack trace. * Use http2 instead of http as the k8s service port names Istio uses port names in k8s services to determine protocols supported by the service. This change allows kservices to support HTTP/2 and gRPC traffic. * Enforce max content length for streaming requests in activator * Don't read original reader again after rewinder is closed * Use chan struct{} instead of chan bool in queue-proxy timeoutHandler * Move LimitReadCloser from activator handler to activator util and add test coverage * Add/fix godoc comments for activator/util * Explictly set Dialer config defaults in h2c.DefaultTransport * Remove timeoutHandler from queue-proxy * Revert k8s Service port name from http2 to http Changing it to http2 for all services breaks services which only support http1. Support for http2 will require selectively setting the port name to http2 only for services that explicitly support it. * Run activator on two ports, to support activating both http1 and h2c targets * Add RevisionProtocolType to API * Dynamically select 'http' or 'http2' for k8s service port name * Dynamically route to the http1 or h2c activator service port based on the revision protocol * Add test coverage for activator.ServicePort * Print errors in EnforceLengthHandler tests

This change makes numerous cleanups to the runtime contract in an attempt to improve the readability of the document and make the document more useful for the intended auidence. * Moves developer facing statements to a new `runtime-user-guide`. Focuses `runtime-contract` on operator/platform-provider. * Add links to Conformance tests that test Runtime Contract statements. * Corrects, updates, or removes statements to more accurately represent today's Knative runtime. * Updates to informative or removes most untestable statements * Copies in important OCI runtime requirements we previously referenced * Removes reference to OCI specification that didn't bring new requirements. Ref: knative#2539, knative#2973, knative#4014, knative#4027

knative-prow-robot requested review from josephburnett, lichuqiang, markusthoemmes and mattmoor November 24, 2018 00:59

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 24, 2018

knative-prow-robot reviewed Nov 24, 2018

View reviewed changes

Comment thread pkg/activator/util/rewinder.go Outdated

mattmoor reviewed Nov 24, 2018

View reviewed changes

Comment thread cmd/queue/main.go Outdated

mattmoor reviewed Nov 24, 2018

View reviewed changes

Comment thread pkg/http/h2c/h2c.go Outdated

markusthoemmes reviewed Nov 26, 2018

View reviewed changes

Comment thread pkg/reconciler/v1alpha1/revision/resources/constants.go Outdated

Comment thread pkg/activator/util/rewinder.go

Comment thread cmd/queue/main.go Outdated

Comment thread cmd/queue/main.go Outdated

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2018

tanzeeb force-pushed the http2-and-grpc-support branch from 1398d27 to 442563d Compare November 27, 2018 15:57

mattmoor assigned evankanderson and tcnghia and unassigned evankanderson Nov 27, 2018

tanzeeb mentioned this pull request Nov 29, 2018

Revision timeoutSecond should only apply until the first byte of the response #2582

Closed

dgerd mentioned this pull request Dec 5, 2018

Allow user-defined ports on Configuration #2642

Merged

tanzeeb force-pushed the http2-and-grpc-support branch from 442563d to 0c252ec Compare December 7, 2018 23:46

tanzeeb force-pushed the http2-and-grpc-support branch from 0c252ec to 58f2b1f Compare December 19, 2018 02:37

mattmoor added this to the Serving 0.4 milestone Jan 3, 2019

tcnghia mentioned this pull request Jan 3, 2019

Support gRPC endpoints #707

Closed

tanzeeb force-pushed the http2-and-grpc-support branch 3 times, most recently from 17987fc to 9641f41 Compare January 9, 2019 23:27

tanzeeb and others added 6 commits January 30, 2019 16:16

Revert k8s Service port name from http2 to http

9258466

Changing it to http2 for all services breaks services which only support http1. Support for http2 will require selectively setting the port name to http2 only for services that explicitly support it.

Run activator on two ports, to support activating both http1 and h2c …

a25b30a

…targets

Add RevisionProtocolType to API

e8200a5

Dynamically select 'http' or 'http2' for k8s service port name

2987952

Dynamically route to the http1 or h2c activator service port based on…

7ce391f

… the revision protocol

Add test coverage for activator.ServicePort

c7f6743

vagababov reviewed Jan 30, 2019

View reviewed changes

Comment thread pkg/activator/handler/enforce_length_handler_test.go

tanzeeb force-pushed the http2-and-grpc-support branch from d7ab918 to 1811bc0 Compare January 30, 2019 21:34

Print errors in EnforceLengthHandler tests

1700fbc

tanzeeb force-pushed the http2-and-grpc-support branch from 1811bc0 to 1700fbc Compare January 30, 2019 21:36

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2019

knative-prow-robot assigned evankanderson Jan 30, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2019

knative-prow-robot merged commit d92cc73 into knative:master Jan 30, 2019

joeholley mentioned this pull request Feb 12, 2019

MMFs on Knative googleforgames/open-match#75

Closed

tanzeeb mentioned this pull request Feb 13, 2019

gRPC streaming responses smaller than 4KB are buffered indefinitely #3188

Closed

dgerd mentioned this pull request May 7, 2019

Clean-up Runtime Contract #4035

Closed

dgerd mentioned this pull request Jun 6, 2019

Support GRPC and HTTP2 without explicit port labeling #4283

Open

feclist mentioned this pull request Jan 9, 2020

gRPC-web and Knative compatibility question #6478

Closed

astefanutti mentioned this pull request Apr 12, 2021

Expose gRPC endpoint with Knative serving apache/camel-k#2205

Closed

dprotaso deleted the http2-and-grpc-support branch September 2, 2025 17:20

Conversation

tanzeeb commented Nov 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

knative-prow-robot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattmoor commented Nov 24, 2018

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tanzeeb commented Nov 26, 2018

Uh oh!

tanzeeb commented Nov 27, 2018

Uh oh!

tanzeeb commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson commented Dec 6, 2018

Uh oh!

tanzeeb commented Dec 6, 2018

Uh oh!

evankanderson commented Dec 7, 2018 via email

Uh oh!

Uh oh!

knative-metrics-robot commented Jan 30, 2019

Uh oh!

evankanderson commented Jan 30, 2019

Uh oh!

tanzeeb commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Plan

Pre-requisites

Test HTTP 1.1

Test HTTP 2.0

Test Unary gRPC

Test Streaming gRPC

Uh oh!

tanzeeb commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evankanderson commented Jan 30, 2019

Uh oh!

knative-prow-robot commented Jan 30, 2019

Uh oh!

evankanderson commented Jan 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tanzeeb commented Nov 24, 2018 •

edited

Loading

tanzeeb commented Dec 5, 2018 •

edited

Loading

tanzeeb commented Jan 30, 2019 •

edited

Loading

tanzeeb commented Jan 30, 2019 •

edited

Loading