Addition of L1 and L2 input validation for roles_config.yml #2517
Conversation
|
Can one of the admins verify this patch? |
input_validation/validate_config.yml
Outdated
| gather_facts: false | ||
| roles: | ||
| - role: validate_input | ||
| tags: ["scheduler", "provision", "security", "monitoring", "local_repo", "k8", "roce", "storage", "proxy", "roles"] |
There was a problem hiding this comment.
- roles is not new, it should come uner provision only
- For now lets comment out monitoring flow
|
Please attached ansible lint check details in UT report |
| from datetime import datetime | ||
| import os | ||
|
|
||
| input_validator_log = '/opt/omnia/log/core/playbooks/input_validator/' |
There was a problem hiding this comment.
@abhishek-sa1 /opt/omnia will be always fixed, or it should be NFS share given by user ?
There was a problem hiding this comment.
/opt/omnia/log/core/playbooks/input_validator this path is always fixed
| files["provision_config"], | ||
| files["network_spec"], | ||
| files["server_spec"], | ||
| files["software_config"], |
There was a problem hiding this comment.
add roles config here
| } | ||
|
|
||
| os_version_ranges = { | ||
| "rhel": ["8.6", "8.8", "9.4"], |
There was a problem hiding this comment.
remove RHEL/Rocky 8.6 and 8.7 as this are not supported with new version.
| os_version_ranges = { | ||
| "rhel": ["8.6", "8.8", "9.4"], | ||
| "rocky": ["8.6", "8.8", "9.4"], | ||
| "ubuntu": ["20.04", "24.04"] |
There was a problem hiding this comment.
ubuntu 22.04 need to add
| vault_passwords = { | ||
| "provision_config_credentials.yml": ".provision_credential_vault_key", | ||
| "telemetry_config.yml": ".telemetry_vault_key", | ||
| "omnia_config.yml": ".omnia_vault_key", |
There was a problem hiding this comment.
comment telemetry_config.yml
| import jsonschema | ||
| import subprocess | ||
| from ansible.module_utils.basic import AnsibleModule | ||
|
|
There was a problem hiding this comment.
for any of this, any extra package need to install in core container?
|
|
||
| # Global dictionary for custom schema regex error messages | ||
| regex_errors = { | ||
| "Groups" : "Groups must be defined in the form of grp<n> where n is 0-99.", |
There was a problem hiding this comment.
can we keep this errors in en_us_validation_msg.py ?
| logger.error(message) | ||
|
|
||
| # Code to run the L2 validation validate_input_logic function. | ||
| def validate_logic(input_file_path, logger, module, omnia_base_dir, project_name): |
There was a problem hiding this comment.
can we add doc string using codeium for each function?
| return False | ||
|
|
||
| # Start validation execution | ||
| message = f"{'#' * 30} START EXECUTION {'#' * 30}" |
There was a problem hiding this comment.
none of the english messege should be in code, it should come from en_us_validation_msg.py
| raise FileNotFoundError(error_message) | ||
|
|
||
| # For each file from the tag names, run schema validation (L1) and logic validation (L2) | ||
| s = {project_name: {"status": [], "tag": tag_names}} |
There was a problem hiding this comment.
give proper name to variable
| "user_name": { | ||
| "type": "string", | ||
| "pattern": "^[a-zA-Z0-9_-]+(,[a-zA-Z0-9_-]+)*$|^$", | ||
| "description": "Usernames for which Kubernetes access needs to be set up. Can be a comma-separated list." |
There was a problem hiding this comment.
can we read description from en_us_validation_msg?
There was a problem hiding this comment.
Looked into this, I don't believe it is possible to directly import the variables from a python file to a json file. There would have to be a script to essentially build the json file or find and replace values in the json file.
| "properties": { | ||
| "repo_store_path": { | ||
| "type": "string", | ||
| "default": "/opt/omnia_repo", |
There was a problem hiding this comment.
this need to change as per the path of pulp container
| }, | ||
| "mlnx_ofed_version": { | ||
| "type": "string", | ||
| "default": "5.4-2.4.1.3" |
There was a problem hiding this comment.
@abhishek-sa1 is this version correct?
There was a problem hiding this comment.
no. we need to update default version
| "type": "string", | ||
| "pattern": "^(1[0-9]|2[0-9]|[1-9])$|^3[0-2]$" | ||
| }, | ||
| "static_range": { |
There was a problem hiding this comment.
remove static range from here
| errors.append(create_error_msg("config_roles.yml,", None, "EMPTY config_roles.yml")) | ||
| else: | ||
| try: | ||
| roles = data.get(ROLES, []) |
There was a problem hiding this comment.
get function will not raise exception if keys not found. data['Roles'] will raise if key not found.
Or
instead of try catch - if data.get(ROLES, {}) and in else we can append in error
| # Check for at least 1 group | ||
| # Check for at least 1 role | ||
| # Check to make sure there are not more than 100 roles | ||
| if not groups or len(groups) == 0: |
There was a problem hiding this comment.
len(groups) not required. if groups is empty dict - not groups will be enough
|
|
||
| for role in roles: | ||
| # Check role-group association, all roles must have a group | ||
| if role[ROLE_GROUPS] and len(role[ROLE_GROUPS]) == 0: |
There was a problem hiding this comment.
- There could be possibility that under any roles groups key is not mentioned, this will raise error.
get function would be better optionrole[ROLE_GROUPS]
Some negative scenerio:
Roles:
-
If roles input is like above len will not be zero it will have one element in list as - [None]
In this even get role.get(ROLE_GROUPS) function will raise error - NoneType object has no method get.
| if role[NAME] in empty_parent_roles and not validation_utils.is_string_empty(groups[group].get(PARENT, None)): | ||
| # If parent is not empty and group is associated with login, compiler, service, k8head, or slurmhead | ||
| errors.append(create_error_msg(group, f'Group {group} should not have parent defined.', en_us_validation_msg.parent_service_node_msg)) | ||
| if not service_role_defined and (role[NAME] == "worker" or role[NAME] == "default"): |
There was a problem hiding this comment.
For worker roles we have role name as k8sworker or slurmworker
| # If a service role is not present, the parent is not empty and the group is associated with worker or default roles. | ||
| if not validation_utils.is_string_empty(groups[group].get(PARENT, None)): | ||
| errors.append(create_error_msg(group, f'Group {group} should not have parent defined.', en_us_validation_msg.parent_service_role_msg)) | ||
| elif not service_role_defined and not validation_utils.is_string_empty(groups[group].get(PARENT, None)): |
There was a problem hiding this comment.
What extra case this covers?
I guess irrespective of service role is defined or not, roles other that worker - k8sworker or slurmworker, should not have parent value or if have then it should have 'NA'
Correct @abhishek-sa1
| or (validation_utils.is_string_empty(groups[group].get(SWITCH_DETAILS, {}).get(IP, None)) and not validation_utils.is_string_empty(groups[group].get(SWITCH_DETAILS, {}).get(PORTS, None)))): | ||
| errors.append(create_error_msg(group, f'Group {group} switch details are incomplete:', en_us_validation_msg.switch_details_incomplete_msg)) | ||
| if ((not validation_utils.is_string_empty(groups[group].get(SWITCH_DETAILS, {}).get(IP, None)) or not validation_utils.is_string_empty(groups[group].get(SWITCH_DETAILS, {}).get(PORTS, None))) | ||
| and validation_utils.is_string_empty(groups[group].get(BMC_DETAILS, {}).get(STATIC_RANGE, None))): |
There was a problem hiding this comment.
In case this check is for if switch details are complete and bmc static range is not present:
- Error should mention this.
- Instead of whether switch ip not empty
orswitch port not empty - we need to haveand - or can use elif bmc_details static range not empty, with the last check where complete switch ip details are checked
- Also if we can store these ip and ports empty checks in some variable it would be better, as we are using it repeatedly, also it becoming confusing to understand and/or operations
input_validation/ansible.cfg
Outdated
| @@ -0,0 +1,15 @@ | |||
| [defaults] | |||
| log_path = /opt/omnia/log/core/playbooks/validate_input.log | |||
There was a problem hiding this comment.
log name should be input_validation.log
| files["software_config"], | ||
| files["storage_config"], | ||
| files["site_config"], | ||
| files["roles_config"] |
There was a problem hiding this comment.
remove site config validation now
| # provision_config.yml | ||
| default_lease_time_fail_msg = "Please provide a valid default_lease_time." | ||
| timezone_fail_msg = "Unsupported Timezone. Please check the timezone.txt file for a list of valid timezones." | ||
| enable_switch_based_fail_msg = "enable_switch_based must be set to either true or false." |
There was a problem hiding this comment.
iso_file_path variable is moved from provision_config.yml to software_config.json. nodename will be removed. switch details moved to roles_config.yml
| try: | ||
| ipaddress.IPv4Address(switch_ip) | ||
| if switch_ip in switch_ip_mapping: | ||
| errors.append(create_error_msg(group, f'Group {group} has a duplicate switch IP with {switch_ip_mapping[switch_ip]}:', en_us_validation_msg.duplicate_switch_ip_msg)) |
There was a problem hiding this comment.
Multiple groups can have same switch ip, in case switch ip is same we need to make sure ports are different.
…d updating other input validator workflows
Description of the Solution
Addition of L1 and L2 input validation for roles_config.yml
Jira: https://jira.cec.lab.emc.com/browse/OMN01D-188
Please see Grace's original PR for original input validator changes: #2466
Suggested Reviewers
If you wish to suggest specific reviewers for this solution, please include them in this section. Be sure to include the @ before the GitHub username.
@priti-parate