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

Support persisting config from kubecfg AuthProvider plugins #24304

Merged
merged 2 commits into from
May 8, 2016
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
3 changes: 3 additions & 0 deletions pkg/client/restclient/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ type Config struct {
// Server requires plugin-specified authentication.
AuthProvider *clientcmdapi.AuthProviderConfig

// Callback to persist config for AuthProvider.
AuthConfigPersister AuthProviderConfigPersister

// TLSClientConfig contains settings to enable transport layer security
TLSClientConfig

Expand Down
19 changes: 16 additions & 3 deletions pkg/client/restclient/plugin.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,22 @@ type AuthProvider interface {
// WrapTransport allows the plugin to create a modified RoundTripper that
// attaches authorization headers (or other info) to requests.
WrapTransport(http.RoundTripper) http.RoundTripper
// Login allows the plugin to initialize its configuration. It must not
// require direct user interaction.
Login() error
}

type Factory func() (AuthProvider, error)
// Factory generates an AuthProvider plugin.
// clusterAddress is the address of the current cluster.
// config is the inital configuration for this plugin.
// persister allows the plugin to save updated configuration.
type Factory func(clusterAddress string, config map[string]string, persister AuthProviderConfigPersister) (AuthProvider, error)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense for the config to be a map[string]interface so that it could support lists of strings as well? I just ran into this case in the work I am doing; I could just as well have a comma-separated list of strings but it's not ideal.

Sorry for saying this so late in the game but it didn't occur to me until I started working with it.

Copy link
Member Author

@cjcullen cjcullen May 4, 2016

Choose a reason for hiding this comment

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

I'd eventually (post 1.3) be interested in making the config use runtime.RawExtension/runtime.Object so that plugins could have strongly typed config. Would you be okay w/ map[string]string until we move to that? If not, I'd be fine with you making the changes in a following PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense for the config to be a map[string]interface so that it could support lists of strings as well?

map[string]interface{} won't work well because we're trying to serialize the data out into json.

map[string]string works well, map[string][]string also works fine. I don't have any objection to tweaking the type to map[string][]string in this pull or in a very close followup. The datatype change would be a breaking serialization change for kubeconfig files, so we'd have to change it before anyone gets a chance to use it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmmm. I'd rather keep it as map[string]string then.

@bobbyrullo, how bad would it be for your case to just use strings.Split for a comma-separated list of strings?

Copy link
Contributor

Choose a reason for hiding this comment

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

strings.Split is just fine.

// AuthProviderConfigPersister allows a plugin to persist configuration info
// for just itself.
type AuthProviderConfigPersister interface {
Persist(map[string]string) error
}

// All registered auth provider plugins.
var pluginsLock sync.Mutex
Expand All @@ -49,12 +62,12 @@ func RegisterAuthProviderPlugin(name string, plugin Factory) error {
return nil
}

func GetAuthProvider(apc *clientcmdapi.AuthProviderConfig) (AuthProvider, error) {
func GetAuthProvider(clusterAddress string, apc *clientcmdapi.AuthProviderConfig, persister AuthProviderConfigPersister) (AuthProvider, error) {
pluginsLock.Lock()
defer pluginsLock.Unlock()
p, ok := plugins[apc.Name]
if !ok {
return nil, fmt.Errorf("No Auth Provider found for name %q", apc.Name)
}
return p()
return p(clusterAddress, apc.Config, persister)
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ package restclient
import (
"fmt"
"net/http"
"reflect"
"strconv"
"testing"

clientcmdapi "k8s.io/kubernetes/pkg/client/unversioned/clientcmd/api"
)

func TestTransportConfigAuthPlugins(t *testing.T) {
func TestAuthPluginWrapTransport(t *testing.T) {
if err := RegisterAuthProviderPlugin("pluginA", pluginAProvider); err != nil {
t.Errorf("Unexpected error: failed to register pluginA: %v", err)
}
Expand Down Expand Up @@ -92,6 +94,83 @@ func TestTransportConfigAuthPlugins(t *testing.T) {
}
}

func TestAuthPluginPersist(t *testing.T) {
// register pluginA by a different name so we don't collide across tests.
if err := RegisterAuthProviderPlugin("pluginA2", pluginAProvider); err != nil {
t.Errorf("Unexpected error: failed to register pluginA: %v", err)
}
if err := RegisterAuthProviderPlugin("pluginPersist", pluginPersistProvider); err != nil {
t.Errorf("Unexpected error: failed to register pluginPersist: %v", err)
}
fooBarConfig := map[string]string{"foo": "bar"}
testCases := []struct {
plugin string
startingConfig map[string]string
expectedConfigAfterLogin map[string]string
expectedConfigAfterRoundTrip map[string]string
}{
// non-persisting plugins should work fine without modifying config.
{"pluginA2", map[string]string{}, map[string]string{}, map[string]string{}},
{"pluginA2", fooBarConfig, fooBarConfig, fooBarConfig},
// plugins that persist config should be able to persist when they want.
{
"pluginPersist",
map[string]string{},
map[string]string{
"login": "Y",
},
map[string]string{
"login": "Y",
"roundTrips": "1",
},
},
{
"pluginPersist",
map[string]string{
"login": "Y",
"roundTrips": "123",
},
map[string]string{
"login": "Y",
"roundTrips": "123",
},
map[string]string{
"login": "Y",
"roundTrips": "124",
},
},
}
for i, tc := range testCases {
cfg := &clientcmdapi.AuthProviderConfig{
Name: tc.plugin,
Config: tc.startingConfig,
}
persister := &inMemoryPersister{make(map[string]string)}
persister.Persist(tc.startingConfig)
plugin, err := GetAuthProvider("127.0.0.1", cfg, persister)
if err != nil {
t.Errorf("%d. Unexpected error: failed to get plugin %q: %v", i, tc.plugin, err)
}
if err := plugin.Login(); err != nil {
t.Errorf("%d. Unexpected error calling Login() w/ plugin %q: %v", i, tc.plugin, err)
}
// Make sure the plugin persisted what we expect after Login().
if !reflect.DeepEqual(persister.savedConfig, tc.expectedConfigAfterLogin) {
t.Errorf("%d. Unexpected persisted config after calling %s.Login(): \nGot:\n%v\nExpected:\n%v",
i, tc.plugin, persister.savedConfig, tc.expectedConfigAfterLogin)
}
if _, err := plugin.WrapTransport(&emptyTransport{}).RoundTrip(&http.Request{}); err != nil {
t.Errorf("%d. Unexpected error round-tripping w/ plugin %q: %v", i, tc.plugin, err)
}
// Make sure the plugin persisted what we expect after RoundTrip().
if !reflect.DeepEqual(persister.savedConfig, tc.expectedConfigAfterRoundTrip) {
t.Errorf("%d. Unexpected persisted config after calling %s.WrapTransport.RoundTrip(): \nGot:\n%v\nExpected:\n%v",
i, tc.plugin, persister.savedConfig, tc.expectedConfigAfterLogin)
}
}

}

// emptyTransport provides an empty http.Response with an initialized header
// to allow wrapping RoundTrippers to set header values.
type emptyTransport struct{}
Expand Down Expand Up @@ -137,7 +216,9 @@ func (*pluginA) WrapTransport(rt http.RoundTripper) http.RoundTripper {
return &wrapTransportA{rt}
}

func pluginAProvider() (AuthProvider, error) {
func (*pluginA) Login() error { return nil }

func pluginAProvider(string, map[string]string, AuthProviderConfigPersister) (AuthProvider, error) {
return &pluginA{}, nil
}

Expand All @@ -161,11 +242,70 @@ func (*pluginB) WrapTransport(rt http.RoundTripper) http.RoundTripper {
return &wrapTransportB{rt}
}

func pluginBProvider() (AuthProvider, error) {
func (*pluginB) Login() error { return nil }

func pluginBProvider(string, map[string]string, AuthProviderConfigPersister) (AuthProvider, error) {
return &pluginB{}, nil
}

// pluginFailProvider simulates a registered AuthPlugin that fails to load.
func pluginFailProvider() (AuthProvider, error) {
func pluginFailProvider(string, map[string]string, AuthProviderConfigPersister) (AuthProvider, error) {
return nil, fmt.Errorf("Failed to load AuthProvider")
}

type inMemoryPersister struct {
savedConfig map[string]string
}

func (i *inMemoryPersister) Persist(config map[string]string) error {
i.savedConfig = make(map[string]string)
for k, v := range config {
i.savedConfig[k] = v
}
return nil
}

// wrapTransportPersist increments the "roundTrips" entry from the config when
// roundTrip is called.
type wrapTransportPersist struct {
rt http.RoundTripper
config map[string]string
persister AuthProviderConfigPersister
}

func (w *wrapTransportPersist) RoundTrip(req *http.Request) (*http.Response, error) {
roundTrips := 0
if rtVal, ok := w.config["roundTrips"]; ok {
var err error
roundTrips, err = strconv.Atoi(rtVal)
if err != nil {
return nil, err
}
}
roundTrips++
w.config["roundTrips"] = fmt.Sprintf("%d", roundTrips)
if err := w.persister.Persist(w.config); err != nil {
return nil, err
}
return w.rt.RoundTrip(req)
}

type pluginPersist struct {
config map[string]string
persister AuthProviderConfigPersister
}

func (p *pluginPersist) WrapTransport(rt http.RoundTripper) http.RoundTripper {
return &wrapTransportPersist{rt, p.config, p.persister}
}

// Login sets the config entry "login" to "Y".
func (p *pluginPersist) Login() error {
p.config["login"] = "Y"
p.persister.Persist(p.config)
return nil
}

func pluginPersistProvider(_ string, config map[string]string, persister AuthProviderConfigPersister) (AuthProvider, error) {
return &pluginPersist{config, persister}, nil
}
2 changes: 1 addition & 1 deletion pkg/client/restclient/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ func HTTPWrappersForConfig(config *Config, rt http.RoundTripper) (http.RoundTrip
func (c *Config) transportConfig() (*transport.Config, error) {
wt := c.WrapTransport
if c.AuthProvider != nil {
provider, err := GetAuthProvider(c.AuthProvider)
provider, err := GetAuthProvider(c.Host, c.AuthProvider, c.AuthConfigPersister)
if err != nil {
return nil, err
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/client/unversioned/clientcmd/api/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,8 @@ type Context struct {

// AuthProviderConfig holds the configuration for a specified auth provider.
type AuthProviderConfig struct {
Name string `json:"name"`
Name string `json:"name"`
Config map[string]string `json:"config,omitempty"`
}

// NewConfig is a convenience function that returns a new Config object with non-nil maps
Expand Down
11 changes: 10 additions & 1 deletion pkg/client/unversioned/clientcmd/api/types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,13 @@ func Example_ofOptionsConfig() {
Token: "my-secret-token",
}
defaultConfig.AuthInfos["black-mage-via-auth-provider"] = &AuthInfo{
AuthProvider: &AuthProviderConfig{Name: "gcp"},
AuthProvider: &AuthProviderConfig{
Name: "gcp",
Config: map[string]string{
"foo": "bar",
"token": "s3cr3t-t0k3n",
},
},
}
defaultConfig.Contexts["bravo-as-black-mage"] = &Context{
Cluster: "bravo",
Expand Down Expand Up @@ -115,6 +121,9 @@ func Example_ofOptionsConfig() {
// black-mage-via-auth-provider:
// LocationOfOrigin: ""
// auth-provider:
// config:
// foo: bar
// token: s3cr3t-t0k3n
// name: gcp
// red-mage-via-token:
// LocationOfOrigin: ""
Expand Down
3 changes: 2 additions & 1 deletion pkg/client/unversioned/clientcmd/api/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,5 +140,6 @@ type NamedExtension struct {

// AuthProviderConfig holds the configuration for a specified auth provider.
type AuthProviderConfig struct {
Name string `json:"name"`
Name string `json:"name"`
Config map[string]string `json:"config"`
}