Bump FQDN regex to allow for 25 characters #4890

SEAjamieD · 2021-03-31T05:49:26Z

🔩 Description: What code changed, and why?

Currently, our FQDN validation caps a the top level domain to 5 characters max. This was an arbitrary decision from when the validation was originally made that should not prevent us from updating. This particular branch updates that arbitrary number to 25 characters.

⛓️ Related Resources

#4891

👍 Definition of Done

When entering an FQDN, a user may now use a word longer than 5 letters

👟 How to Build and Test the Change

Navigate to https://a2-dev.test/infrastructure/chef-servers
Click "Add Chef Server"
in Name field: Enter a name of you choice
in FQDN field: enter chef-internal
in IP field, enter any valid IP i.e. 1.2.3.4
Submit the form by clicking on "Add Chef Server"
Observe a blue successful creation of server ribbon.

✅ Checklist

All PRs must tick these:

I have read the CONTRIBUTING document.
All commits signed-off for the Developer Certification of Origin.

With occasional exceptions, all PRs from Progress employees must tick these:

Is the code clear? (complicated code or lots of comments--subdivide and use well-named methods, meaningful variable names, etc.)
Consistency checked? (user notifications, user prompts, visual patterns, code patterns, variable names)
Repeated code blocks eliminated? (adapt and reuse existing components, blocks, functions, etc.)
Spelling, grammar, typos checked? (at a minimum use make spell in any component directory)
Code well-formatted? (indents, line breaks, etc. improve rather than hinder readability)

All PRs from Progress employees should tick these if appropriate:

Tests added/updated? (all new code needs new tests)
Docs added/updated? (all customer-facing changes)

Please add a note next to any checkbox above if you are NOT ticking it.

📷 Screenshots, if applicable

AFTER:

netlify · 2021-03-31T05:49:31Z

Deploy preview for chef-automate processing.

Building with commit e9e046f

https://app.netlify.com/sites/chef-automate/deploys/606795705b71e90008264869

SEAjamieD · 2021-03-31T05:51:40Z

Issue for posterity: https://github.com/chef/customer-bugs/issues/340

SEAjamieD · 2021-03-31T06:04:04Z

components/automate-ui/src/app/helpers/auth/regex.ts

@@ -23,7 +23,8 @@ export class Regex {
    NO_MIXED_WILDCARD_ALLOW_SPECIAL: '^(\\*|[^:*]+)$',

    // Allows valid FQDN only
-    VALID_FQDN: /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,5}(:[0-9]{1,5})?(\/.*)?$/,
+    // Top level domain is limited to a maximum number of 25 characters
+    VALID_FQDN: /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,25}(:[0-9]{1,5})?(\/.*)?$/,


This change from {2,5} to {2,25} allows a user to utilize a top level domain in their FQDN longer than 5 characters.

For example, previously a user was limited to something like chef.net or chef.times and would get a error if they used chef.cooking. Now, we can go as high as 25 characters. i.e. chef.itspizzanight

SEAjamieD · 2021-03-31T06:13:17Z

@kagarmoe - Is this something that needs documentation updating? I didn't find any specific numbers listed in the docs the FQDN of Adding a Chef Server, but this is also the first time this has ever come up.

Thanks!

ChefAustin · 2021-03-31T12:27:11Z

Encountering this bug and was delighted to see that a fix was already pending.

Thank you, Camp Chef!

msorens

I am going to have to "ding" you for lack of unit tests on this one. 😉
I thought there were some related to this--check earlier PRs that touched this line that might have indirect tests (e.g. validating that an infra server FQDN in a form is legal or not).
But even if there is not another section to add to, this definitely warrants its own new set of tests--mainly for documentation purposes. It will allow readers not fluent in regex to know what is and is not allowed.

SEAjamieD · 2021-04-01T16:11:40Z

I am going to have to "ding" you for lack of unit tests on this one. 😉
I thought there were some related to this--check earlier PRs that touched this line that might have indirect tests (e.g. validating that an infra server FQDN in a form is legal or not).
But even if there is not another section to add to, this definitely warrants its own new set of tests--mainly for documentation purposes. It will allow readers not fluent in regex to know what is and is not allowed.

