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

specifying features.knative.dev/http-full-duplex and h2c container port breaks the app #15688

Open
dprotaso opened this issue Jan 14, 2025 · 1 comment
Milestone

Comments

@dprotaso
Copy link
Member

apiVersion: serving.knative.dev/v1
kind: Service
metadata:
  name: hoge
  namespace: hoge
spec:
  template:
    metadata:
      annotations:
        autoscaling.knative.dev/minScale: "0"
        features.knative.dev/http-full-duplex: Enabled
        instrumentation.opentelemetry.io/inject-nodejs: opentelemetry-collector/instrumentation
        refresh: "true"
      creationTimestamp: null
    spec:
      containerConcurrency: 0
      containers:
      - envFrom:
        - secretRef:
            name: hoge
        image: xxxxx
        name: hoge
        ports:
        - containerPort: 50052
          name: h2c
          protocol: TCP
        readinessProbe:
          successThreshold: 1
          tcpSocket:
            port: 0
        resources:
          limits:
            memory: 500Mi
          requests:
            cpu: 10m
            memory: 300Mi
      responseStartTimeoutSeconds: 1800
      serviceAccountName: hoge
      timeoutSeconds: 60
  traffic:
  - latestRevision: true
    percent: 100

You'll see in the queue-proxy logs Unable to enable full duplex and it doesn't work

{"severity":"error","timestamp":"2025-01-13T04:25:38.690Z","logger":"queueproxy","caller":"sharedmain/handlers.go:137","message":"Unable to enable full duplex","commit":"6a27004","knative.dev/key":"xxx/xxxx","knative.dev/pod":"xxxx","error":"feature not supported","stacktrace":"knative.dev/serving/pkg/queue/sharedmain.mainHandler.withFullDuplex.func5\n\tknative.dev/serving/pkg/queue/sharedmain/handlers.go:137\nnet/http.HandlerFunc.ServeHTTP\n\tnet/http/server.go:2171\nknative.dev/pkg/network/handlers.(*Drainer).ServeHTTP\n\tknative.dev/[email protected]/network/handlers/drain.go:113\ngolang.org/x/net/http2.(*serverConn).runHandler\n\tgolang.org/x/[email protected]/http2/server.go:2439"}
@dprotaso dprotaso added this to the v1.18 milestone Jan 14, 2025
@skonto
Copy link
Contributor

skonto commented Jan 14, 2025

@dprotaso This comes from the fact that HTTP2 is used (h2c, http2 cleartext) along with the full-duplex feature. The latter was meant for HTTP1. Am I missing something? Copying from net/http:

// EnableFullDuplex indicates that the request handler will interleave reads from [Request.Body]
// with writes to the [ResponseWriter].
//
// For HTTP/1 requests, the Go HTTP server by default consumes any unread portion of
// the request body before beginning to write the response, preventing handlers from
// concurrently reading from the request and writing the response.
// Calling EnableFullDuplex disables this behavior and permits handlers to continue to read
// from the request while concurrently writing the response.
//
// For HTTP/2 requests, the Go HTTP server always permits concurrent reads and responses.
func (c *ResponseController) EnableFullDuplex() error {
	rw := c.rw
	for {
		switch t := rw.(type) {
		case interface{ EnableFullDuplex() error }:
			return t.EnableFullDuplex()
		case rwUnwrapper:
			rw = t.Unwrap()
		default:
			return errNotSupported()
		}
	}
}

Should we catch this earlier e.g. when we validate the revision?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants