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

Add redirect support to SpdyRoundTripper #44451

Merged
merged 2 commits into from
Apr 26, 2017

Conversation

ncdc
Copy link
Member

@ncdc ncdc commented Apr 13, 2017

Add support for following redirects to the SpdyRoundTripper. This is
necessary for clients using it directly (e.g. the apiserver talking
directly to the kubelet) because the CRI streaming server issues a
redirect for streaming requests.

We need this in OpenShift because we have code that executes inside our apiserver that talks directly to the node to perform an attach request, and we need to be able to follow that redirect.

This code was adapted from the upgrade-aware proxy handler.

cc @smarterclayton @sttts @liggitt @timstclair @kubernetes/sig-api-machinery-pr-reviews

@ncdc ncdc added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 13, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Apr 13, 2017
@ncdc
Copy link
Member Author

ncdc commented Apr 13, 2017

I seem to have broken TestServePortForward and am looking into it

@ncdc ncdc force-pushed the spdy-follow-redirects branch 2 times, most recently from d157bb4 to 23d8754 Compare April 13, 2017 16:35
@@ -1552,11 +1552,20 @@ func TestServePortForward(t *testing.T) {
url = fmt.Sprintf("%s/portForward/%s/%s", fw.testHTTPServer.URL, podNamespace, podName)
}

upgradeRoundTripper := spdy.NewRoundTripper(nil)
c := &http.Client{Transport: upgradeRoundTripper}
// Don't follow redirects, since we want to inspect the redirect response.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this case was odd... it seems like it was explicitly testing a scenario the spdy round tripper wouldn't handle, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but I think it was testing to make sure that the kubelet server could return a redirect. My change here made it start breaking, because the round tripper started following the redirect, so I updated this test case to match what's done in textExecAttach.

method = "GET"
}

r, err := http.NewRequest(method, location.String(), req.Body)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remind me again why reusing the original req.Body doesn't do bad things if we hit this loop more than once?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request body is never used for spdy requests, but otherwise, we'd probably need to be more careful. This appears to be what the proxy code is doing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the request body isn't used, can you just pass nil? That's what the proxy code does for subsequent requests (

intermediateConn, err = h.connectBackend("GET", location, initialReq.Header, nil)
)

@ncdc
Copy link
Member Author

ncdc commented Apr 13, 2017

@k8s-bot verify test this (some verify generated protobuf rsync issue)

@ncdc
Copy link
Member Author

ncdc commented Apr 13, 2017

@k8s-bot cvm gce e2e test this

method = "GET"
}

r, err := http.NewRequest(method, location.String(), req.Body)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the request body isn't used, can you just pass nil? That's what the proxy code does for subsequent requests (

intermediateConn, err = h.connectBackend("GET", location, initialReq.Header, nil)
)


s.conn = intermediateConn
intermediateConn = nil // Don't close the connection when we return it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix comment (not returning intermediateConn)

header.Add(httpstream.HeaderConnection, httpstream.HeaderUpgrade)
header.Add(httpstream.HeaderUpgrade, HeaderSpdy31)

redirectLoop:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This duplicates a lot of code from the proxy (

). Is it possible to extract a shared method? Maybe something that takes a connectBackend function?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will look and see what I can do.

@ncdc
Copy link
Member Author

ncdc commented Apr 17, 2017

@timstclair @liggitt updated to extract the common logic shared between spdy and the upgrade aware proxy. PTAL. Also, naming is hard, so I'm open to any suggestions for what to call the new interface and method.

func (s *SpdyRoundTripper) TLSClientConfig() *tls.Config {
return s.tlsConfig
}

// SendRequest implements k8s.io/apimachinery/pkg/util/net.RequestSender.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better use an anonymous var with type cast instead of a comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix

intermediateConn net.Conn
rawResponse = bytes.NewBuffer(make([]byte, 0, 256))
body = originalBody
err error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err can be local in the loop body.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because that would result in intermediateConn being shadowed.

@sttts
Copy link
Contributor

sttts commented Apr 18, 2017

Some nits. Otherwise, looks good now that the redirect loop is factored out.

@ncdc
Copy link
Member Author

ncdc commented Apr 18, 2017

Updated and squashed the code review commits

