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.

Sign up for GitHub

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

Jump to bottom

pilot: add support for grpc-web prefixed ports #10064

Merged
merged 1 commit into from
Nov 21, 2018
Merged

pilot: add support for grpc-web prefixed ports #10064

merged 1 commit into from
Nov 21, 2018

Conversation

venilnoronha
Copy link
Member

@venilnoronha venilnoronha commented Nov 20, 2018 •
edited
Loading

Description

This adds support for grpc-web prefixed port names. Whenever Istio notices a Service port prefixed with grpc-web, an additional envoy.grpc_web filter will be added to the http filters list in the HTTP connection manager for inbound sidecars.

Fixes #1368

Usage

To enable the envoy.grpc_web filter, users will now just need to use a service config similar to below. The grpc-web prefix in name: grpc-web-port will let Pilot know that the envoy.grpc_web filter should be enabled.

apiVersion: v1
kind: Service
metadata:
  name: some-service
  labels:
    app: some-service
spec:
  ports:
  - name: grpc-web-port
    port: 9000
  selector:
    app: some-service

One additional thing to do after deploying the service is to enable CORS on the VirtualService i.e. if the gRPC-Web based service is accessed externally. The following configuration should do the job.

apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: virtual-service
spec:
  hosts:
  - "*"
  gateways:
  - gateway
  http:
  - match:
    - port: 80
    route:
    - destination:
        host: some-service
        port:
          number: 9000
    corsPolicy:
      allowOrigin:
        - "*"
      allowMethods:
        - POST
        - GET
        - OPTIONS
        - PUT
        - DELETE
      allowHeaders:
        - grpc-timeout
        - content-type
        - keep-alive
        - user-agent
        - cache-control
        - content-type
        - content-transfer-encoding
        - custom-header-1
        - x-accept-content-transfer-encoding
        - x-accept-response-streaming
        - x-user-agent
        - x-grpc-web
      maxAge: 1728s
      exposeHeaders:
        - custom-header-1
        - grpc-status
        - grpc-message
      allowCredentials: true

See https://github.com/grpc/grpc-web/tree/master/net/grpc/gateway/examples/helloworld#configure-the-proxy for more information.

@venilnoronha venilnoronha changed the title pilot: add support for grpc-web- prefixed ports [WIP] pilot: add support for grpc-web- prefixed ports Nov 20, 2018
@istio-testing istio-testing added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 20, 2018
@venilnoronha venilnoronha changed the title [WIP] pilot: add support for grpc-web- prefixed ports [WIP] pilot: add support for grpc-web prefixed ports Nov 20, 2018
rshriram
rshriram reviewed Nov 20, 2018
View reviewed changes
Copy link
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..

@venilnoronha venilnoronha changed the title [WIP] pilot: add support for grpc-web prefixed ports pilot: add support for grpc-web prefixed ports Nov 20, 2018
@istio-testing istio-testing removed the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Nov 20, 2018
@codecov
Copy link

codecov bot commented Nov 20, 2018 •
edited
Loading

Codecov Report

Merging #10064 into release-1.1 will increase coverage by 1%.
The diff coverage is 64%.

Impacted file tree graph

@@              Coverage Diff              @@
##           release-1.1   #10064    +/-   ##
=============================================
+ Coverage           70%      71%    +1%     
=============================================
  Files              442      442            
  Lines            41187    41147    -40     
=============================================
+ Hits             28640    28820   +180     
+ Misses           11141    10921   -220     
  Partials          1406     1406
