Limit Apiserver Proxy Redirects #95128

riaankleinhans · 2020-09-28T19:53:20Z

What type of PR is this?*
/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #95129

Testgrid Link:

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?:

Yes

Release note:

kube-apiserver: requests to node, service, and pod `/proxy` subresources with no additional URL path now only automatically redirect GET and HEAD requests.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

NONE

/sig testing
/sig architecture
/networking

fedebongio · 2020-09-29T20:08:44Z

/assign @deads2k

liggitt · 2020-09-30T03:15:50Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

@@ -207,7 +208,7 @@ func (h *UpgradeAwareHandler) ServeHTTP(w http.ResponseWriter, req *http.Request
 	// Redirect requests with an empty path to a location that ends with a '/'
 	// This is essentially a hack for http://issue.k8s.io/4958.
 	// Note: Keep this code after tryUpgrade to not break that flow.
-	if len(loc.Path) == 0 {
+	if len(loc.Path) == 0 && (method == http.MethodGet || method == http.MethodHead) {


limiting the redirect to GET/HEAD requests looks good to me.

by continuing with the normal proxy path for other methods (e.g. PUT/POST), what is the HTTP request this change would end up passing to the underlying pod if len(loc.Path) was 0 here?

From my tests the other methods will pass through to the pod unaffected. Using kubectl proxy, porter and curl I can get 200 OK response back from the pod for the other http methods.

$ curl -I -X POST http://localhost:9999/api/v1/namespaces/default/pods/agnhost-7bb97c7fb7-6k9dq/proxy HTTP/1.1 200 OK Cache-Control: no-cache, private Content-Length: 3 Content-Type: text/plain; charset=utf-8 Date: Wed, 30 Sep 2020 18:43:36 GMT

Using this PR with an updated e2e test for NamespacedPodProxy endpoints gives

STEP: Creating a Pod Oct 1 01:17:33.579: INFO: pod created Oct 1 01:17:33.591: INFO: Pod Quantity: 1 Status: Pending Oct 1 01:17:34.596: INFO: Pod Quantity: 1 Status: Pending Oct 1 01:17:35.597: INFO: Pod Quantity: 1 Status: Pending Oct 1 01:17:36.596: INFO: Pod Status: Running Oct 1 01:17:36.596: INFO: Starting http.Client for https://127.0.0.1:41813/api/v1/namespaces/proxy-9664/pods/proxy-pod-target/proxy Oct 1 01:17:36.600: INFO: http.Client request:GET StatusCode:301 Oct 1 01:17:36.603: INFO: http.Client request:HEAD StatusCode:301 Oct 1 01:17:36.608: INFO: http.Client request:DELETE StatusCode:200 Oct 1 01:17:36.612: INFO: http.Client request:OPTIONS StatusCode:200 Oct 1 01:17:36.616: INFO: http.Client request:PATCH StatusCode:200 Oct 1 01:17:36.621: INFO: http.Client request:POST StatusCode:200 Oct 1 01:17:36.624: INFO: http.Client request:PUT StatusCode:200

what is the HTTP request this change would end up passing to the underlying pod if len(loc.Path) was 0 here?

I wanted to make sure the request sent to the underlying pod was a valid HTTP request (especially that a non-empty request-uri was sent to the backing pod)

https://www.w3.org/Protocols/HTTP/1.1/rfc2616bis/draft-lafon-rfc2616bis-03.html#request-uri

Note that the absolute path cannot be empty; if none is present in the original URI, it must be given as "/" (the server root).

Updated PR so 'absolute path cannot be empty' now.

Checked how the pod handles the request & payload sent back to the client.

Tested with kubectl proxy, curl, porter and tcpdump

10.244.0.1.42256 > 10.244.0.5.80: Flags [P.], cksum 0x16e4 (incorrect -> 0x7749), seq 211:419, ack 120, win 502, options [nop,nop,TS val 1754840149 ecr 1101893751], length 208: HTTP, length: 208 DELETE / HTTP/1.1 Host: localhost:9999 User-Agent: via-proxy Accept: */* Accept-Encoding: gzip X-Forwarded-For: 127.0.0.1, 172.18.0.1 X-Forwarded-Uri: /api/v1/namespaces/default/pods/agnhost2/proxy/ 02:58:21.755520 IP (tos 0x0, ttl 64, id 11616, offset 0, flags [DF], proto TCP (6), length 52) 10.244.0.5.80 > 10.244.0.1.42256: Flags [.], cksum 0x1614 (incorrect -> 0x25f4), ack 419, win 507, options [nop,nop,TS val 1101914757 ecr 1754840149], length 0 02:58:21.755774 IP (tos 0x0, ttl 64, id 11617, offset 0, flags [DF], proto TCP (6), length 171) 10.244.0.5.80 > 10.244.0.1.42256: Flags [P.], cksum 0x168b (incorrect -> 0x5f0d), seq 120:239, ack 419, win 507, options [nop,nop,TS val 1101914758 ecr 1754840149], length 119: HTTP, length: 119 HTTP/1.1 200 OK Date: Tue, 06 Oct 2020 02:58:21 GMT Content-Length: 3 Content-Type: text/plain; charset=utf-8 foo[!http]

Why would we limit to GET & HEAD? Why couldn't people be using POST PUT etc? Isn't this a compatibility breakage?

The request body associated with a PUT/POST/PATCH/etc request doesn't get sent to the backend, and the clients I know of drop their PUT/POST/PATCH content in response to a 301 request, and switch to making a GET request to the new location (including golang and browsers, which is what this redirect was originally added for)

That makes returning a 301 redirect for anything other than GET/HEAD lose data and not be usable in the scenarios I can see

liggitt · 2020-10-06T13:18:33Z

staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

+			return
+		default:
+			path := req.URL.Path
+			req.URL.Path = path + "/" + queryPart


this is doing a few things that don't seem quite right:

it is adding duplicating query data into the path (I think a request with query parameters would get an incorrect path, need to add a unit test to exercise this)

it is modifying req.URL.Path but not loc.Path (which means the newReq.URL = &loc code path below is still wrong... need a unit test to exercise that path as well)

My question in #95128 (comment) was more of a question about behavior ... I'm not actually sure we need to append a / ... the go http client might default to sending / if there is not path present... if this default case is simply dropped, does the pod still get a $METHOD / HTTP/1.1 call?

fejta-bot · 2021-01-04T13:28:49Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

riaankleinhans · 2021-01-06T20:36:36Z

/remove-lifecycle stale

heyste · 2021-08-19T22:43:34Z

/test pull-kubernetes-node-e2e-containerd
unrelated flake E2eNode Suite: [sig-storage] HostPath should support r/w [NodeConformance]

heyste · 2021-08-19T23:59:28Z

After retesting the original fix with kind, kubectl proxy and a custom porter, the following output shows that no empty request is sent to the pod (i.e a missing slash).

$ curl -XPOST http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy && echo
{"Method":"POST","RequestURI":"/","Body":"foo"}
 
$ curl -XPOST http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy?id=123 && echo
{"Method":"POST","RequestURI":"/?id=123","Body":"foo"}

$ curl -XDELETE http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy && echo
{"Method":"DELETE","RequestURI":"/","Body":"foo"}

$ curl -XDELETE http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy?id=123 && echo
{"Method":"DELETE","RequestURI":"/?id=123","Body":"foo"}

Checking APISnoop results

#+begin_src sql-mode :eval never-export :exports both :session none
select distinct  endpoint, right(useragent,83) AS useragent
from testing.audit_event
where endpoint ilike '%proxy%'
order by endpoint
limit 10;
#+end_src

#+RESULTS:
#+begin_SRC example
               endpoint                |  useragent
---------------------------------------+-------------
 connectCoreV1DeleteNamespacedPodProxy | curl/7.68.0
 connectCoreV1PostNamespacedPodProxy   | curl/7.68.0
(2 rows)

#+end_SRC

@liggitt Can you please confirm that the logs above do address your outstanding query and the PR can now be lgtm & approved? TIA

liggitt · 2021-08-24T12:38:54Z

thanks for checking the impact on backend behavior, the change lgtm

unit test coverage of client side behavior (redirect received for get/head, no redirect received for put/post/delete/etc) and backend behavior (request with / received for all assuming the client followed redirects) would ensure the behavior you manually verified doesn't regress in the future

smarterclayton · 2021-09-02T17:47:38Z

If jordan's question is answered to his satisfaction this is approved and also lgtm, will tag as such unless he comments otherwise in next day.

/approve

liggitt · 2021-09-02T17:51:21Z

the change lgtm but without test coverage nothing would catch a regression

heyste · 2021-09-21T23:44:49Z

@liggitt For the new unit test to check the status code (either 301/200) for various methods I'm having an issue getting passed the following code block

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

Lines 210 to 212 in 92ddd4d

    
           if !strings.HasSuffix(loc.Path, "/") && strings.HasSuffix(req.URL.Path, "/") { 
        
           	loc.Path += "/" 
        
           }

Using the current TestServeHTTP scenario with a requestPath: "" it will get replaced with a / which means it will then skips the next check

kubernetes/staging/src/k8s.io/apimachinery/pkg/util/proxy/upgradeaware.go

Line 218 in 92ddd4d

if len(loc.Path) == 0 {

Question: Based on the output below does that get processed somewhere inside of pkg/registry/core/pod/rest/subresources.go or have I missed something in the process of getting from curl to the pod ?

curl -I http://localhost:12345/api/v1/namespaces/default/pods/agnhost2-5b47dc9979-tfdj5/proxy
HTTP/1.1 301 Moved Permanently
Audit-Id: 42beb19b-50e8-4491-a7de-9ed5c9d6a842
Cache-Control: no-cache, private
Date: Tue, 21 Sep 2021 23:33:39 GMT
Location: /api/v1/namespaces/default/pods/agnhost2-5b47dc9979-tfdj5/proxy/

TIA for any tips you can provide.

liggitt · 2021-09-28T15:05:49Z

Using the current TestServeHTTP scenario with a requestPath: "" it will get replaced with a /

You can't send a completely empty path to a server. If the test server roots the proxy handler at /, I don't think there's a way to entirely omit a path in a request to it.

In real use, the proxy handler is rooted at /api/v1/namespaces/$ns/pods/$pod/proxy, so a request to ``/api/v1/namespaces/$ns/pods/$pod/proxy(no trailing slash) would be handled by the proxy handler, would have the/api/v1/namespaces/$ns/pods/$pod/proxy` prefix removed, and would end up with an empty path with no trailing slash it was expected to proxy through.

heyste · 2021-10-06T03:24:07Z

/test pull-kubernetes-node-e2e-containerd
unrelated flake

W1006 03:17:04.446] Could not detect Kubernetes master node.  Make sure you've launched a cluster with 'kube-up.sh'
I1006 03:17:04.546] Master not detected. Is the cluster up?

- Extract the current redirect code into a function (proxyRedirectsforRootPath) and redirect for only http.MethodGet or http.MethodHead - Create a unit test that confirms that only GET & HEAD methods are redirected

heyste · 2021-10-06T22:26:28Z

@liggitt I've moved the redirect code into a function so I have a more direct way of unit testing the various methods. The unit test checks GET, PUT, HEAD and DELETE. Manual testing the latest changes with porter still works as shown below.

$ curl -I -XGET http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy
HTTP/1.1 301 Moved Permanently
Audit-Id: 6eee31cd-67b1-43d6-adc5-3f1f9015f953
Cache-Control: no-cache, private
Content-Length: 0
Date: Wed, 06 Oct 2021 18:56:02 GMT
Location: /api/v1/namespaces/default/pods/debug-proxy/proxy/

$ curl  -XDELETE http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy
{"Method":"DELETE","RequestURI":"/","Body":"foo"}
$ curl  -XPUT http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy
{"Method":"PUT","RequestURI":"/","Body":"foo"}
$ curl  -XPOST http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy
{"Method":"POST","RequestURI":"/","Body":"foo"}
$ curl  -XPATCH http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy && echo
{"Method":"PATCH","RequestURI":"/","Body":"foo"}
$ curl  -XOPTIONS http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy
{"Method":"OPTIONS","RequestURI":"/","Body":"foo"}

$ curl -I -XHEAD http://127.0.0.1:11111/api/v1/namespaces/default/pods/debug-proxy/proxy
HTTP/1.1 301 Moved Permanently
Audit-Id: f6302f94-2d8b-4126-b232-bc15619ce385
Cache-Control: no-cache, private
Date: Wed, 06 Oct 2021 18:57:42 GMT
Location: /api/v1/namespaces/default/pods/debug-proxy/proxy/

Is this okay to lgtm now?

liggitt · 2021-10-11T18:29:32Z

/lgtm
/approve

k8s-ci-robot · 2021-10-11T18:29:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, Riaankl, smarterclayton

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:

~~staging/src/k8s.io/apimachinery/pkg/OWNERS~~ [liggitt,smarterclayton]

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

riaankleinhans · 2021-10-11T20:12:30Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

Unreleated flake

[Fail] [sig-api-machinery] CustomResourcePublishOpenAPI [Privileged:ClusterAdmin] [It] works for CRD with validation schema [Conformance] 
I1011 19:51:16.925] /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/github.com/onsi/ginkgo/internal/leafnodes/runner.go:113

riaankleinhans · 2021-10-11T21:12:51Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

Unrelated flake

 [Fail] [sig-node] PreStop [It] graceful pod terminated should wait until preStop hook completes the process 
I1011 21:01:43.621] /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/node/pre_stop.go:213

heyste · 2021-10-11T23:22:52Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd
unrelated flake

Kubernetes  e2e suite: [sig-storage] In-tree Volumes [Driver:  local][LocalVolumeType: dir] [Testpattern: Pre-provisioned PV (default  fs)] subPath should support existing directory

/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/storage/testsuites/subpath.go:205
 Oct 11 22:48:06.345: Unexpected error:     <*errors.errorString \| 0xc00237b2a0>: {
         s: "failed to get logs from pod-subpath-test-preprovisionedpv-bsgb for test-container-volume-preprovisionedpv-bsgb: an error on the server (\"unknown\") has prevented the request from succeeding (get pods pod-subpath-test-preprovisionedpv-bsgb)",
     }
     failed to get logs from pod-subpath-test-preprovisionedpv-bsgb for test-container-volume-preprovisionedpv-bsgb: an error on the server ("unknown") has prevented the request from succeeding (get pods pod-subpath-test-preprovisionedpv-bsgb)
 occurred
 /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/test/e2e/framework/util.go:743

heyste · 2021-10-11T23:25:17Z

/test pull-kubernetes-e2e-gce-100-performance
unrelated flake e2e.go:Up

scp: /var/log/cluster-autoscaler.log*: No such file or directory
scp: /var/log/fluentd.log*: No such file or directory
scp: /var/log/kubelet.cov*: No such file or directory
scp: /var/log/cl2-**: No such file or directory
scp: /var/log/startupscript.log*: No such file or directory
ERROR: (gcloud.compute.scp) [/usr/bin/scp] exited with return code [1].

k8s-ci-robot requested review from derekwaynecarr and smarterclayton September 28, 2020 19:54

riaankleinhans changed the title ~~Extend Agnhost Image: Support Conformance testing of PodProxy Endpoints~~ Limit Apiserver Proxy Redirects Sep 28, 2020

k8s-ci-robot assigned deads2k Sep 29, 2020

riaankleinhans mentioned this pull request Sep 29, 2020

Update SnoopDB Readme.md with user and developer instructions kubernetes-sigs/apisnoop#391

Closed

liggitt reviewed Sep 30, 2020

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 6, 2020

liggitt reviewed Oct 6, 2020

View reviewed changes

hh mentioned this pull request Oct 8, 2020

Override audit policy for conformance informing jobs kubernetes/test-infra#19502

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2021

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 6, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 2, 2021

heyste force-pushed the remove-unwanted-redirects branch from 5787c1e to 0dcf9cf Compare October 6, 2021 02:56

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 6, 2021

Redirect proxy requests for only GET & HEAD methods

be65bc3

- Extract the current redirect code into a function (proxyRedirectsforRootPath) and redirect for only http.MethodGet or http.MethodHead - Create a unit test that confirms that only GET & HEAD methods are redirected

heyste force-pushed the remove-unwanted-redirects branch from 0dcf9cf to be65bc3 Compare October 6, 2021 07:57

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 11, 2021

k8s-ci-robot assigned liggitt Oct 11, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 11, 2021

k8s-ci-robot merged commit d2f6eb6 into kubernetes:master Oct 12, 2021

k8s-ci-robot added this to the v1.23 milestone Oct 12, 2021

riaankleinhans mentioned this pull request Dec 5, 2021

HEAD & OPTIONS http verbs don’t seem to show up in the apiserver audit logs when we hit proxy endpoints. #95966

Closed

liggitt mentioned this pull request Sep 11, 2023

Stream Translator Proxy and FallbackExecutor for WebSockets #119186

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limit Apiserver Proxy Redirects #95128

Limit Apiserver Proxy Redirects #95128

riaankleinhans commented Sep 28, 2020 •

edited by liggitt

Loading

fedebongio commented Sep 29, 2020

liggitt Sep 30, 2020

heyste Sep 30, 2020

heyste Oct 1, 2020

liggitt Oct 1, 2020

heyste Oct 6, 2020

lavalamp Oct 1, 2021

liggitt Oct 1, 2021

liggitt Oct 6, 2020

fejta-bot commented Jan 4, 2021

riaankleinhans commented Jan 6, 2021

heyste commented Aug 19, 2021

heyste commented Aug 19, 2021

liggitt commented Aug 24, 2021

smarterclayton commented Sep 2, 2021

liggitt commented Sep 2, 2021

heyste commented Sep 21, 2021

liggitt commented Sep 28, 2021

heyste commented Oct 6, 2021

heyste commented Oct 6, 2021

liggitt commented Oct 11, 2021

k8s-ci-robot commented Oct 11, 2021

riaankleinhans commented Oct 11, 2021

riaankleinhans commented Oct 11, 2021

heyste commented Oct 11, 2021

heyste commented Oct 11, 2021

Limit Apiserver Proxy Redirects #95128

Limit Apiserver Proxy Redirects #95128

Conversation

riaankleinhans commented Sep 28, 2020 • edited by liggitt Loading

fedebongio commented Sep 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Jan 4, 2021

riaankleinhans commented Jan 6, 2021

heyste commented Aug 19, 2021

heyste commented Aug 19, 2021

liggitt commented Aug 24, 2021

smarterclayton commented Sep 2, 2021

liggitt commented Sep 2, 2021

heyste commented Sep 21, 2021

liggitt commented Sep 28, 2021

heyste commented Oct 6, 2021

heyste commented Oct 6, 2021

liggitt commented Oct 11, 2021

k8s-ci-robot commented Oct 11, 2021

riaankleinhans commented Oct 11, 2021

riaankleinhans commented Oct 11, 2021

heyste commented Oct 11, 2021

heyste commented Oct 11, 2021

riaankleinhans commented Sep 28, 2020 •

edited by liggitt

Loading