add cluster-trust-bundle feature #51
Conversation
| // stores information if any container was modified | ||
| var result bool | ||
|
|
||
| for _, cs := range [][]corev1.Container{p.Spec.Containers, p.Spec.InitContainers} { |
There was a problem hiding this comment.
In case we will have two or more containers the volume mounts will be modified only for the first one. || is a conditional operator so that if the first argument is true the second will not be evaluated.
I think the code should be modified as follows:
if handleVolumeMount(cs) {
result = true
}| return result | ||
| } | ||
|
|
||
| addVolumeMount := func(modified bool, p *corev1.Pod) bool { |
There was a problem hiding this comment.
The names of the functions could be improved. I would rename addVolumeMount to handleVolumeMounts or similar. Current naming (addVolumeMount for function and handleVolumeMount for single volume increases cognitive load)
| "replace.me": "ghcr.io", | ||
| "example.com": "ghcr.io" | ||
| }, | ||
| "clusterTrustBundleMapping": { |
There was a problem hiding this comment.
I'm not fully convinced to the name of the element. Instead of using clusterTrustBundleMapping we could do something like:
"clusterTrustBundle": {
"name": "rt-bootstrapper-k3d.test:ctb:1",
"certWritePath": "kube-apiserver-serving.pem",
"volumeMountPath": "/etc/ssl/certs",
"volumeName": "rt-bootstrapper-certs"
}There was a problem hiding this comment.
Lets do this refactor in the this PR just to avoid collisions.
| return defaultPod(addImgPullSecret, annotationsSetPullSecret) | ||
| } | ||
|
|
||
| func BuildDefaulterAddClusterTrustBundle(mapping k8s.ClusterTrustBundleMapping) PodDefaulter { |
There was a problem hiding this comment.
Just a general remark: the defaulters could be tested by unit tests. I don't think we must add such unit tests right now, but it would nicely fit into the entire testing idea for the webhook.
There was a problem hiding this comment.
Fully agree, we will add tests for all defaulters.
I was planning a small refactor afer we merge everything that is under development, I would add the tests then.
akgalwas
left a comment
akgalwas
left a comment
There was a problem hiding this comment.
We need to fix the bug in addVolumeMount
| return result | ||
| } | ||
|
|
||
| addVolumeMount := func(modified bool, p *corev1.Pod) bool { |
There was a problem hiding this comment.
The modified and result names are too generic. The fist one is true if volume was added or updated. The second is true if at least one volumeMount was added.
| } | ||
|
|
||
| if reflect.DeepEqual(p.Spec.Volumes[index], vol) { | ||
| slog.Debug("equal volume found, nothing to do") |
There was a problem hiding this comment.
This message is not accurate. It should say that volume was already added.
Description
Changes proposed in this pull request:
Related issue(s)