vtctldclient: Fix Apply (Shard | Keyspace| Table) Routing Rules commands

[rohit-nayak-ps]

Description

The Apply cli commands were incorrectly migrated to vtctldclient. This PR fixes them and adds and e2e test for validating the Apply commands for table, shard and keyspace routing rule types.

Backports

The root cause has been there since the first port of the ApplyRoutingRules command to vtctldclient. So part of this PR will need to be backported to all supported versions depending on which commands were migrated for each. Also the e2e test may not be portable in older versions since it uses some commands migrated much later.

Related Issue(s)

#16095

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.24%. Comparing base (b49494e) to head (dd0236b).
Report is 9 commits behind head on main.

Files	Patch %	Lines
...cmd/vtctldclient/command/keyspace_routing_rules.go	0.00%	1 Missing ⚠️
go/cmd/vtctldclient/command/routing_rules.go	0.00%	1 Missing ⚠️
go/cmd/vtctldclient/command/shard_routing_rules.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16096      +/-   ##
==========================================
+ Coverage   68.23%   68.24%   +0.01%     
==========================================
  Files        1543     1543              
  Lines      197555   197589      +34     
==========================================
+ Hits       134796   134848      +52     
+ Misses      62759    62741      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mattlord]

@rohit-nayak-ps We should revert this change here in your PR too IMO: https://github.com/vitessio/vitess/pull/15933/files#diff-ae8d127b4d448b7e712421d7364839b341d9ccdabd05f50c1e85370fbce6362bR82-R89

And backport this PR to v20 (which you likely already planned), if you don't mind 🙏

Since the reason for why I made that change would now be gone after this PR and it avoids any potential unintended side effects for users that are parsing the results in their own tooling. We could also optionally change that to json2.MarshalPB() there if we want (it's equivalent behavior today as it only calls protojson.Marshal() w/o any options).

The changes I made there would also impact the output of vtctlclient TopoCat -- --decode_proto_json <path>:

vitess/go/vt/vtctl/topo.go

Lines 201 to 212 in 6fb0f0e

	func (d JSONTopologyDecoder) decode(ctx context.Context, topoPaths []string, conn topo.Conn, wr *wrangler.Wrangler, long bool) error {
	hasError := false
	var jsonData []any
	for _, topoPath := range topoPaths {
	data, version, err := conn.Get(ctx, topoPath)
	if err != nil {
	hasError = true
	wr.Logger().Printf("TopoCat: Get(%v) failed: %v\n", topoPath, err)
	continue
	}
	decoded, err := topo.DecodeContent(topoPath, data, true)

Which is why it's better to leave it as it was (although it's not likely people are relying on the key names in the output for that command).

[dbussink]

I think we should audit all calls to json2.Unmarshal and see if we need to switch those to json2.UnmarshalPB then.

[dbussink]

One nit on the error checking style in the tests.

[rohit-nayak-ps]

@rohit-nayak-ps We should revert this change here in your PR too IMO: https://github.com/vitessio/vitess/pull/15933/files#diff-ae8d127b4d448b7e712421d7364839b341d9ccdabd05f50c1e85370fbce6362bR82-R89

I suggest we do the revert in a separate PR since it not related to the specific bug this PR is addressing. Easier to review the changes for both then and also to track related changes in the future.

[mattlord]

@rohit-nayak-ps We should revert this change here in your PR too IMO: https://github.com/vitessio/vitess/pull/15933/files#diff-ae8d127b4d448b7e712421d7364839b341d9ccdabd05f50c1e85370fbce6362bR82-R89

I suggest we do the revert in a separate PR since it not related to the specific bug this PR is addressing. Easier to review the changes for both then and also to track related changes in the future.

It is related to this bug though. The only reason that was added was because this bug existed. I incorrectly assumed that this must have been an issue specific to GetTopologyPath and thus fixed the issue there in that way. But now we know it was a wider issue and we're fixing it properly here so we no longer need that fix/change.

I'm OK doing a separate PR, but if so we should do that ASAP (I can do it) so that we can get it into the v20 RC if possible. Then there would be no output change in a GA release.

[mattlord]

Thanks, @rohit-nayak-ps ! Nice work on this. I only had a few nits that you can address as you prefer.

[mattlord]

Seems like there may be other places where it is worth changing the usage to UnmarshalPB:

❯ git grep json2\.Unmarshal | grep -vE 'test|wrangler|vtctl.go'
go/cmd/vtctldclient/command/keyspace_routing_rules.go:	if err := json2.Unmarshal(rulesBytes, &krr); err != nil {
go/cmd/vtctldclient/command/routing_rules.go:	if err := json2.Unmarshal(rulesBytes, &rr); err != nil {
go/cmd/vtctldclient/command/shard_routing_rules.go:	if err := json2.Unmarshal(rulesBytes, &srr); err != nil {
go/cmd/vtctldclient/command/vschemas.go:		err = json2.Unmarshal(schema, &vs)
go/vt/tableacl/tableacl.go:		if jsonErr := json2.Unmarshal(data, config); jsonErr != nil {
go/vt/vtctl/workflow/traffic_switcher.go:		if err := json2.Unmarshal([]byte(wrap), ks); err != nil {
go/vt/vtexplain/vtexplain_vtgate.go:	err := json2.Unmarshal([]byte(wrappedStr), &srvVSchema)
go/vt/vtexplain/vtexplain_vtgate.go:	err := json2.Unmarshal([]byte(ksShardMapStr), &ksShardMap)
go/vt/vtgate/planbuilder/fuzz.go:	err = json2.Unmarshal(data, formal)
go/vt/vtgate/vindexes/vschema.go:	err = json2.Unmarshal(data, formal)
go/vt/vtgate/vindexes/vschema.go:	err = json2.Unmarshal(data, formal)

We don't have to change them all here but would be nice to IMO so that we don't potentially leave current or future latent cases of the same kind around.

[rohit-nayak-ps]

There are other such places across the code base. We should clean them all up separately.