@ncdc
Copy link
Member Author

ncdc commented Apr 18, 2017

@k8s-bot gci gce e2e test this (one node didn't come up for some reason).

time="2017-04-18T15:22:47.254030486Z" level=info msg="API listen on /var/run/docker.sock"
time="2017-04-18T15:22:47.280588165Z" level=debug msg="Calling POST /v1.23/images/load?quiet=1"
+ [ ! -s /var/lib/docker/repositories-overlay ]
+ rm -f /var/lib/docker/repositories-overlay
time="2017-04-18T15:23:16.566334781Z" level=info msg="New containerd process, pid: 778\n"
time="2017-04-18T15:23:18.037310875Z" level=error msg="invalid image sha256:0de6cfa909121ffb45151d67e23dd6d769ad481d5f4d7a172725996718524b1e, failed to verify image: sha256:0de6cfa909121ffb45151d67e23dd6d769ad481d5f4d7a172725996718524b1e"
time="2017-04-18T15:23:18.039494774Z" level=fatal msg="Error starting daemon: layer does not exist"

return nil, nil, fmt.Errorf("too many redirects (%d)", redirects)
}
if redirects != 0 {
// Redirected requests switch to "GET" according to the HTTP spec:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: if you want, you can move these down to the Prepare to follow the redirect. section and make unconditional

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point... Since you've asked for a comment for 'future us' below, I'll make the change.

if redirectStr == "" {
return nil, nil, fmt.Errorf("%d response missing Location header", resp.StatusCode)
}
location, err = location.Parse(redirectStr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment about parsing relative to the last requested location would be helpful here for future us

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do

@liggitt
Copy link
Member

liggitt commented Apr 18, 2017

lgtm

@ncdc
Copy link
Member Author

ncdc commented Apr 18, 2017

@timstclair @liggitt @sttts ready for final review. Any other comments?

@liggitt
Copy link
Member

liggitt commented Apr 18, 2017

none from me

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2017
@ncdc
Copy link
Member Author

ncdc commented Apr 20, 2017

Just pushed up the new changes. PTAL. I'll say it's a bit weird to have pkg/client/unversioned/remotecommand/remotecommand.go looking at the feature gate...

@timstclair
Copy link

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2017
@@ -78,7 +80,8 @@ func NewExecutor(config *restclient.Config, method string, url *url.URL) (Stream
return nil, err
}

upgradeRoundTripper := spdy.NewRoundTripper(tlsConfig)
followRedirects := utilfeature.DefaultFeatureGate.Enabled(genericfeatures.StreamingProxyRedirects)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having feature gates in client code is awkward. Any reason to have this feature gate at all here? Why don't we enable redirection for remotecommand? If the kubelet still depends on the gate, that's fine and another topic.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't actually used in client code, is it? Maybe I'm confused about how this is used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy to remove it. Do we all agree on that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 25, 2017
@ncdc
Copy link
Member Author

ncdc commented Apr 25, 2017

Rebased and squashed. PTAL.

/release-note

@k8s-ci-robot 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-label-needed labels Apr 25, 2017
@smarterclayton
Copy link
Contributor

smarterclayton commented Apr 25, 2017 via email

@timstclair
Copy link

I don't want to keep blocking this - I'm ok with or without the gate.

@ncdc
Copy link
Member Author

ncdc commented Apr 25, 2017 via email

Add support for following redirects to the SpdyRoundTripper. This is
necessary for clients using it directly (e.g. the apiserver talking
directly to the kubelet) because the CRI streaming server issues a
redirect for streaming requests.

Also extract common logic for following redirects.
@ncdc
Copy link
Member Author

ncdc commented Apr 26, 2017

Ok, removed the flag from the client code. Hopefully this is the last revision :-)

@ncdc ncdc removed the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Apr 26, 2017
@ncdc
Copy link
Member Author

ncdc commented Apr 26, 2017

@timstclair @sttts @smarterclayton @liggitt I think this should be good to go, if you're all ok with it

@smarterclayton
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ncdc, smarterclayton, timstclair

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 274df99 into kubernetes:master Apr 26, 2017
@ncdc ncdc deleted the spdy-follow-redirects branch April 26, 2017 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants