Network Server GetDevAddrAssignment RPC #758

furtiman · 2019-05-26T17:09:06Z

Summary

Closes #294

Changes

Added NS service and API definitions for it
Added grpc_ns.go and grpc_ns_test.go files
Added GenerateDevAddrAssignment rpc in grpc_ns.go
Added TestGenerateDevAddrAssignment test in grpc_ns_test.go.

Notes for Reviewers

Props @htdvisser for helping out

coveralls · 2019-05-26T17:19:15Z

Coverage decreased (-0.03%) to 73.095% when pulling c634ffa on furtiman:feature/294-devAddr-generating-ns-rpc into 9bfc00b on TheThingsNetwork:master.

rvolosatovs · 2019-05-27T08:10:52Z

api/networkserver.proto

+
+service Ns {
+  // GetDevAddrAssignment requests a device address assignment from the Network Server.
+  rpc GetDevAddrAssignment(GetDevAddrAssignmentRequest) returns (GetDevAddrAssignmentResponse) {


Why not just use google.protobuf.Empty as the request here?

IMO Get here implies that you're getting something that already exists, however that's not the case - DevAddr is generated when this RPC is called. How about AssignDevAddr, CreateDevAddr, GenerateDevAddr?

rvolosatovs · 2019-05-27T09:03:04Z

pkg/networkserver/grpc_ns_test.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package networkserver


This should be networkserver_test

rvolosatovs · 2019-05-27T09:04:27Z

pkg/networkserver/grpc_ns_test.go

+				},
+			})).(*NetworkServer)
+
+		if !a.So(ns.devAddrPrefixes, should.HaveLength, 1) {


This does not belong here

rvolosatovs · 2019-05-27T09:04:35Z

pkg/networkserver/grpc_ns_test.go

+		if !a.So(ns.devAddrPrefixes, should.HaveLength, 1) {
+			t.FailNow()
+		}
+		a.So(ns.devAddrPrefixes[0], should.Resemble, types.DevAddrPrefix{


rvolosatovs · 2019-05-27T09:05:01Z

pkg/networkserver/grpc_ns_test.go

+		})
+
+		devAddr, _ := ns.GetDevAddrAssignment(test.Context(), nil)
+		a.So(devAddr.DevAddr.HasPrefix(ns.devAddrPrefixes[0]), should.BeTrue)


Specify the prefix explicitly

rvolosatovs · 2019-05-27T09:08:34Z

pkg/networkserver/grpc_ns_test.go

+				}
+			}
+		}
+		a.So(seen[ns.devAddrPrefixes[0]], should.BeGreaterThan, 0)


This is non-deterministic, right? Eventually this test will start consistently failing in CI, since the RNG will omit one of the prefixes.
Check each devAddr returned explicitly - i.e. in the loop assert that it has one of the prefixes configured above in the config.
Also, do not use internal NS fields(ns.devAddrPrefixes)

rvolosatovs · 2019-05-27T21:35:27Z

pkg/networkserver/grpc_ns_test.go

+				seen[testPrefix3]++
+			}
+		}
+		a.So(seen[testPrefix1], should.BeGreaterThan, 0)


This is still non-deterministic, drop the seen and assert the prefix in the loop per each generated DevAddr.(i.e. assert that each generated DevAddr has one of the 3 prefixes specified)

This comes from my suggested code where we test the generation itself. I agree that we should assert that, but also that all prefixes are present. With N of 100 that, statistically, must be the case if the RNG isn’t broken. I’d like to test that. We can increase N though, but all prefixes must be there.

So what would be the best way to resolve that, if we talk about assertion AND check if all of the prefixes present? add assertion without dropping the seen?

Yeah, or sum the seen and check if that number matches the number of generated, or fatal the test if the prefix doesn't match in the loop

But we don't need to test the RNG here, right?
Why not just do this on L84:

hasOneOfPrefixes := func(devAddr types.DevAddr, prefixes ...string) bool { for _, p := range prefixes { if devAddr.HasPrefix(p) { return true } } return false } a.So(hasOneOfPrefixes(devAddr.DevAddr, testPrefix1, testPrefix2, testPrefix3), should.BeTrue)

It's obvious what happens here and it's easy to debug.

But we don't need to test the RNG here, right?

We need to test if all the configured prefixes are considered. That is complementary to testing whether all DevAddrs fit in one of the configured prefixes.

If you want to test that NS actually uses the RNG, then you can leave the seen assertions as is, but you do need to assert that devAddr generated is actually valid

Yes, that is the long version of "complementary" 😉

rvolosatovs · 2019-05-27T21:38:54Z

