Add FieldIsZero, validate non-zero fields in middleware

[rvolosatovs]

Summary

In it's current state field mask validation only partially restricts the fields that can be set.
For example if RPC supports paths A and A.B, sending value with A.C non-empty and field mask A should result in an error - the RPC implementations currently must perform such checks, but that is error-prone and hard to maintain (in case of NS Set RPC this is several hundreds lines of checks)- this PR addresses that.
An example of real-word case where this is very relevant is key envelopes or MAC state.

Changes

• Implement FieldIsZero on EndDevice and related messages
• If FieldIsZero is implemented for a message and fieldmask is specified - ensure that disallowed fields are zero in the interceptor.

Testing

Tested creating a new class C device and sending a bunch of uplinks. Could not test downlink flow, because application downlink is broken cc @adriansmares

Regressions

All clients, which are currently sending invalid messages will be broken - that is expected and desired.
That probably means that CLI should also ensure only the allowed fields are sent. cc @htdvisser

Checklist

• Scope: The referenced issue is addressed, there are no unrelated changes.
• Compatibility: The changes are backwards compatible with existing API, storage, configuration and CLI, according to the compatibility commitments in README.md for the chosen target branch.
• Documentation: Relevant documentation is added or updated.
• Changelog: Significant features, behavior changes, deprecations and fixes are added to CHANGELOG.md.
• Commits: Commit messages follow guidelines in CONTRIBUTING.md, there are no fixup commits left.

[rvolosatovs]

Do we want to do this?
If so, I will file an issue and add a reference according to the guidelines, otherwise I will remove this else
Ideally these methods should be generated, but that should be done after #2798 is solved

[johanstokking]

So if msg is of type EndDevice, and FieldIsZero is implemented on MACState, it doesn't get called unless EndDevice implements some umbrella FieldIsZero? I think it makes sense to check the nearest FieldIsZero of sp here.

If we don't do this, this FieldIsZero support on msg should be done before the loop.

[rvolosatovs]

I think it makes sense to check the nearest FieldIsZero of sp here.

1. Since these methods will be auto-generated(see Add FieldIsZero, validate non-zero fields in middleware #3647 (comment)), all relevant messages will have this implemented
2. We do not have a generic way to access value of type T under arbitrary path a.b.c, so that is not possible unless we loop over all field tree in the struct using reflection and I don't think we should do that for all RPCs on each call.
3. FieldIsZero check is done inside the loop to avoid unnecessary type-assert on each RPC. This check is only performed when there's a path, which needs to be zero - there's no reason to check FieldIsZero, if we don't need to call it.

[htdvisser]

As @johanstokking also wrote, hand-writing those FieldIsZero funcs is not going to be very maintainable. Can we perhaps generate those funcs?

[rvolosatovs]

@htdvisser @johanstokking please see #3647 (comment) I already wrote when opening the PR

Ideally these methods should be generated, but that should be done after #2798 is solved

[rvolosatovs]

blocking https://github.com/TheThingsIndustries/lorawan-stack/issues/2417

[johanstokking]

In general I'm okay with the current state.

I'm assuming that as long as the struct is part of the test table and the list of all fields is up to date, there's a guard against forgetting the zero checks per field. Right?

---

Just out of curiosity; why can't we do this with reflection? We would have to look at the request type, traverse the fields recursively, and once the field is found, see which type it is and check if the value is zero.

[rvolosatovs]

I'm assuming that as long as the struct is part of the test table and the list of all fields is up to date, there's a guard against forgetting the zero checks per field. Right?

Yes

Just out of curiosity; why can't we do this with reflection? We would have to look at the request type, traverse the fields recursively, and once the field is found, see which type it is and check if the value is zero.

1. The traversing part is not trivial and would depend on custom gogo logic even with reflection, since the path we get are protobuf paths and we have to convert them to Go paths. Unless Migrate away from gogo/protobuf #2798 is done I think it's wasted effort to implement it this way
2. Reflection is slow and we'd do this on each RPC with potentially 100s of fields to validate.
3. Generating would be simpler and more efficient, but requires Migrate away from gogo/protobuf #2798

[johanstokking]

OK I'm fine with this, thanks for clarifying @rvolosatovs