feat: Added support for setting env vars in feast services in feast controller by tmihalac · Pull Request #4739 · feast-dev/feast · GitHub

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

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

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

Done

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

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

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

@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

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

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

@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

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?

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

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

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?

lgtm

@tmihalac looks like one of the unit tests are failing

@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

Signed-off-by: Theodor Mihalache <tmihalac@redhat.com>

…ontroller (feast-dev#4739) Add support for setting env vars in services Signed-off-by: Theodor Mihalache <tmihalac@redhat.com>

# [0.42.0](v0.41.0...v0.42.0) (2024-12-05) ### Bug Fixes * Add adapters for sqlite datetime conversion ([#4797](#4797)) ([e198b17](e198b17)) * Added grpcio extras to default feature-server image ([#4737](#4737)) ([e9cd373](e9cd373)) * Changing node version in release ([7089918](7089918)) * Feast create empty online table when FeatureView attribute online=False ([#4666](#4666)) ([237c453](237c453)) * Fix db store types in Operator CRD ([#4798](#4798)) ([f09339e](f09339e)) * Fix the config issue for postgres ([#4776](#4776)) ([a36f7e5](a36f7e5)) * Fixed example materialize-incremental and improved explanation ([#4734](#4734)) ([ca8a7ab](ca8a7ab)) * Fixed SparkSource docstrings so it wouldn't used inhereted class docstrings ([#4722](#4722)) ([32e6aa1](32e6aa1)) * Fixing PGVector integration tests ([#4778](#4778)) ([88a0320](88a0320)) * Incorrect type passed to assert_permissions in materialize endpoints ([#4727](#4727)) ([b72c2da](b72c2da)) * Issue of DataSource subclasses using parent abstract class docstrings ([#4730](#4730)) ([b24acd5](b24acd5)) * Operator envVar positioning & tls.SecretRef.Name ([#4806](#4806)) ([1115d96](1115d96)) * Populates project created_time correctly according to created ti… ([#4686](#4686)) ([a61b93c](a61b93c)) * Reduce feast-server container image size & fix dev image build ([#4781](#4781)) ([ccc9aea](ccc9aea)) * Removed version func from feature_store.py ([#4748](#4748)) ([f902bb9](f902bb9)) * Support registry instantiation for read-only users ([#4719](#4719)) ([ca3d3c8](ca3d3c8)) * Syntax Error in BigQuery While Retrieving Columns that Start wit… ([#4713](#4713)) ([60fbc62](60fbc62)) * Update release version in a pertinent Operator file ([#4708](#4708)) ([764a8a6](764a8a6)) ### Features * Add api contract to fastapi docs ([#4721](#4721)) ([1a165c7](1a165c7)) * Add Couchbase as an online store ([#4637](#4637)) ([824859b](824859b)) * Add Operator support for spec.feastProject & status.applied fields ([#4656](#4656)) ([430ac53](430ac53)) * Add services functionality to Operator ([#4723](#4723)) ([d1d80c0](d1d80c0)) * Add TLS support to the Operator ([#4796](#4796)) ([a617a6c](a617a6c)) * Added feast Go operator db stores support ([#4771](#4771)) ([3302363](3302363)) * Added support for setting env vars in feast services in feast controller ([#4739](#4739)) ([84b24b5](84b24b5)) * Adding docs outlining native Python transformations on singletons ([#4741](#4741)) ([0150278](0150278)) * Adding first feast operator e2e test. ([#4791](#4791)) ([8339f8d](8339f8d)) * Adding github action to run the operator end-to-end tests. ([#4762](#4762)) ([d8ccb00](d8ccb00)) * Adding ssl support for registry server. ([#4718](#4718)) ([ccf7a55](ccf7a55)) * Adding SSL support for the React UI server and feast UI command. ([#4736](#4736)) ([4a89252](4a89252)) * Adding support for native Python transformations on a single dictionary ([#4724](#4724)) ([9bbc1c6](9bbc1c6)) * Adding TLS support for offline server. ([#4744](#4744)) ([5d8d03f](5d8d03f)) * Building the feast image ([#4775](#4775)) ([6635dde](6635dde)) * File persistence definition and implementation ([#4742](#4742)) ([3bad4a1](3bad4a1)) * Object store persistence in operator ([#4758](#4758)) ([0ae86da](0ae86da)) * OIDC authorization in Feast Operator ([#4801](#4801)) ([eb111d6](eb111d6)) * Operator will create k8s serviceaccount for each feast service ([#4767](#4767)) ([cde5760](cde5760)) * Printing more verbose logs when we start the offline server ([#4660](#4660)) ([9d8d3d8](9d8d3d8)) * PVC configuration and impl ([#4750](#4750)) ([785a190](785a190)) * Qdrant vectorstore support ([#4689](#4689)) ([86573d2](86573d2)) * RBAC Authorization in Feast Operator ([#4786](#4786)) ([0ef5acc](0ef5acc)) * Support for nested timestamp fields in Spark Offline store ([#4740](#4740)) ([d4d94f8](d4d94f8)) * Update the go feature server from Expedia code repo. ([#4665](#4665)) ([6406625](6406625)) * Updated feast Go operator db stores ([#4809](#4809)) ([2c5a6b5](2c5a6b5)) * Updated sample secret following review ([#4811](#4811)) ([dc9f825](dc9f825))

* Add adapters for sqlite datetime conversion ([feast-dev#4797](feast-dev#4797)) ([e198b17](feast-dev@e198b17)) * Added grpcio extras to default feature-server image ([feast-dev#4737](feast-dev#4737)) ([e9cd373](feast-dev@e9cd373)) * Changing node version in release ([7089918](feast-dev@7089918)) * Feast create empty online table when FeatureView attribute online=False ([feast-dev#4666](feast-dev#4666)) ([237c453](feast-dev@237c453)) * Fix db store types in Operator CRD ([feast-dev#4798](feast-dev#4798)) ([f09339e](feast-dev@f09339e)) * Fix the config issue for postgres ([feast-dev#4776](feast-dev#4776)) ([a36f7e5](feast-dev@a36f7e5)) * Fixed example materialize-incremental and improved explanation ([feast-dev#4734](feast-dev#4734)) ([ca8a7ab](feast-dev@ca8a7ab)) * Fixed SparkSource docstrings so it wouldn't used inhereted class docstrings ([feast-dev#4722](feast-dev#4722)) ([32e6aa1](feast-dev@32e6aa1)) * Fixing PGVector integration tests ([feast-dev#4778](feast-dev#4778)) ([88a0320](feast-dev@88a0320)) * Incorrect type passed to assert_permissions in materialize endpoints ([feast-dev#4727](feast-dev#4727)) ([b72c2da](feast-dev@b72c2da)) * Issue of DataSource subclasses using parent abstract class docstrings ([feast-dev#4730](feast-dev#4730)) ([b24acd5](feast-dev@b24acd5)) * Operator envVar positioning & tls.SecretRef.Name ([feast-dev#4806](feast-dev#4806)) ([1115d96](feast-dev@1115d96)) * Populates project created_time correctly according to created ti… ([feast-dev#4686](feast-dev#4686)) ([a61b93c](feast-dev@a61b93c)) * Reduce feast-server container image size & fix dev image build ([feast-dev#4781](feast-dev#4781)) ([ccc9aea](feast-dev@ccc9aea)) * Removed version func from feature_store.py ([feast-dev#4748](feast-dev#4748)) ([f902bb9](feast-dev@f902bb9)) * Support registry instantiation for read-only users ([feast-dev#4719](feast-dev#4719)) ([ca3d3c8](feast-dev@ca3d3c8)) * Syntax Error in BigQuery While Retrieving Columns that Start wit… ([feast-dev#4713](feast-dev#4713)) ([60fbc62](feast-dev@60fbc62)) * Update release version in a pertinent Operator file ([feast-dev#4708](feast-dev#4708)) ([764a8a6](feast-dev@764a8a6)) * Add api contract to fastapi docs ([feast-dev#4721](feast-dev#4721)) ([1a165c7](feast-dev@1a165c7)) * Add Couchbase as an online store ([feast-dev#4637](feast-dev#4637)) ([824859b](feast-dev@824859b)) * Add Operator support for spec.feastProject & status.applied fields ([feast-dev#4656](feast-dev#4656)) ([430ac53](feast-dev@430ac53)) * Add services functionality to Operator ([feast-dev#4723](feast-dev#4723)) ([d1d80c0](feast-dev@d1d80c0)) * Add TLS support to the Operator ([feast-dev#4796](feast-dev#4796)) ([a617a6c](feast-dev@a617a6c)) * Added feast Go operator db stores support ([feast-dev#4771](feast-dev#4771)) ([3302363](feast-dev@3302363)) * Added support for setting env vars in feast services in feast controller ([feast-dev#4739](feast-dev#4739)) ([84b24b5](feast-dev@84b24b5)) * Adding docs outlining native Python transformations on singletons ([feast-dev#4741](feast-dev#4741)) ([0150278](feast-dev@0150278)) * Adding first feast operator e2e test. ([feast-dev#4791](feast-dev#4791)) ([8339f8d](feast-dev@8339f8d)) * Adding github action to run the operator end-to-end tests. ([feast-dev#4762](feast-dev#4762)) ([d8ccb00](feast-dev@d8ccb00)) * Adding ssl support for registry server. ([feast-dev#4718](feast-dev#4718)) ([ccf7a55](feast-dev@ccf7a55)) * Adding SSL support for the React UI server and feast UI command. ([feast-dev#4736](feast-dev#4736)) ([4a89252](feast-dev@4a89252)) * Adding support for native Python transformations on a single dictionary ([feast-dev#4724](feast-dev#4724)) ([9bbc1c6](feast-dev@9bbc1c6)) * Adding TLS support for offline server. ([feast-dev#4744](feast-dev#4744)) ([5d8d03f](feast-dev@5d8d03f)) * Building the feast image ([feast-dev#4775](feast-dev#4775)) ([6635dde](feast-dev@6635dde)) * File persistence definition and implementation ([feast-dev#4742](feast-dev#4742)) ([3bad4a1](feast-dev@3bad4a1)) * Object store persistence in operator ([feast-dev#4758](feast-dev#4758)) ([0ae86da](feast-dev@0ae86da)) * OIDC authorization in Feast Operator ([feast-dev#4801](feast-dev#4801)) ([eb111d6](feast-dev@eb111d6)) * Operator will create k8s serviceaccount for each feast service ([feast-dev#4767](feast-dev#4767)) ([cde5760](feast-dev@cde5760)) * Printing more verbose logs when we start the offline server ([feast-dev#4660](feast-dev#4660)) ([9d8d3d8](feast-dev@9d8d3d8)) * PVC configuration and impl ([feast-dev#4750](feast-dev#4750)) ([785a190](feast-dev@785a190)) * Qdrant vectorstore support ([feast-dev#4689](feast-dev#4689)) ([86573d2](feast-dev@86573d2)) * RBAC Authorization in Feast Operator ([feast-dev#4786](feast-dev#4786)) ([0ef5acc](feast-dev@0ef5acc)) * Support for nested timestamp fields in Spark Offline store ([feast-dev#4740](feast-dev#4740)) ([d4d94f8](feast-dev@d4d94f8)) * Update the go feature server from Expedia code repo. ([feast-dev#4665](feast-dev#4665)) ([6406625](feast-dev@6406625)) * Updated feast Go operator db stores ([feast-dev#4809](feast-dev#4809)) ([2c5a6b5](feast-dev@2c5a6b5)) * Updated sample secret following review ([feast-dev#4811](feast-dev#4811)) ([dc9f825](feast-dev@dc9f825))

tmihalac requested a review from a team as a code owner November 5, 2024 15:06

tmihalac force-pushed the add-env-var-to-services branch from ccfbbd4 to 0eade70 Compare November 5, 2024 15:11

tchughesiv requested changes Nov 5, 2024

View reviewed changes

tmihalac force-pushed the add-env-var-to-services branch from 0eade70 to 6000298 Compare November 5, 2024 16:37

tchughesiv reviewed Nov 5, 2024

View reviewed changes

tchughesiv requested changes Nov 5, 2024

View reviewed changes

tmihalac force-pushed the add-env-var-to-services branch 2 times, most recently from 1feddcf to 6313fc3 Compare November 5, 2024 18:04

tchughesiv approved these changes Nov 5, 2024

View reviewed changes

Add support for setting env vars in services

c80e534

Signed-off-by: Theodor Mihalache <tmihalac@redhat.com>

tmihalac force-pushed the add-env-var-to-services branch from 6313fc3 to c80e534 Compare November 5, 2024 20:28

tchughesiv approved these changes Nov 5, 2024

View reviewed changes

lokeshrangineni added the ok-to-test label Nov 5, 2024

lokeshrangineni approved these changes Nov 5, 2024

View reviewed changes

lokeshrangineni merged commit 84b24b5 into feast-dev:master Nov 5, 2024

tchughesiv mentioned this pull request Nov 11, 2024

Feast Go Operator #4561

Closed

shuchu pushed a commit to shuchu/feast that referenced this pull request Nov 21, 2024

feat: Added support for setting env vars in feast services in feast c…

20a1987

…ontroller (feast-dev#4739) Add support for setting env vars in services Signed-off-by: Theodor Mihalache <tmihalac@redhat.com>

tchughesiv mentioned this pull request Dec 3, 2024

fix: Operator envVar positioning & tls.SecretRef.Name #4806

Merged

               		})
+              		It("should properly set container env variables", func() {
+              			By("Reconciling the created resource")

+              	}
+              	return true
+              }

feat: Added support for setting env vars in feast services in feast controller by tmihalac · Pull Request #4739 · feast-dev/feast · GitHub

Uh oh!

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

feat: Added support for setting env vars in feast services in feast controller #4739lokeshrangineni merged 1 commit intofeast-dev:masterfeast-dev/feast:masterfrom tmihalac:add-env-var-to-servicestmihalac/feast:add-env-var-to-servicesCopy head branch name to clipboard

Conversation

tmihalac commented Nov 5, 2024

What this PR does / why we need it:

Which issue(s) this PR fixes:

Uh oh!

tchughesiv Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

tmihalac Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

dmartinol Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

tchughesiv Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmihalac Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmihalac Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchughesiv Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

tchughesiv Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

tmihalac Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchughesiv Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmihalac Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchughesiv Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

tchughesiv Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchughesiv left a comment

Choose a reason for hiding this comment

Uh oh!

tchughesiv commented Nov 5, 2024

Uh oh!

tmihalac commented Nov 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat: Added support for setting env vars in feast services in feast controller #4739
lokeshrangineni merged 1 commit into
feast-dev:masterfrom
tmihalac:add-env-var-to-services

tchughesiv Nov 5, 2024 •
edited
Loading

tmihalac Nov 5, 2024 •
edited
Loading

tmihalac Nov 5, 2024 •
edited
Loading

tmihalac Nov 5, 2024 •
edited
Loading

tchughesiv Nov 5, 2024 •
edited
Loading

tmihalac Nov 5, 2024 •
edited
Loading

tchughesiv Nov 5, 2024 •
edited
Loading

tmihalac commented Nov 5, 2024 •
edited
Loading