Supports gRPC as a protocol (without TLS) #987

dprotaso · 2018-05-30T15:08:50Z

Fixes #707, #813

Proposed Changes

Supports gRPC as a protocol (without TLS)

TODO

e2e test
activator support
switching to using an h2(c) server (non TLS) in ela-queue & activator

Outstanding Questions

what metrics is the autoscaler interested in when the protocol is grpc & the exchange is bidirectional
istio + grpc + mtls

googlebot · 2018-05-30T15:08:53Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

mattmoor

I took a cursory look, my main thoughts are in docs/spec/spec.md.

mattmoor · 2018-05-31T13:31:26Z

sample/grpc-ping/README.md

+
+1. [Install Elafros](https://github.com/elafros/install/blob/master/README.md)
+1. Install [docker](https://www.docker.com/)
+1. Install [ko](https://github.com/google/go-containerregistry/tree/master/cmd/ko)


tl;dr remove either Docker or ko

My biases in sample/ lately:

Document it with Dockerfile in README.md (which is probably most familiar to users)

Make it work with Bazel / ko for contributors

Does that make sense?

mattmoor · 2018-05-31T13:52:43Z

docs/spec/spec.md

@@ -256,6 +258,10 @@ spec:
  # scaling to/from 0.
  servingState: Active | Reserve | Retired

+  # The protocol used for this Revision. This will define the Istio ingress spec
+  # and service spec required for this Revision.
+  protocol: grpc | http


tl;dr It feels to me like this belongs on Route not Revision.

cc @vaikas-google @evankanderson @cooperneil

I have a hard time imagining a use-case where a Route splits traffic over multiple Revisions with different protocols, which is reflected in some of the getProtocolFromTrafficTargets logic and TODOs that need to cope with the potential heterogeneity here. Let's just not allow it.

I believe the main reason this is needed on Revision in its current form is because you make use of it in ela-queue to proxy arbitrary TCP traffic. Two questions:

Can we switch completely to tcpproxy thereby eliminating the need for the config @ Revision level?

How does this codepath expose auto-scaling metrics (the core purpose of ela-queue today)?

I suspect we also need to change the activator in a similar manner. cc @josephburnett

@dprotaso and I struggled with where the protocol belongs ourselves and decided to specify the protocol on the Revision and open it up for discussion.

Both the route controller and revision controller need to be aware of the protocol due to the following:

The route controller needs to create the appropriate Istio ingress and route service with the protocol specified

The revision controller also needs to create the revision service with the servicePort name specified as the protocol

Istio docs that explains it here

We are currently in the process of removing the need for the protocol in the ela-queue container by utilizing a h2cserver that can proxy both h2c and http traffic based on the request protocol. The reason why we couldn't use a tcp proxy was because we wouldn't be able to collect request statistics for autoscaling.

We're also in the process of changing the activator to utilize a h2c server as well.

FYI the Envoy in the istio sidecar collects request statistics similar to the ones ela-queue collects. Using them hasn't been a high priority yet, but if the queue-proxy is a blocker that could change. It's unclear whether they can fully replace the queue proxy stats collection.

FWIW, the Istio docs on the port naming restrictions for ingress seem to have been removed between 0.7 and 0.8. I'm not sure why earlier versions of Istio had this restriction (digging, it appears to have been copied from the pod injection docs).

dprotaso · 2018-05-31T18:43:52Z

/test pull-elafros-elafros-integration-tests

google-prow-robot · 2018-06-04T18:44:22Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @evankanderson 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:

OWNERS

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

knative-metrics-robot · 2018-06-05T14:11:52Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	77.9%	78.1%	0.2%
pkg/controller/route/route_test.go	77.9%	78.1%	0.2%
pkg/webhook/configuration.go	95.2%	92.1%	-3.0%

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-05T14:48:18Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	77.9%	78.1%	0.2%
pkg/controller/route/route_test.go	77.9%	78.1%	0.2%
pkg/webhook/configuration.go	95.2%	92.1%	-3.0%

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-05T14:58:21Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	77.9%	78.1%	0.2%
pkg/controller/route/route_test.go	77.9%	78.1%	0.2%
pkg/webhook/configuration.go	95.2%	92.1%	-3.0%

*TestCoverage feature is being tested, do not rely on any info here yet

knative-metrics-robot · 2018-06-05T15:41:44Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	77.9%	78.1%	0.2%
pkg/controller/route/route_test.go	77.9%	78.1%	0.2%
pkg/webhook/configuration.go	95.2%	92.1%	-3.0%

*TestCoverage feature is being tested, do not rely on any info here yet

bsnchan · 2018-06-05T16:12:53Z

/test pull-knative-serving-integration-tests

bsnchan · 2018-06-05T17:37:59Z

/test pull-knative-serving-integration-tests

knative-metrics-robot · 2018-06-05T17:55:05Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	77.9%	78.1%	0.2%
pkg/controller/route/route_test.go	77.9%	78.1%	0.2%
pkg/webhook/configuration.go	95.2%	92.1%	-3.0%

*TestCoverage feature is being tested, do not rely on any info here yet

bsnchan · 2018-06-05T17:59:20Z

/test pull-knative-serving-unit-tests

bsnchan · 2018-06-05T18:29:29Z

/test pull-knative-serving-integration-tests

bsnchan · 2018-06-05T18:51:37Z

/test pull-knative-serving-integration-tests

bsnchan · 2018-06-05T21:25:23Z

/test pull-knative-serving-integration-tests

knative-metrics-robot · 2018-06-06T15:32:07Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	77.9%	78.1%	0.2%
pkg/controller/route/route_test.go	77.9%	78.1%	0.2%
pkg/webhook/configuration.go	95.2%	92.1%	-3.0%

*TestCoverage feature is being tested, do not rely on any info here yet

* We needed to create an activator service per protocol Since services cannot share a port with different protocols Co-authored-by: Dave Protasowski <[email protected]>

Co-authored-by: Dave Protasowski <[email protected]>

* When the Ingress is updated for gRPC, the service takes some time to mark the upstream as healthy

* it was only being used in the sample grpc app so we now pull in the depency via the sample app's dockerfile * also removed unused variables from cmd/queue/main.go

ignore the folders * google.golang.org/grpc is not vendored in since it is only used in the sample apps so don't attempt to build or test the sample grpc apps

* This is allows go build ./sample/... to run successfully since the grpc dependencies are not vendored in

* removes golang/protobuf from Gopkg.toml * alias h2c package as h2cutil * adds a comment for NewTransport * removes refactoring changes (will submit them as a separate pr) * updates sample app's readme to use buildtag when running client code

knative-metrics-robot · 2018-06-18T17:55:26Z

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

File	Old Coverage	New Coverage	Delta
pkg/controller/route/route.go	78.7%	79.0%	0.3
pkg/controller/route/route_test.go	78.7%	79.0%	0.3
pkg/webhook/configuration.go	95.2%	92.1%	-3.0

*TestCoverage feature is being tested, do not rely on any info here yet

bsnchan · 2018-06-18T17:55:47Z

Hey @evankanderson - is anything blocking this from getting merged? Having to rebase constantly is a bit of a pain. It would be nice to mark this as hold if we're waiting for another PR to go in first.

evankanderson · 2018-06-18T19:31:38Z

Good point -- I have two concerns with this CL (one of which you're clearly having to wrestle with):

It's pretty big, and will have a lot of conflicts.
It's introducing a "fork" into the data plane which may be difficult to take out later, and which shouldn't be needed after Replace Ingress and RouteRule with Gateway and VirtualService respectively #1047 is fixed (presumably by Migrate to Istio v1alpha3 Gateway / VirtualService #1228)

Maybe we can break this into 2-4 items?

queue and activator changes seem non-controversial for item 2, and will be a long-term benefit in making our request path HTTP/2 clean.
API and route and configuration controller changes are where my concerns are -- introducing forking data paths increases the changes that each data path will end up with different capabilities. Then we end up making a chart explaining which paths support which things, and it's always out of date with the code. As much as possible, I'd like to keep a single data path which is signaled on L7 traffic which is compatible with HTTP framing (websocket, http2, grpc all meet this goal).
The grpc sample app, which seems like a great way to test that the e2e is working.

bsnchan · 2018-06-18T19:45:52Z

@evankanderson That make sense. I agree with waiting for #1228 to be completed as it would probably simplify a lot of the API/controller changes.

To clarify - I can submit separate PRs for:

queue to support h2c
activator to support h2c
any controller changes required after Migrate to Istio v1alpha3 Gateway / VirtualService #1228 is completed
grpc sample app

Would you accept the first two changes as PR's today? Or would you prefer for the Istio upgrade PR to land first?

evankanderson · 2018-06-18T20:23:40Z

I'd be happy to review the first two PRs today.

Ideally, the third PR would not involve any API surface changes.

I'm comfortable committing the 4th item as a simple app even before it's working, as long as we document in the README that it is aspirational on #1047.

bsnchan · 2018-06-18T21:40:15Z

Closing this PR - followed up with #1257, #1260 and #1261.

Update: I can't close this PR since @dprotaso created it :(

dprotaso · 2018-06-19T01:46:19Z

closing

…ces/release-v1.15 Update Konflux references (release-v1.15)

google-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 30, 2018

google-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 30, 2018

mattmoor reviewed May 31, 2018

View reviewed changes

evankanderson assigned evankanderson and mattmoor and unassigned evankanderson May 31, 2018

bsnchan force-pushed the grpc branch from c08f3d1 to a7d21ed Compare June 5, 2018 14:09

bsnchan force-pushed the grpc branch 2 times, most recently from 8fb9a4a to 4c039c0 Compare June 5, 2018 14:46

bsnchan force-pushed the grpc branch from 4c039c0 to 7154efe Compare June 5, 2018 14:56

bsnchan force-pushed the grpc branch from 7154efe to fefb769 Compare June 5, 2018 15:39

bsnchan force-pushed the grpc branch from fefb769 to f18cb25 Compare June 5, 2018 17:52

bsnchan force-pushed the grpc branch from 509bcbe to f4ff392 Compare June 6, 2018 15:20

bsnchan force-pushed the grpc branch from 8fbce0e to 7569fdd Compare June 6, 2018 15:52

bsnchan and others added 15 commits June 18, 2018 13:52

Update the activator to use an h2c server

92123eb

* We needed to create an activator service per protocol Since services cannot share a port with different protocols Co-authored-by: Dave Protasowski <[email protected]>

Update grpc-ping sample

962b768

Co-authored-by: Dave Protasowski <[email protected]>

Refactors sample grpc-ping app

b2bb3aa

Adds e2e test for gRPC application

1f64ec3

Rebasing changes after knative renaming

07bab67

Bump Request Timeout to 90s for e2e tests

f6456c0

* When the Ingress is updated for gRPC, the service takes some time to mark the upstream as healthy

Bump timeout to 2 minutes because the tests are flakey

96b4fe1

Updates tests with new ingress name

8c8a7cc

More rebasing changes, Removes long timeout due to e2e flakiness

fa6ddee

Removes vendored grpc-go dependency

c875b4c

* it was only being used in the sample grpc app so we now pull in the depency via the sample app's dockerfile * also removed unused variables from cmd/queue/main.go

Moves h2c package to third_party folder

c001fd7

Prepend grpc related sample apps with '_' so go build and go tests

0301aa5

ignore the folders * google.golang.org/grpc is not vendored in since it is only used in the sample apps so don't attempt to build or test the sample grpc apps

Removes grpc e2e test

678c753

Label grpc-ping with a build tag

0864f31

* This is allows go build ./sample/... to run successfully since the grpc dependencies are not vendored in

Changes based on suggestions

8a4e384

* removes golang/protobuf from Gopkg.toml * alias h2c package as h2cutil * adds a comment for NewTransport * removes refactoring changes (will submit them as a separate pr) * updates sample app's readme to use buildtag when running client code

bsnchan force-pushed the grpc branch from e7c04b2 to 8a4e384 Compare June 18, 2018 17:53

This was referenced Jun 18, 2018

Updates queue container to support h2c #1257

Merged

Updates activator to support h2c #1260

Merged

Adds sample gRPC application #1261

Merged

dprotaso closed this Jun 19, 2018

tanzeeb mentioned this pull request Dec 5, 2018

HTTP2 and gRPC support #2539

Merged

skonto pushed a commit to skonto/serving that referenced this pull request Dec 3, 2021

Fix 004-grpc.patch (knative#987)

483c538

ReToCode added a commit to ReToCode/serving that referenced this pull request Nov 5, 2024

Merge pull request knative#987 from openshift-knative/konflux/referen…

568985a

…ces/release-v1.15 Update Konflux references (release-v1.15)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supports gRPC as a protocol (without TLS) #987

Supports gRPC as a protocol (without TLS) #987

dprotaso commented May 30, 2018 •

edited

Loading

googlebot commented May 30, 2018

mattmoor left a comment

mattmoor May 31, 2018

mattmoor May 31, 2018

bsnchan May 31, 2018

grantr May 31, 2018

evankanderson May 31, 2018

dprotaso commented May 31, 2018

google-prow-robot commented Jun 4, 2018

knative-metrics-robot commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

knative-metrics-robot commented Jun 6, 2018

knative-metrics-robot commented Jun 18, 2018

bsnchan commented Jun 18, 2018

evankanderson commented Jun 18, 2018

bsnchan commented Jun 18, 2018 •

edited

Loading

evankanderson commented Jun 18, 2018

bsnchan commented Jun 18, 2018 •

edited

Loading

dprotaso commented Jun 19, 2018

Supports gRPC as a protocol (without TLS) #987

Supports gRPC as a protocol (without TLS) #987

Conversation

dprotaso commented May 30, 2018 • edited Loading

Proposed Changes

TODO

Outstanding Questions

googlebot commented May 30, 2018

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor May 31, 2018

Choose a reason for hiding this comment

mattmoor May 31, 2018

Choose a reason for hiding this comment

bsnchan May 31, 2018

Choose a reason for hiding this comment

grantr May 31, 2018

Choose a reason for hiding this comment

evankanderson May 31, 2018

Choose a reason for hiding this comment

dprotaso commented May 31, 2018

google-prow-robot commented Jun 4, 2018

knative-metrics-robot commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

knative-metrics-robot commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

bsnchan commented Jun 5, 2018

knative-metrics-robot commented Jun 6, 2018

knative-metrics-robot commented Jun 18, 2018

bsnchan commented Jun 18, 2018

evankanderson commented Jun 18, 2018

bsnchan commented Jun 18, 2018 • edited Loading

evankanderson commented Jun 18, 2018

bsnchan commented Jun 18, 2018 • edited Loading

dprotaso commented Jun 19, 2018

dprotaso commented May 30, 2018 •

edited

Loading

bsnchan commented Jun 18, 2018 •

edited

Loading

bsnchan commented Jun 18, 2018 •

edited

Loading