Activator Header Fixes #2047

dprotaso · 2018-09-18T00:48:31Z

Fixes #2046

Activator now reads the last header value for Revision Name, Revision Namespace and Configuration Name. This prevents external clients (outside the mesh) from activating the wrong revision
Activator and Queue Proxy now remove the activator headers when making requests. This prevents inadvertent Knative apps & functions from forwarding these headers

dprotaso · 2018-09-18T00:49:42Z

Holding for verification

/hold

dprotaso · 2018-09-18T01:15:46Z

/test pull-knative-serving-integration-tests

…

On Mon, Sep 17, 2018 at 21:14 Knative Prow Robot ***@***.***> wrote: @dprotaso <https://github.com/dprotaso>: The following test *failed*, say /retest to rerun them all: Test name Commit Details Rerun command pull-knative-serving-integration-tests 55d5545 <55d5545> link <https://gubernator.knative.dev/build/knative-prow/pr-logs/pull/knative_serving/2047/pull-knative-serving-integration-tests/1041851527704088578/> /test pull-knative-serving-integration-tests Full PR test history <https://gubernator.knative.dev/pr/knative_serving/2047>. Your PR dashboard <https://gubernator.knative.dev/pr/dprotaso>. Instructions for interacting with me using PR comments are available here <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://go.k8s.io/bot-commands>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2047 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIgjrNRBkMx4m6XvGLfzXBgGk--O6xks5ucEjzgaJpZM4Ws-Tp> .

markusthoemmes

I'm not sure this actually fixes #2046 since that issue is about confusion inside of the activator if an external entity sets these headers. This fixes the case where a service would forward the request as-is (and thus act as an external entity setting these headers), correct?

markusthoemmes · 2018-09-18T08:11:10Z

pkg/activator/handler/handler.go

+
+		for _, h := range headersToRemove {
+			if r.Header.Get(h) != "" {
+				r.Header.Del(h)


I think you can omit the surrounding if since Del will delegate to delete which is a noop if nothing is there to delete.

markusthoemmes · 2018-09-18T08:13:52Z

pkg/activator/handler/handler.go

+	}
+
+	orig := p.Director
+	p.Director = func(r *http.Request) {


For my own understanding: Is it not possible to just drop the headers from the incoming request? Is it done this way to keep the incoming request object intact so other methods can happily use the headers just like today?

markusthoemmes · 2018-09-18T08:14:30Z

pkg/activator/handler/handler.go

+		activator.ConfigurationHeader,
+		activator.RevisionHeaderName,
+		activator.RevisionHeaderNamespace,
+	}


Should this be a global constant/variable to avoid reallocations?

dprotaso · 2018-09-18T13:05:21Z

I'm not sure this actually fixes #2046 since that issue is about confusion inside of the activator if an external entity sets these headers.

It's not the external entity (ie. outside the service mesh) setting the header - it's the virtual service.

I would say that's another related issue. Ideally we need the ingress to prune these header when they come from outside the mesh.

This fixes the case where a service would forward the request as-is (and thus act as an external entity setting these headers), correct?

Actually - this requires changes to the queue proxy. I'll update the PR to include that

dprotaso · 2018-09-18T15:18:20Z

/assign @markusthoemmes
/assign @josephburnett

markusthoemmes

Implementation is LGTM, got some nits in the comments.

markusthoemmes · 2018-09-18T15:54:06Z

pkg/activator/util/header.go

+	activator.RevisionHeaderNamespace,
+}
+
+func SetupHeaderPruning(p *httputil.ReverseProxy) {


Please add a comment.

markusthoemmes · 2018-09-18T15:54:11Z

pkg/activator/util/header.go

+	}
+}
+
+func GetLastHeaderValue(r *http.Request, key string) string {


Please add a comment.

Should we narrow the interface to pass in r.Headers vs. passing in the whole request?

I was debating about that - it's obvious from the test that it should receive http.Header - i just chose to ignore it.

I'll make the change

pkg/activator/util/header.go

scothis · 2018-09-18T16:01:56Z

Testing with this PR, the chain of functions that failed previously will now scale from zero as expected.

markusthoemmes

/lgtm

Nice one, great tests! 🎉

dprotaso · 2018-09-18T17:16:57Z

@markusthoemmes sorry made a minor style change

http.Header{
	headerKey: []string{"first", "second"},
},

to

http.Header{
	headerKey: {"first", "second"},
},

dprotaso · 2018-09-18T17:17:25Z

/hold cancel

markusthoemmes · 2018-09-18T17:17:27Z

/lgtm

dprotaso · 2018-09-18T17:38:12Z

/test pull-knative-serving-integration-tests

dprotaso · 2018-09-18T18:04:57Z

/test pull-knative-serving-integration-tests

dprotaso · 2018-09-18T18:59:36Z

/test pull-knative-serving-integration-tests

josephburnett

/lgtm
/approve

josephburnett · 2018-09-18T22:37:46Z

pkg/activator/util/header.go

+}
+
+// SetupHeaderPruning will cause the http.ReverseProxy
+// to no forward activator headers


Nit: ... to no[t] forward ...

josephburnett · 2018-09-18T22:42:29Z

/assign @mattmoor

googlebot · 2018-09-18T22:47:13Z

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.

dprotaso · 2018-09-18T22:47:56Z

/consent

…

