flyte icon indicating copy to clipboard operation
flyte copied to clipboard

[BUG] New versions of viper breaks config loading

Open Sovietaced opened this issue 1 year ago • 3 comments

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](https://github.com/flyteorg/flyte/blob/master/flytestdlib/config/viper/viper.go#L303). 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/[email protected]`

proceed to run unit tests in `flyteplugins`

### Screenshots

v1.11.0
<img width="508" alt="v1 11 0" src="https://github.com/flyteorg/flyte/assets/1274471/61feb4a4-d3ee-4297-8232-5e38a7250290">

v.1.18.2
<img width="507" alt="v1 18 2" src="https://github.com/flyteorg/flyte/assets/1274471/30f36d9e-38b5-4bc1-8786-e60ab870ae3c">



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

- [X] Yes

### Have you read the Code of Conduct?

- [X] Yes

Sovietaced avatar Jun 11 '24 18:06 Sovietaced

Thank you for opening your first issue here! 🛠

welcome[bot] avatar Jun 11 '24 18:06 welcome[bot]

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 avatar Jun 11 '24 20:06 Sovietaced

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

Sovietaced avatar Jun 11 '24 20:06 Sovietaced

Hi @Sovietaced, I can't reproduce the issue just after upgrading the viper version. Is there any other package had been changed?

go version: go version go1.22.8 darwin/arm64.

Terryhung avatar Nov 11 '24 07:11 Terryhung

Hi @Sovietaced, I can't reproduce the issue just after upgrading the viper version. Is there any other package had been changed?

go version: go version go1.22.8 darwin/arm64.

Here is my diff, which fails: https://github.com/flyteorg/flyte/commit/75e81706e29832075c012782bc1cdd704e165b5b

I'm curious to see what you did..

Sovietaced avatar Nov 12 '24 01:11 Sovietaced

I just run the go get github.com/spf13/[email protected].

Then this is the diff after upgrading the viper: https://github.com/Terryhung/flyte/commit/6a440cb57d3509eed8d8bf80da6563f6f9eeb6bb.

I found the viper version in your diff is: v1.19.0

Terryhung avatar Nov 12 '24 06:11 Terryhung

I just run the go get github.com/spf13/[email protected].

Then this is the diff after upgrading the viper: https://github.com/Terryhung/flyte/commit/6a440cb57d3509eed8d8bf80da6563f6f9eeb6bb.

I found the viper version in your diff is: v1.19.0

I did 1.18.2 as well. The issue is you're updating the global go mod but flyteplugins has its own go mod, and that's where the failing unit test is.

Sovietaced avatar Nov 12 '24 06:11 Sovietaced

@Sovietaced After reviewing the code in viper, I found that it case insensitive problem can't be fix in flyte.

The code in viper.

Moreover, viper already has an old issue....

Terryhung avatar Nov 12 '24 08:11 Terryhung

"Hello 👋, this issue has been inactive for over 90 days. To help maintain a clean and focused backlog, we'll be marking this issue as stale and will close the issue if we detect no activity in the next 7 days. Thank you for your contribution and understanding! 🙏"

github-actions[bot] avatar May 18 '25 00:05 github-actions[bot]

Hello 👋, this issue has been inactive for over 90 days and hasn't received any updates since it was marked as stale. We'll be closing this issue for now, but if you believe this issue is still relevant, please feel free to reopen it. Thank you for your contribution and understanding! 🙏

github-actions[bot] avatar May 26 '25 00:05 github-actions[bot]