Allow for a period in the GUID of the External ID#1034
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
| var validateServiceClassName = apivalidation.NameIsDNSSubdomain | ||
|
|
||
| const guidFmt string = "[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?" | ||
| const guidFmt string = "[a-zA-Z0-9]([-a-zA-Z0-9\.]*[a-zA-Z0-9])?" |
There was a problem hiding this comment.
You should add a unit test case for this in serviceclass_test.go
There was a problem hiding this comment.
Also, the escape sequence isn't valid - you need to escape the backslash :)
There was a problem hiding this comment.
ok, will do.
add unit test case
emaildanwilson
force-pushed
the
allowForPeriodInGUID
branch
from
70d0c4e to
034981e
Compare
|
is this sufficient? I don't think we should have to wait for the next broker to break us - can't we just make it a string w/o any constraints? The spec does not require it to have any special constraints aside from being non-empty so we shouldn't add new ones w/o a very big reason. |
|
I added a test case and actually ran them locally this time 😄. It turns out that the period is evaluated as a literal inside the square brackets so it didn't need the backslash at all. doh. |
| var validateServiceClassName = apivalidation.NameIsDNSSubdomain | ||
|
|
||
| const guidFmt string = "[a-zA-Z0-9]([-a-zA-Z0-9]*[a-zA-Z0-9])?" | ||
| const guidFmt string = "[a-zA-Z0-9]([-a-zA-Z0-9.]*[a-zA-Z0-9])?" |
There was a problem hiding this comment.
let's drop all checks except for non-empty string.
There was a problem hiding this comment.
I'm ok with changing that. @pmorie are you ok with this too?
There was a problem hiding this comment.
I would prefer not to drop them all, or we will never get a clear picture of what kind of stuff people return in this field.
There was a problem hiding this comment.
why do we care since we don't do anything with this data except echo it back to the broker?
|
@duglin I was going for the least impactful change. If removing the check is preferred I'm ok with that. Are there any other downstream risks with allowing for all special characters? |
|
I don't think there are (but yea we should check), because these IDs are not used by us or even really exposed to our users (they're just visible, not used by them) - we just use them when we talk to the brokers. We may need to make sure we escape them properly when we encode them in URLs tho. |
|
I would prefer to loosen the validation on a case-by-case basis so that we can gather datapoints for the OSB WG. Ideally I would like to have the API spec to contain a validation regex |
|
@pmorie the OSB API spec just says its a non-empty string, not other requirement. It only "recommends" a GUID. |
|
@duglin okay, does that mean I can send emoji? I feel like this field is in practice conformant to some regex, and 'non-empty string' is way too permissive to relax to. Remember that loosening a validation is OK, tightening is not. Hence my interest in gradually relaxing the regex. |
|
yes an emoji is ok ;-) Ultimately, I think our hands are tied as the original issue proved - people are already treating it as a string, the spec says its a string so its a string. If we want to tighten it then its not an issue for svc-cat, its an issue for osb api and we'd need to change it there first. As I said earlier, the last thing I want to have happen is for us to have the svc-cat fail for a customer because they tried to use a broker that's spec compliant but we reject their IDs unnecessarily. Or put another way... IMO, svc-cat isn't spec compliant if we tighten it. |
5 participants
The hashicorp/cf-vault-service-broker appends the plan name onto the GUID for the External ID using a period as the separator. The existing regEx validation does not allow for periods. The Pivotal docs do not specify an exact format and simply state
using a GUID is recommended.This change relaxes the regEx check to allow for periods in the External ID so this and other brokers that use this style will work for k8s.