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

Make UpgradeAwareProxyHandler use transport.Dial if provided #10529

Merged
merged 2 commits into from
Jul 1, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 38 additions & 5 deletions pkg/registry/generic/rest/proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -174,23 +174,56 @@ func (h *UpgradeAwareProxyHandler) tryUpgrade(w http.ResponseWriter, req *http.R
func (h *UpgradeAwareProxyHandler) dialURL() (net.Conn, error) {
dialAddr := netutil.CanonicalAddr(h.Location)

var dialer func(network, addr string) (net.Conn, error)
Copy link
Member

Choose a reason for hiding this comment

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

Can't you set this to be net.dial and lose all the conditionals?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not without subtly changing the behavior in the https case. tls.Dial allows tlsConfig to be nil and will treat as the zero config. tls.Client requires non-nil tlsConfig, with either InsecureSkipVerify: true or ServerName provided.

Copy link
Member

Choose a reason for hiding this comment

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

hrm, I'm sorry to be so pedantic about this, but how can tls.Dial take a nil config? does it set ServerAddress based on dialAddr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per tls documentation:

Dial connects to the given network address using net.Dial and then initiates a TLS handshake, returning the resulting TLS connection. Dial interprets a nil configuration as equivalent to the zero configuration; see the documentation of Config for the defaults.

I don't know whether that code path is actually ever hit. I would expect we either have a transport with a valid tlsConfig or we are in the http case.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add a

// TODO: this TLS logic can probably be cleaned up; it's messy in an attempt to preserve behavior that we don't know for sure is exercised.

And then I will stop bothering you about it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

if httpTransport, ok := h.Transport.(*http.Transport); ok && httpTransport.Dial != nil {
dialer = httpTransport.Dial
}

switch h.Location.Scheme {
case "http":
if dialer != nil {
return dialer("tcp", dialAddr)
}
return net.Dial("tcp", dialAddr)
case "https":
// TODO: this TLS logic can probably be cleaned up; it's messy in an attempt
// to preserve behavior that we don't know for sure is exercised.

// Get the tls config from the transport if we recognize it
var tlsConfig *tls.Config
var tlsConn *tls.Conn
var err error
if h.Transport != nil {
httpTransport, ok := h.Transport.(*http.Transport)
if ok {
tlsConfig = httpTransport.TLSClientConfig
}
}
if dialer != nil {
// We have a dialer; use it to open the connection, then
// create a tls client using the connection.
netConn, err := dialer("tcp", dialAddr)
if err != nil {
return nil, err
}
// tls.Client requires non-nil config
if tlsConfig == nil {
glog.Warningf("using custom dialer with no TLSClientConfig. Defaulting to InsecureSkipVerify")
tlsConfig = &tls.Config{
InsecureSkipVerify: true,
}
}
tlsConn = tls.Client(netConn, tlsConfig)
if err := tlsConn.Handshake(); err != nil {
return nil, err
}

// Dial
tlsConn, err := tls.Dial("tcp", dialAddr, tlsConfig)
if err != nil {
return nil, err
} else {
// Dial
tlsConn, err = tls.Dial("tcp", dialAddr, tlsConfig)
if err != nil {
return nil, err
}
}

// Verify
Expand All @@ -202,7 +235,7 @@ func (h *UpgradeAwareProxyHandler) dialURL() (net.Conn, error) {

return tlsConn, nil
default:
return nil, fmt.Errorf("Unknown scheme: %s", h.Location.Scheme)
return nil, fmt.Errorf("unknown scheme: %s", h.Location.Scheme)
}
}

Expand Down
88 changes: 88 additions & 0 deletions test/e2e/kubectl.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ import (
"encoding/json"
"errors"
"fmt"
"io/ioutil"
"net/http"
"path/filepath"
"regexp"
"strings"
"time"

Expand All @@ -42,6 +45,14 @@ const (
kubectlProxyPort = 8011
guestbookStartupTimeout = 10 * time.Minute
guestbookResponseTimeout = 3 * time.Minute
simplePodSelector = "name=nginx"
simplePodName = "nginx"
nginxDefaultOutput = "Welcome to nginx!"
simplePodPort = 80
)

var (
portForwardRegexp = regexp.MustCompile("Forwarding from 127.0.0.1:([0-9]+) -> 80")
)

var _ = Describe("Kubectl client", func() {
Expand Down Expand Up @@ -127,8 +138,85 @@ var _ = Describe("Kubectl client", func() {
})
})

Describe("Simple pod", func() {
var podPath string

BeforeEach(func() {
podPath = filepath.Join(testContext.RepoRoot, "examples/pod.yaml")
By("creating the pod")
runKubectl("create", "-f", podPath, fmt.Sprintf("--namespace=%v", ns))
checkPodsRunningReady(c, ns, []string{simplePodName}, podStartTimeout)

})
AfterEach(func() {
cleanup(podPath, ns, simplePodSelector)
})

It("should support exec", func() {
By("executing a command in the container")
execOutput := runKubectl("exec", fmt.Sprintf("--namespace=%v", ns), simplePodName, "echo", "running", "in", "container")
expectedExecOutput := "running in container"
if execOutput != expectedExecOutput {
Failf("Unexpected kubectl exec output. Wanted '%s', got '%s'", execOutput, expectedExecOutput)
}
})
It("should support port-forward", func() {
By("forwarding the container port to a local port")
cmd := kubectlCmd("port-forward", fmt.Sprintf("--namespace=%v", ns), "-p", simplePodName, fmt.Sprintf(":%d", simplePodPort))
defer func() {
if err := cmd.Process.Kill(); err != nil {
Logf("ERROR failed to kill kubectl port-forward command! The process may leak")
}
}()
// This is somewhat ugly but is the only way to retrieve the port that was picked
// by the port-forward command. We don't want to hard code the port as we have no
// way of guaranteeing we can pick one that isn't in use, particularly on Jenkins.
Logf("starting port-forward command and streaming output")
stdout, stderr, err := startCmdAndStreamOutput(cmd)
if err != nil {
Failf("Failed to start port-forward command: %v", err)
}
defer stdout.Close()
defer stderr.Close()

buf := make([]byte, 128)
var n int
Logf("reading from `kubectl port-forward` command's stderr")
if n, err = stderr.Read(buf); err != nil {
Failf("Failed to read from kubectl port-forward stderr: %v", err)
}
portForwardOutput := string(buf[:n])
match := portForwardRegexp.FindStringSubmatch(portForwardOutput)
if len(match) != 2 {
Failf("Failed to parse kubectl port-forward output: %s", portForwardOutput)
}
By("curling local port output")
localAddr := fmt.Sprintf("http://localhost:%s", match[1])
body, err := curl(localAddr)
if err != nil {
Failf("Failed http.Get of forwarded port (%s): %v", localAddr, err)
}
if !strings.Contains(body, nginxDefaultOutput) {
Failf("Container port output missing expected value. Wanted:'%s', got: %s", nginxDefaultOutput, body)
}
})
})

})

func curl(addr string) (string, error) {
resp, err := http.Get(addr)
if err != nil {
return "", err
}
defer resp.Body.Close()
body, err := ioutil.ReadAll(resp.Body)
if err != nil {
return "", err
}
return string(body[:]), nil
}

func validateGuestbookApp(c *client.Client, ns string) {
Logf("Waiting for frontend to serve content.")
if !waitForGuestbookResponse(c, "get", "", `{"data": ""}`, guestbookStartupTimeout, ns) {
Expand Down
15 changes: 15 additions & 0 deletions test/e2e/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package e2e
import (
"bytes"
"fmt"
"io"
"io/ioutil"
"math"
"math/rand"
Expand Down Expand Up @@ -832,6 +833,20 @@ func runKubectl(args ...string) string {
return strings.TrimSpace(stdout.String())
}

func startCmdAndStreamOutput(cmd *exec.Cmd) (stdout, stderr io.ReadCloser, err error) {
stdout, err = cmd.StdoutPipe()
if err != nil {
return
}
stderr, err = cmd.StderrPipe()
if err != nil {
return
}
Logf("Asyncronously running '%s %s'", cmd.Path, strings.Join(cmd.Args, " "))
err = cmd.Start()
return
}

// testContainerOutput runs testContainerOutputInNamespace with the default namespace.
func testContainerOutput(scenarioName string, c *client.Client, pod *api.Pod, containerIndex int, expectedOutput []string) {
testContainerOutputInNamespace(scenarioName, c, pod, containerIndex, expectedOutput, api.NamespaceDefault)
Expand Down