[services] Tunnelbroker - Removing of 'sessionID' from 'MessageToTunnelbroker'
ClosedPublic
Actions

Authored by • max on Nov 2 2022, 2:12 PM.

Details

Reviewers

• jon
tomek
ashoat

Commits

rCOMM3cf94dbbc3f7: [services] Tunnelbroker - Removing of 'sessionID' from 'MessageToTunnelbroker'

Summary

This diff removes the sessionID from the messages incoming to the Tunnelbroker. The purpose of this field was client authentication by providing the sessionID from the client. This method is not efficient because we will repeat the sessionID on every message instead of providing it once when the stream is opening.

Instead of repeating the sessionID on every message, we should use a gRPC canonical way for the authentication which is about using the context metadata. The full context is available in the Linear task.

Linear task: ENG-1359

Test Plan

Services were successfully built, protobuf file was successfully compiled.

Diff Detail

Repository

rCOMM Comm

Branch

remove-sessionid-from-proto

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• max created this revision.Nov 2 2022, 2:12 PM

• max held this revision as a draft.

Herald added a reviewer: • jon. · View Herald TranscriptNov 2 2022, 2:12 PM

Herald added subscribers: • abosh, atul, tomek, ashoat. · View Herald Transcript

Harbormaster completed remote builds in B13144: Diff 18039.Nov 2 2022, 2:27 PM

• max added a child revision: D5528: [services] Tunnelbroker - Adding of the client authentication by the `sessionID` from metadata.Nov 2 2022, 2:50 PM

• max published this revision for review.Nov 2 2022, 4:19 PM

• max edited the summary of this revision. (Show Details)

• max edited the test plan for this revision. (Show Details)

• max added a reviewer: tomek.

The Linear task mentions that it is a good practice to use a comment to describe metadata - can we do that?

When the service is finally live, we shouldn't be reusing the messages and instead increase the numbers. For now it is ok, but we have to be careful in the future.

Metadata seems to be something additional, so a better ordering of the diffs would be to first use the metadata and then remove the old field so that at any point the service is usable. This also will be a concern in the future but is fine for now.

In D5525#163498, @tomek wrote:

The Linear task mentions that it is a good practice to use a comment to describe metadata - can we do that?

Yes, I think it's a good idea to add it here. Thanks, @tomek !

When the service is finally live, we shouldn't be reusing the messages and instead increase the numbers.

What do you mean by "instead increase the numbers"? I think you mean that we will push a few messages into a one stream message or the number of stream messages will increase?

For now it is ok, but we have to be careful in the future.

What are your concerns about the current API in the future?

Metadata seems to be something additional, so a better ordering of the diffs would be to first use the metadata and then remove the old field so that at any point the service is usable. This also will be a concern in the future but is fine for now.

The D5528 diff includes the usage of the metadata (I'll add this to the description).
It was removed now and early because this gRPC API (single bidirectional stream) is not yet used anywhere and has not been implemented yet.

Adding a comment about the metadata.

Harbormaster failed remote builds in B13151: Diff 18048!Nov 3 2022, 3:29 AM

Seems that the Nix build error is unrelated to the last changes as the only comment was added here.

When the service is finally live, we shouldn't be reusing the messages and instead increase the numbers.

What do you mean by "instead increase the numbers"? I think you mean that we will push a few messages into a one stream message or the number of stream messages will increase?

I was talking about https://developers.google.com/protocol-buffers/docs/proto3#assigning_field_numbers field numbers - according to the docs:

These field numbers are used to identify your fields in the message binary format, and should not be changed once your message type is in use.

So when a service is live, we should never reuse the number and instead assign new values to the new fields. In case of deleting a field, we should not update the numbers for fields that are left.