go: Bump github.com/spf13/viper to v1.21.0 #6346
Conversation
✅ Deploy Preview for oasisprotocol-oasis-core canceled.
|
| // v1.5.0 has broken uint parsing, use my commit with fixes instead until | ||
| // the maintainers merge my PR: https://github.com/spf13/cast/pull/144 | ||
| github.com/spf13/cast => github.com/oasisprotocol/cast v0.0.0-20220606122631-eba453e69641 | ||
| // v1.18.0+ has broken handling of bound flags. | ||
| github.com/spf13/viper => github.com/spf13/viper v1.17.0 |
There was a problem hiding this comment.
Quickly looking at the cast PR mentioned this was fixed upstream.
Not sure about bound flags error (pending a check)
There was a problem hiding this comment.
cast: The minimal working example now works, so it looks that it was fixed.
viper: Unit tests with v18.0 pass, so this problem is not captured with our tests. @kostko any idea what was wrong and if this has been fixed?
There was a problem hiding this comment.
Not sure about bound flags error (pending a check)
This is still an issue in the newer versions. E.g. running e2e/runtime/runtime-dynamic scenario previously MaxInRuntimeMessages was set to 128 but after this change is set to 0, thus this check fails.
rt.Executor.MaxMessages and params.MaxRuntimeMessages were set to 0 and 128 respectively, but after this change this value is swapped, thus this check fails.
Happy to work on that but I assume this will not be a trivial fix, else it would have been done at that time already?
There was a problem hiding this comment.
Unit tests with v18.0 pass, so this problem is not captured with our tests.
It seems this is only relevant for e2e tests, where we do all the config gymnastics... Maybe is not even that hard to fix... update: or not captured via unit as you say.
There was a problem hiding this comment.
This was more relevant in the past where flags were used for everything. Now most things are passed via the YAML config file. So it shouldn't be that hard to work around?
There was a problem hiding this comment.
Now most things are passed via the YAML config file. So it shouldn't be that hard to work around?
We still do this for the genesis command which seems to be the reason why e2e/runtime/runtime-dynamic scenario is failing. Moreover, searching for viper.BindPFlags, it seems there are quite many places that rely on this pattern.
I tried debugging example above, to see why the default value is no longer used after this upgrade:
- During package
initwe store the default value intoinitGenesisFlags. - Bind
initGenesisFlagsto viper. - Add
initGenesisFlagstoinitGenesisCmdflags. - Read
CfgRoothashMaxRuntimeMessagesduring genesis init command execution.- (Why not pass init genesis flagset explicitly and read from there?
I see two possible reasons why this upgrade broke things:
-
// v1.18.0+ has broken handling of bound flags.
- Most likely reason, but I am still lacking some context/understanding.
- Same flag name for different cobra commands does not work spf13/viper#233
- Also saw this one - not sure it applies here.
Related to above, (ideally) we should try to avoid global state (config) and init functions as they makes code hard to reason and test. Furthermore, as from the documentation viper global instance may get deprecated .
Given my limited knowledge of pflags, viper and cobra this seems far from trivial task, taking the above anti-patterns into consideration. (That does not mean I am not willing to spent time fixing it).
Since this is blocking master, my suggestion would be to temporary add CVE-2025-11065 to .nancy-ignore, unless we find a simple solution for it tmr.
There was a problem hiding this comment.
Since we use the config file for most things now, why use viper at all? Just use the flags directly. It shouldn't be too hard to change that, at least in the affected locations.
There was a problem hiding this comment.
Added CVE-2025-11065 to .nancy-ignore until 2025-12-01 as the vulnerability seems quite irrelevant so we can do this bump in peace.
There was a problem hiding this comment.
So the last commit in this PR should also undo commit de3cc372d902ceec8f04c8e40b74676e580d345d from #6348.
|
Anything in particular you would recommend to test/could be broken after this upgrade? |
martintomazic
force-pushed
the
martin/internal/bump-viper
branch
from
23f2f38 to
8045b42
Compare
martintomazic
requested review from
kostko,
peternose and
ptrus
as code owners
|
^^ Nice CI caught smtg, will explore :): level=error ts=2025-10-06T13:09:56.471894282Z caller=root.go:448 module=test-runner msg="failed to run scenario" err="root: failed to run scenario: failed to register runtime: failed to submit tx: exit status 1" scenario=e2e/runtime/trust-root/change run_id=0 |
Motivation: Fixes Audit go dependencies. See vulnerability.
Consideration:
v.1.20was breaking, after a quick glance we should not be affected.