Update provider name field's validation rules to align with k8s rules #713

sgratch · 2023-09-03T18:12:04Z

Fixes: #712

The current UI validation rules for the Provider resource name field are not aligned with Kubernetes DNS Subdomain Name standards:

(https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names)

The '.' character should be allowed
Allow starting with an alphanumeric character.

This patch fixes the validation rules for this field.

yaacov · 2023-09-04T05:10:03Z

can you separate this PR to:
a. PR that fix the validation method in common
b. PR that enhance the help strings
?

yaacov · 2023-09-04T05:27:36Z

packages/forklift-console-plugin/src/modules/Providers/utils/validators/common.ts

+const DNS_LABEL_NAME_REGEXP = '[a-z0-9]([-a-z0-9]*[a-z0-9])?';
+const DNS_SUBDOMAINS_NAME_REGEXP = new RegExp(
+  `^(${DNS_LABEL_NAME_REGEXP}([.]${DNS_LABEL_NAME_REGEXP})*)$`,
+);


Suggested change

const DNS_LABEL_NAME_REGEXP = '[a-z0-9]([-a-z0-9]*[a-z0-9])?';

const DNS_SUBDOMAINS_NAME_REGEXP = new RegExp(

`^(${DNS_LABEL_NAME_REGEXP}([.]${DNS_LABEL_NAME_REGEXP})*)$`,

);

const DNS_SUBDOMAINS_NAME_REGEXP = /^[a-z0-9][a-z0-9-.]{0,251}[a-z0-9]$/;

this changes:
a. add numeric char as valid first char
b. add [.] as valid char in the middle of the name.

problems with current solution:
a. removes the length check, ( so it is required it in the validation method )
b. allow the last char to be . or - , the documentation say "end with an alphanumeric character
"

note: this change in the regexp requires removal of the length check in the method below because it is now done in the regexp, like we do in the rest of the regexp in this file.

Few comments:

The current solution doesn't allow the last char to be '.' or '-'. Why do you think it is?

The difference between your suggested solution to current is that the following examples are invalid in current and valid in suggested:
"a..b" (2 sequential dots)
"a.-b" ("-" following ".")
"a-.b" ("." following "-")

The current code that I used was copied from backend forklift validation code and is also relevant to other k8s entities, for example Pod name.
So even though the k8s doc doesn't mention that 2 sequential dots are invalid, it's verified for other entities and used in backend, I guess for validating domain names.
I didn't mention that in our validation text message since it's detailed and not mentioned in k8s validation text message as well. But can be added if you think otherwise.

The current code that I used was copied from backend forklift validation code and is also relevant to other k8s entities, for example Pod name.

you should follow RFC 1123

