Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Svcat bind params now supports --params-json #1889

Merged
merged 5 commits into from
Mar 30, 2018
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 30 additions & 6 deletions cmd/svcat/binding/bind_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,8 @@ type bindCmd struct {
bindingName string
secretName string
rawParams []string
params map[string]string
jsonParams string
params interface{}
rawSecrets []string
secrets map[string]string
}
Expand All @@ -45,6 +46,16 @@ func NewBindCmd(cxt *command.Context) *cobra.Command {
Example: `
svcat bind wordpress
svcat bind wordpress-mysql-instance --name wordpress-mysql-binding --secret-name wordpress-mysql-secret
svcat bind wordpress-instance --params type=admin
svcat bind wordpress-instance --params-json '{
"type": "admin",
"teams": [
"news",
"weather",
"sports"
]
}
'
`,
PreRunE: command.PreRunE(bindCmd),
RunE: command.RunE(bindCmd),
Expand All @@ -65,10 +76,11 @@ func NewBindCmd(cxt *command.Context) *cobra.Command {
"The name of the secret. Defaults to the name of the instance.",
)
cmd.Flags().StringSliceVarP(&bindCmd.rawParams, "param", "p", nil,
"Additional parameter to use when binding the instance, format: NAME=VALUE")
"Additional parameter to use when binding the instance, format: NAME=VALUE. Cannot be combined with --params-json")
cmd.Flags().StringSliceVarP(&bindCmd.rawSecrets, "secret", "s", nil,
"Additional parameter, whose value is stored in a secret, to use when binding the instance, format: SECRET[KEY]")

cmd.Flags().StringVar(&bindCmd.jsonParams, "params-json", "",
Copy link
Contributor

Choose a reason for hiding this comment

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

I like it.

I would like a version of this that was --secret-params-json and let you put a json blob into a secret that was referenced.

Copy link
Contributor Author

@n3wscott n3wscott Mar 30, 2018

Choose a reason for hiding this comment

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

Would the result of that command create a new secret and then use the secret just like we currently do with --secret-name NAME ?

We would have to have some way to clean that secret up after the operation has finished. I think this sounds like a good idea and I am guessing most users would not understand that sending params via svcat exposes those to the other users of k8s.

update: Oh I needed to read better. We would be updating the contents of the secret with the blob, and we pass in the name of the secret.

Yeah! nice. made and issue: #1898

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to make sure I understand, the difference between --secret and this new flag would be that it would handle making the secret for you and setting the secret paramsfrom on the instance for you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@carolynvs I misunderstood the suggestion at first. I think @pmorie was suggesting the simpler case of supporting exactly what the current --secret does except provide the secret pairs as a json blob provided by --secret-json

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! @pmorie can you weigh in when you get a chance? 😀

"Additional parameters to use when binding the instance, provided as a JSON object. Cannot be combined with --param")
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a message (fine to be in a follow up) warning people not to use this for secret information.

return cmd
}

