chore(orc8r): Protobuf upgrade

[alexzurbonsen]

Summary

Upgrades github.com/golang/protobuf in orc8r/cloud/go and orc8r/gateway/go to v1.5.2.

This is a prerequisite for the Go upgrade in #12151.

More details on protobuf modules: There are two APIs arount. API v1 is published in github.com/golang/protobuf, API v2 in google.golang.org/protobuf. But versions >=v1.4.0 of github.com/golang/protobuf rely on the proto implementation that backs API v2 - while keeping API v1.

This makes some changes necessary:

In the v2 implementation message structs contain a sync.Mutex which is supposed to discourage copying of messages. Every attempt to do so fails in govet copylocks checks. (The new API is published in google.golang.org/protobuf, see https://go.dev/blog/protobuf-apiv2 for more details). Consequently protobuf messages have to be passed as pointers.

Comparisons can be done with proto.Equal instead of using assert.Equal (which also compares implementation details). String comparison can be tricky, since protobuf doesn't guarantee the stability of the representation of the generated strings.

We actually encountered tests, that failed due to randomly appearing whitespaces in strings. Turns out that protobuf generates these into string representations (see for example golang/protobuf#1269). To work around that, we add a function in orc8r/cloud/go/test_utils/unit_test_proto_utils.go which divides the message into parts at inserted separators and checks the parts.

Copying, where necessary, can be done with proto.Clone.

The most significant required change is the modification of configurator APIs/interfaces which were erroneously designed to pass & return config structures by value, the rest of the changes are mostly related to unit tests.

Further changes:

Pin golang.org/x/net to revision that is compatible with go versions in build environments. Can be removed with Go upgrade, tracked in #12559.

Test Plan

• CI
• precommit check in orc8r/cloud/docker, ./build.py -c
• local tests with running orc8r and AGW: tested various workflows like
  • setting configs with swagger
  • propagation of mconfig to AGW
• cwag precommit test, vagrant up cwag && vagrant ssh cwag && cd magma/cwf/gateway && make precommit
• cwag integ test: https://docs.magmacore.org/docs/cwf/dev_testing#run-integration-tests

Additional Information

• Done in pairing with @sebathomas, @voisey, @ajahl.

• This change is backwards-breaking

[github-actions]

Thanks for opening a PR! 💯

A couple initial guidelines

• All commits must be signed off. This is enforced by DCO check.
• All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

Howto

• Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
• Checks. All required CI checks must pass before merge.
• Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

• Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

• Developer Onboarding for onboarding as a new Magma developer
• Development Workflow for guidance on your first pull request
• CI Checks for points of contact for failing or flaky CI checks
• GitHub-to-Slack mappings for Magma maintainers for guidance on how to contact maintainers on Slack

[github-actions]

feg-workflow

    2 files  202 suites   38s ⏱️
371 tests 371 ✔️ 0 💤 0 ❌
385 runs  385 ✔️ 0 💤 0 ❌

Results for commit 1d9642a.

♻️ This comment has been updated with latest results.

[github-actions]

cloud-workflow

    7 files  356 suites   2m 12s ⏱️
974 tests 974 ✔️ 0 💤 0 ❌

Results for commit 1d9642a.

♻️ This comment has been updated with latest results.

[github-actions]

agw-workflow

     77 files     122 suites   7m 25s ⏱️
1 158 tests 1 149 ✔️ 9 💤 0 ❌
1 159 runs  1 150 ✔️ 9 💤 0 ❌

Results for commit 1d9642a.

♻️ This comment has been updated with latest results.

[alexzurbonsen]

@emakeev The summary of #12526 contains some lines that sound pretty helpful. Can we add them here as well?

[maxhbr]

This looks very good to me (up to very minor comments, that can be skipped). The split into commits makes it very easy to review!
🎉

Hope this gets merged soon, as it is already mostly covered by reviews in #12151 (and the approach is also supported by #12526)

[emakeev]

Thanks for splitting Go 1.18 PR. a few minor comments inline. Also - would it be possible to get rid of most of the cloning calls that are still left in the implementation?
The PR improved configurator interface by eliminating passing the message structs by value, but there is still quite a few intentional struct copies left there (please see inline).

[emakeev]

test_utils.AssertListsEqual(t, want[i], got.IpMappings[i])

and then in test_utils.go:

// AssertListsEqual compares two lists of proto messages.
func AssertListsEqual(t *testing.T, expected, actual interface{}) {
	e, a := reflect.ValueOf(expected), reflect.ValueOf(actual)
	if ekind, akind := e.Kind(), a.Kind(); ekind != akind || ekind != reflect.Slice {
		assert.Equal(t, expected, actual)
	}
	assert.Equal(t, e.Len(), a.Len())
	for i := 0; i < e.Len(); i++ {
		expVal, actVal := e.Index(i).Interface(), a.Index(i).Interface()
		expMsg, msgOk := expVal.(proto.Message)
		actualMsg, actOk := actVal.(proto.Message)
		if msgOk && actOk && proto.Equal(expMsg, actualMsg) {
			continue
		}
		assert.Equal(t, expVal, actVal)
	}
}

[voisey]

Hi @magma/approvers-domain-proxy, could we please ask one of you to take a look at this if you get the chance? Any help with getting this through would be really appreciated as it's blocking a couple of things. Thanks! 🙂

[xbend]

Hi @magma/approvers-domain-proxy, could we please ask one of you to take a look at this if you get the chance? Any help with getting this through would be really appreciated as it's blocking a couple of things. Thanks! 🙂

[xbend]

Sorry, unintentional misclick. -.-
Approval from domain proxy granted.

[alexzurbonsen]

Thanks for the reviews! :)

[alexzurbonsen]

Thanks for splitting Go 1.18 PR. a few minor comments inline. Also - would it be possible to get rid of most of the cloning calls that are still left in the implementation? The PR improved configurator interface by eliminating passing the message structs by value, but there is still quite a few intentional struct copies left there (please see inline).

@emakeev Thank you for the review! Left some questions underneath some of your comments.
But most of the proto.Clones are gone now.

As for the comments referring to test_utils.AssertListsEqual we intend to look into that in a separate PR, after things got merged. They are tracked in this issue #12571. Let us know if you prefer a different approach though.
Otherwise we would land this PR as soon as CI is repaired.

Edit: Also added your line about the changes in configurator API to the summary of this PR.

[uri200]

Let us know when is this ready to be merged

[alexzurbonsen]

Let us know when is this ready to be merged

Feel free to merge!