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

Support for OSB PR #452 to allow periods in the name field #1849

Merged
duglin merged 1 commit into from
Mar 19, 2018
Merged

Support for OSB PR #452 to allow periods in the name field #1849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 10 additions & 1 deletion pkg/apis/servicecatalog/validation/instance_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,15 @@ func TestValidateServiceInstance(t *testing.T) {
instance: validServiceInstance(),
valid: true,
},
{
name: "valid planName",
instance: func() *servicecatalog.ServiceInstance {
i := validServiceInstance()
i.Spec.ClusterServicePlanExternalName = "9651.JVHbebe"
return i
}(),
valid: true,
},
{
name: "missing namespace",
instance: func() *servicecatalog.ServiceInstance {
Expand Down Expand Up @@ -154,7 +163,7 @@ func TestValidateServiceInstance(t *testing.T) {
name: "invalid planName",
instance: func() *servicecatalog.ServiceInstance {
i := validServiceInstance()
i.Spec.ClusterServicePlanExternalName = "9651.JVHbebe"
i.Spec.ClusterServicePlanExternalName = "9651_JVHbebe"
return i
}(),
valid: false,
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/servicecatalog/validation/serviceclass.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
)

const serviceClassNameFmt string = `[-a-zA-Z0-9]+`
const serviceClassNameFmt string = `[-.a-zA-Z0-9]+`
Copy link
Copy Markdown
Contributor

@nilebox nilebox Mar 19, 2018

Choose a reason for hiding this comment

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

This change is compatible with Kubernetes names, see https://kubernetes.io/docs/concepts/overview/working-with-objects/names/

By convention, the names of Kubernetes resources should be up to maximum length of 253 characters and consist of lower case alphanumeric characters, -, and ., but certain resources have more specific restrictions.

Even though it's not critical now, it might be important in the future.

Copy link
Copy Markdown
Contributor Author

@amshuman-kr amshuman-kr Mar 19, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@nilebox Thanks for the review!

This change brings service catalog up-to-speed on the OSB PR#452 to support period ('.') characters in service brokers' service class/plan names.

This helps in integrating some existing service brokers which use the '.' characters in the names without having to the change the brokers' catalog (which might cause some migration problems).

Copy link
Copy Markdown
Contributor

@nilebox nilebox Mar 19, 2018

Choose a reason for hiding this comment

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

@amshuman-kr yeah it's all good, I just left the comment for other reviewers :)

Copy link
Copy Markdown
Contributor

@duglin duglin Mar 19, 2018

Choose a reason for hiding this comment

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

do we need to escape the . so its not "any char" ?

Copy link
Copy Markdown
Contributor Author

@amshuman-kr amshuman-kr Mar 19, 2018

Choose a reason for hiding this comment

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

. is considered a normal character inside regex character classes. So, no escaping is required.

Just to be sure, I have added a test case where _ (underscore) is detected as an invalid character.

Copy link
Copy Markdown
Contributor

@duglin duglin Mar 19, 2018

Choose a reason for hiding this comment

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

great thanks

const serviceClassNameMaxLength int = 63

var serviceClassNameRegexp = regexp.MustCompile("^" + serviceClassNameFmt + "$")
Expand Down
13 changes: 11 additions & 2 deletions pkg/apis/servicecatalog/validation/serviceclass_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ func TestValidateClusterServiceClass(t *testing.T) {
}(),
valid: true,
},
{
name: "valid serviceClass - period in externalName",
serviceClass: func() *servicecatalog.ClusterServiceClass {
s := validClusterServiceClass()
s.Spec.ExternalName = "abc.com"
return s
}(),
valid: true,
},
{
name: "invalid serviceClass - has namespace",
serviceClass: func() *servicecatalog.ClusterServiceClass {
Expand Down Expand Up @@ -114,10 +123,10 @@ func TestValidateClusterServiceClass(t *testing.T) {
valid: false,
},
{
name: "invalid serviceClass - period in externalName",
name: "invalid serviceClass - underscore in externalName",
serviceClass: func() *servicecatalog.ClusterServiceClass {
s := validClusterServiceClass()
s.Spec.ExternalName = "abc.com"
s.Spec.ExternalName = "test_serviceclass"
return s
}(),
valid: false,
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/servicecatalog/validation/serviceplan.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (
sc "github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog"
)

const servicePlanNameFmt string = `[-a-zA-Z0-9]+`
const servicePlanNameFmt string = `[-.a-zA-Z0-9]+`
const servicePlanNameMaxLength int = 63

var servicePlanNameRegexp = regexp.MustCompile("^" + servicePlanNameFmt + "$")
Expand Down
9 changes: 9 additions & 0 deletions pkg/apis/servicecatalog/validation/serviceplan_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ func TestValidateClusterServicePlan(t *testing.T) {
servicePlan: validClusterServicePlan(),
valid: true,
},
{
name: "valid servicePlan - period in externalName",
servicePlan: func() *servicecatalog.ClusterServicePlan {
s := validClusterServicePlan()
s.Spec.ExternalName = "test.plan"
return s
}(),
valid: true,
},
{
name: "missing name",
servicePlan: func() *servicecatalog.ClusterServicePlan {
Expand Down