Expand All @@ -79,9 +91,21 @@ func (c *bindCmd) Validate(args []string) error {
c.instanceName = args[0]

var err error
c.params, err = parameters.ParseVariableAssignments(c.rawParams)
if err != nil {
return fmt.Errorf("invalid --param value (%s)", err)

if c.jsonParams != "" && len(c.rawParams) > 0 {
return fmt.Errorf("--params-json cannot be used with --param")
}

if c.jsonParams != "" {
c.params, err = parameters.ParseVariableJSON(c.jsonParams)
if err != nil {
return fmt.Errorf("invalid --params-json value (%s)", err)
}
} else {
c.params, err = parameters.ParseVariableAssignments(c.rawParams)
if err != nil {
return fmt.Errorf("invalid --param value (%s)", err)
}
}

c.secrets, err = parameters.ParseKeyMaps(c.rawSecrets)
Expand Down
2 changes: 1 addition & 1 deletion cmd/svcat/instance/provision_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ func (c *provisonCmd) Validate(args []string) error {
if c.jsonParams != "" {
c.params, err = parameters.ParseVariableJSON(c.jsonParams)
if err != nil {
return fmt.Errorf("invalid --params value (%s)", err)
return fmt.Errorf("invalid --params-json value (%s)", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch!

}
} else {
c.params, err = parameters.ParseVariableAssignments(c.rawParams)
Expand Down
89 changes: 89 additions & 0 deletions cmd/svcat/svcat_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,20 @@ import (
"net/url"
"os"
"path/filepath"
"reflect"
"regexp"
"strings"
"testing"
"text/template"

clientgotesting "k8s.io/client-go/testing"

"encoding/json"

"github.com/kubernetes-incubator/service-catalog/cmd/svcat/command"
"github.com/kubernetes-incubator/service-catalog/cmd/svcat/plugin"
"github.com/kubernetes-incubator/service-catalog/internal/test"
"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/v1beta1"
"github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/fake"
"github.com/kubernetes-incubator/service-catalog/pkg/svcat"
"github.com/kubernetes-incubator/service-catalog/pkg/svcat/service-catalog"
Expand Down Expand Up @@ -63,6 +69,9 @@ func TestCommandValidation(t *testing.T) {
{"provision does not accept --param and --params-json",
`provision name --class class --plan plan --params-json '{}' --param k=v`,
"--params-json cannot be used with --param"},
{"bind does not accept --param and --params-json",
`bind name --params-json '{}' --param k=v`,
"--params-json cannot be used with --param"},
}

for _, tc := range testcases {
Expand Down Expand Up @@ -208,6 +217,86 @@ func TestNamespacedCommands(t *testing.T) {
}
}

// TestParametersForBinding confirms that parameters given as --param or --param-json work the same way
func TestParametersForBinding(t *testing.T) {
testcases := []struct {
name string
cmd string
params map[string]interface{}
}{
{
name: "bind with --param",
cmd: "bind NAME --param foo=bar --param baz=boo",
params: map[string]interface{}{
"foo": "bar",
"baz": "boo",
},
},
{
name: "bind with --params-json",
cmd: "bind NAME --params-json {\"foo\":\"bar\",\"baz\":\"boo\"}",
params: map[string]interface{}{
"foo": "bar",
"baz": "boo",
},
},
{
name: "bind with --params-json with a sub object",
cmd: "bind NAME --params-json {\"foo\":{\"faa\":\"bar\",\"baz\":\"boo\"}}",
params: map[string]interface{}{
"foo": map[string]interface{}{
"faa": "bar",
"baz": "boo",
},
},
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
fakeClient := fake.NewSimpleClientset()

cxt := newContext()
cxt.App = &svcat.App{
SDK: &servicecatalog.SDK{ServiceCatalogClient: fakeClient},
}
cxt.Output = ioutil.Discard

executeFakeCommand(t, tc.cmd, cxt, true)

if c := fakeClient.Actions(); len(c) != 1 {
t.Fatal("Expected only 1 action, got ", c)
}
action := fakeClient.Actions()[0]

if action.GetVerb() != "create" {
t.Fatal("Expected a create action, but got ", action.GetVerb())
}
createAction, ok := action.(clientgotesting.CreateAction)
if !ok {
t.Fatal(t, "Unexpected type; failed to convert action %+v to CreateAction", action)

}

fakeObject := createAction.GetObject()

binding, ok := fakeObject.(*v1beta1.ServiceBinding)
if !ok {
t.Fatal(t, "Failed to cast object to binding: ", fakeObject)
}

var params map[string]interface{}
if err := json.Unmarshal(binding.Spec.Parameters.Raw, &params); err != nil {
t.Error("failed to unmarshal binding.Spec.Parameters")
}

if eq := reflect.DeepEqual(params, tc.params); !eq {
t.Error(fmt.Sprintf("parameters mismatch, \nwant: %+v, \ngot: %+v", tc.params, params))
}
})
}
}

// executeCommand runs a svcat command against a fake k8s api,
// returning the cli output.
func executeCommand(t *testing.T, cmd string, continueOnErr bool) string {
Expand Down
6 changes: 5 additions & 1 deletion cmd/svcat/testdata/plugin.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@ tree:
desc: The name of the binding. Defaults to the name of the instance.
- name: param
shorthand: p
desc: 'Additional parameter to use when binding the instance, format: NAME=VALUE'
desc: 'Additional parameter to use when binding the instance, format: NAME=VALUE.
Cannot be combined with --params-json'
- name: params-json
desc: Additional parameters to use when binding the instance, provided as a JSON
object. Cannot be combined with --param
- name: secret
desc: 'Additional parameter, whose value is stored in a secret, to use when binding
the instance, format: SECRET[KEY]'
Expand Down
4 changes: 2 additions & 2 deletions pkg/svcat/service-catalog/binding.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ func (sdk *SDK) RetrieveBindingsByInstance(instance *v1beta1.ServiceInstance,

// Bind an instance to a secret.
func (sdk *SDK) Bind(namespace, bindingName, instanceName, secretName string,
params map[string]string, secrets map[string]string) (*v1beta1.ServiceBinding, error) {
params interface{}, secrets map[string]string) (*v1beta1.ServiceBinding, error) {

// Manually defaulting the name of the binding
// I'm not doing the same for the secret since the API handles defaulting that value.
Expand Down Expand Up @@ -137,7 +137,7 @@ func (sdk *SDK) Unbind(ns, instanceName string) ([]v1beta1.ServiceBinding, error
}

//Range over the deleted bindings to build a slice to return
deleted := []v1beta1.ServiceBinding{}
deleted := []v1beta1.ServiceBinding(nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel silly asking but what does this change do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[]v1beta1.ServiceBinding{} is creating an empty slice with the literal initialized.

[]v1beta1.ServiceBinding(nil) creates with the explicate nil initializer.

TBH: goland tells me using the (nil) way is better than {}

Copy link
Contributor

Choose a reason for hiding this comment

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

I bow to the superior skills of Goland. Good to know what that means, thanks! 👍

for b := range deletedBindings {
deleted = append(deleted, b)
}
Expand Down