[Labels] Implement internal labels

[thanodnl]

To make sure that applications can't interfere with ringpop internals on labels we reserve all labels prefixed with __ (double lodash) for ringpop. Applications will receive an error when they try to set a label that starts with this prefix.

[coveralls]

Coverage decreased (-0.08%) to 92.369% when pulling 2e8b11978f6255d8db308dafd033c7e4cf42a731 on feature/labels-private into 86779e9b332c8eac5dd7b8f28c59827c27c56813 on feature/labels.

[motiejus]

Are we 100% sure applications will never need to access rongpop internal labels?

If not, may I suggest to not err, but mangle the private labels? In the spirit of like Python does with "private" methods.

Do not allow setting then easily, but leave it to people who Know What They Are Doing.

Thoughts?

On Aug 12, 2016 17:49, "Nils Dijk" notifications@github.com wrote:

To make sure that applications can't interfere with ringpop internals on labels we reserve all labels prefixed with __ (double lodash) for ringpop. Applications will receive an error when they try to set a label that starts

with this prefix.

You can view, comment on, or merge this pull request online at:

https://github.com/uber/ringpop-go/pull/172 Commit Summary

• add function to test if label is a private label
• Add test for isPrivateLabel
• Protect public labels interface for altering private keys

File Changes

• M swim/labels.go https://github.com/uber/ringpop-go/pull/172/files#diff-0 (28)
• A swim/labels_test.go https://github.com/uber/ringpop-go/pull/172/files#diff-1 (21)

Patch Links:

• https://github.com/uber/ringpop-go/pull/172.patch
• https://github.com/uber/ringpop-go/pull/172.diff

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/uber/ringpop-go/pull/172, or mute the thread https://github.com/notifications/unsubscribe-auth/AAGkyAFxzOuAUlEPN7wdrRhi2maLNtfNks5qfJYigaJpZM4JjNt_ .

[mennopruijssers]

• Do we allow reading private labels?
• I'm missing a test to check if 'isPrivateLabelis checked from theset` method.

[thanodnl]

@Motiejus I'm not completely sure if an application would never want to edit ringpop's private labels. I think ringpop should always provide functions to interact with its private labels. However I do not want to allow application to unintentionally interfere with internal ringpop labels. That is the reason why I return errors back to the application if they try and change them through the public interface.

@mennopruijssers Currently I allow reading of the private labels yeah, so maybe private is the wrong name here. It is more like a write protection through the public labels interface to prevent an application to interfere with ringpop internals (like an identity label).

Wrt the tests if the function is called, I still need to write the tests for those functions and there it should test the functionality to see that such labels can't be set through the public interface.

[mennopruijssers]

@mennopruijssers Currently I allow reading of the private labels yeah, so maybe private is the wrong name here. It is more like a write protection through the public labels interface to prevent an application to interfere with ringpop internals (like an identity label).

Maybe call them internal?

Wrt the tests if the function is called, I still need to write the tests for those functions and there it should test the functionality to see that such labels can't be set through the public interface.

Ok, makes sense. Lets add a todo to the parent PR (I assume you're tackling the tests there?).

[thanodnl]

I renamed the private labels to internal labels.

[coveralls]

Coverage decreased (-0.1%) to 92.314% when pulling 0384faad26e47028e29c0a89a4f86c49a7ecb64a on feature/labels-private into 86779e9b332c8eac5dd7b8f28c59827c27c56813 on feature/labels.

[coveralls]

Coverage decreased (-0.1%) to 92.314% when pulling 4841e786fb3cf5d8e1c7fc9017c4c390048ce12f on feature/labels-private into 86779e9b332c8eac5dd7b8f28c59827c27c56813 on feature/labels.

[coveralls]

Coverage decreased (-0.05%) to 92.392% when pulling 4841e786fb3cf5d8e1c7fc9017c4c390048ce12f on feature/labels-private into 86779e9b332c8eac5dd7b8f28c59827c27c56813 on feature/labels.

[coveralls]

Coverage decreased (-0.03%) to 95.084% when pulling 42cd008f71e608f2dddb4d76100e0bb2eeade719 on feature/labels-private into 5a725369579ea16ae8cfdeba2012b27025bb4d21 on feature/labels.

[coveralls]

Coverage decreased (-0.03%) to 95.084% when pulling 42cd008f71e608f2dddb4d76100e0bb2eeade719 on feature/labels-private into 5a725369579ea16ae8cfdeba2012b27025bb4d21 on feature/labels.

[coveralls]

Coverage decreased (-0.007%) to 95.106% when pulling 42cd008f71e608f2dddb4d76100e0bb2eeade719 on feature/labels-private into 5a725369579ea16ae8cfdeba2012b27025bb4d21 on feature/labels.

[mennopruijssers]

LGTM!

[motiejus]

@thanodnl I agree we don't want application to unintentionally change internal labels. But we can do it by prefixing the labels with, e.g., the node's identity:

_<identity>__<internal label>

Similarly how Python's __ works:

>>> class A:
...   def __bac():
...     return "bac"
...
>>> A.__bac()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: type object 'A' has no attribute '__bac'
>>> A._A__bac()
'bac'

This way, it is nearly impossible to accidentally call the private method, but if you really want to, you can.

Thoughts?

[thanodnl]

TL;DR: if users need to jump through hoops to interact with internals we should provide a usable public interface that is supported instead of obscuring the place where we store it.

I can see where you come from with obscuring the private namespace instead of putting write protection on it. But the background here is deliberately chosen to write protect it.

The idea was to make it easy for ringpop internals to add extra information that should be gossiped around that is future proof on the protocol. With this we can easily add arbitrary information to the state of a node without having to worry about the deep internals of the protocol and its priority rules.

Since we want to protect applications from unintentionally polluting the gossip protocol with big blobs of information we want to set somewhat conservative limits on the number and size of label information. It felt unfair to let internals of ringpop eat into those limits.

The first use for a private label would be to let ringpop add its identity to such a label. I don't think it is desirable to let applications overwrite this via the public interface of labels because they might unintentionally pick the same label for it. If there is a need to change the identity at a later stage we should provide a public interface to safely do this, which internally sets the identity in what ever mechanism we choose to communicate it over.

The more I think about it we might not want to reuse the public labels mechanism for our internals. Now that we have the code for one set of labels we can add a separate dictionary for internal state that is completely separate from this feature. Or even more extreme add separate fields for every possible internally synchronized field, but I would advise highly against that because for every addition we need to have a good upgrade story on the protocol level. I'd rather have an easy and proven way to add extra metadata to a node instead of dedicated code for everything.

[motiejus]

LGTM!

[coveralls]

Coverage increased (+0.05%) to 95.05% when pulling c6cdee2769ef4d53a6320ddb4621264d3fc3d042 on feature/labels-private into de4d3009e06963cd8d2f9584fbea6aee2744390f on feature/labels.

[coveralls]

Coverage increased (+0.05%) to 95.05% when pulling c6cdee2769ef4d53a6320ddb4621264d3fc3d042 on feature/labels-private into de4d3009e06963cd8d2f9584fbea6aee2744390f on feature/labels.

[coveralls]

Coverage increased (+0.05%) to 95.05% when pulling c6cdee2769ef4d53a6320ddb4621264d3fc3d042 on feature/labels-private into de4d3009e06963cd8d2f9584fbea6aee2744390f on feature/labels.

[coveralls]

Coverage increased (+0.05%) to 95.05% when pulling c6cdee2769ef4d53a6320ddb4621264d3fc3d042 on feature/labels-private into de4d3009e06963cd8d2f9584fbea6aee2744390f on feature/labels.