Impacted Files Coverage Δ
pilot/pkg/networking/plugin/plugin.go 75% <100%> (ø) ⬆️
pilot/pkg/serviceregistry/kube/conversion.go 92% <100%> (+4%) ⬆️
pilot/pkg/model/service.go 86% <100%> (ø) ⬇️
pilot/pkg/networking/core/v1alpha3/listener.go 78% <42%> (ø) ⬇️
mixer/adapter/signalfx/cumulative.go 67% <0%> (-4%) ⬇️
mixer/adapter/signalfx/tracing.go 80% <0%> (-3%) ⬇️
galley/pkg/kube/converter/proto.go 86% <0%> (-2%) ⬇️
mixer/adapter/fluentd/fluentd.go 74% <0%> (-1%) ⬇️
galley/pkg/crd/validation/config.go 66% <0%> (-1%) ⬇️
...ter/kubernetesenv/template/template_handler.gen.go 96% <0%> (-1%) ⬇️
... and 31 more

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 60e6e9b...eb7c6f1. Read the comment docs.

@venilnoronha venilnoronha changed the base branch from master to release-1.1 November 21, 2018 02:20
@googlebot
Copy link
Collaborator

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 all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. and removed cla: yes labels Nov 21, 2018
rshriram
rshriram approved these changes Nov 21, 2018
View reviewed changes
Copy link
Member

@rshriram rshriram left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@istio-testing
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rshriram, venilnoronha

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

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

11 similar comments
@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
Copy link
Member Author

/test e2e-bookInfoTests-envoyv2-v1alpha3

@venilnoronha
pilot: add support for grpc-web prefixed ports
f2c7f6f 
This adds support for grpc-web prefixed port names. Whenever Istio
notices a Service port prefixed with grpc-web, an additional envoy.grpc_web
filter will be added to the http filters list in the HTTP connection
manager for inbound sidecars.

See https://github.com/grpc/grpc-web/tree/master/net/grpc/gateway/examples/helloworld#configure-the-proxy
for more information.

Signed-off-by: Venil Noronha <veniln@vmware.com>
@istio-testing
Copy link
Collaborator

@venilnoronha: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
prow/e2e-simpleTests-cni.sh c0aeae37fe7ff2ee9f427dfa41d10438ba4ac592 link /test e2e-simpleTests-cni
prow/istio-integ-k8s-tests.sh ffccda99d00f71422ba323cbd13d25cfadf9f9df link /test istio-integ-k8s-tests
prow/e2e-mixer-no_auth-mcp.sh f2c7f6f link /test e2e-mixer-no_auth-mcp

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. I understand the commands that are listed here.

@istio-testing istio-testing merged commit ca67fe3 into istio:release-1.1 Nov 21, 2018
@venilnoronha venilnoronha mentioned this pull request Nov 21, 2018
Closed
@xmlking xmlking mentioned this pull request Sep 3, 2019
Closed
This was referenced Jan 24, 2020
Closed
Merged
istio-testing pushed a commit that referenced this pull request Jan 28, 2020
@nak3 @istio-testing
Add envoy gRPC web filter to gateway when grpc-web is enabled (#20537)
4fe6913 
This patch adds `envoy.grpc_web` filter to gateway when Gateway's
port.protocol is `GRPC-WEB`.

Currently istio-sidecar adds `envoy.grpc_web` filter when grpc-web
prefixed port is used thanks to
#10064. But it always needs
side-car injection.
@bradenwright bradenwright mentioned this pull request Jun 3, 2022
Closed
@bradenwright-opunai bradenwright-opunai mentioned this pull request Jun 7, 2022
Closed
@krapie krapie mentioned this pull request Mar 2, 2023
Closed
@wSedlacek wSedlacek mentioned this pull request Dec 17, 2023
Open
antonioberben pushed a commit to antonioberben/istio that referenced this pull request Jan 29, 2024
@cpanato @k8s-ci-robot
bump operator to 1.8.2 and add recommended label (istio#10064)
8ef9e62 
Signed-off-by: cpanato <ctadeu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rshriram rshriram rshriram approved these changes

@GregHanson GregHanson Awaiting requested review from GregHanson

@ZackButcher ZackButcher Awaiting requested review from ZackButcher

@kyessenov kyessenov Awaiting requested review from kyessenov

Assignees

@rshriram rshriram

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@venilnoronha @googlebot @istio-testing @rshriram