proxy e2e test improvements #10070

lavalamp · 2015-06-18T23:57:04Z

This improves the proxy test to make multiple attempts. It also adds a URL rewriting check, which should verify that the correct stack of transports is being used.

It also fixes kubectl proxy to allow traffic to the ui again.

k8s-bot · 2015-06-19T00:10:38Z

GCE e2e build/test failed for commit 748438b0acb88804129b84cdf14b9ad72165a86c.

k8s-bot · 2015-06-19T00:48:12Z

GCE e2e build/test failed for commit e9ff9f277989e71c127e59e2cc222049f16faacd.

satnam6502 · 2015-06-19T00:49:09Z

@k8s-bot ok to test

satnam6502 · 2015-06-19T00:50:42Z

@lavalamp why does the e2e fail?

k8s-bot · 2015-06-19T00:52:12Z

GCE e2e build/test failed for commit e9ff9f277989e71c127e59e2cc222049f16faacd.

lavalamp · 2015-06-19T00:56:32Z

e2e fails because this is broken :)

Let's see if it's fixed now.

lavalamp · 2015-06-19T01:08:22Z

OK, I think the test is diagnosing our system as being wrong. We probably shouldn't submit until I get a fix.

k8s-bot · 2015-06-19T01:12:41Z

GCE e2e build/test failed for commit 33a7e5a445fc609f58ccfbb9ab1d4f1bffd59035.

k8s-bot · 2015-06-19T01:31:28Z

GCE e2e build/test failed for commit 55152797448666ff745fdddad898c28335c6e1d5.

lavalamp · 2015-06-19T23:36:24Z

Sorry I did not get a chance to update this. I still think it's the system that's broken, not the test.

k8s-bot · 2015-06-23T01:43:32Z

GCE e2e build/test failed for commit a3ebdfda4e25c89fb597251446abbbfc95ba0014.

lavalamp · 2015-06-23T01:58:32Z

@stephenR In the "Make UI work through kubectl proxy (again)" commit, I'm allowing traffic to more paths in the apiserver. Do you see any security issue with this?

k8s-bot · 2015-06-23T01:59:31Z

GCE e2e build/test failed for commit 773a754efaa245b5743084552e51f7c0586faded.

davidopp · 2015-06-23T16:55:10Z

@lavalamp Is this ready to review? If so, @satnam6502 can you review this today, since it's for a v1.0 issue?

satnam6502 · 2015-06-23T17:16:06Z

contrib/for-tests/porter/porter.go

