Enable refactoring queue-proxy binary with out-of-tree extensions #13133

davidhadas · 2022-07-19T13:17:58Z

Fixes #13118
Alternative to #13122

Proposed Changes

Enabling downstream and anyone creating an image of queue proxy to extend queue proxy
This extendibility is a second step to enable the runtime security proposal here
Next possible steps: (1) Support QP mutation pattern (2) support for annotations or another method to enable activating/configuring extensions
Items for discussion: (a) Should we add the script to help demonstrate how extendibility works (or as a potential basis of an e2e testing)? (b)Should we allow extensions to change logger or env?

Release Note

Refactored queue-proxy binary to more easily support out-of-tree extension.

Fixes knative#13118 Alternative to knative#13122 Proposed Changes Enabling downstream and anyone creating an image of queue proxy to extend queue proxy This extendibility is a step to enable the runtime security proposal here Release Note Refactored queue-proxy binary to more easily support out-of-tree extension.

knative-prow · 2022-07-19T13:18:12Z

Hi @davidhadas. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

codecov · 2022-07-19T13:25:01Z

Codecov Report

Merging #13133 (5137f03) into main (017b9d0) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main   #13133      +/-   ##
==========================================
- Coverage   86.78%   86.72%   -0.07%     
==========================================
  Files         196      196              
  Lines       14410    14416       +6     
==========================================
- Hits        12506    12502       -4     
- Misses       1609     1619      +10     
  Partials      295      295

Impacted Files	Coverage Δ
pkg/queue/sharedmain/main.go	`0.64% <0.00%> (-0.03%)`	⬇️
pkg/autoscaler/statserver/server.go	`75.55% <0.00%> (-2.23%)`	⬇️
pkg/reconciler/revision/controller.go	`92.95% <0.00%> (+3.62%)`	⬆️

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 017b9d0...5137f03. Read the comment docs.

psschwei · 2022-07-19T14:31:51Z

/ok-to-test

psschwei · 2022-07-19T14:42:19Z

Items for discussion: (a) Should we add the script to help demonstrate how extendibility works (or as a potential basis of an e2e testing)?

I think initially this would belong in a sandbox/private repo rather than in serving core.

(b)Should we allow extensions to change logger or env?

My gut reaction to this one is "no", as I'd assume that any needed changes could be done in the configs rather than needing to change them. Is there a particular use case you have in mind?

psschwei · 2022-07-19T15:02:39Z

pkg/queue/sharedmain/main.go

+	Env
+}
+
+type Env struct {
+	ServingNamespace     string `split_words:"true" required:"true"`
+	ServingRevision      string `split_words:"true" required:"true"`
+	ServingConfiguration string `split_words:"true" required:"true"`
+	ServingPodIP         string `split_words:"true" required:"true"`
+	ServingPod           string `split_words:"true" required:"true"`
+	ServingService       string `split_words:"true"` // optional


a couple of minor nits:

maybe don't rename the config struct ?

perhaps a more descriptive name than Env (PodDetails? Caveat here is that I'm terrible at naming...)

I'd put Env under the metrics config section (i.e. where the old fields were removed)

Note that Env struct is identical in content to:
type RequestLogRevision struct from https://pkg.go.dev/knative.dev/serving/pkg/http#RequestLogRevision which is also being used later in main.go, How about naming it "Revision"

I am not sure it should be under
// Metrics configuration

I think "Env" is a good name, as it's describing the "environment" (i.e. Pod) where the queue-proxy is running.

A few bits (like ServingService) won't vary per queue-proxy, but this struct seems to be "where am I on the map" rather than "what am I doing".

davidhadas · 2022-07-19T15:02:40Z

/assign @davidhadas

evankanderson

It looks like this shouldn't collide with the internal encryption work, which is nice!

I'm going to let @psschwei / @dprotaso do the final approval, but this LGTM.

A few smaller comments / nits. I appreciate the goal of reducing code movement throughout the file, but I think there might be a bigger benefit of consistently using the state in Defaults, rather than mirroring it into/out of the struct at strategic times. (In particular if we added a second extension point in the future.)

evankanderson · 2022-07-19T16:38:37Z

pkg/queue/sharedmain/main.go

+	Env
+}
+
+type Env struct {
+	ServingNamespace     string `split_words:"true" required:"true"`
+	ServingRevision      string `split_words:"true" required:"true"`
+	ServingConfiguration string `split_words:"true" required:"true"`
+	ServingPodIP         string `split_words:"true" required:"true"`
+	ServingPod           string `split_words:"true" required:"true"`
+	ServingService       string `split_words:"true"` // optional


I think "Env" is a good name, as it's describing the "environment" (i.e. Pod) where the queue-proxy is running.

A few bits (like ServingService) won't vary per queue-proxy, but this struct seems to be "where am I on the map" rather than "what am I doing".

evankanderson · 2022-07-19T16:40:47Z

pkg/queue/sharedmain/main.go

+type Defaults struct {
+	Ctx       context.Context
+	Logger    *zap.SugaredLogger
+	Transport http.RoundTripper
+	Env       Env
+}


Hmm, I'm wondering if Main should be a method on Defaults.

I suspect not.

I'm also wondering about the name... Configuration, Process, Execution, QueueProxy?

This list includes two parts:

parameters whose value is determined by the service and provided for the extensions to use (but not to change)

parameters whose default is determined by the service and provided for the extensions such that may change them
Maybe we should have divided this struct into two:

type Defaults { Ctx context.Context Transport http.RoundTripper } type Config { Env Logger *zap.SugaredLogger } type QueueProxy struct { Invariables Config Mutables Defaults }

I wouldn't split, I'd just note in comments which ones are considered mutable and which ones aren't.

Since they're exported types, an overview comment on the Defaults and Env structs themselves wouldn't be a bad idea.

evankanderson · 2022-07-19T16:41:17Z

pkg/queue/sharedmain/main.go

+	d := &Defaults{
+		Ctx: signals.NewContext(),
+	}


I'd prefer to make d concrete here, rather than a pointer.

evankanderson · 2022-07-19T16:43:29Z

pkg/queue/sharedmain/main.go

+		return err
 	}

+	d.Env = env.Env


I can see that you're slicing the "where I'm at" out of the larger configuration. I'm wondering whether we should simply pass along all the config/privateEnv, and accept that plugins could (for example) adjust EnableProfiling without other high-level config.

This could enable splitting out envconfig into a pluggable module, though I'm not sure I'd recommend it, since I think we want that functionality enabled by default rather than every caller needing to say sharedmain.EnvConfig.

This ensures that extensions receive a copy of the Env and cannot change the Env used by the service which I think is appropriate here.

I think just having a read-only Env is ok for now - I think as people request more we can move properties from private => public with a discussion

evankanderson · 2022-07-19T16:45:16Z

pkg/queue/sharedmain/main.go

 		}.String()),
 		zap.String(logkey.Pod, env.ServingPod))

