flyteorg / flyte

Scalable and flexible workflow orchestration platform that seamlessly unifies data, ML and analytics stacks.
https://flyte.org
Apache License 2.0
5.17k stars 550 forks source link

[BUG] New versions of viper breaks config loading #5469

Open Sovietaced opened 2 weeks ago

Sovietaced commented 2 weeks ago

Describe the bug

My custom Flyte plugin has a depenency that pulls in a more recent version of viper v1.11.0 -> v1.18.2 and this appears to break config loading since flyteplugins config_load_test fails. I realize that this is not a present bug but I figured I would open this up in case anyone has an issue and in the event that Flyte eventually updates viper to a more recent version.

It appears that the latest entry in defined in a YAML map will be the only entry in the map that is parsed. For example:

=== RUN   TestLoadConfig/k8s-config-test
    config_load_test.go:30: 
            Error Trace:    /home/jparraga/code/flyte/flyteplugins/go/tasks/config_load_test.go:30
            Error:          Not equal: 
                            expected: map[string]string{"annotationKey1":"annotationValue1", "annotationKey2":"annotationValue2"}
                            actual  : map[string]string{"annotationkey2":"annotationValue2"}

                            Diff:
                            --- Expected
                            +++ Actual
                            @@ -1,4 +1,3 @@
                            -(map[string]string) (len=2) {
                            - (string) (len=14) "annotationKey1": (string) (len=16) "annotationValue1",
                            - (string) (len=14) "annotationKey2": (string) (len=16) "annotationValue2"
                            +(map[string]string) (len=1) {
                            + (string) (len=14) "annotationkey2": (string) (len=16) "annotationValue2"
                             }
            Test:           TestLoadConfig/k8s-config-test

It appears the error happens in decoding the custom config into the root config. From what I've seen there is a different in the typing of the configuration that is parsed by viper. Notably, map entries used to be parsed as map[interface{}]interface{} and are now parsed as map[string]interface{}. The code is bit hair but I figured I'd file a ticket before I dig deeper here.

Expected behavior

I would expect config loading to work correctly and unit tests to pass.

Additional context to reproduce

go get github.com/spf13/viper@v1.18.2

proceed to run unit tests in flyteplugins

Screenshots

v1.11.0

v1 11 0

v.1.18.2

v1 18 2

Are you sure this issue hasn't been raised already?

Have you read the Code of Conduct?

welcome[bot] commented 2 weeks ago

Thank you for opening your first issue here! 🛠

Sovietaced commented 2 weeks ago

This appears to be related to some custom decoding which turns slices into maps: https://github.com/flyteorg/flyte/blob/master/flytestdlib/config/viper/viper.go#L177-L203

Fixing this includes all the key/values but unfortunately the keys are lowercased.

Sovietaced commented 2 weeks ago

Instead of dumping more time into this I ended up just forcing the old version of viper.

replace github.com/spf13/viper v1.18.2 => github.com/spf13/viper v1.11.0