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 a transport package for clients #16126

Merged
merged 1 commit into from
Nov 20, 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
83 changes: 83 additions & 0 deletions pkg/client/transport/cache.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
/*
Copyright 2015 The Kubernetes Authors All rights reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package transport

import (
"fmt"
"net"
"net/http"
"sync"
"time"
)

// TlsTransportCache caches TLS http.RoundTrippers different configurations. The
// same RoundTripper will be returned for configs with identical TLS options If
// the config has no custom TLS options, http.DefaultTransport is returned.
type tlsTransportCache struct {
mu sync.Mutex
transports map[string]*http.Transport
}

var tlsCache = &tlsTransportCache{transports: make(map[string]*http.Transport)}

func (c *tlsTransportCache) get(config *Config) (http.RoundTripper, error) {
key, err := tlsConfigKey(config)
if err != nil {
return nil, err
}

// Ensure we only create a single transport for the given TLS options
c.mu.Lock()
defer c.mu.Unlock()

// See if we already have a custom transport for this config
if t, ok := c.transports[key]; ok {
return t, nil
}

// Get the TLS options for this client config
tlsConfig, err := TLSConfigFor(config)
if err != nil {
return nil, err
}
// The options didn't require a custom TLS config
if tlsConfig == nil {
return http.DefaultTransport, nil
}

// Cache a single transport for these options
c.transports[key] = &http.Transport{
Copy link
Member

Choose a reason for hiding this comment

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

these defaults aren't equivalent to what we had before... it is not setting the same Dial impl as the DefaultTransport

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I missed that. sorry.

Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason not to use the same defaulting function as before, just setting the custom tlsConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Avoiding bloated util libraries to prevent needless dependencies. One of
the main overall goals of the client work is to make it easier for people
to import this.
On Nov 16, 2015 5:20 PM, "Jordan Liggitt" notifications@github.com wrote:

In pkg/client/transport/cache.go
#16126 (comment)
:

  • if t, ok := c.transports[key]; ok {
  •   return t, nil
    
  • }
  • // Get the TLS options for this client config
  • tlsConfig, err := TLSConfigFor(config)
  • if err != nil {
  •   return nil, err
    
  • }
  • // The options didn't require a custom TLS config
  • if tlsConfig == nil {
  •   return http.DefaultTransport, nil
    
  • }
  • // Cache a single transport for these options
  • c.transports[key] = &http.Transport{

Is there a reason not to use the same defaulting function as before, just
setting the custom tlsConfig?


Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/16126/files#r45010982.

Copy link
Member

Choose a reason for hiding this comment

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

If anything, that function belongs in this package. Just don't want that code copied and pasted in lots of places again (for the exact reason that the refactor caused a behavior change...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps. It's really not that much code. And I don't want to add more to this PR.

Proxy: http.ProxyFromEnvironment,
TLSHandshakeTimeout: 10 * time.Second,
TLSClientConfig: tlsConfig,
Dial: (&net.Dialer{
Timeout: 30 * time.Second,
KeepAlive: 30 * time.Second,
}).Dial,
}
return c.transports[key], nil
}

// tlsConfigKey returns a unique key for tls.Config objects returned from TLSConfigFor
func tlsConfigKey(c *Config) (string, error) {
// Make sure ca/key/cert content is loaded
if err := loadTLSFiles(c); err != nil {
return "", err
}
// Only include the things that actually affect the tls.Config
return fmt.Sprintf("%v/%x/%x/%x", c.TLS.Insecure, c.TLS.CAData, c.TLS.CertData, c.TLS.KeyData), nil
}
114 changes: 114 additions & 0 deletions pkg/client/transport/cache_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
/*
Copyright 2015 The Kubernetes Authors All rights reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package transport

import (
"net/http"
"testing"
)

func TestTLSConfigKey(t *testing.T) {
// Make sure config fields that don't affect the tls config don't affect the cache key
identicalConfigurations := map[string]*Config{
"empty": {},
"basic": {Username: "bob", Password: "password"},
"bearer": {BearerToken: "token"},
"user agent": {UserAgent: "useragent"},
"transport": {Transport: http.DefaultTransport},
"wrap transport": {WrapTransport: func(http.RoundTripper) http.RoundTripper { return nil }},
}
for nameA, valueA := range identicalConfigurations {
for nameB, valueB := range identicalConfigurations {
keyA, err := tlsConfigKey(valueA)
if err != nil {
t.Errorf("Unexpected error for %q: %v", nameA, err)
continue
}
keyB, err := tlsConfigKey(valueB)
if err != nil {
t.Errorf("Unexpected error for %q: %v", nameB, err)
continue
}
if keyA != keyB {
t.Errorf("Expected identical cache keys for %q and %q, got:\n\t%s\n\t%s", nameA, nameB, keyA, keyB)
continue
}
}
}

// Make sure config fields that affect the tls config affect the cache key
uniqueConfigurations := map[string]*Config{
"no tls": {},
"insecure": {TLS: TLSConfig{Insecure: true}},
"cadata 1": {TLS: TLSConfig{CAData: []byte{1}}},
"cadata 2": {TLS: TLSConfig{CAData: []byte{2}}},
"cert 1, key 1": {
TLS: TLSConfig{
CertData: []byte{1},
KeyData: []byte{1},
},
},
"cert 1, key 2": {
TLS: TLSConfig{
CertData: []byte{1},
KeyData: []byte{2},
},
},
"cert 2, key 1": {
TLS: TLSConfig{
CertData: []byte{2},
KeyData: []byte{1},
},
},
"cert 2, key 2": {
TLS: TLSConfig{
CertData: []byte{2},
KeyData: []byte{2},
},
},
"cadata 1, cert 1, key 1": {
TLS: TLSConfig{
CAData: []byte{1},
CertData: []byte{1},
KeyData: []byte{1},
},
},
}
for nameA, valueA := range uniqueConfigurations {
for nameB, valueB := range uniqueConfigurations {
// Don't compare to ourselves
if nameA == nameB {
continue
}

keyA, err := tlsConfigKey(valueA)
if err != nil {
t.Errorf("Unexpected error for %q: %v", nameA, err)
continue
}
keyB, err := tlsConfigKey(valueB)
if err != nil {
t.Errorf("Unexpected error for %q: %v", nameB, err)
continue
}
if keyA == keyB {
t.Errorf("Expected unique cache keys for %q and %q, got:\n\t%s\n\t%s", nameA, nameB, keyA, keyB)
continue
}
}
}
}
81 changes: 81 additions & 0 deletions pkg/client/transport/config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
/*
Copyright 2015 The Kubernetes Authors All rights reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package transport

import "net/http"

// Config holds various options for establishing a transport.
type Config struct {
Copy link
Member

Choose a reason for hiding this comment

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

I'm still having a hard time seeing the benefit of deeply nested config structs... presumably, we would eventually want client.Config to contain transport.Config, at which point we'd have this:

client.Config
  transport.Config
    transport.AuthConfig
      transport.TokenAuthConfig
        Token string

That seems like a lot of levels. Just like you have the UserAgent string field that drives the creation of the user-agent round tripper wrapper, it seems simpler to keep the username,password fields to drive the basic auth wrapper, and the token field to drive the bearer auth wrapper.

The value seems to be in the helper functions you are checking to drive the wrapper creation (like hasBasicAuth, etc), and those could simply check the fields directly in the transport config, they don't needs structs within structs within structs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it provides a clearer grouping of related data.

Also, the clients we plan on generating will take an *http.Client an know nothing else of the transport layer. So it won't be nested like you say.

Copy link
Member

Choose a reason for hiding this comment

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

The transport config is still something every consumer will have to build, right? It's nice that the client gets a *http.Client with a transport already built, but the options that drive that come from somewhere... what benefit is a four-level structure like this providing?

  transport.Config
    transport.AuthConfig
      transport.TokenAuthConfig
        Token string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The benefit still is better organization and separation of data, but for understanding and manipulation. With this you could have a function that just returns AuthConfig and assign that into your struct instead of either it taking a config and assigning the fields it needs to or requiring the user to merge it themselves.

// UserAgent is an optional field that specifies the caller of this
// request.
UserAgent string

// The base TLS configuration for this transport.
TLS TLSConfig

// Username and password for basic authentication
Username string
Password string

// Bearer token for authentication
BearerToken string

// Transport may be used for custom HTTP behavior. This attribute may
// not be specified with the TLS client certificate options. Use
// WrapTransport for most client level operations.
Transport http.RoundTripper

// WrapTransport will be invoked for custom HTTP behavior after the
// underlying transport is initialized (either the transport created
// from TLSClientConfig, Transport, or http.DefaultTransport). The
// config may layer other RoundTrippers on top of the returned
// RoundTripper.
WrapTransport func(rt http.RoundTripper) http.RoundTripper
}

// HasCA returns whether the configuration has a certificate authority or not.
func (c *Config) HasCA() bool {
return len(c.TLS.CAData) > 0 || len(c.TLS.CAFile) > 0
}

// HasBasicAuth returns whether the configuration has basic authentication or not.
func (c *Config) HasBasicAuth() bool {
return len(c.Username) != 0
}

// HasTokenAuth returns whether the configuration has token authentication or not.
func (c *Config) HasTokenAuth() bool {
return len(c.BearerToken) != 0
}

// HasCertAuth returns whether the configuration has certificate authentication or not.
func (c *Config) HasCertAuth() bool {
return len(c.TLS.CertData) != 0 || len(c.TLS.CertFile) != 0
}

// TLSConfig holds the information needed to set up a TLS transport.
type TLSConfig struct {
CAFile string // Path of the PEM-encoded server trusted root certificates.
CertFile string // Path of the PEM-encoded client certificate.
KeyFile string // Path of the PEM-encoded client key.

Insecure bool // Server should be accessed without verifying the certificate. For testing only.

CAData []byte // Bytes of the PEM-encoded server trusted root certificates. Supercedes CAFile.
CertData []byte // Bytes of the PEM-encoded client certificate. Supercedes CertFile.
KeyData []byte // Bytes of the PEM-encoded client key. Supercedes KeyFile.
}