+	d.Logger = logger


Why not put this on 147? Reducing code movement?

I'd be inclined to s/logger/d.Logger/ throughout.

This ensures that extensions receive a copy of the logger and cannot change the logger used by the service which I think is appropriate here.

I'm ok either way.

Longer term I would expect the logger be pluggable in the future

func WithLogger(l *zap.SugaredLogger) func(q *Defaults) { return func(d *Defaults) { d.Logger = l return } }

evankanderson · 2022-07-19T16:59:25Z

pkg/queue/sharedmain/main.go

+	ctx := d.Ctx
+	transport := d.Transport


Again, I'd be inclined to substitute these throughout.

evankanderson · 2022-07-19T16:59:56Z

pkg/queue/sharedmain/main.go

+
+	// allow extensions to read d and return modified context and transport
+	for _, opts := range opts {
+		opts(d)


Slight preference for opts(&d) to make it clear that d is modified in-place here, but I don't care strongly.

pkg/queue/sharedmain/main.go

davidhadas · 2022-07-20T18:26:04Z

/retest

davidhadas · 2022-07-20T20:06:51Z

Tests fail, but it does not seem like a code issue
/retest

psschwei

In general this LGTM but want to give Dave a chance to comment before merging.

/assign @dprotaso

psschwei · 2022-07-21T16:32:15Z

pkg/queue/sharedmain/main.go

+type Defaults struct {
+	Ctx       context.Context
+	Logger    *zap.SugaredLogger
+	Transport http.RoundTripper
+	Env       Env
+}


I wouldn't split, I'd just note in comments which ones are considered mutable and which ones aren't.

Since they're exported types, an overview comment on the Defaults and Env structs themselves wouldn't be a bad idea.

psschwei · 2022-07-21T16:33:50Z

cmd/queue/main.go

+	if sharedmain.Main() != nil {
+		os.Exit(1)
+	}


we should also probably log the error here

Since we let downstream change this file, I think it is best if we log inside sharedmain to ensure consistent logging.

davidhadas · 2022-07-25T13:08:03Z

/retest

davidhadas · 2022-07-25T13:18:21Z

/retest

knative-prow · 2022-07-25T14:22:45Z

@davidhadas: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build-tests_serving_main
/test contour-latest_serving_main
/test contour-tls_serving_main
/test gateway-api-latest_serving_main
/test istio-latest-no-mesh-tls_serving_main
/test istio-latest-no-mesh_serving_main
/test kourier-stable-tls_serving_main
/test kourier-stable_serving_main
/test unit-tests_serving_main
/test upgrade-tests_serving_main

The following commands are available to trigger optional jobs:

/test https_serving_main
/test istio-latest-mesh-short_serving_main
/test istio-latest-mesh-tls_serving_main
/test istio-latest-mesh_serving_main
/test performance-tests-kperf_serving_main
/test performance-tests-mako_serving_main

Use /test all to run the following jobs that were automatically triggered:

build-tests_serving_main
istio-latest-no-mesh-tls_serving_main
istio-latest-no-mesh_serving_main
unit-tests_serving_main
upgrade-tests_serving_main

In response to this:

/test e2e / test (v1.22.9, istio, runtime) (pull_request)

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.

dprotaso · 2022-07-25T21:11:45Z

pkg/queue/sharedmain/main.go

+		return err
 	}

