settings: don't validate settings updates from gossip

[giorgosp]

Fixes #44289

[cockroach-teamcity]

This change is 

[giorgosp]

I left the StateMachineSetting methods intact since the ValidateGossipUpdate wants to crash if the cluster version setting couldn't be updated. Do we want to remove this validation too?

[tbg]

Thanks @giorgosp! And sorry about the large turnaround, things have been quite busy on my end.

I left the StateMachineSetting methods intact since the ValidateGossipUpdate wants to crash if the cluster version setting couldn't be updated. Do we want to remove this validation too?

ValidateGossipUpdate never returns an error (it fatals internally), so you can remove the error return of it and rename set to override to match everything else. Does that sound good?

Reviewed 11 of 11 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained

[giorgosp]

Hi Tobias no problem, thanks for checking this out! Please check my comment on the issue (sorry for cross commenting in the PR and the issue).

Seems like we also need to make the settings table read-only and then remove all these validations in the updater.
Because now there are tests testing that a wrong (manual or not) insert in the system.settings table is not propagated to the other nodes.

Reviewable status: complete! 0 of 0 LGTMs obtained

[tbg]

Thanks, I just caught up on #44289 (comment). I think making the system.settings table read-only is out of scope here (just because it takes us too far out in the weeds), though if someone from SQL is interested maybe they can guide you (@asubiotto?).

We can either

• adapt the test so that the "wrong" insert returns an error from override and call it a day - the semantics will be that yes, if you insert right into the settings table the validation function will be bypassed. It's not ideal that this is possible

• scrap the change. Instead, we do something only to preserveDowngradeVersion.Validate so that it doesn't erroneously refuse requests.

@dt do you have a preference?

[dt]

Eh, I suppose they could be read-only, but directly writing to system tables is already a "void your warranty" situation -- there are (safety-checked, validated, etc) setter methods like SET CLUSTER SETTING there for a reason and if you go muck with it directly, that's on you (just like you could mess with SSTs, or mess with the code, etc). I'm fine with validation only being in the setter even if the raw table isn't read-only. /2¢

[tbg]

@giorgosp sorry for not closing the loop here - I think this PR is basically good to go! I will get this merged for you when we're in the next release cycle (we're in the stability period now, and I don't think this is worth risking).

If you're up for more work in the meantime, I'll easily find something, let me know!

[giorgosp]

@giorgosp sorry for not closing the loop here - I think this PR is basically good to go! I will get this merged for you when we're in the next release cycle (we're in the stability period now, and I don't think this is worth risking).

No problem! When you have more time to check this PR, we may need to consider scraping it and just change preserveDowngradeVersion.Validate as you suggested, because now a lot of assertuibs are deleted to make tests pass.

If you're up for more work in the meantime, I'll easily find something, let me know!

Yes please!