Add 'envFrom' generic configuration for backend spec

[sashokbg]

Summary

Added envFrom configuration for backends
EnvFrom can be used to add arbitrary env variables 'in bulk' to the job container template.
This feature can be useful in implementing some generic authentication

Signed-off-by: Alexander KIRILOV aleksandar.kirilov@proxym.fr

Checklist

• Keep pull requests small so they can be easily reviewed.
• Categorize the PR by setting a good title and adding one of the labels:
  bug, enhancement, documentation, change, breaking,
  as they show up in the changelog
• Update the documentation.
• Update tests.
• Link this PR to related issues.

[Kidswiss]

Hi @sashokbg

Thanks for your contribution! I'm having a look at this right now.

[Kidswiss]

Overall LGTM.

Just one minor thing, so that it works as intended.

EDIT: just had a look at the failing test, there seems to be some nil pointer problem.

[Kidswiss]

@sashokbg there should probably be a check for a nil backend before accessing the EnvFrom. Otherwise, a backup or schedule definition without a backend crashes the operator.

[sashokbg]

@Kidswiss I am really sorry for the question but since it is my first time writing Go code - how should I implement the null check ?
A simple will work ?

if restore.Spec.Backend.EnvFrom != nil {
	j.Spec.Template.Spec.Containers[0].EnvFrom = restore.Spec.Backend.EnvFrom
}

[Kidswiss]

@sashokbg No need to apologize :)

You'll need to check the backend itself for nil:

if restore.Spec.Backend!= nil {
	j.Spec.Template.Spec.Containers[0].EnvFrom = restore.Spec.Backend.EnvFrom
}

The problem arises because you want to access a field of backend. The actual envFrom being nil should be fine.

[sashokbg]

@Kidswiss thank you for your feedback - I have updated the PR to reflect the requested changes. I have tested the integration tests locally and everything passes

[ccremer]

Hi.
Thanks for this high-quality PR! I was rather surprised to see a tutorial page :)

A few styling suggestions in the documentation, and some overlooked nil checks.

Additionally, I would love to see unit tests added that verify this nil check. Mainly to ensure that a later refactoring or code doesn't introduce a regression later on.

[sashokbg]

Hello @ccremer I will try to add a couple of unit tests to cover the null exception. Being a total novice in go this will probably take me a some time :)

[ccremer]

I had a look at it again and I figured we could optimize the code a bit.
Instead of doing a nil check in every occurrence, we add a function to the spec struct that accepts a container spec and adds the env vars. That way this should be easier to unit test and the logic is in a single place only.

k8up/api/v1/runnable_types.go

Line 8 in a92ff23

type RunnableSpec struct {

func (in *RunnableSpec) AppendEnvFromToContainer(containerSpec *corev1.ContainerSpec) {
  if in.Backend != nil {
    containerSpec.EnvFrom = append(containerSpec.EnvFrom, in.Backend.EnvFrom...)
  }
}

then in each executor instead of the nil check call the function like this:

  b.backup.Spec.AppendEnvFromToContainer(&backupJob.Spec.Template.Spec.Containers[0])

WDYT?

[sashokbg]

Hello @ccremer thank you for your time and helpful feedback. I think what you proposed is a good idea and went ahead and implemented it as you specified. I have also included a unit test that tests the new function

[ccremer]

Actually I've found something to be missing

[ccremer]

I think you need to amend your commits for the DCO checks, other than that it's LGTM

[Kidswiss]

I like the idea of moving it to it's own method.

LGTM

[ccremer]

@sashokbg thank you for your contribution 🎉