the fix I suggest will not block users from entering correct names, but you are correct it can be more restrictive (not sure it's a good thing ... ), here is a more restrictive option:

Suggested change

const DNS_LABEL_NAME_REGEXP = '[a-z0-9]([-a-z0-9]*[a-z0-9])?';

const DNS_SUBDOMAINS_NAME_REGEXP = new RegExp(

`^(${DNS_LABEL_NAME_REGEXP}([.]${DNS_LABEL_NAME_REGEXP})*)$`,

);

const DNS_SUBDOMAINS_NAME_REGEXP = /^(?=.{1,253}$)[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$/;

this regexp is more complex, but does the job, i don't see the benefit of separating it to label name and subdomain name.

I'm ok with the simple (less restrictive) regexp, and more restrictive options too, both will not block users from entring valid names, one is simple but will allow invalid names, the other more restrictive but also more complex.

a comment in the code with refernce to RFC 1123 or link to k8s documentation will be nice touch

The current code that I used was copied from backend forklift validation

I can't find the reference code in forklift, can you add a link to the reference implementation in forklift ?

i don't see the benefit of separating it to label name and subdomain name.

If there will be no need to validate DNS-LABEL based names then ok. I thought it's more flexible to use the SUBDOMAIN based on the LABEL format, as done on k8s api validation, but sure. We can always add it later on if needed.

can't find the reference code in forklift, can you add a link to the reference implementation in forklift ?

The API validation code for DNS-SUBDOMAIN names (e.g. pod name) is: https://github.com/kubernetes/apimachinery/blob/0d057e543013c79e20abb000df19bc16d286b777/pkg/util/validation/validation.go#L205

In Forklift we mainly validate based on the DNS-LABEL format for the VM name (e.g. https://github.com/kubev2v/forklift/blob/a1671edf995a93d728c50e90cccff007a6fec3fc/pkg/controller/plan/validation.go#L398)
No validation for the provider name, but since other k8s entities names uses the DNS-SUBDOMAIN validation then I thought we should do the same.

If there will be no need to validate DNS-LABEL based names then ok. I thought it's more flexible to use the SUBDOMAIN based on the LABEL format, as done on k8s api validation, but sure. We can always add it later on if needed.

Nice 👍

The API validation code for DNS-SUBDOMAIN names (e.g. pod name) is: https://github.com/kubernetes/apimachinery/blob/0d057e543013c79e20abb000df19bc16d286b777/pkg/util/validation/validation.go#L205

Greate, we can add a link as remark in our code 🔥

If there will be no need to validate DNS-LABEL based names then ok.

Yes, they have more specific needs, we only need to be more or less accurate, the "real" validation will happen when the API call is made, and then we will show the msg we get.

Lets do something simple (restrictive or less restrictive ) in our end, we can trust the back-end to do the full validation.

yaacov · 2023-09-04T05:28:21Z

packages/forklift-console-plugin/src/modules/Providers/utils/validators/common.ts

@@ -74,7 +77,7 @@ export function validateFingerprint(fingerprint: string) {
 }

 export function validateK8sName(k8sName: string) {
-  return DNS_SUBDOMAINS_NAME_REGEXP.test(k8sName);
+  return DNS_SUBDOMAINS_NAME_REGEXP.test(k8sName) && k8sName?.length <= 253;


Suggested change

return DNS_SUBDOMAINS_NAME_REGEXP.test(k8sName) && k8sName?.length <= 253;

return DNS_SUBDOMAINS_NAME_REGEXP.test(k8sName);

the change in https://github.com/kubev2v/forklift-console-plugin/pull/713/files#r1314450788 make it unnecessary to check the length here

yaacov · 2023-09-04T05:30:37Z

...ages/forklift-console-plugin/src/modules/Providers/utils/validators/__tests__/common.test.ts

    });

    it('invalidates k8s names with invalid characters', () => {
      expect(validateK8sName('k8s_name')).toBe(false);
-      expect(validateK8sName('k8s.name')).toBe(false);
+      expect(validateK8sName('k8s%name')).toBe(false);


Suggested change

expect(validateK8sName('k8s%name')).toBe(false);

expect(validateK8sName('k8s%name')).toBe(false);

expect(validateK8sName('k8sname.')).toBe(false);

expect(validateK8sName('.k8sname')).toBe(false);

ahadas · 2023-09-04T09:11:11Z

packages/forklift-console-plugin/locales/en/plugin__forklift-console-plugin.json

@@ -97,7 +97,7 @@
  "Error: CA Certificate must be valid.": "Error: CA Certificate must be valid.",
  "Error: Fingerprint is required and must be valid.": "Error: Fingerprint is required and must be valid.",
  "Error: Insecure Skip Verify must be a boolean value.": "Error: Insecure Skip Verify must be a boolean value.",
-  "Error: Name is required and must be a unique and valid Kubernetes name.": "Error: Name is required and must be a unique and valid Kubernetes name.",
+  "Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e. must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).": "Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e. must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",


Suggested change

"Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e. must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).": "Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e. must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",

"Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e., must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).": "Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e., must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",

ahadas · 2023-09-04T09:13:20Z

...forklift-console-plugin/src/modules/Providers/views/create/components/ProviderCreateForm.tsx

@@ -106,7 +106,7 @@ export const ProvidersCreateForm: React.FC<ProvidersCreateFormProps> = ({
              helperText={t('Unique Kubernetes resource name identifier')}
              validated={state.validation.name}
              helperTextInvalid={t(
-                'Error: Name is required and must be a unique and valid Kubernetes name.',
+                "Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e. must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",


Suggested change

"Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e. must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",

"Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e., must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",

sgratch · 2023-09-05T08:28:21Z

can you separate this PR to: a. PR that fix the validation method in common b. PR that enhance the help strings ?

@yaacov sure, but what's the purpose of separating if this fix won't be part of 2.5?

yaacov · 2023-09-05T09:02:17Z

what's the purpose of separating if this fix won't be part of 2.5?

fixing RFC 952 to RFC 1123 (#712) is a bug in the way we do validations, we need it tracked and verified

fixing the tooltips (#646) is about help texts, this can change and update as we know more about the fields and what users know and don't know when the use the form.

The current UI validation rules for the Provider resource name value are not aligned with Kubernetes DNS Subdomain Name standards: (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-subdomain-names) - The '.' character should be allowed - Allow starting with an alphanumeric character. Signed-off-by: Sharon Gratch <[email protected]>

sonarqubecloud · 2023-09-05T15:14:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Update the validation error message for the 'Provider resource name' field to align with k8s rules. This is following the validation fix: kubev2v#713 Signed-off-by: Sharon Gratch <[email protected]>

sgratch requested review from yaacov and rszwajko September 3, 2023 18:24

yaacov added the bug Categorizes issue or PR as related to a bug. label Sep 4, 2023

yaacov added this to the 2.5.0 milestone Sep 4, 2023

yaacov reviewed Sep 4, 2023

View reviewed changes

ahadas reviewed Sep 4, 2023

View reviewed changes

ahadas modified the milestones: 2.5.0, 2.5.1 Sep 5, 2023

sgratch force-pushed the fix-provider-name-validation branch from 8b5f966 to 40ac479 Compare September 5, 2023 14:06

sgratch changed the title ~~Update provider name field's validation rules and validation text to align with k8s rules~~ Update provider name field's validation rules to align with k8s rules Sep 5, 2023

sgratch requested a review from yaacov September 5, 2023 15:02

sgratch force-pushed the fix-provider-name-validation branch from 40ac479 to 8151241 Compare September 5, 2023 15:13

yaacov approved these changes Sep 5, 2023

View reviewed changes

yaacov merged commit 3f6a515 into kubev2v:main Sep 5, 2023

sgratch mentioned this pull request Sep 7, 2023

Update validation text msg for the provider name field #716

Merged

sgratch deleted the fix-provider-name-validation branch September 7, 2023 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update provider name field's validation rules to align with k8s rules #713

Update provider name field's validation rules to align with k8s rules #713

sgratch commented Sep 3, 2023 •

edited

Loading

yaacov commented Sep 4, 2023

yaacov Sep 4, 2023

sgratch Sep 5, 2023 •

edited

Loading

yaacov Sep 5, 2023

yaacov Sep 5, 2023

yaacov Sep 5, 2023

sgratch Sep 5, 2023

yaacov Sep 5, 2023

yaacov Sep 4, 2023

yaacov Sep 4, 2023

ahadas Sep 4, 2023

ahadas Sep 4, 2023

sgratch commented Sep 5, 2023

yaacov commented Sep 5, 2023

sonarqubecloud bot commented Sep 5, 2023

	return DNS_SUBDOMAINS_NAME_REGEXP.test(k8sName) && k8sName?.length <= 253;
	return DNS_SUBDOMAINS_NAME_REGEXP.test(k8sName);

	"Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e. must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",
	"Error: Name is required and must be a unique within a namespace and valid Kubernetes name (i.e., must contain no more than 253 characters, consists of lower case alphanumeric characters , '-' or '.' and starts and ends with an alphanumeric character).",

Update provider name field's validation rules to align with k8s rules #713

Update provider name field's validation rules to align with k8s rules #713

Conversation

sgratch commented Sep 3, 2023 • edited Loading

yaacov commented Sep 4, 2023

Choose a reason for hiding this comment

sgratch Sep 5, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgratch commented Sep 5, 2023

yaacov commented Sep 5, 2023

sonarqubecloud bot commented Sep 5, 2023

sgratch commented Sep 3, 2023 •

edited

Loading

sgratch Sep 5, 2023 •

edited

Loading