+	d.Env = env.Env


I think just having a read-only Env is ok for now - I think as people request more we can move properties from private => public with a discussion

pkg/queue/sharedmain/main.go

dprotaso · 2022-07-25T21:27:52Z

pkg/queue/sharedmain/main.go

 		}.String()),
 		zap.String(logkey.Pod, env.ServingPod))

+	d.Logger = logger


I'm ok either way.

Longer term I would expect the logger be pluggable in the future

func WithLogger(l *zap.SugaredLogger) func(q *Defaults) { return func(d *Defaults) { d.Logger = l return } }

dprotaso · 2022-07-25T21:57:15Z

/lgtm
/approve

knative-prow · 2022-07-25T21:57:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidhadas, dprotaso

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:

~~cmd/queue/OWNERS~~ [dprotaso]
~~pkg/queue/OWNERS~~ [dprotaso]

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

This builds on knative#13133 to make it possible to adjust settings on the *httputil.ReverseProxy that queue-proxy uses, or to replace it entirely with any http.Handler, using the out-of-tree extension pattern.

* Make queue-proxy's reverse proxy handler extendable This builds on #13133 to make it possible to adjust settings on the *httputil.ReverseProxy that queue-proxy uses, or to replace it entirely with any http.Handler, using the out-of-tree extension pattern. * Ensure configured Transport is used If an integrator customizes the Transport in an Options function, we want to apply the provided Transport to our default httputil.ReverseProxy regardless of which Option sets the Transport and regardless of whether or not other Option functions replace our default http.Handler. Add some tests for this since the interactions beteween Options.Transport and Options.ProxyHandler can be subtle.

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/autoscale area/networking area/test-and-release It flags unit/e2e/conformance/perf test issues for product features labels Jul 19, 2022

knative-prow bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 19, 2022

davidhadas mentioned this pull request Jul 19, 2022

[WIP] Adding extendibility to QP via QPExtension #13122

Closed

knative-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 19, 2022

psschwei reviewed Jul 19, 2022

View reviewed changes

knative-prow bot assigned davidhadas Jul 19, 2022

Removing script as to not delay the merge, can add it later if decided

c5a8030

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 19, 2022

evankanderson reviewed Jul 19, 2022

View reviewed changes

making Defaults concrete

bddc07e

knative-prow bot added the area/API API objects and controllers label Jul 20, 2022

davidhadas added 2 commits July 20, 2022 12:24

renamed back to config

e98327f

use d.Transport directly

dbcf283

psschwei reviewed Jul 21, 2022

View reviewed changes

knative-prow bot assigned dprotaso Jul 21, 2022

davidhadas mentioned this pull request Jul 24, 2022

A refactor of the queue volume constants #13146

Merged

an overview comment on the Defaults and Env

9a4d6d9

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 25, 2022

davidhadas added a commit to davidhadas/serving that referenced this pull request Jul 25, 2022

merged changes to QPOptions branch (pull request knative#13133)

4f6e8f1

dprotaso reviewed Jul 25, 2022

View reviewed changes

added printout in case of error during config

5137f03

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2022

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2022

knative-prow bot merged commit 63523f9 into knative:main Jul 25, 2022

mbaynton mentioned this pull request Sep 26, 2025

Make queue-proxy's reverse proxy handler extendable #16097

Merged

Enable refactoring queue-proxy binary with out-of-tree extensions #13133

Enable refactoring queue-proxy binary with out-of-tree extensions #13133

Uh oh!

Conversation

davidhadas commented Jul 19, 2022

Proposed Changes

Uh oh!

knative-prow bot commented Jul 19, 2022

Uh oh!

codecov bot commented Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

psschwei commented Jul 19, 2022

Uh oh!

psschwei commented Jul 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhadas Jul 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhadas commented Jul 19, 2022

Uh oh!

evankanderson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhadas Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidhadas commented Jul 20, 2022

Uh oh!

davidhadas commented Jul 20, 2022

Uh oh!

psschwei left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhadas Jul 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidhadas commented Jul 25, 2022

Uh oh!

davidhadas commented Jul 25, 2022

Uh oh!

codecov bot commented Jul 19, 2022 •

edited

Loading

davidhadas Jul 19, 2022 •

edited

Loading

davidhadas Jul 20, 2022 •

edited

Loading

davidhadas Jul 25, 2022 •

edited

Loading