@@ -33,7 +33,7 @@ const prefix = "SERVE_PORT_"

 func main() {
 	for _, vk := range os.Environ() {
-		parts := strings.Split(vk, "=")
+		parts := strings.SplitN(vk, "=", 2)


I find it useful to have a comment or example for the use of things like SplitN to help others understand what is going on.

k8s-bot · 2015-06-23T19:29:47Z

GCE e2e build/test failed for commit 859b7d2d482dea34f444609d796cfc93cb3605df.

k8s-bot · 2015-06-23T19:32:48Z

GCE e2e build/test failed for commit 56528a59d8eff40bf06384816cd91178ce8e808e.

satnam6502 · 2015-06-24T00:25:26Z

Are you ready for me to PTAL?

stephenR · 2015-06-24T11:25:52Z

@stephenR In the "Make UI work through kubectl proxy (again)" commit, I'm allowing traffic to more paths in the apiserver. Do you see any security issue with this?

I don't have a good overview of what paths become available through this, but since POST requests are blocked anyway, this should be fine. An issue would be if there are any state changing endpoints in the new paths that can be triggered via a GET request, similar to the /api/*/{exec,run} handlers.

lavalamp · 2015-06-24T18:41:39Z

@stephenR The UI is read only at the moment. Here's a list of top level paths. IMO we shouldn't have ANY state changing GET operations--that is like http 101--and we need to change the ones you mentioned.

{
  "paths": [
    "/api",
    "/api/v1",
    "/api/v1beta3",
    "/healthz",
    "/healthz/ping",
    "/logs/",
    "/metrics",
    "/static/",
    "/swagger-ui/",
    "/swaggerapi/",
    "/ui/",
    "/version"
  ]
}

But really I don't understand this mechanism of hiding mutating paths; I think people will just run without this filter because typically you're running this proxy because you want to do something.

lavalamp · 2015-06-24T20:19:04Z

OK, final change-- I think this should make e2e pass.

k8s-bot · 2015-06-24T20:37:38Z

GCE e2e build/test failed for commit e90bcff0ca20601cd0415d7f82bd2420ae08cfff.

Fixes kubernetes#10115.

lavalamp · 2015-06-24T20:54:52Z

OK. Final push. Should be ready to go now. PTAL

k8s-bot · 2015-06-24T21:15:22Z

GCE e2e build/test passed for commit 5eb5b4a.

lavalamp · 2015-06-24T21:32:15Z

YAY IT PASSED

lavalamp · 2015-06-24T21:32:59Z

@quinton-hoole @brendandburns I can haz LGTMs/ok-to-merge?

satnam6502 · 2015-06-24T21:32:59Z

LGTM

davidopp · 2015-06-24T22:04:07Z

I can take a look.

davidopp · 2015-06-24T22:19:16Z

LGTM

davidopp · 2015-06-24T22:20:05Z

I guess technically it still needs an LGTM from @brendandburns or @quinton-hoole

ghost · 2015-06-24T22:30:50Z

I can take a look now.

ghost · 2015-06-24T22:43:27Z

High level comment. This really is a lot of code to add so late before v1.0. Do we really need all of this right now? @lavalamp ?

ghost · 2015-06-24T22:44:36Z

PS: Can someone with more context help to decide whether the milestone for this is

v1.0-post
v1.0-candidate
v1.0

?

lavalamp · 2015-06-24T23:01:15Z

@quinton-hoole a large majority of this is test code. The non-test code changes are either fixing bugs or adding logging.

lavalamp · 2015-06-24T23:12:18Z

The test code here, had it been in, would have saved us grief in GKE recently.

stephenR · 2015-06-25T09:37:42Z

But really I don't understand this mechanism of hiding mutating paths; I think people will just run without this filter because typically you're running this proxy because you want to do something.

I agree, I think this is just an intermediate solution and in the end we'll have to think of a good way to do proper xsrf protection.

ghost · 2015-06-25T19:19:30Z

OK, I won't bother doing a full forth code review after @davidopp @satnam6502 and @stephenR . Based on what I see it's relatively low risk, and high value. ok-to-merge

proxy e2e test improvements

googlebot added the cla: yes label Jun 18, 2015

satnam6502 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2015

satnam6502 self-assigned this Jun 19, 2015

lavalamp force-pushed the e2eProxyFix branch from e9ff9f2 to 33a7e5a Compare June 19, 2015 00:55

lavalamp removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2015

lavalamp force-pushed the e2eProxyFix branch from 33a7e5a to 5515279 Compare June 19, 2015 01:14

lavalamp force-pushed the e2eProxyFix branch from 5515279 to a3ebdfd Compare June 23, 2015 01:42

lavalamp mentioned this pull request Jun 23, 2015

Authentication error running kubectl proxy #10115

Closed

satnam6502 reviewed Jun 23, 2015
View reviewed changes

lavalamp added 4 commits June 24, 2015 13:54

proxy e2e test improvements

32114f6

Fix proxy rewriting

ddbe4c9

Make UI work through kubectl proxy (again)

689a3ee

Fixes kubernetes#10115.

Test kubectl proxy in test-cmd.sh

5eb5b4a

lavalamp force-pushed the e2eProxyFix branch from e90bcff to 5eb5b4a Compare June 24, 2015 20:54

davidopp added ok-to-merge and removed ok-to-merge labels Jun 24, 2015

ghost added the ok-to-merge label Jun 25, 2015

mbforbes added a commit that referenced this pull request Jun 25, 2015

Merge pull request #10070 from lavalamp/e2eProxyFix

5e748c1

proxy e2e test improvements

mbforbes merged commit 5e748c1 into kubernetes:master Jun 25, 2015

lavalamp deleted the e2eProxyFix branch September 1, 2020 22:57

proxy e2e test improvements #10070

proxy e2e test improvements #10070

Conversation

lavalamp commented Jun 18, 2015

k8s-bot commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

satnam6502 commented Jun 19, 2015

satnam6502 commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

lavalamp commented Jun 19, 2015

lavalamp commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

k8s-bot commented Jun 19, 2015

lavalamp commented Jun 19, 2015

k8s-bot commented Jun 23, 2015

lavalamp commented Jun 23, 2015

k8s-bot commented Jun 23, 2015

davidopp commented Jun 23, 2015

satnam6502 Jun 23, 2015

Choose a reason for hiding this comment

k8s-bot commented Jun 23, 2015

k8s-bot commented Jun 23, 2015

satnam6502 commented Jun 24, 2015

stephenR commented Jun 24, 2015

lavalamp commented Jun 24, 2015

lavalamp commented Jun 24, 2015

k8s-bot commented Jun 24, 2015

lavalamp commented Jun 24, 2015

k8s-bot commented Jun 24, 2015

lavalamp commented Jun 24, 2015

lavalamp commented Jun 24, 2015

satnam6502 commented Jun 24, 2015

davidopp commented Jun 24, 2015

davidopp commented Jun 24, 2015

davidopp commented Jun 24, 2015

ghost commented Jun 24, 2015

ghost commented Jun 24, 2015

ghost commented Jun 24, 2015

lavalamp commented Jun 24, 2015

lavalamp commented Jun 24, 2015

stephenR commented Jun 25, 2015

ghost commented Jun 25, 2015