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.

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

Allow underscores to be used in template-name #214

Closed
wants to merge 1 commit into from

Conversation

zxiiro
Copy link
Contributor

@zxiiro zxiiro commented Aug 20, 2018

The underscore _ character used to be a valid character. The regex
should allow the use of this character.

@tykeal
Copy link

tykeal commented Aug 20, 2018

This looks good to me. I'll note that we use the '_' in several of our template names so dropping it breaks us.

@askb
Copy link

askb commented Aug 20, 2018

LGTM

The underscore _ character used to be a valid character. The regex
should allow the use of this character.

Signed-off-by: Thanh Ha <zxiiro@linux.com>
@olivergondza
Copy link
Member

The original code comes from https://github.com/jclouds/jclouds/blob/master/core/src/main/java/org/jclouds/predicates/validators/DnsNameValidator.java.

As you have noticed, the validator was essentially inefficient until fixed recently so that might be the reason you ware permitted to put underscores there. Frankly, I have no idea where did I get the original regex but it feels wrong at least in accepting uppercase letters. I have not studied the internet hostname specification at length, but it appears hostnames (template name is used as part of the hostname) must not contain uppercase letters or underscores.

From the pragmatic standpoint, this is what we ask openstack to name the server and apparently it does the necessary validation/correction before using it as OS hostname. If it can translate the server name into a valid hostname, then I do not think we have to worry about validity of the template name all that much.

I have just tested OpenStack accepts insanities like /*[$]*/ in server name and actually uses it for resource name while linux hostname is purged of all disallowed characters. Would you be ok to relax the restrictions to https://github.com/jenkinsci/jenkins/blob/c926d5b79f4bdd8b9e744035473a20f5ceb4926c/core/src/main/java/jenkins/model/Jenkins.java#L3894-L3919 (the template name can be used as url chunk so better not have it completely unrestricted)?

@zxiiro
Copy link
Contributor Author

zxiiro commented Aug 21, 2018

@olivergondza I hadn't thought about the DNS issue. I tend to agree with you that if template-name is used as hostname then what we pass in as input should match output in openstack. Perhaps I can flip this patch to restrict the upper case characters?

@olivergondza
Copy link
Member

@zxiiro, that is what used to be a concern back in jclouds days, though I question if that still is a concern today. As I said, based on my testing, OpenStack does the right thing for us. My suggestion is to make the name sane for us and rely on OpenStack sanitation until we have a report that it does not work in certain cases and then and only then handle those cases. At least, we would know why we are restricting something...

@zxiiro
Copy link
Contributor Author

zxiiro commented Aug 21, 2018

@olivergondza that's fine with me. In this case I'll just close this PR as it's not necessarily a problem and my team can resolve it on our side as a policy.

@zxiiro zxiiro closed this Aug 21, 2018
@zxiiro
Copy link
Contributor Author

zxiiro commented Aug 21, 2018

Ah I wasn't thinking about the underscore patch in my previous reply. @olivergondza feel free to reopen and accept the patch if you want to allow underscores.

I am fine with whatever you choose as long as it doesn't break on the backend. I can confirm that underscores (_) are translated to dashes (-) by openstack in the backend so it should be safe to use.

@olivergondza
Copy link
Member

Fixed in f043930. Thanks for pointing this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants