fix: Regex for network, subnetwork, CIDR block and Workload Pool in standard and autopilot cluster #2479
Conversation
Daisyprakash
requested review from
a team,
apeabody and
ericyz
as code owners
Summary of ChangesHello @Daisyprakash, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of GKE cluster deployments by introducing client-side input validation for key configuration parameters. By enforcing strict formatting rules for network names, subnetworks, CIDR blocks, and workload pools, it aims to prevent common misconfigurations, streamline the deployment process, and improve the overall reliability and user experience when provisioning GKE clusters. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds regex validation for several input fields across both the GKE autopilot and standard cluster modules, which is a great enhancement for ensuring valid user input. My review focuses on the correctness and accuracy of the added regular expressions. I've found a critical typo in the CIDR validation regex that would break IPv6 validation, and I've also identified that the regex for network and subnetwork names is overly restrictive compared to GCP's naming conventions. Please see the detailed comments for suggestions on how to fix these issues.
|
/gcbrun |
apeabody
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds regex validations for several configuration fields in both standard and autopilot GKE cluster modules. The intent is good, as it improves input validation. However, I've found several issues with the implemented regular expressions:
- The regex for network and subnetwork names incorrectly restricts the maximum length.
- The regex for the workload pool is too permissive and doesn't properly validate the project ID format.
- The complex regex for CIDR blocks has inconsistencies in handling whitespace for IPv4 and IPv6 notations.
I've provided suggestions to correct these issues. These changes are important to ensure that valid configurations are accepted and invalid ones are rejected correctly.
| regexValidation: ^[a-z]([a-z0-9-]{0,60}[a-z0-9])?$ | ||
| validation: Network name must start with a lowercase letter followed by up to 62 lowercase letters, numbers, or hyphens and cannot end with a hyphen. |
There was a problem hiding this comment.
The regex for the subnetwork name is incorrect. Similar to network names, GCP subnetwork names can be up to 63 characters long, but this regex restricts them to 62. The validation message is also confusing. I've suggested a corrected regex and a clearer validation message.
regexValidation: ^[a-z]([a-z0-9-]{0,61}[a-z0-9])?$
validation: Must be 1-63 characters, start with a lowercase letter, contain only lowercase letters, numbers, or hyphens, and cannot end with a hyphen.| regexValidation: ^.+\.svc\.id\.goog$ | ||
| validation: Workload pool must be in the format <project_id>.svc.id.goog. |
There was a problem hiding this comment.
The regex for the workload pool is too permissive. It allows any characters for the project ID part. The format for a project ID is more restrictive (6-30 characters, starting with a letter, no trailing hyphen). I've provided a stricter regex that correctly validates the project ID format within the workload pool string.
regexValidation: ^[a-z]([-a-z0-9]{4,28}[a-z0-9])\.svc\.id\.goog$
validation: Workload pool must be in the format <project_id>.svc.id.goog, where <project_id> is a valid GCP project ID.| regexValidation: ^[a-z]([a-z0-9-]{0,60}[a-z0-9])?$ | ||
| validation: Network name must start with a lowercase letter followed by up to 62 lowercase letters, numbers, or hyphens and cannot end with a hyphen. |
There was a problem hiding this comment.
The regex for the subnetwork name is incorrect. Similar to network names, GCP subnetwork names can be up to 63 characters long, but this regex restricts them to 62. The validation message is also confusing. I've suggested a corrected regex and a clearer validation message.
regexValidation: ^[a-z]([a-z0-9-]{0,61}[a-z0-9])?$
validation: Must be 1-63 characters, start with a lowercase letter, contain only lowercase letters, numbers, or hyphens, and cannot end with a hyphen.| regexValidation: ^.+\.svc\.id\.goog$ | ||
| validation: Workload pool must be in the format <project_id>.svc.id.goog. |
There was a problem hiding this comment.
The regex for the workload pool is too permissive. It allows any characters for the project ID part. The format for a project ID is more restrictive (6-30 characters, starting with a letter, no trailing hyphen). I've provided a stricter regex that correctly validates the project ID format within the workload pool string.
regexValidation: ^[a-z]([-a-z0-9]{4,28}[a-z0-9])\.svc\.id\.goog$
validation: Workload pool must be in the format <project_id>.svc.id.goog, where <project_id> is a valid GCP project ID.| cidr_block: | ||
| name: cidr_block | ||
| title: Cidr Block | ||
| regexValidation: ^((((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/(3[0-2]|[12]?[0-9]))|((\s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))(%.+)?\s*\/(12[0-8]|1[01][0-9]|[1-9]?[0-9]))))$ |
There was a problem hiding this comment.
The regex for CIDR validation is very complex and contains an inconsistency. It allows optional whitespace (\s*) for IPv6 CIDRs, both at the beginning of the string and before the /, but not for IPv4 CIDRs. For consistency and stricter validation, it's better to remove the whitespace allowance from the IPv6 part.
regexValidation: ^((((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/(3[0-2]|[12]?[0-9]))|((((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))(%.+)?\/(12[0-8]|1[01][0-9]|[1-9]?[0-9]))))$| cidr_block: | ||
| name: cidr_block | ||
| title: Cidr Block | ||
| regexValidation: ^((((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/(3[0-2]|[12]?[0-9]))|((\s*((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))(%.+)?\s*\/(12[0-8]|1[01][0-9]|[1-9]?[0-9]))))$ |
There was a problem hiding this comment.
The regex for CIDR validation is very complex and contains an inconsistency. It allows optional whitespace (\s*) for IPv6 CIDRs, both at the beginning of the string and before the /, but not for IPv4 CIDRs. For consistency and stricter validation, it's better to remove the whitespace allowance from the IPv6 part.
regexValidation: ^((((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\/(3[0-2]|[12]?[0-9]))|((((([0-9A-Fa-f]{1,4}:){7}([0-9A-Fa-f]{1,4}|:))|(([0-9A-Fa-f]{1,4}:){6}(:[0-9A-Fa-f]{1,4}|((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){5}(((:[0-9A-Fa-f]{1,4}){1,2})|:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3})|:))|(([0-9A-Fa-f]{1,4}:){4}(((:[0-9A-Fa-f]{1,4}){1,3})|((:[0-9A-Fa-f]{1,4})?:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){3}(((:[0-9A-Fa-f]{1,4}){1,4})|((:[0-9A-Fa-f]{1,4}){0,2}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){2}(((:[0-9A-Fa-f]{1,4}){1,5})|((:[0-9A-Fa-f]{1,4}){0,3}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(([0-9A-Fa-f]{1,4}:){1}(((:[0-9A-Fa-f]{1,4}){1,6})|((:[0-9A-Fa-f]{1,4}){0,4}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:))|(:(((:[0-9A-Fa-f]{1,4}){1,7})|((:[0-9A-Fa-f]{1,4}){0,5}:((25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)(\.(25[0-5]|2[0-4]\d|1\d\d|[1-9]?\d)){3}))|:)))(%.+)?\/(12[0-8]|1[01][0-9]|[1-9]?[0-9]))))$
apeabody
changed the title
|
Can we also include regex validation for the node pool name. currently it could take any character in the input |
Done. |
|
/gcbrun |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds regex validation for several input fields across the gke-autopilot-cluster, gke-standard-cluster, and gke-node-pool modules to improve input validation. The changes are generally good and improve the robustness of the modules. I've left a few comments regarding a very complex regex for CIDR validation, suggesting improvements for consistency and maintainability. I also found a minor copy-paste error in a validation message for subnetwork names. Otherwise, the changes look solid.
|
/gcbrun |
3 participants
No description provided.