Yeah thats fair - to be honest, at the time I made the PR I couldn't think of a good way to test this regex specifically, but now I'm realizing I should be testing behavior. So I can basically check whether or not the "create button" is active and the error is/is not showing.

.../app/modules/infra-proxy/create-chef-server-modal/create-chef-server-modal.component.spec.ts

msorens · 2021-04-01T21:28:44Z

.../app/modules/infra-proxy/create-chef-server-modal/create-chef-server-modal.component.spec.ts

+        expect(errors['pattern']).toBeTruthy();
+      });
+
+      it('when the fqdn Top Level Domain is less than 2 characters', () => {


Right idea, but you actually only have two invalid and one valid variation of FQDN and there are many more that should be tested.

I just did a quick search and found that the FQDN is already being tested here:

automate/components/automate-ui/src/app/modules/infra-proxy/chef-servers-list/chef-servers-list.component.spec.ts

Line 199 in df87097

describe('create server form validation', () => {

There are eight variations tested there--better than three, yes, but still not nearly enough. If you just look at the cyclomatic complexity implicit in the regex...

...you should appreciate there are more than 8 test cases lurking there. There are, in fact, a couple hundred just at a quick glance.
But it is tricky to get the right regex. Consider this exercise In search of the perfect URL validation regex by Mathias Bynens. Among all of those suggested regexes NONE of them got a perfect score. Only one (@diegoperini) even got a great score. And though there are a lot of good tests there that we could use, it does not have one for the case at hand in this PR!

So my suggestion. Either:
(a) use one of the regexes from Bynens chart -- @diegoperini while almost perfect, is too long (thus too unwieldy) for our needs. I would go with a shorter one that has none (or very few false negatives); less concerned about false positives.
(b) shorten our existing one (again, because it is just not that critical). Possibly like this:

^(https?:\/\/)?([a-zA-Z0-9_-]+)*\.[a-zA-Z0-9_-]{2,25}(:[0-9]{1,5})?(\/.*)?$

which renders like this, so a lot less variations to test:

Then I would suggest the using construct for testing a whole bunch of valid URLs and another using for testing a whole bunch of invalid ones. Each datum should have a test url and a name/description.
That will make a much more concise list of tests even than what is in chef-servers-list.component.spec.ts.

For the focus of this PR, valid ones should include:

http://foo.com.a, one-char suffix

http://foo.com.ab, two-char suffix

http://foo.com.1234567890123456789012345, 25-char suffix

Invalid ones should have

http://foo.com.12345678901234567890123456, 26-char suffix

http://foo.com., 0-char suffix

Other things to test, to give you a starting point:

two adjacent dots (fail)

with or without http:// (succeed)

with or without https:// (succeed)

with hyphens (succeed)

with underscores (succeed)

with digits (succeed)

with ports like 0, 23, 19999 (succeed)

with ports like a, 123456 (fail)

with final colon but no port chars following (fail)

kagarmoe · 2021-04-01T23:06:59Z

Docs change in: #4900

SEAjamieD · 2021-04-02T17:52:44Z

components/automate-ui/src/app/helpers/auth/regex.ts

@@ -23,7 +23,9 @@ export class Regex {
    NO_MIXED_WILDCARD_ALLOW_SPECIAL: '^(\\*|[^:*]+)$',

    // Allows valid FQDN only
-    VALID_FQDN: /^(http:\/\/www\.|https:\/\/www\.|http:\/\/|https:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,5}(:[0-9]{1,5})?(\/.*)?$/,
+    // Top level domain is limited to a maximum number of 25 characters
+    VALID_FQDN: /^(https?:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,25}(:[0-9]{1,5})?(\/.*)?$/,


I ended up shortening just the first capture group. When playing around with the sample you provided, I kept running into "Catastrophic Backtracing". Even though this isn't a perfect Regex (as you demonstrated is nearly impossible). Its really a pretty good one for our needs.

SEAjamieD · 2021-04-02T17:53:40Z

.../app/modules/infra-proxy/create-chef-server-modal/create-chef-server-modal.component.spec.ts

+      it('when name is missing', () => {
+        createForm.controls['id'].setValue('test');
+        createForm.controls['fqdn'].setValue('test.net');
+        createForm.controls['ip_address'].setValue('1.2.3.4');
+
+        errors = createForm.controls['name'].errors || {};
+
+        expect(createForm.valid).toBeFalsy();
+        expect(errors['required']).toBeTruthy();
+      });


Updated each of these to test when each individual input is missing.

SEAjamieD · 2021-04-02T17:54:48Z

.../app/modules/infra-proxy/create-chef-server-modal/create-chef-server-modal.component.spec.ts

+      // Many testing ideas attributed to https://mathiasbynens.be/demo/url-regex
+
+      using([
+        ['is using something other than http or https', 'httpld://www.chef.io'],
+        ['contains two periods', 'chef..internal'],
+        ['there is no TLD suffix', 'http://foo.com.'],
+        ['contains hypens in the TLD', 'chef.this-will-not'],
+        ['has a TLD that is longer than 25 characters', 'chef.thisisareallylongtldandwontwork'],
+        ['has a TLD that is shorter than 2 characters', 'chef.i'],
+        ['has numbers in the TLD', 'chef.017'],
+        ['has a port number that is too high', 'https://chef.io:987274892'],
+        ['has a colon but no port number', 'https://chef.io:'],
+        ['has a letter in the port', 'https://chef.io:123a'],
+        ['has no domain', 'http://'],
+        ['has no secure domain', 'https://'],
+        ['domain is dots', 'https://..'],
+        ['domain is hash', 'http://#'],
+        ['domain has a space', 'http:// shouldfail.net'],
+        ['contains all numbers', 'http://10.1.1.0']
+      ], function (description: string, input: string) {
+        it(('when the fqdn ' + description), () => {
+          createForm.controls['name'].setValue('test');
+          createForm.controls['id'].setValue('test');
+          createForm.controls['ip_address'].setValue('1.2.3.4');
+
+          createForm.controls['fqdn'].setValue(input);
+          errors = createForm.controls['fqdn'].errors || {};
+
+          expect(createForm.valid).toBeFalsy();
+          expect(errors['pattern']).toBeTruthy();


While I agree we could have hundreds of tests here, I feel like that is a bit overkill for us, and that these dozen or so are a good cross section.

same feelings for Valid tests below.

This change allows 25 characters off the final end of the FQDN i.e. instead of just .com they can do as much as .internal or .ourprivatesite Signed-off-by: seajamied <jdegnan@chef.io>

Signed-off-by: seajamied <jdegnan@chef.io>

msorens

Yes! 🎉

.../app/modules/infra-proxy/create-chef-server-modal/create-chef-server-modal.component.spec.ts

msorens · 2021-04-02T21:30:56Z

.../app/modules/infra-proxy/create-chef-server-modal/create-chef-server-modal.component.spec.ts

+        ['has a port number', 'https://chef.io:123'],
+        ['contains hyphens in the domain', 'new-company-who-dis.nice'],
+        ['contains underscores in the domain', 'new_company_who_dis.chef'],
+        ['contains digits in the domain', '8675309.jenny'],


😮 8675309.jenny is valid (this line) but chef.017 (L137) is not; curious...

As it sits today, this is by our design.

Currently, the TLD cannot be all numbers, so chef.017 will fail.
Conversely, a TLD of all letters should pass as long as it is between 2 and 25 characters.

looking at the Regex though, we certainly could update the TLD portion
from
(https?:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z]{2,25}(:[0-9]{1,5})?(\/.*)?$

to something like
(https?:\/\/)?[a-zA-Z0-9_]+([\-\.]{1}[a-zA-Z0-9]+)*\.[a-zA-Z change here -> 0-9 <- ]{2,25}(:[0-9]{1,5})?(\/.*)?$

Signed-off-by: seajamied <jdegnan@chef.io>

SEAjamieD self-assigned this Mar 31, 2021

SEAjamieD marked this pull request as ready for review March 31, 2021 06:00

SEAjamieD commented Mar 31, 2021

View reviewed changes

SEAjamieD added the customer-reported issues reported by customers label Mar 31, 2021

vivek-yadav self-requested a review March 31, 2021 06:56

SEAjamieD requested review from kalroy and msorens March 31, 2021 16:57

msorens suggested changes Apr 1, 2021

View reviewed changes

vivek-yadav approved these changes Apr 1, 2021

View reviewed changes

SEAjamieD requested a review from msorens April 1, 2021 18:27

msorens reviewed Apr 1, 2021

View reviewed changes

kagarmoe mentioned this pull request Apr 1, 2021

FQDN up to 25 characters #4900

Merged

9 tasks

SEAjamieD commented Apr 2, 2021

View reviewed changes

SEAjamieD requested a review from msorens April 2, 2021 17:55

seajamied added 8 commits April 2, 2021 11:54

Bump FQDN regex to allow for 25 characters

5bfa307

This change allows 25 characters off the final end of the FQDN i.e. instead of just .com they can do as much as .internal or .ourprivatesite Signed-off-by: seajamied <jdegnan@chef.io>

Add code comment about limiting TLD

7fea288

Signed-off-by: seajamied <jdegnan@chef.io>

Add unit tests for the Add Chef Server form

2ef35bc

Signed-off-by: seajamied <jdegnan@chef.io>

Remove unnecessary imports

0292edd

Signed-off-by: seajamied <jdegnan@chef.io>

Formatting correction

3d126cc

Signed-off-by: seajamied <jdegnan@chef.io>

Added more test cases for errors

4e0c31a

Signed-off-by: seajamied <jdegnan@chef.io>

Add tests for each missing input

1e4473c

Signed-off-by: seajamied <jdegnan@chef.io>

Add a bunch of different failure tests for fqdn

6e4ddb8

Signed-off-by: seajamied <jdegnan@chef.io>

seajamied added 3 commits April 2, 2021 11:54

WIP - adding more success tests

15ef518

Signed-off-by: seajamied <jdegnan@chef.io>

Shorten up fqdn regex a little big, add tests

e053a40

Signed-off-by: seajamied <jdegnan@chef.io>

More tests added

144af23

Signed-off-by: seajamied <jdegnan@chef.io>

SEAjamieD force-pushed the jamie/340-customer-bug branch from 4887e02 to 144af23 Compare April 2, 2021 18:54

msorens approved these changes Apr 2, 2021

View reviewed changes

Fix spelling error

e9e046f

Signed-off-by: seajamied <jdegnan@chef.io>

kalroy approved these changes Apr 5, 2021

View reviewed changes

kalroy merged commit cdf0365 into master Apr 5, 2021

kalroy deleted the jamie/340-customer-bug branch April 5, 2021 14:11

kalroy added the acceptance: verified label Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bump FQDN regex to allow for 25 characters #4890

Bump FQDN regex to allow for 25 characters #4890

SEAjamieD commented Mar 31, 2021 •

edited

Loading

netlify bot commented Mar 31, 2021 •

edited

Loading

SEAjamieD commented Mar 31, 2021

SEAjamieD Mar 31, 2021

SEAjamieD commented Mar 31, 2021

ChefAustin commented Mar 31, 2021

msorens left a comment

SEAjamieD commented Apr 1, 2021

msorens Apr 1, 2021

kagarmoe commented Apr 1, 2021 •

edited

Loading

SEAjamieD Apr 2, 2021

SEAjamieD Apr 2, 2021

SEAjamieD Apr 2, 2021

msorens left a comment

msorens Apr 2, 2021

SEAjamieD Apr 2, 2021

SEAjamieD Apr 2, 2021

Bump FQDN regex to allow for 25 characters #4890

Bump FQDN regex to allow for 25 characters #4890

Conversation

SEAjamieD commented Mar 31, 2021 • edited Loading

🔩 Description: What code changed, and why?

⛓️ Related Resources

👍 Definition of Done

👟 How to Build and Test the Change

✅ Checklist

📷 Screenshots, if applicable

netlify bot commented Mar 31, 2021 • edited Loading

SEAjamieD commented Mar 31, 2021

Choose a reason for hiding this comment

SEAjamieD commented Mar 31, 2021

ChefAustin commented Mar 31, 2021

msorens left a comment

Choose a reason for hiding this comment

SEAjamieD commented Apr 1, 2021

Choose a reason for hiding this comment

kagarmoe commented Apr 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msorens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SEAjamieD commented Mar 31, 2021 •

edited

Loading

netlify bot commented Mar 31, 2021 •

edited

Loading

kagarmoe commented Apr 1, 2021 •

edited

Loading