Init container updates for persisting kibana plugins. #8389
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Michael Montgomery <[email protected]>
updating unit tests. Signed-off-by: Michael Montgomery <[email protected]>
|
Should we reuse the existing init container for this? What is the overhead of adding another container? |
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
The problem with using the existing init container is that the |
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
|
Ok, I've verified that this is working as intended by:
I'm going to now see about adding this functionality to the existing init container, and not adding a 2nd.... |
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
|
Logs from recent commit showing init container that does both plugins initialization/copy, and config init. Also keystore init container logs: |
naemono
requested review from
thbkrkr,
pebrc,
barkbay,
rhr323 and
kvalliyurnatt
naemono
changed the title
pebrc
reviewed
Jan 9, 2025
Comment on lines
+23
to
+33
| verify: func(v volume.ConfigMapVolume) error { | ||
| if v.Name() != "kibana-scripts" { | ||
| return fmt.Errorf("unexpected name: %s", v.Name()) | ||
| } | ||
| if v.VolumeMount().MountPath != "/mnt/elastic-internal/scripts" { | ||
| return fmt.Errorf("unexpected mount path: %s", v.VolumeMount().MountPath) | ||
| } | ||
| if *v.Volume().ConfigMap.DefaultMode != 0755 { | ||
| return fmt.Errorf("unexpected default mode: %d", *v.Volume().ConfigMap.DefaultMode) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Could this not be simple equality check on the struct? Or even use something like go-snaps?
There was a problem hiding this comment.
Unfortunately the struct's fields aren't exported, so that's why this was a bit... odd.
type ConfigMapVolume struct {
configMapName string
name string
mountPath string
items []corev1.KeyToPath
defaultMode int32
}
There was a problem hiding this comment.
Let me know if this needs changing.
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
Signed-off-by: Michael Montgomery <[email protected]>
pebrc
reviewed
Jan 9, 2025
pkg/controller/kibana/pod.go
Outdated
| builder.WithContainersSecurityContext(defaultSecurityContext). | ||
| WithPodSecurityContext(defaultPodSecurityContext). | ||
| WithVolumes(LogsVolume.Volume()).WithVolumeMounts(LogsVolume.VolumeMount()). | ||
| WithVolumes(PluginsVolume.Volume()).WithVolumeMounts(PluginsVolume.VolumeMount()). | ||
| WithVolumes(TempVolume.Volume()).WithVolumeMounts(TempVolume.VolumeMount()) | ||
| WithVolumes(TempVolume.Volume()).WithVolumeMounts(TempVolume.VolumeMount()). | ||
| WithVolumes(scriptsConfigMapVolume.Volume()).WithVolumeMounts(scriptsConfigMapVolume.VolumeMount()) |
There was a problem hiding this comment.
The scripts volume mount has to be present also when the security context is disabled.
There was a problem hiding this comment.
I have some unit tests to fix tomorrow, but it shouldn't block the continued review of this.
Signed-off-by: Michael Montgomery <[email protected]>
|
buildkite test this -f E2E_TAGS=kb -m p=gke,p=ocp |
barkbay
reviewed
Jan 10, 2025
|
|
||
| import ( | ||
| "bytes" | ||
| "html/template" |
There was a problem hiding this comment.
Do we really need the html/template package?
Suggested change
| "html/template" | |
| "text/template" |
Comment on lines
+179
to
+182
| err = initcontainer.ReconcileScriptsConfigMap(ctx, d.client, *kb, params.SetDefaultSecurityContext) | ||
| if err != nil { | ||
| return results.WithError(err) | ||
| } |
There was a problem hiding this comment.
Suggested change
| err = initcontainer.ReconcileScriptsConfigMap(ctx, d.client, *kb, params.SetDefaultSecurityContext) | |
| if err != nil { | |
| return results.WithError(err) | |
| } | |
| if err := initcontainer.ReconcileScriptsConfigMap(ctx, d.client, *kb, params.SetDefaultSecurityContext); err != nil { | |
| return results.WithError(err) | |
| } |
Comment on lines
+97
to
+98
| // RenderInitScript renders the init script that will be run by the init container. | ||
| func RenderInitScript(kb kbv1.Kibana, setDefaultSecurityContext bool) (string, error) { |
There was a problem hiding this comment.
Suggested change
| // RenderInitScript renders the init script that will be run by the init container. | |
| func RenderInitScript(kb kbv1.Kibana, setDefaultSecurityContext bool) (string, error) { | |
| // renderInitScript renders the init script that will be run by the init container. | |
| func renderInitScript(kb kbv1.Kibana, setDefaultSecurityContext bool) (string, error) { |
Comment on lines
+82
to
+83
| // RenderScriptTemplate renders initFsScriptTemplate using the given TemplateParams | ||
| func RenderScriptTemplate(params templateParams) (string, error) { |
There was a problem hiding this comment.
Suggested change
| // RenderScriptTemplate renders initFsScriptTemplate using the given TemplateParams | |
| func RenderScriptTemplate(params templateParams) (string, error) { | |
| // renderScriptTemplate renders initFsScriptTemplate using the given TemplateParams | |
| func renderScriptTemplate(params templateParams) (string, error) { |
Comment on lines
+88
to
+89
| !reflect.DeepEqual(expected.Labels, reconciled.Labels) || | ||
| !reflect.DeepEqual(expected.Annotations, reconciled.Annotations) |
There was a problem hiding this comment.
Do we really want a strict equality?
Suggested change
| !reflect.DeepEqual(expected.Labels, reconciled.Labels) || | |
| !reflect.DeepEqual(expected.Annotations, reconciled.Annotations) | |
| !maps.IsSubset(expected.Labels, reconciled.Labels) || | |
| !maps.IsSubset(expected.Annotations, reconciled.Annotations) |
| var HardenedSecurityContextSupportedVersion = version.From(7, 9, 0) | ||
|
|
||
| // NewScriptsConfigMapVolume creates a new volume for the ConfigMap containing scripts used by the Kibana init container. | ||
| func NewScriptsConfigMapVolume(kbName string) volume.ConfigMapVolume { |
There was a problem hiding this comment.
I don't think configmaps are watched by this controller:
cloud-on-k8s/pkg/controller/kibana/controller.go
Lines 66 to 106 in 6c1bf95
| } | ||
| } | ||
|
|
||
| // ReconcileScriptsConfigMap reconciles the ConfigMap containing scripts used by the Kibana init container. |
There was a problem hiding this comment.
nit: just to make it clear that it is not used by the one used for the keystore?
Suggested change
| // ReconcileScriptsConfigMap reconciles the ConfigMap containing scripts used by the Kibana init container. | |
| // ReconcileScriptsConfigMap reconciles the ConfigMap containing scripts used by the Kibana elastic-internal-init init container. |
| } | ||
|
|
||
| // newConfigMapWithData constructs a new ConfigMap with the given data. | ||
| func newConfigMapWithData(cm, kb types.NamespacedName, data map[string]string) corev1.ConfigMap { |
There was a problem hiding this comment.
Should we just inline this function? IMHO I'm not sure it adds much, and it would make the code easier to read.
| } | ||
|
|
||
| // reconcileConfigMap checks for an existing ConfigMap and updates it or creates one if it does not exist. | ||
| func reconcileConfigMap( |
There was a problem hiding this comment.
Same here, this function is just calling reconciler.ReconcileResource I would inline it.
Comment on lines
+26
to
+28
| // IncludePlugins indicates whether the script should include | ||
| // the plugins persistence logic. | ||
| IncludePlugins bool |
There was a problem hiding this comment.
nit, naming...
Suggested change
| // IncludePlugins indicates whether the script should include | |
| // the plugins persistence logic. | |
| IncludePlugins bool | |
| // CopyPlugins indicates whether the script should copy the plugins from the Kibana image. | |
| // This should be set to true when the file system is in read only mode and the plugin directory is replaced by an emptyDir. | |
| CopyPlugins bool |
barkbay
reviewed
Jan 10, 2025
| if err != nil { | ||
| return corev1.Container{}, err // error unlikely and should have been caught during validation | ||
| } | ||
| enablePluginsMounts := v.GTE(HardenedSecurityContextSupportedVersion) && setDefaultSecurityContext |
There was a problem hiding this comment.
Why setDefaultSecurityContext is required to be true? Does it mean that if a user is running on OCP and wants to set readOnlyRootFilesystem: true for example, then this volume and the init-container should be added/adjusted manually?
There was a problem hiding this comment.
We could just always copy the plugins and avoid a lot of conditional logic?
barkbay
reviewed
Jan 10, 2025
| Resources: defaultResources, | ||
| } | ||
|
|
||
| if enablePluginsMounts { |
There was a problem hiding this comment.
nit: we can remove the variable (or move it closer to where it is used)
Suggested change
| if enablePluginsMounts { | |
| if v.GTE(HardenedSecurityContextSupportedVersion) && setDefaultSecurityContext { | |
| container.VolumeMounts = append(container.VolumeMounts, PluginsSharedVolume.InitContainerVolumeMount()) | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves: #8388
With the recent changes in #7787 we broke users being able to install custom Kibana plugins. This updates the existing init container for Kibana configuration, and adds the ability to copy plugins to an
emptyDirvolume, and then mount this to/usr/share/kibana/pluginsin the primary Kibana container. (effectively mirroring what we do with Elasticsearch)Review Notes
Changes
settingspackage to avoid import cycles.Testing