CORTX-32157: Refactor some Chart values

keithpine commented 2 years ago

Description

Refactor the values to be more consistent with Helm conventions, reduce duplication, etc. (general cleanup).

Remove the configmap value and relocate/consolidate child values to the top-level, or within their appropriate component sections.
Default clusterName to the release fullname instead of a hard-coded name.
Hard-code the RGW "auth_secret". This is not configurable as the Secret is created with the exact value. This is consistent with all of the other secrets, and we have never exposed this setting in solution.yaml.
Set the secret name to an empty default, and flag it as required so the chart install fails if not specified. Existing secret names can also be a Helm template.
Hard-code the secret's volume name in the Pod specs, there's not much reason to customize it.

Type of change

[ ] Bug fix (non-breaking change that fixes an issue)
[X] New feature (non-breaking change that adds new functionality)
[ ] Breaking change (bug fix or new feature that breaks existing functionality)
[ ] Third-party dependency update
[ ] Documentation additions or improvements
[ ] Code quality improvements to existing code or test additions/updates

Applicable issues

This change fixes an issue: CORTX-32157
This change is related to an issue: #

How was this tested?

Deployment, ran util scripts, S3 I/O, CSM request, etc.

Since existingSecret is required, and is empty by default, a custom "ci" values file was added for chart linting (ct).

Additional information

Checklist

[x] The change is tested and works locally.
[ ] New or changed settings in the solution YAML are documented clearly in the README.md file.
[x] All commits are signed off and are in agreement with the CORTX Community DCO and CLA policy.

If this change requires newer CORTX or third party image versions:

[ ] The image fields in solution.example.yaml have been updated to use the required versions.
[ ] The appVersion field of the Helm chart has been updated to use the new CORTX version.

If this change addresses a CORTX Jira issue:

[x] The title of the PR starts with the issue ID (e.g. CORTX-XXXXX:)

keithpine commented 2 years ago

@tanujashinde0405 I'm not aware of any documentation in regards to the allowed values for the cluster.yaml file's cluster.name and cluster.id fields. Could you confirm whether there are any character or length restrictions? As far as I can tell, there isn't. The Chart Release fullname is limited to 63 characters, with no trailing -.

https://github.com/Seagate/cortx-k8s/blob/153fa5700cace4e7ab0f74c4bcc11c415f086143/charts/cortx/templates/_helpers.tpl#L8-L24

tanujashinde0405 commented 2 years ago

@tanujashinde0405 I'm not aware of any documentation in regards to the allowed values for the cluster.yaml file's cluster.name and cluster.id fields. Could you confirm whether there are any character or length restrictions? As far as I can tell, there isn't. The Chart Release fullname is limited to 63 characters, with no trailing -.

https://github.com/Seagate/cortx-k8s/blob/153fa5700cace4e7ab0f74c4bcc11c415f086143/charts/cortx/templates/_helpers.tpl#L8-L24

@nitin-seagate Can you please confirm this?

nitin-seagate commented 2 years ago

onfirm whether there are any character or length restrictions? As far as I can tell, there isn't. The Chart Release fullname is limited to 63 characters, with no trailing

Yes there is no specification for allowed values wrt CLUSTER-NAME, we can assume default character-set (alphanumeric with limited special characters)... The CLUSTER-ID should be generic uuid (we can govern and define format for the same in-line with what we followed in LR) @tanujashinde0405 Please confirm CLUSTER-ID format for LR?

tanujashinde0405 commented 2 years ago

IN LR also Cluster-id was a uuid we used to generate during deployment : https://github.com/Seagate/cortx-prvsnr/blob/legacy-main/api/python/provisioner/commands/cluster_id.py#L87

keithpine commented 2 years ago

Reverted the change to the default cluster ID values. However, previously the dashes (-) were removed from the generated UUID, but now those are kept so it is a real UUIDv4, in the same way the LR Provisioner generated it.

I'm not sure it makes sense from a Core perspective that the ID should be required to be a UUID, especially since the software is already allowing regular strings. But that's a discussion for another time and place.

Seagate / cortx-k8s