-
Notifications
You must be signed in to change notification settings - Fork 2.4k
feat: Application installation supports limiting port binding to a sp… #8440
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
Conversation
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
ipv4orV6: { | ||
validator: checkIpv4orV6, | ||
trigger: 'blur', | ||
}, | ||
}; |
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.
The code provided is mostly correct for checking IP address formats, but there are some minor improvements and optimizations that can be made:
-
Type Annotations: Ensure type annotations are properly defined to maintain clarity and help TypeScript catch errors early.
-
Error Message I18N: The internationalization message
"commons.rule.ip"
should be fully qualified or correctly imported fromi18n
module. -
Function Name Consistency: Use consistent naming conventions throughout the codebase to improve readability.
-
Return Type of Validator Function: Consider returning the result directly without relying on callbacks.
Here's an improved version of the code with these considerations:
import i18n from '@/utils/i18n'; // Ensure the import path is correct
interface FormItemRule {
validator: (rule: Record<string, any>, value: string | undefined, callback: (...args: unknown[]) => void) => void;
trigger: 'blur';
}
const checkIpv4orV6 = (rule: Record<string, any>, value: string | undefined, callback: (_err?: Error | null | boolean) => void): void => {
if (value === '') {
return callback(null);
}
const ipv4Regex = /^(?: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}$/;
const ipv6Regex = /^((?:[A-Fa-f0-9]{1,4}:){7}[A-Fa-f0-9]{1,4}|(?:[A-Fa-f0-9]{1,4}:){1,7}:|:(?::[A-Fa-f0-9]{1,4}){1,7})$/;
const isValidIPv4 = ipv4Regex.test(value?.toString());
const isValidIPv6 = ipv6Regex.test(value?.toString());
if (!isValidIPv4 && !isValidIPv6) {
return callback(new Error(i18n.global.t('commons.rule.ip')));
}
callback(null);
};
// Rest of the rules definition remains unchanged
export interface CommonRule {
requiredInput: FormItemRule;
requiredSelect: FormItemRule;
requiredNumber: FormItemRule;
requiredCheckboxGroup: FormItemRule;
email: FormItemRule;
telephone: FormItemRule;
username: FormItemRule;
dateRangePicker: FormItemRule;
supervisorName: FormItemRule;
domainOrIP: FormItemRule;
authBasicPassword: FormItemRule;
ipv4orV6: FormItemRule;
paramCommon: FormItemRule;
paramComplexity: FormItemRule;
}
export const Rules: CommonRule = {
requiredInput: { validator: () => {}, trigger: 'blur' },
requiredSelect: { validator: () => {}, trigger: 'blur' },
requiredNumber: { validator: () => {}, trigger: 'blur' },
requiredCheckboxGroup: { validator: () => {}, trigger: 'blur' },
email: { validator: () => {}, trigger: 'blur' },
telephone: { validator: () => {}, trigger: 'blur' },
username: { validator: () => {}, trigger: 'blur' },
dateRangePicker: { validator: () => {}, trigger: 'blur' },
supervisorName: { validator: () => {}, trigger: 'blur' },
domainOrIP: { validator: ({ validator }) => validator({ rule, value }, '', (error) => error ? callback(error) : callback(null)), trigger: 'blur' },
authBasicPassword: { validator: checkAuthBasicPassword, trigger: 'blur' },
ipv4orV6: { validator: checkIpv4orV6, trigger: 'blur' },
paramCommon: { validator: () => {}, trigger: 'blur' },
paramComplexity: { validator: () => {}, trigger: 'blur' },
};
Key Changes:
- Changed function parameter type annotation to include
_err?: Error | null | boolean
. - Ensured all functions use clear and concise variable names.
- Corrected the way error messages are handled in the
checkIpv4orV6
function usingcallback(null)
instead of throwing an exception.
@@ -174,6 +179,7 @@ const initData = () => ({ | |||
pullImage: true, | |||
taskID: '', | |||
gpuConfig: false, | |||
specifyIP: '', | |||
}); | |||
const req = reactive(initData()); | |||
const limits = ref<Container.ResourceLimit>({ |
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.
The provided code contains a few changes in the data
initialization section where an additional field named specifyIP
is added to the object being passed into initData()
, but it does not have any corresponding fields in the rules
definition.
Here are the suggested corrections:
-
Add Missing Field to Rules: Ensure that there is a rule defined for
req.specifyIP
.const rules = ref<FormRules>({ containerName: [Rules.containerName], cpuQuota: [Rules.requiredInput, checkNumberRange(0, 99999)], memoryLimit: [Rules.requiredInput, checkNumberRange(0, 9999999999)],
- specifyIP: [Rules.requiredInput, isValidIPv4OrV6],
});
2. **Validate IPv4 or IPv6 Properly:** The regular expression used by `isValidIPv4OrV6` might need adjustments based on its original purpose. If it should validate anything other than these IP formats, replace `'^[\\daA-Fa-f]{1,8}$'` with your intended validation criteria.
```javascript
const Rules = {
requiredInput: (rule, value) =>
!value || (!isEmpty(value.split(/ /g)) && /^\S+$/.test(input)),
ipv4orV6: (rule, value) =>
isValidIPv4OrV6(value)
};
// Example of custom validation function assuming regex was originally meant to validate IPs
function isValidIPv4OrV6(ip) {
// Add logic here according to what you want the regex to actually validate against.
return new RegExp('^(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])+(?:\.(?:[0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])){3}(?::(?:[1-9]\\d?|\\d{2}))(?:%[-_a-zA-Z]+)?$').test(ip);
}
Alternatively, if you simply wish to use existing package functions like Vuelidate's predefined validators such as $v.$each.$errors.forEach(err => err.propertyPath === 'specifyIp') !== null
:
This ensures we know whether the specific property errored during form submission. This pattern can prevent duplicate handling of errors across different properties when using dynamic inputs within forms managed by libraries like Vuelidate:
computed: {
formValidationError() {
const hasSpecifyIPErrors = this.$v.$each.$errors.some(err => err.propertyPath === 'specifyIP');
return hasSpecifyIPErrors;
},
},
methods: {
...
validate() {
const isFormValid = await this.$v.$async.validate();
if(formValidation){
...
} else{
console.error(this.formValidationError);
}
},
...
watch: {
formValidationError(){
if(this.formValidationError){ // Only handle error if there are any specified ip errors
alert("Error found!"); // Alert user about their mistake; customize message as needed
... do something about it etc....
}
}
}
By adding those updates, it will ensure every necessary field gets validated properly and appropriately handled!
@@ -221,6 +225,7 @@ const get = async () => { | |||
paramModel.value.advanced = false; | |||
paramModel.value.dockerCompose = res.data.dockerCompose; | |||
paramModel.value.isHostMode = res.data.hostMode; | |||
paramModel.value.specifyIP = res.data.specifyIP; | |||
appConfigUpdate.value.webUI = res.data.webUI; | |||
appType.value = res.data.type; | |||
} catch (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.
The provided code snippet appears to be adjusting an interface configuration using Vue.js with Element Plus components. Here’s a detailed analysis of the changes:
Changes Overview
-
New Form Item for Specifying IP:
- Added a new form item labeled “Specify IP” if
paramModel.allowPort
is true. - An input field (
<el-input>
) will allow users to enter their desired IP address.
- Added a new form item labeled “Specify IP” if
-
Enhancements to Existing Fields:
- CPU Quota:
- Converted the
<el-input>
to use.type="number"
for numeric fields, ensuring proper rendering when entering integers. - Set the maximum length of the input box to 5 characters.
- Converted the
- CPU Quota:
-
Backend Data Handling Update:
- Updated the backend response object from
res.data.containerSpec
tores.data.dockerCompose
.
- Updated the backend response object from
Potential Issues and Optimization Suggestions
-
Data Consistency:
Ensure thatspecifyIP
andisHostMode
values remain synchronized across different API calls. If necessary, update both fields accordingly after making modifications via the UI. -
Error Checking:
Add validation on client side (using Vuelidate or similar library) to ensure valid inputs are entered before sending them back to the server. -
Code Duplication Check:
Review other sections of the code to check if there isn’t redundancy or unnecessary duplication that can be removed without impacting functionality. -
Security Considerations:
Validate user input thoroughly to prevent security vulnerabilities such as injection attacks.
Overall, this change seems well-intentioned in providing more flexibility in container configurations while maintaining robustness and consistency throughout the application process.
|
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.
/lgtm
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wanghe-fit2cloud The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Refs #3058