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.

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

Jump to bottom

add k8s compliant validation for label #2414

Closed
wants to merge 1 commit into from
Closed

add k8s compliant validation for label #2414

wants to merge 1 commit into from

Conversation

mjudeikis
Copy link

@mjudeikis mjudeikis commented Nov 1, 2017 •
edited
Loading

If I have storageclass like this:

kind: StorageClass
apiVersion: storage.k8s.io/v1beta1
metadata:
  name: fast
provisioner: kubernetes.io/mystorageprvider
parameters:
  pool: default
  description: Kubernetes volume
  fsType: ext4
  adminSecretNamespace: default
  adminSecretName: mystorageprvider-secret

I want to use UI and create storage claim and provide label selector. As per K8S docs labels can have . and / as keys (https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/)

But our validation does not allow this from UI:
image

We want this to be valid:
image

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 1, 2017
@mjudeikis mjudeikis added the kind/bug Categorizes issue or PR as related to a bug. label Nov 1, 2017
spadgett
spadgett requested changes Nov 1, 2017
View reviewed changes
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Thanks for pointing out this problem and submitting a fix.

app/views/directives/osc-persistent-volume-claim.html
@@ -221,8 +221,8 @@
entries="claim.selectedLabels"
key-placeholder="label"
value-placeholder="value"
key-validator="[a-zA-Z][a-zA-Z0-9_-]*"
key-validator-error-tooltip="A valid label name is an alphanumeric (a-z and 0-9) string beginning with a letter that may contain underscores and dashes."
key-validator="[a-zA-Z][a-zA-Z0-9_.-\/]*"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the label validation is more complicated than this, and the original regex on this page was incorrectly copied from an env variables editor. See

https://github.com/spadgett/origin-web-console/blob/57ae92d906931b0f60b6fa73833c9dc800f26fca/app/scripts/directives/labels.js#L38-L41

Can you try to use the label-editor directive here instead? Here's an example:

https://github.com/spadgett/origin-web-console/blob/57ae92d906931b0f60b6fa73833c9dc800f26fca/app/views/create-route.html#L21-L26

Copy link
Author

Choose a reason for hiding this comment

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

But this one still does not do "/" - like kubernetes.io/test ?

Copy link
Author

Choose a reason for hiding this comment

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

Do we want to update label regexp too?

Copy link
Member

Choose a reason for hiding this comment

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

It should split on / before testing the regex if you look further down.

https://github.com/spadgett/origin-web-console/blob/57ae92d906931b0f60b6fa73833c9dc800f26fca/app/scripts/directives/labels.js#L75

Copy link
Author

Choose a reason for hiding this comment

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

k, let me check what I can do :)

Copy link
Author

Choose a reason for hiding this comment

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

k, having some trouble to use label-editor directive... there is some styles clashing, which makes directive not visible (my assumption)... my UI skills are rusty :/

Copy link
Author

Choose a reason for hiding this comment

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

when it comes to directives and angularjs - I have to say - pass :/

Copy link
Member

Choose a reason for hiding this comment

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

@mjudeikis I hope you don't mind: I went ahead and opened #2423 to use the label-editor

Copy link
Author

Choose a reason for hiding this comment

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

all good :) Thanks

@spadgett spadgett mentioned this pull request Nov 1, 2017
Merged
@mjudeikis mjudeikis closed this Nov 1, 2017
openshift-merge-robot added a commit that referenced this pull request Nov 1, 2017
@openshift-merge-robot
Merge pull request #2423 from spadgett/fix-label-regex
b5efba1 
Automatic merge from submit-queue.

Use label-editor for PVC labels

Fixes a problem where the regex used for validating label names is
wrong. Names with a `/` were incorrectly blocked.

See discussion in #2414

![openshift web console 2017-11-01 12-12-24](https://user-images.githubusercontent.com/1167259/32284599-1f5aa9fe-befe-11e7-8f97-f8f64d25579f.png)

![openshift web console 2017-11-01 12-19-48](https://user-images.githubusercontent.com/1167259/32284866-faafbb2a-befe-11e7-9508-68be77e4f191.png)

/kind bug
/assign @jwforres 
cc @mjudeikis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spadgett spadgett spadgett requested changes

Assignees
No one assigned
Labels
kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mjudeikis @spadgett @openshift-ci-robot