Skip to content
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

WIP: Refactoring certs and volumes for sts #169

Closed

Conversation

sergeimonakhov
Copy link
Contributor

This pull request introduces code changes related to managing volumes for the STS object. Key modifications include the addition of structures to store certificate information and the definition of paths to certificate files. Additionally, changes have been made to the creation and mounting of the data volume for the ETCD container. These alterations aim to enhance certificate handling and data management within the controller's context.

@sergeimonakhov sergeimonakhov changed the title Refactoring for certs and volumes for sts Refactoring certs and volumes for sts Apr 12, 2024
@sircthulhu sircthulhu added enhancement New feature or request ok-to-test Indicates a non-member PR verified by an org member that is safe to test. labels Apr 13, 2024
@sircthulhu
Copy link
Member

@sergeimonakhov
Please run make lint-fix to resolve pre-commit errors and fix tests

@sergeimonakhov
Copy link
Contributor Author

@sircthulhu I added even more refactoring and fixed the code to make the tests run. Overall, the base code was very poor; we need to be more stringent in our code reviews because it will become impossible to maintain if we keep accepting sloppy work.

internal/controller/factory/statefulset.go Outdated Show resolved Hide resolved
Comment on lines +635 to +639
var volumeMounts []corev1.VolumeMount

for _, list := range lists {
volumeMounts = append(volumeMounts, list...)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to use copy:

volumeMounts := make([]corev1.VolumeMount, len(list))
copy(volumeMounts, list)

Copy link
Contributor Author

@sergeimonakhov sergeimonakhov Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to use copy:

volumeMounts := make([]corev1.VolumeMount, len(list))
copy(volumeMounts, list)

It won't work that way because the function mergeVolumeMounts accepts ...[]corev1.VolumeMount as an argument, which allows passing N slices. While it's possible to calculate the total length of the slices, that would require a loop. In this case, using copy versus append would make no difference.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or simply back on line 196

volumeMounts := append(tlsVolumeMounts, dataVolumeMount...)

Copy link
Contributor Author

@sergeimonakhov sergeimonakhov Apr 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or simply back on line 196

volumeMounts := append(tlsVolumeMounts, dataVolumeMount...)

Ok, How would it look if you need to add a third list for merging?
The mergeVolumeMounts function is specifically designed to be universal, allowing the merging of N lists.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used in the only place, function which accepts slice as an argument and merges this slice with computed slice in its body.
IMO, it makes no sense to write dedicated function for merging slices of concrete type which accepts variadic number of arguments if it is used in one place where the number of arguments which will be passed to that function is fixed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't work that way because the function mergeVolumeMounts accepts ...[]corev1.VolumeMount as an argument, which allows passing N slices. While it's possible to calculate the total length of the slices, that would require a loop. In this case, using copy versus append would make no difference.

Yes, my bad. Missed that the argument is not a slice, but the slice of slices

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is used in the only place, function which accepts slice as an argument and merges this slice with computed slice in its body. IMO, it makes no sense to write dedicated function for merging slices of concrete type which accepts variadic number of arguments if it is used in one place where the number of arguments which will be passed to that function is fixed.

Initially, there were no certificates, but then they appeared. Now, let's say some config maps and who knows what else will appear later. How would the code look then? Right now, it's very simple: we just add slices to volumeMounts := mergeVolumeMounts(tlsVolumeMounts, dataVolumeMount, configMapVolumeMount, etcVolumeMount).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially, there were no certificates, but then they appeared. Now, let's say some config maps and who knows what else will appear later. How would the code look then? Right now, it's very simple: we just add slices to volumeMounts := mergeVolumeMounts(tlsVolumeMounts, dataVolumeMount, configMapVolumeMount, etcVolumeMount).

In case of any volume mounts defined by the user, I would expect is that function generateVolumeMounts will accept as arguments:

  1. predefined mounts - data, tls certs, whatnot else
  2. container volumeMounts defined in cluster spec by the user

and merge these two lists. Computation of predefined mounts is the separate step where we need to take into account provided cluster configuration, like security config.
Bam, profit.

@sergeimonakhov sergeimonakhov changed the title Refactoring certs and volumes for sts WIP: Refactoring certs and volumes for sts Apr 14, 2024
@sergeimonakhov sergeimonakhov marked this pull request as draft April 14, 2024 14:12
@kvaps
Copy link
Member

kvaps commented May 28, 2024

closed in favor #224

@kvaps kvaps closed this May 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants