-
Notifications
You must be signed in to change notification settings - Fork 374
upcoming: [M3-9424] - Review nodebalancers validation schemas #11910
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
base: develop
Are you sure you want to change the base?
upcoming: [M3-9424] - Review nodebalancers validation schemas #11910
Conversation
…hub.com/harsh-akamai/manager into M3-9424-review-nb-vpc-validation-schema
@@ -97,6 +97,7 @@ export const vpcsValidateIP = ({ | |||
} | |||
|
|||
if (isIPv6) { | |||
// @todo update the IPv6 prefix if required for NB-VPC integration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// @todo update the IPv6 prefix if required for NB-VPC integration | |
// @TODO NB-VPC: update the IPv6 prefix if required for NB-VPC integration |
ipv6_range: string() //TBD | ||
.notRequired() | ||
.nullable() | ||
.matches(PRIVATE_IP_REGEX, 'Must be a valid private IPv6 address.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IPv6 has a different format than IPv4, so an IPv6-specific regex will need to be used and we should revert the name change to the existing regex variable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected!
.required('Subnet ID is required.'), | ||
ipv4_range: string() | ||
.typeError('IPv4 address is required.') | ||
.required('IPv4 address is required.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the case? I would imagine it could have either an IPv4 range or IPv6 range, or both, but not neither. Assuming what I described is accurate, we could use similar logic as what was done for createSubnetSchemaWithIPv6
in vpcs.schema.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds right. I've made changes to adapt to the logic present in createSubnetSchemaWithIPv6
Can we also get a |
…dpoints in /v4/nodebalancers (& /v4beta/nodebalancers) for NB-VPC Integration
|
||
const PORT_WARNING = 'Port must be between 1 and 65535.'; | ||
const LABEL_WARNING = 'Label must be between 3 and 32 characters.'; | ||
|
||
export const PRIVATE_IPv4_REGEX = /^10\.|^172\.1[6-9]\.|^172\.2[0-9]\.|^172\.3[0-1]\.|^192\.168\.|^fd/; | ||
export const PRIVATE_IPv6_REGEX = /^(fc|fd)\./; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this regex will match valid IPv6 addresses. We should test the regex expressions with a tool like regex101.com to ensure it works as intended. We'll also want it to account for the mask since it's an IPv6 range
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noted. I've made changes to the regex and I've also added a comment. Thanks for catching this 🙌
then: (schema) => | ||
schema | ||
.required(IP_EITHER_BOTH_NOT_NEITHER) | ||
.matches(PRIVATE_IPv4_REGEX, 'Must be a valid private IPv4 address.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's store the 'Must be a valid private IPv4 address.'
string in a constant since we use it several times
const createNodeBalancerVPCsSchema = object({ | ||
subnet_id: number() | ||
.typeError('Subnet ID must be a number.') | ||
.required('Subnet ID is required.'), | ||
ipv4_range: string().when('ipv6_range', { | ||
is: (value: unknown) => | ||
value === '' || value === null || value === undefined, | ||
then: (schema) => | ||
schema | ||
.required(IP_EITHER_BOTH_NOT_NEITHER) | ||
.matches(PRIVATE_IPv4_REGEX, 'Must be a valid private IPv4 address.') | ||
.test({ | ||
name: 'IPv4 CIDR format', | ||
message: 'The IPv4 range must be in CIDR format.', | ||
test: (value) => | ||
vpcsValidateIP({ | ||
value, | ||
shouldHaveIPMask: true, | ||
mustBeIPMask: false, | ||
}), | ||
}), | ||
otherwise: (schema) => | ||
lazy((value: string | undefined) => { | ||
switch (typeof value) { | ||
case 'undefined': | ||
return schema.notRequired().nullable(); | ||
|
||
case 'string': | ||
return schema | ||
.notRequired() | ||
.matches( | ||
PRIVATE_IPv4_REGEX, | ||
'Must be a valid private IPv4 address.' | ||
) | ||
.test({ | ||
name: 'IPv4 CIDR format', | ||
message: 'The IPv4 range must be in CIDR format.', | ||
test: (value) => | ||
vpcsValidateIP({ | ||
value, | ||
shouldHaveIPMask: true, | ||
mustBeIPMask: false, | ||
}), | ||
}); | ||
|
||
default: | ||
return schema.notRequired().nullable(); | ||
} | ||
}), | ||
}), | ||
ipv6_range: string().when('ipv6_range', { | ||
is: (value: unknown) => | ||
value === '' || value === null || value === undefined, | ||
then: (schema) => | ||
schema | ||
.required(IP_EITHER_BOTH_NOT_NEITHER) | ||
.matches(PRIVATE_IPv6_REGEX, 'Must be a valid private IPv6 address.') | ||
.test({ | ||
name: 'valid-ipv6-range', | ||
message: 'Must be a valid IPv6 range, e.g. 2001:db8:abcd:0012::0/64.', | ||
test: (value) => | ||
vpcsValidateIP({ | ||
value, | ||
shouldHaveIPMask: true, | ||
mustBeIPMask: false, | ||
}), | ||
}), | ||
}), | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now, Cloud Manager doesn't load and we get the following console error:
const createNodeBalancerVPCsSchema = object({ | |
subnet_id: number() | |
.typeError('Subnet ID must be a number.') | |
.required('Subnet ID is required.'), | |
ipv4_range: string().when('ipv6_range', { | |
is: (value: unknown) => | |
value === '' || value === null || value === undefined, | |
then: (schema) => | |
schema | |
.required(IP_EITHER_BOTH_NOT_NEITHER) | |
.matches(PRIVATE_IPv4_REGEX, 'Must be a valid private IPv4 address.') | |
.test({ | |
name: 'IPv4 CIDR format', | |
message: 'The IPv4 range must be in CIDR format.', | |
test: (value) => | |
vpcsValidateIP({ | |
value, | |
shouldHaveIPMask: true, | |
mustBeIPMask: false, | |
}), | |
}), | |
otherwise: (schema) => | |
lazy((value: string | undefined) => { | |
switch (typeof value) { | |
case 'undefined': | |
return schema.notRequired().nullable(); | |
case 'string': | |
return schema | |
.notRequired() | |
.matches( | |
PRIVATE_IPv4_REGEX, | |
'Must be a valid private IPv4 address.' | |
) | |
.test({ | |
name: 'IPv4 CIDR format', | |
message: 'The IPv4 range must be in CIDR format.', | |
test: (value) => | |
vpcsValidateIP({ | |
value, | |
shouldHaveIPMask: true, | |
mustBeIPMask: false, | |
}), | |
}); | |
default: | |
return schema.notRequired().nullable(); | |
} | |
}), | |
}), | |
ipv6_range: string().when('ipv6_range', { | |
is: (value: unknown) => | |
value === '' || value === null || value === undefined, | |
then: (schema) => | |
schema | |
.required(IP_EITHER_BOTH_NOT_NEITHER) | |
.matches(PRIVATE_IPv6_REGEX, 'Must be a valid private IPv6 address.') | |
.test({ | |
name: 'valid-ipv6-range', | |
message: 'Must be a valid IPv6 range, e.g. 2001:db8:abcd:0012::0/64.', | |
test: (value) => | |
vpcsValidateIP({ | |
value, | |
shouldHaveIPMask: true, | |
mustBeIPMask: false, | |
}), | |
}), | |
}), | |
}); | |
const createNodeBalancerVPCsSchema = object().shape( | |
{ | |
subnet_id: number() | |
.typeError('Subnet ID must be a number.') | |
.required('Subnet ID is required.'), | |
ipv4_range: string().when('ipv6_range', { | |
is: (value: unknown) => | |
value === '' || value === null || value === undefined, | |
then: (schema) => | |
schema | |
.required(IP_EITHER_BOTH_NOT_NEITHER) | |
.matches(PRIVATE_IPv4_REGEX, 'Must be a valid private IPv4 address.') | |
.test({ | |
name: 'IPv4 CIDR format', | |
message: 'The IPv4 range must be in CIDR format.', | |
test: (value) => | |
vpcsValidateIP({ | |
value, | |
shouldHaveIPMask: true, | |
mustBeIPMask: false, | |
}), | |
}), | |
otherwise: (schema) => | |
lazy((value: string | undefined) => { | |
switch (typeof value) { | |
case 'undefined': | |
return schema.notRequired().nullable(); | |
case 'string': | |
return schema | |
.notRequired() | |
.matches( | |
PRIVATE_IPv4_REGEX, | |
'Must be a valid private IPv4 address.' | |
) | |
.test({ | |
name: 'IPv4 CIDR format', | |
message: 'The IPv4 range must be in CIDR format.', | |
test: (value) => | |
vpcsValidateIP({ | |
value, | |
shouldHaveIPMask: true, | |
mustBeIPMask: false, | |
}), | |
}); | |
default: | |
return schema.notRequired().nullable(); | |
} | |
}), | |
}), | |
ipv6_range: string().when('ipv4_range', { | |
is: (value: unknown) => | |
value === '' || value === null || value === undefined, | |
then: (schema) => | |
schema | |
.required(IP_EITHER_BOTH_NOT_NEITHER) | |
.matches(PRIVATE_IPv6_REGEX, 'Must be a valid private IPv6 address.') | |
.test({ | |
name: 'valid-ipv6-range', | |
message: | |
'Must be a valid IPv6 range, e.g. 2001:db8:abcd:0012::0/64.', | |
test: (value) => | |
vpcsValidateIP({ | |
value, | |
shouldHaveIPMask: true, | |
mustBeIPMask: false, | |
}), | |
}), | |
}), | |
}, | |
[ | |
['ipv4_range', 'ipv6_range'], | |
['ipv6_range', 'ipv4_range'], | |
] | |
); |
The array at the end prevents the cyclic dependency error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harsh-akamai LGTM! Can you resolve the ESLint warning.
@@ -1,9 +1,12 @@ | |||
import { array, boolean, mixed, number, object, string } from 'yup'; | |||
import { array, boolean, lazy, mixed, number, object, string } from 'yup'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harsh-akamai Can you look into this ESLint warnings.
211feb4
to
291f89d
Compare
…424-review-nb-vpc-validation-schema
.test( | ||
'unique subnet IDs', | ||
'Subnet IDs must be unique.', | ||
function (value?: any[] | null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to include a type annotation as yup
will infer one for you!
function (value?: any[] | null) { | |
function (value) { |
Cloud Manager UI test results🔺 1 failing test on test run #11 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/objectStorageMulticluster/bucket-details-multicluster.spec.ts" |
Description 📝
Update validation schemas for the changes in POST endpoints in /v4/nodebalancers (& /v4beta/nodebalancers) for NB-VPC Integration
How to test 🧪
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 Doing a self review
❔ Our contribution guidelines
🤏 Splitting feature into small PRs
➕ Adding a changeset
🧪 Providing/improving test coverage
🔐 Removing all sensitive information from the code and PR description
🚩 Using a feature flag to protect the release
👣 Providing comprehensive reproduction steps
📑 Providing or updating our documentation
🕛 Scheduling a pair reviewing session
📱 Providing mobile support
♿ Providing accessibility support
As an Author, before moving this PR from Draft to Open, I confirmed ✅