feat: Added support for setting env vars in feast services in feast controller

[tmihalac]

What this PR does / why we need it:

Allow users to add or override the container env vars for each deployed feast service using the new env list in the CR

Which issue(s) this PR fixes:

Relates to #4561

For example:

let's say the operator sets the offline container env vars like this by default -

env:
  - name: FEATURE_STORE_YAML_BASE64
    value: <string>
  - name: VAR1
    value: test

If the user deployed the following CR -

apiVersion: feast.dev/v1alpha1
kind: FeatureStore
metadata:
  name: sample
spec:
  feastProject: my_project
  services:
    offlineStore:
      env:
       - name: VAR1
         value: override
       - name: VAR2
         value: demo

The resulting offline container env vars would look like this -

      env:
        - name: FEATURE_STORE_YAML_BASE64
          value: <string>
        - name: VAR1
          value: override
        - name: VAR2
          value: demo

[tchughesiv]

can we add an env variable test for the following type of envvar?

     env:
        - name: MY_POD_NAMESPACE
          valueFrom:
            fieldRef:
              fieldPath: metadata.namespace

[tmihalac]

Done

[dmartinol]

Shouldn't we also support envFrom options to read from Secrets or ConfigMap?
@tchughesiv @tmihalac

[tchughesiv]

@dmartinol that's what this added test will confirm support for... EnvVarSource contains fieldRef, as well as configMapKeyRef & secretKeyRef

[tmihalac]

It is supported, there is just no specific test for that.
I don't see a lot of added value in adding those tests since the only thing we are doing is overriding the original EnvVar object from the container with the ones from the spec if they have the same name

[tmihalac]

@tchughesiv

@dmartinol that's what this added test will confirm support for... EnvVarSource contains fieldRef, as well as configMapKeyRef & secretKeyRef

I've added a test for fieldRef, do you think we should add for configMapKeyRef & secretKeyRef as well?
see my previous comment

[tchughesiv]

@tmihalac imo, we don't need a test for the others

[tchughesiv]

i wonder if reflect.DeepEqual() might be a better approach? instead of this areEnvVarArraysEqual function?

[tmihalac]

@tchughesiv

i wonder if reflect.DeepEqual() might be a better approach? instead of this areEnvVarArraysEqual function?

It doesn't take into account 2 arrays are the same if they have the same elements but different order:

import (
	"fmt"
	"reflect"
)

func main() {
	arr1 := [3]int{1, 2, 3}
	arr2 := [3]int{1, 2, 3}
	arr3 := [3]int{3, 2, 1}

	fmt.Println(reflect.DeepEqual(arr1, arr2)) // Output: true
	fmt.Println(reflect.DeepEqual(arr1, arr3)) // Output: false

[tchughesiv]

the main problem i see with the areEnvVarArraysEqual function is its not checking env.ValueFrom. maybe we use reflect.DeepEqual(envVar, envVar) at the EnvVar level somehow... within this custom function?

[tmihalac]

Now its done updated the function to !reflect.DeepEqual(envMap[env.Name], env)

[tchughesiv]

maybe DeepEqual isn't even needed? envMap[env.Name] == env ??

[tchughesiv]

the main problem i see with the areEnvVarArraysEqual function is its not checking env.ValueFrom. maybe we use reflect.DeepEqual(envVar, envVar) at the EnvVar level somehow... within this custom function?

[tchughesiv]

lgtm

[tchughesiv]

@tmihalac looks like one of the unit tests are failing

[tmihalac]

@tchughesiv
Yes the in deploy.Spec.Template.Spec.Containers[0].Env the APIVersion parameter is initialized with v1 and in the spec I do not set that parameter

result = {v1.EnvVar} 
 Name = {string} "fieldRefName"
 Value = {string} ""
 ValueFrom = {*v1.EnvVarSource | 0xc0004a6080} 
  FieldRef = {*v1.ObjectFieldSelector | 0xc0004a60a0} 
   APIVersion = {string} "v1" <-
   FieldPath = {string} "metadata.namespace"
  ResourceFieldRef = {*v1.ResourceFieldSelector | 0x0} nil
  ConfigMapKeyRef = {*v1.ConfigMapKeySelector | 0x0} nil
  SecretKeyRef = {*v1.SecretKeySelector | 0x0} nil

So they are not equal, I am thinking of reverting the deepEquals