On Tue, Sep 18, 2018 at 18:47 Knative Prow Robot ***@***.***> wrote: [APPROVALNOTIFIER] This PR is *NOT APPROVED* This pull-request has been approved by: *dprotaso <#2047#>*, *josephburnett <#2047 (review)>* To fully approve this pull request, please assign additional approvers. We suggest the following additional approver: *mattmoor* If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready. The full list of commands accepted by this bot can be found here <https://go.k8s.io/bot-commands>. The pull request process is described here <https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process> Needs approval from an approver in each of these files: - *OWNERS <https://github.com/knative/serving/blob/master/OWNERS>* Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2047 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABIgp9S4w2aABbV7bE3EJsPe47FCbDMks5ucXfzgaJpZM4Ws-Tp> .

josephburnett · 2018-09-18T22:49:55Z

/consent
/please?

mattmoor · 2018-09-18T22:50:46Z

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state.

We need an admin to merge this. I will talk to @isdal

googlebot · 2018-09-18T22:51:30Z

CLAs look good, thanks!

knative-metrics-robot · 2018-09-18T22:53:29Z

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/activator/util/header.go	Do not exist	100.0%
pkg/http/header.go	Do not exist	100.0%

mattmoor · 2018-09-18T22:56:24Z

/lgtm
/approve

knative-prow-robot · 2018-09-18T22:56:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, josephburnett, mattmoor

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~~ [mattmoor]

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

knative-prow-robot requested review from josephburnett and mdemirhan September 18, 2018 00:48

knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 18, 2018

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2018

markusthoemmes reviewed Sep 18, 2018

View reviewed changes

dprotaso force-pushed the activator-header-fix branch from 55d5545 to 871d657 Compare September 18, 2018 15:13

knative-prow-robot 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 Sep 18, 2018

dprotaso changed the title ~~Remove Istio headers in outbound activator requests~~ Activator Header Fixes Sep 18, 2018

knative-prow-robot assigned josephburnett and markusthoemmes Sep 18, 2018

dprotaso force-pushed the activator-header-fix branch from 871d657 to 6f02de4 Compare September 18, 2018 15:30

markusthoemmes approved these changes Sep 18, 2018

View reviewed changes

dprotaso force-pushed the activator-header-fix branch from 6f02de4 to 674b7e1 Compare September 18, 2018 17:08

markusthoemmes approved these changes Sep 18, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

dprotaso added 2 commits September 18, 2018 13:14

Activator now looks at the last value for the activator headers

ba529ab

Activator and queue proxy no longer forward activator headers

ef109d6

dprotaso force-pushed the activator-header-fix branch from 674b7e1 to ef109d6 Compare September 18, 2018 17:14

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 18, 2018

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

josephburnett approved these changes Sep 18, 2018

View reviewed changes

knative-prow-robot assigned mattmoor Sep 18, 2018

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

fix typo

1d6f042

dprotaso force-pushed the activator-header-fix branch from c913ba3 to 1d6f042 Compare September 18, 2018 22:51

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2018

knative-prow-robot merged commit 4155662 into knative:master Sep 18, 2018

dprotaso deleted the activator-header-fix branch September 19, 2018 00:34

josephburnett mentioned this pull request Nov 7, 2018

Add markusthoemmes as autoscaling-approver #2425

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Activator Header Fixes #2047

Activator Header Fixes #2047

dprotaso commented Sep 18, 2018 •

edited

Loading

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018 via email

markusthoemmes left a comment

markusthoemmes Sep 18, 2018

markusthoemmes Sep 18, 2018

markusthoemmes Sep 18, 2018

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

markusthoemmes left a comment

markusthoemmes Sep 18, 2018

markusthoemmes Sep 18, 2018

dprotaso Sep 18, 2018

scothis commented Sep 18, 2018

markusthoemmes left a comment

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

markusthoemmes commented Sep 18, 2018

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

josephburnett left a comment

josephburnett Sep 18, 2018

josephburnett commented Sep 18, 2018

googlebot commented Sep 18, 2018

dprotaso commented Sep 18, 2018 via email

josephburnett commented Sep 18, 2018

mattmoor commented Sep 18, 2018

googlebot commented Sep 18, 2018

knative-metrics-robot commented Sep 18, 2018

mattmoor commented Sep 18, 2018

knative-prow-robot commented Sep 18, 2018

Activator Header Fixes #2047

Activator Header Fixes #2047

Conversation

dprotaso commented Sep 18, 2018 • edited Loading

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018 via email

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes Sep 18, 2018

Choose a reason for hiding this comment

markusthoemmes Sep 18, 2018

Choose a reason for hiding this comment

markusthoemmes Sep 18, 2018

Choose a reason for hiding this comment

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes Sep 18, 2018

Choose a reason for hiding this comment

markusthoemmes Sep 18, 2018

Choose a reason for hiding this comment

dprotaso Sep 18, 2018

Choose a reason for hiding this comment

scothis commented Sep 18, 2018

markusthoemmes left a comment

Choose a reason for hiding this comment

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

markusthoemmes commented Sep 18, 2018

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

dprotaso commented Sep 18, 2018

josephburnett left a comment

Choose a reason for hiding this comment

josephburnett Sep 18, 2018

Choose a reason for hiding this comment

josephburnett commented Sep 18, 2018

googlebot commented Sep 18, 2018

dprotaso commented Sep 18, 2018 via email

josephburnett commented Sep 18, 2018

mattmoor commented Sep 18, 2018

googlebot commented Sep 18, 2018

knative-metrics-robot commented Sep 18, 2018

mattmoor commented Sep 18, 2018

knative-prow-robot commented Sep 18, 2018

dprotaso commented Sep 18, 2018 •

edited

Loading