pkg/networkserver/grpc_ns_test.go

+				},
+			})).(*NetworkServer)
+
+		devAddr, _ := ns.GenerateDevAddrAssignment(test.Context(), nil)


Call this via the gRPC client(https://github.com/TheThingsNetwork/lorawan-stack/pull/758/files#diff-e699b03edbcaf36d0a1bd906c75d1d22R584)
Look at the registry tests for an example on how to do that.

rvolosatovs · 2019-05-27T21:40:09Z

api/networkserver.proto

+}
+
+service Ns {
+  // GetDevAddrAssignment requests a device address assignment from the Network Server.


Wrong comment

rvolosatovs · 2019-05-27T21:40:23Z

api/networkserver.proto

@@ -69,3 +69,16 @@ service NsEndDeviceRegistry {
  };
 }

+message GetDevAddrAssignmentResponse {


Wrong name(does not reflect the rename)

rvolosatovs · 2019-05-27T21:41:43Z

api/networkserver.proto

+
+service Ns {
+  // GetDevAddrAssignment requests a device address assignment from the Network Server.
+  rpc GenerateDevAddrAssignment(google.protobuf.Empty) returns (GetDevAddrAssignmentResponse) {


Why not just GenerateDevAddr?

furtiman · 2019-05-28T18:03:07Z

1 - In the networkserver.proto:
@rvolosatovs:

Why not just use google.protobuf.Empty as the request here?

For some reason I get

WARN Task failed                             panic: runtime error: invalid memory address or nil pointer dereference [recovered]
    panic: runtime error: invalid memory address or nil pointer dereference

when I try to use the google.protobuf.Empty and an according type for the function parameter. Also, the API with an empty request was initially suggested by @htdvisser , and now it works, so yeah

2 - I have rewritten the tests and now they indeed include both of the discussed complementary properties ;)

rvolosatovs · 2019-05-29T07:59:11Z

api/networkserver.proto

@@ -69,3 +69,20 @@ service NsEndDeviceRegistry {
  };
 }

+message GenerateDevAddrRequest {
+


You should either use google.protobuf.Empty in the RPC or this message should contain a TODO with references to issues, which describe what fields are missing here

rvolosatovs · 2019-05-29T08:00:54Z

pkg/networkserver/grpc_ns_test.go

+		DevAddrPrefix types.DevAddrPrefix
+	}{
+		{
+			Name:  "Prefix from NS NedID 1",


NetID?
Also other occurences

rvolosatovs · 2019-05-29T08:01:33Z

pkg/networkserver/grpc_ns_test.go

+	"go.thethings.network/lorawan-stack/pkg/util/test/assertions/should"
+)
+
+func pureNewDevAddr(netID types.NetID) types.DevAddr {


Redundant, just use test.Must(types.NewDevAddr).(types.DevAddr)
Also, the naming is mustX or MustX, e.g. mustNewDevAddr

rvolosatovs · 2019-05-29T08:03:24Z

pkg/networkserver/grpc_ns_test.go

+			test.Must(nil, ns.Start())
+			defer ns.Close()
+
+			devAddr, _ := ttnpb.NewNsClient(ns.LoopbackConn()).GenerateDevAddr(test.Context(), nil)


Assert that error is nil

rvolosatovs · 2019-05-29T08:04:02Z

pkg/networkserver/grpc_ns_test.go

+
+			seen := map[types.DevAddrPrefix]int{}
+			for i := 0; i < 100; i++ {
+				devAddr, _ := ttnpb.NewNsClient(ns.LoopbackConn()).GenerateDevAddr(test.Context(), nil)


rvolosatovs · 2019-05-29T08:04:20Z

pkg/networkserver/grpc_ns_test.go

+					},
+				})).(*NetworkServer)
+
+			ns.AddContextFiller(func(ctx context.Context) context.Context {


rvolosatovs · 2019-05-29T08:04:26Z

pkg/networkserver/grpc_ns_test.go

+				_ = cancel
+				return ctx
+			})
+			ns.AddContextFiller(func(ctx context.Context) context.Context {


rvolosatovs · 2019-05-29T08:04:40Z

pkg/networkserver/grpc_ns_test.go

+					},
+				})).(*NetworkServer)
+
+			ns.AddContextFiller(func(ctx context.Context) context.Context {


rvolosatovs · 2019-05-29T08:04:45Z

pkg/networkserver/grpc_ns_test.go

+				_ = cancel
+				return ctx
+			})
+			ns.AddContextFiller(func(ctx context.Context) context.Context {


pkg/networkserver/grpc_ns_test.go

rvolosatovs · 2019-05-29T08:08:02Z

You probably returned nil instead of ttnpb.Empty, which caused the panic when using google.protobuf.Empty

rvolosatovs

LGTM

rvolosatovs · 2019-05-29T11:43:18Z

pkg/networkserver/grpc_ns_test.go

+	"go.thethings.network/lorawan-stack/pkg/util/test/assertions/should"
+)
+
+func TestGenerateDevAddrAssignment(t *testing.T) {


TestGenerateDevAddr

rvolosatovs · 2019-05-29T11:43:38Z

pkg/networkserver/grpc_ns_test.go

+
+	"github.com/smartystreets/assertions"
+	"go.thethings.network/lorawan-stack/pkg/component"
+


There should be no empty line here

[Quickfix] Fixup #758

furtiman added the c/network server This is related to the Network Server label May 26, 2019

furtiman added this to the May 2019 milestone May 26, 2019

furtiman requested review from rvolosatovs and johanstokking May 26, 2019 17:09

furtiman requested a review from htdvisser as a code owner May 26, 2019 17:09

furtiman self-assigned this May 26, 2019

furtiman changed the title ~~ns: Add GetDevAddrAssignment rpc and test~~ Network Server GetDevAddrAssignment RPC May 26, 2019

furtiman added the compat/api This could affect API compatibility label May 26, 2019

rvolosatovs suggested changes May 27, 2019

View reviewed changes

furtiman requested a review from rvolosatovs May 27, 2019 13:52

rvolosatovs suggested changes May 27, 2019

View reviewed changes

rvolosatovs reviewed May 27, 2019

View reviewed changes

furtiman requested a review from rvolosatovs May 28, 2019 18:16

rvolosatovs suggested changes May 29, 2019

View reviewed changes

ns: Add GetDevAddrAssignment rpc and test

c634ffa

furtiman requested a review from rvolosatovs May 29, 2019 09:18

rvolosatovs approved these changes May 29, 2019

View reviewed changes

furtiman merged commit 07355e1 into TheThingsNetwork:master May 29, 2019

furtiman deleted the feature/294-devAddr-generating-ns-rpc branch May 29, 2019 11:54

rvolosatovs pushed a commit to rvolosatovs/lorawan-stack-fork that referenced this pull request May 29, 2019

ns: Fixup TheThingsNetwork#758

6c53a82

rvolosatovs mentioned this pull request May 29, 2019

[Quickfix] Fixup #758 #773

Merged

rvolosatovs added a commit that referenced this pull request May 29, 2019

Merge pull request #773 from rvolosatovs/fix/formatting

29a2588

[Quickfix] Fixup #758

kschiffer mentioned this pull request Jun 25, 2019

Fill issued JoinEUIs and DevAddrs on Device create #820

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Network Server GetDevAddrAssignment RPC #758

Network Server GetDevAddrAssignment RPC #758

furtiman commented May 26, 2019 •

edited

Loading

coveralls commented May 26, 2019 •

edited

Loading

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019 •

edited

Loading

rvolosatovs May 27, 2019 •

edited

Loading

johanstokking May 28, 2019 •

edited

Loading

furtiman May 28, 2019

johanstokking May 28, 2019

rvolosatovs May 28, 2019

johanstokking May 28, 2019

rvolosatovs May 28, 2019

johanstokking May 28, 2019

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019

rvolosatovs May 27, 2019

furtiman commented May 28, 2019 •

edited

Loading

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019

rvolosatovs commented May 29, 2019

rvolosatovs left a comment

rvolosatovs May 29, 2019

rvolosatovs May 29, 2019


		"github.com/smartystreets/assertions"
		"go.thethings.network/lorawan-stack/pkg/component"

Network Server GetDevAddrAssignment RPC #758

Network Server GetDevAddrAssignment RPC #758

Conversation

furtiman commented May 26, 2019 • edited Loading

Summary

Changes

Notes for Reviewers

coveralls commented May 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvolosatovs May 27, 2019 • edited Loading

Choose a reason for hiding this comment

rvolosatovs May 27, 2019 • edited Loading

Choose a reason for hiding this comment

johanstokking May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furtiman commented May 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvolosatovs commented May 29, 2019

rvolosatovs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furtiman commented May 26, 2019 •

edited

Loading

coveralls commented May 26, 2019 •

edited

Loading

rvolosatovs May 27, 2019 •

edited

Loading

rvolosatovs May 27, 2019 •

edited

Loading

johanstokking May 28, 2019 •

edited

Loading

furtiman commented May 28, 2019 •

edited

Loading