-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Supports gRPC as a protocol (without TLS) #987
Changes from all commits
d7254f9
51b5627
92123eb
962b768
b2bb3aa
1f64ec3
07bab67
f6456c0
96b4fe1
8c8a7cc
fa6ddee
c875b4c
c001fd7
0301aa5
678c753
0864f31
8a4e384
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,14 +32,15 @@ import ( | |
"syscall" | ||
"time" | ||
|
||
"github.com/knative/serving/pkg" | ||
|
||
"github.com/knative/serving/cmd/util" | ||
"github.com/knative/serving/pkg" | ||
"github.com/knative/serving/pkg/apis/serving/v1alpha1" | ||
"github.com/knative/serving/pkg/autoscaler" | ||
h2cutil "github.com/knative/serving/pkg/h2c" | ||
"github.com/knative/serving/pkg/logging" | ||
"github.com/knative/serving/pkg/logging/logkey" | ||
"github.com/knative/serving/pkg/queue" | ||
"github.com/knative/serving/third_party/h2c" | ||
"go.uber.org/zap" | ||
|
||
"github.com/gorilla/websocket" | ||
|
@@ -66,18 +67,21 @@ const ( | |
) | ||
|
||
var ( | ||
podName string | ||
elaNamespace string | ||
elaConfiguration string | ||
elaRevision string | ||
elaAutoscaler string | ||
elaAutoscalerPort string | ||
statChan = make(chan *autoscaler.Stat, statReportingQueueLength) | ||
reqChan = make(chan queue.ReqEvent, requestCountingQueueLength) | ||
kubeClient *kubernetes.Clientset | ||
statSink *websocket.Conn | ||
proxy *httputil.ReverseProxy | ||
logger *zap.SugaredLogger | ||
podName string | ||
elaNamespace string | ||
elaConfiguration string | ||
elaRevision string | ||
elaAutoscaler string | ||
elaAutoscalerPort string | ||
statChan = make(chan *autoscaler.Stat, statReportingQueueLength) | ||
reqChan = make(chan queue.ReqEvent, requestCountingQueueLength) | ||
kubeClient *kubernetes.Clientset | ||
statSink *websocket.Conn | ||
logger *zap.SugaredLogger | ||
|
||
h2cProxy *httputil.ReverseProxy | ||
httpProxy *httputil.ReverseProxy | ||
|
||
concurrencyQuantumOfTime = flag.Duration("concurrencyQuantumOfTime", 100*time.Millisecond, "") | ||
concurrencyModel = flag.String("concurrencyModel", string(v1alpha1.RevisionRequestConcurrencyModelMulti), "") | ||
singleConcurrencyBreaker = queue.NewBreaker(singleConcurrencyQueueDepth, 1) | ||
|
@@ -141,18 +145,29 @@ func statReporter() { | |
} | ||
} | ||
|
||
func proxyForRequest(req *http.Request) *httputil.ReverseProxy { | ||
if req.ProtoMajor == 2 { | ||
return h2cProxy | ||
} | ||
|
||
return httpProxy | ||
} | ||
|
||
func isProbe(r *http.Request) bool { | ||
// Since K8s 1.8, prober requests have | ||
// User-Agent = "kube-probe/{major-version}.{minor-version}". | ||
return strings.HasPrefix(r.Header.Get("User-Agent"), "kube-probe/") | ||
} | ||
|
||
func handler(w http.ResponseWriter, r *http.Request) { | ||
proxy := proxyForRequest(r) | ||
|
||
if isProbe(r) { | ||
// Do not count health checks for concurrency metrics | ||
proxy.ServeHTTP(w, r) | ||
return | ||
} | ||
|
||
// Metrics for autoscaling | ||
reqChan <- queue.ReqIn | ||
defer func() { | ||
|
@@ -253,7 +268,10 @@ func main() { | |
if err != nil { | ||
logger.Fatal("Failed to parse localhost url", zap.Error(err)) | ||
} | ||
proxy = httputil.NewSingleHostReverseProxy(target) | ||
|
||
httpProxy = httputil.NewSingleHostReverseProxy(target) | ||
h2cProxy = httputil.NewSingleHostReverseProxy(target) | ||
h2cProxy.Transport = h2cutil.NewTransport() | ||
|
||
logger.Infof("Queue container is starting, concurrencyModel: %s", *concurrencyModel) | ||
config, err := rest.InClusterConfig() | ||
|
@@ -281,10 +299,15 @@ func main() { | |
} | ||
}() | ||
|
||
server := &http.Server{ | ||
Addr: fmt.Sprintf(":%d", queue.RequestQueuePort), Handler: nil} | ||
adminServer := &http.Server{ | ||
Addr: fmt.Sprintf(":%d", queue.RequestQueueAdminPort), Handler: nil} | ||
Addr: fmt.Sprintf(":%d", queue.RequestQueueAdminPort), | ||
Handler: nil, | ||
} | ||
|
||
h2cServer := h2c.Server{Server: &http.Server{ | ||
Addr: fmt.Sprintf(":%d", queue.RequestQueuePort), | ||
Handler: http.HandlerFunc(handler), | ||
}} | ||
|
||
// Add a SIGTERM handler to gracefully shutdown the servers during | ||
// pod termination. | ||
|
@@ -294,11 +317,12 @@ func main() { | |
<-sigTermChan | ||
// Calling server.Shutdown() allows pending requests to | ||
// complete, while no new work is accepted. | ||
server.Shutdown(context.Background()) | ||
|
||
h2cServer.Shutdown(context.Background()) | ||
adminServer.Shutdown(context.Background()) | ||
os.Exit(0) | ||
}() | ||
http.HandleFunc("/", handler) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need this because we're setting Handler in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes |
||
go server.ListenAndServe() | ||
|
||
go h2cServer.ListenAndServe() | ||
setupAdminHandlers(adminServer) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,7 @@ | |
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
name: activator-service | ||
name: activator-http-service | ||
namespace: knative-serving-system | ||
labels: | ||
app: activator | ||
|
@@ -28,3 +28,20 @@ spec: | |
port: 80 | ||
targetPort: 8080 | ||
type: NodePort | ||
--- | ||
apiVersion: v1 | ||
kind: Service | ||
metadata: | ||
name: activator-grpc-service | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need a second service and app here still? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do need a second service while we are still using Istio (v1alpha1). If the backend service port name does not match the servicePort on the ingress using the protocol convention, traffic won't be able to proxied through to the revision. The app should be the same |
||
namespace: knative-serving-system | ||
labels: | ||
app: activator | ||
spec: | ||
selector: | ||
app: activator | ||
ports: | ||
- name: grpc | ||
protocol: TCP | ||
port: 80 | ||
targetPort: 8080 | ||
type: NodePort |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -181,6 +181,8 @@ spec: | |
livenessProbe: ... # Optional | ||
readinessProbe: ... # Optional | ||
|
||
# +optional protcol (http or grpc). Defaults to http. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the purpose of having "protocol" here? Is the concern libraries which only support h2/h2c, and not HTTP/1.1? From my reading of the gRPC protocol, it looks like it should be possible for general HTTP/2 and gRPC to operate on the same server. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is due to the named ports requirement in Istio 0.7. We couldn't use http2 since that assumes that the traffic is encrypted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my other comment:
FWIW, I'm still hunting for time to get an SSL cert set up on my Istio cluster to verify that Istio can do https ingress --> h2c pod. I think this should work from my reading of the docs, but I need to verify. |
||
protocol: ... | ||
# +optional concurrency strategy. Defaults to Multi. | ||
concurrencyModel: ... | ||
# +optional. max time the instance is allowed for responding to a request | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tl;dr It feels to me like this belongs on cc @vaikas-google @evankanderson @cooperneil I have a hard time imagining a use-case where a I believe the main reason this is needed on
I suspect we also need to change the activator in a similar manner. cc @josephburnett There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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:
Istio docs that explains it here We are currently in the process of removing the need for the protocol in the We're also in the process of changing the activator to utilize a h2c server as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
|
||
# Some function or server frameworks or application code may be written to | ||
# expect that each request will be granted a single-tenant process to run | ||
# (i.e. that the request code is run single-threaded). | ||
|
@@ -331,6 +337,7 @@ spec: # One of "runLatest" or "pinned" | |
- ... | ||
livenessProbe: ... # Optional | ||
readinessProbe: ... # Optional | ||
protocol: ... | ||
concurrencyModel: ... | ||
timeoutSeconds: ... | ||
serviceAccountName: ... # Name of the service account the code should run as | ||
|
@@ -353,6 +360,7 @@ spec: # One of "runLatest" or "pinned" | |
- ... | ||
livenessProbe: ... # Optional | ||
readinessProbe: ... # Optional | ||
protocol: ... | ||
concurrencyModel: ... | ||
timeoutSeconds: ... | ||
serviceAccountName: ... # Name of the service account the code should run as | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just "server" again, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes