Skip to content

Bug 1605136 - allow empty string values for non-generated parameters #21371

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 3 commits into from
Dec 2, 2018
Merged

Bug 1605136 - allow empty string values for non-generated parameters #21371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f574e2f
allow empty string values for non-generated parameters
Oct 27, 2018
f6f49c0
replace empty string check with len()
Nov 5, 2018
b8df8c5
add unit test for ensureSecret()
Nov 5, 2018
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
13 changes: 9 additions & 4 deletions pkg/templateservicebroker/servicebroker/provision.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

// ensureSecret ensures the existence of a Secret object containing the template
// configuration parameters.
func (b *Broker) ensureSecret(u user.Info, namespace string, brokerTemplateInstance *templateapiv1.BrokerTemplateInstance, instanceID string, preq *api.ProvisionRequest, didWork *bool) (*kapiv1.Secret, *api.Response) {
func (b *Broker) ensureSecret(u user.Info, namespace string, brokerTemplateInstance *templateapiv1.BrokerTemplateInstance, instanceID string, preq *api.ProvisionRequest, template *templateapiv1.Template, didWork *bool) (*kapiv1.Secret, *api.Response) {
glog.V(4).Infof("Template service broker: ensureSecret")

blockOwnerDeletion := true
Expand All @@ -42,10 +42,15 @@ func (b *Broker) ensureSecret(u user.Info, namespace string, brokerTemplateInsta
Data: map[string][]byte{},
}

// allow empty string values for non-generated parameters.
// empty string values will be ignored for generated parameter
for k, v := range preq.Parameters {
secret.Data[k] = []byte(v)
for _, param := range template.Parameters {
if param.Name == k && ((len(v) == 0 && param.Generate == "") || len(v) != 0) {
secret.Data[k] = []byte(v)
Copy link
Copy Markdown
Contributor

@bparees bparees Oct 27, 2018

Choose a reason for hiding this comment

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

have you confirmed that setting emptystring here ultimately results in the default value for the parameter being used during instantiation?

Copy link
Copy Markdown
Author

@johnkim76 johnkim76 Oct 27, 2018

Choose a reason for hiding this comment

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

@bparees I was able to verify the following

  • if the default values were left alone, it's passed along as-is
  • if the default values were removed/deleted, the parameters were set to an empty value
  • empty values for the generated parameters were indeed generated, where it did not before.

However, I had difficulty getting any of the provisions to successfully complete with the cluster that @spadgett was nice enough to let me test with.

I'll change this PR as WIP, until I can fully verify a successful provision.

Copy link
Copy Markdown
Member

@spadgett spadgett Oct 27, 2018

Choose a reason for hiding this comment

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

However, I had difficulty getting any of the provisions to successfully complete with the cluster that @spadgett was nice enough to let me test with.

I'll change this PR as WIP, until I can fully verify a successful provision.

It looks like react-jsonschema-form leaves out string values that are cleared by the user. The empty string isn't treated as an error for required properties in JSON schema, so they switched to undefined.

rjsf-team/react-jsonschema-form#442

I'll have to look at how to handle this from the client.

Copy link
Copy Markdown
Member

@spadgett spadgett Oct 27, 2018

Choose a reason for hiding this comment

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

if the default values were removed/deleted, the parameters were set to an empty value

Just to clarify, if the parameter is missing entirely from the request, we'd still want to use the default value. We'd only want this behavior if it's explicitly set to "" (i.e., the user explicitly cleared the value in the web console form)

@bparees Does that make sense to you?

Copy link
Copy Markdown
Contributor

@bparees bparees Oct 27, 2018

Choose a reason for hiding this comment

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

yes

Copy link
Copy Markdown
Contributor

@mfojtik mfojtik Oct 29, 2018

Choose a reason for hiding this comment

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

@johnkim76 can you please add unit test to exercise this?

Copy link
Copy Markdown
Author

@johnkim76 johnkim76 Oct 29, 2018

Choose a reason for hiding this comment

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

@mfojtik will do

}
}
}

if err := util.Authorize(b.kc.Authorization().SubjectAccessReviews(), u, &authorizationv1.ResourceAttributes{
Namespace: namespace,
Verb: "create",
Expand Down Expand Up @@ -374,7 +379,7 @@ func (b *Broker) Provision(u user.Info, instanceID string, preq *api.ProvisionRe
// TODO remove this when https://github.com/kubernetes/kubernetes/issues/54940 is fixed
time.Sleep(b.gcCreateDelay)

secret, resp := b.ensureSecret(u, namespace, brokerTemplateInstance, instanceID, preq, &didWork)
secret, resp := b.ensureSecret(u, namespace, brokerTemplateInstance, instanceID, preq, template, &didWork)
if resp != nil {
return resp
}
Expand Down
162 changes: 162 additions & 0 deletions pkg/templateservicebroker/servicebroker/provision_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"reflect"
"testing"

templateapiv1 "github.com/openshift/api/template/v1"
templatev1 "github.com/openshift/api/template/v1"
faketemplatev1 "github.com/openshift/client-go/template/clientset/versioned/typed/template/v1/fake"
templatelister "github.com/openshift/client-go/template/listers/template/v1"
Expand Down Expand Up @@ -72,3 +73,164 @@ func TestProvisionConflict(t *testing.T) {
t.Errorf("got response %#v, expected 202", *resp)
}
}

func TestEnsureSecret(t *testing.T) {
fakekc := fake.NewSimpleClientset()

fakekc.PrependReactor("create", "subjectaccessreviews", func(action clienttesting.Action) (bool, runtime.Object, error) {
return true, &authorizationv1.SubjectAccessReview{Status: authorizationv1.SubjectAccessReviewStatus{Allowed: true}}, nil
})

faketemplateclient := &faketemplatev1.FakeTemplateV1{Fake: &clienttesting.Fake{}}

bti := &templateapiv1.BrokerTemplateInstance{}

b := &Broker{
kc: fakekc,
templateclient: faketemplateclient,
}

didWork := true

const (
defaultInstanceID = "12345"
defaultSourceRepoUrl = "https://github.com/testorg/testrepo"
defaultContextDir = "tools"
defaultSourceRepoRef = "v1.0.1"
defaultClusterPasswd = "abcd1234"
)

template := &templateapiv1.Template{
Parameters: []templateapiv1.Parameter{
{
Name: "SOURCE_REPOSITORY_URL",
DisplayName: "Git source URI for application",
Description: "Git Repository URL",
Value: defaultSourceRepoUrl,
Required: true,
},
{
Name: "CONTEXT_DIR",
DisplayName: "Context Directory",
Description: "Path within Git project to build; empty for root project directory.",
Value: defaultContextDir,
Required: false,
},
{
Name: "SOURCE_REPOSITORY_REF",
DisplayName: "Git Reference",
Description: "Git branch/tag reference",
Value: defaultSourceRepoRef,
Required: false,
},
{
Name: "CLUSTER_PASSWORD",
DisplayName: "Cluster Password",
Description: "Cluster Password",
Generate: "expression",
From: "[a-zA-Z0-9]{8}",
Required: false,
},
},
}

testCases := []struct {
name string
namespace string
preq *api.ProvisionRequest
expectedSourceRepoUrl string
expectedContextDir string
expectedSourceRepoDef string
expectedClusterPasswd string
}{
{
name: "provision with all default values",
namespace: "tc1",
preq: &api.ProvisionRequest{
Parameters: map[string]string{
"SOURCE_REPOSITORY_URL": defaultSourceRepoUrl,
"CONTEXT_DIR": defaultContextDir,
"SOURCE_REPOSITORY_REF": defaultSourceRepoRef,
"CLUSTER_PASSWORD": defaultClusterPasswd,
},
},
expectedSourceRepoUrl: defaultSourceRepoUrl,
expectedContextDir: defaultContextDir,
expectedSourceRepoDef: defaultSourceRepoRef,
expectedClusterPasswd: defaultClusterPasswd,
},
{
name: "provision with different CONTEXT_DIR and SOURCE_REPOSITORY_REF",
namespace: "tc2",
preq: &api.ProvisionRequest{
Parameters: map[string]string{
"SOURCE_REPOSITORY_URL": defaultSourceRepoUrl,
"CONTEXT_DIR": "/master",
"SOURCE_REPOSITORY_REF": "vbeta.0.1",
"CLUSTER_PASSWORD": defaultClusterPasswd,
},
},
expectedSourceRepoUrl: defaultSourceRepoUrl,
expectedContextDir: "/master",
expectedSourceRepoDef: "vbeta.0.1",
expectedClusterPasswd: defaultClusterPasswd,
},
{
name: "provision with no CONTEXT_DIR and SOURCE_REPOSITORY_REF",
namespace: "tc3",
preq: &api.ProvisionRequest{
Parameters: map[string]string{
"SOURCE_REPOSITORY_URL": defaultSourceRepoUrl,
"CONTEXT_DIR": "",
"SOURCE_REPOSITORY_REF": "",
"CLUSTER_PASSWORD": defaultClusterPasswd,
},
},
expectedSourceRepoUrl: defaultSourceRepoUrl,
expectedContextDir: "",
expectedSourceRepoDef: "",
expectedClusterPasswd: defaultClusterPasswd,
},
{
name: "provision with no CONTEXT_DIR, SOURCE_REPOSITORY_REF, and CLUSTER_PASSWORD",
namespace: "tc4",
preq: &api.ProvisionRequest{
Parameters: map[string]string{
"SOURCE_REPOSITORY_URL": defaultSourceRepoUrl,
"CONTEXT_DIR": "",
"SOURCE_REPOSITORY_REF": "",
"CLUSTER_PASSWORD": "",
},
},
expectedSourceRepoUrl: defaultSourceRepoUrl,
expectedContextDir: "",
expectedSourceRepoDef: "",
expectedClusterPasswd: "",
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
s, resp := b.ensureSecret(&user.DefaultInfo{}, tc.namespace, bti, defaultInstanceID, tc.preq, template, &didWork)
if resp != nil {
t.Errorf("got response %#v, expected nil", *resp)
}
if string(s.Data["SOURCE_REPOSITORY_URL"]) != tc.expectedSourceRepoUrl {
t.Errorf("SOURCE_REPOSITORY_URL param does not match! got '%s', expected '%s'",
string(s.Data["SOURCE_REPOSITORY_URL"]), tc.expectedSourceRepoUrl)
}
if string(s.Data["CONTEXT_DIR"]) != tc.expectedContextDir {
t.Errorf("CONTEXT_DIR param does not match! got '%s', expected '%s'",
string(s.Data["CONTEXT_DIR"]), tc.expectedContextDir)
}
if string(s.Data["SOURCE_REPOSITORY_REF"]) != tc.expectedSourceRepoDef {
t.Errorf("SOURCE_REPOSITORY_REF param does not match! got '%s', expected '%s'",
string(s.Data["SOURCE_REPOSITORY_REF"]), tc.expectedSourceRepoDef)
}
if string(s.Data["CLUSTER_PASSWORD"]) != tc.expectedClusterPasswd {
t.Errorf("CLUSTER_PASSWORD param does not match! got '%s', expected '%s'",
string(s.Data["CLUSTER_PASSWORD"]), tc.expectedClusterPasswd)
}
})
}
}