Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zerotier: split configuration #24773

Merged
merged 1 commit into from
Sep 17, 2024
Merged

zerotier: split configuration #24773

merged 1 commit into from
Sep 17, 2024

Conversation

mwarning
Copy link
Contributor

Split configuration in global and per-network sections.
This change is not backwards compatible!

Maintainer: me
Compile tested: WR902AC-v3 (ramips/mt76x8) with OpenWrt master
Run tested: connects to Zerotier global network

@mwarning
Copy link
Contributor Author

mwarning commented Aug 12, 2024

@ogarcia fyi, I have but you in the signed-off section. But I will change it when I find out how to say that you are actually the author. :P

@feckert
Copy link
Member

feckert commented Aug 14, 2024

Do we need an uci-default upgrade skript there for old configs?

@ogarcia
Copy link

ogarcia commented Aug 14, 2024

Do we need an uci-default upgrade skript there for old configs?

In principle it should not be necessary as it is the user's duty to check the configuration after an update. This is the analysis we have performed (full analysis here):

When switching from a multi-instance model to a single instance the current configuration no longer works. This should not be a problem since, unless the user has called global to its configuration, what will happen is that the current configuration will be ignored. In this case the message disabled in /etc/config/zerotier will also appear, which will already give an important clue to the user that his configuration is not working.

In the hypothetical case that the user has a configuration called global two things can happen.

  1. If the user has all the configuration in his own config_path he will not notice anything. It will boot without problems and will work (this is the only 100% backwards compatible case).
  2. Otherwise, the normal case is that the user has a list join 'XXXXX', as this is not read what will happen is that ZT will boot but will not join any network.

In short (TL/DR), either the daemon simply does not start (this will happen in almost all cases) or it does but does not connect against anything (this will happen if the user has called his vpn global which, frankly, I doubt there is any such case). In neither case is this a critical situation since the most that can happen is that you don't connect to the VPN.

@mwarning
Copy link
Contributor Author

it would be nice if we can transform update the setting automatically. I will try to take a look on the weekend.

@yousong
Copy link
Member

yousong commented Aug 15, 2024

In principle it should not be necessary as it is the user's duty to check the configuration after an update. This is the analysis we have performed (full analysis here):

I do not agree. It is the maintainer's duty to try to make sure an upgrade does not break users' existing configuration.

@ogarcia
Copy link

ogarcia commented Aug 15, 2024

If there is any documentation or any example of how these updates are handled (as to what file this should be done or if there is something specific for it) and you can provide me a link to it I'll take a look and try to make an automagic script.

Although I am of the opinion that it is duty of the user to check the configurations ALWAYS after an update since by many automatisms that you put always there will be cases not contemplated.

@mwarning
Copy link
Contributor Author

@ogarcia the migration script would go into e.g. packages/net/zerotier/files/etc/uci-defaults/80-zt-migration.
Script in the folder uci-defaults are executed once on package installation or upgrade and will then be deleted.

option allow_managed '1'
option allow_global '0'
option allow_default '0'
option allow_dns '0'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If allowing network specific config is the goal of this pull request, I think it's possible to extend current uci config while not breaking backward compatibility. join is still an config option in zerotier config section, with network specific configuration in added network section. E.g.

config zerotier sample_config
    ...
    list join 8056c2e21c000001

config network 8056c2e21c000001
#	option allow_managed '1'
#	option allow_global '0'
#	option allow_default '0'
#	option allow_dns '0'

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea really goes beyond that. What we pursue is what is commented in the MR, to simplify the configuration and the startup script and to eliminate a possibility (the one of starting multiple zerotier daemons) that we know that it does not work correctly and that in addition it does not make too much sense.

Copy link
Member

@yousong yousong Aug 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Limiting number of zerotier daemon to 1 is reasonable if it's known fact that multiple instances do not work. It's a fix, not going to break anyone's expectation.

From a user point of view, I do not see how the current zerotier uci config is more complex than the new one. Rewriting the init script is another thing though. It's impl detail. Compat is my concern. As a user really I do not intend to check the code unless something breaks.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, the reduction of complexity is for the future and possible new configuration options that may be included in the file. Besides, this configuration is more similar to the one used in other packages, so I understand that being something “more known” it will be less complex for the user.

In any case with the uci-defaults script we have added the configuration migration is covered. I have tested it a few times and it works correctly so there should be no more problems.

Copy link
Contributor Author

@mwarning mwarning Aug 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multiple Zerotier instances are not useful, since a single instance can connect to multiple networks as well.
Also the current configuration does not allow any settings on a per network basis. This chance is going to fix this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yousong it is true that with the example that you comment the current configuration would work as it is without changing, but having the migration script that does exactly the same it is preferable to go to the new configuration and leave the package ready for future possibilities.

Let's say that right now we do what you say, we leave a configuration that looks “weird” because I have to declare twice the network I want to join (first a “list join” and then a “network config”). In the next update we would have to change it again and again we would be at “square one”.

I fully understand the initial reluctance of not making the change without the migration script, but now that the /etc/uci-defaults/80-zt-migration is already included I don't see the problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say that right now we do what you say, we leave a configuration that looks “weird” because I have to declare twice the network I want to join (first a “list join” and then a “network config”). In the next update we would have to change it again and again we would be at “square one”.

Why "In the next update we would have to change it again"?

Looking at it from another angle, I see it as a convenience feature. Only those present in join list will be joined. We can easily join/leave a network by inserting/removing a #

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why "In the next update we would have to change it again"?

It is very likely that this will have to be done if new functionality is added.

Looking at it from another angle, I see it as a convenience feature. Only those present in join list will be joined. We can easily join/leave a network by inserting/removing a #

You can do the same by commenting out the only two mandatory lines when configuring a network, e.g:

#config network 'mynet'
#	option id '8056c2e21c000001'

The advantage of being able to name the networks is that they are easily identifiable by the user.

As I said before, having a migration script that adapts the current configuration to the new one, I don't see the problem.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact, we have not mentioned it, but one of the objectives we are looking for with this change is precisely the possibility that the user can name their networks and thus be able to identify them much more easily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name option can be added to the network section if needed.

Well, concerns expressed and still I am not a fan of the incompat change. The migration script will be a unnecessary burden. That's it for me at the moment. Maintainers have the say.

@ogarcia
Copy link

ogarcia commented Aug 16, 2024

@ogarcia the migration script would go into e.g. packages/net/zerotier/files/etc/uci-defaults/80-zt-migration. Script in the folder uci-defaults are executed once on package installation or upgrade and will then be deleted.

Ok. Today I'll try to find some time to make a script that does this automagically 😉

@mwarning
Copy link
Contributor Author

mwarning commented Aug 20, 2024

@ogarcia I have added your migration script and did some minor changes. Would you be able to test this? My own devices do not have enough storage to perform a zerotier update. My dry tests look good. But we should make no mistake here or the mob will get us. ;-)

Btw., since you have already put so much effort into the zerotier package, I like to offer you to take over maintainership of the Zerotier package. I do not have the time to answer as quick as I would like to. :-)

@ogarcia
Copy link

ogarcia commented Aug 21, 2024

Btw., since you have already put so much effort into the zerotier package, I like to offer you to take over maintainership of the Zerotier package. I do not have the time to answer as quick as I would like to. :-)

@mwarning I prefer you to be the leader of the project because you have more experience than me in these matters. It doesn't matter if you take more or less time to review what I send you, in the end all this is with the best effort and each one of us dedicates the time we have (sometimes it can be more and sometimes less).

In any case I will be lending a hand in any way I can.

@ogarcia
Copy link

ogarcia commented Aug 22, 2024

IMHO, this is ready to merge. But in any case, any changes let me know and I will gladly review it.

@1715173329
Copy link
Member

I have but you in the signed-off section. But I will change it when I find out how to say that you are actually the author. :P

git commit --amend --author=""

There's an extra space between you R-b tag, please check.

@1715173329
Copy link
Member

Is it ready to go? If you set yourself as the commit author, please add your SoB too.

@mwarning
Copy link
Contributor Author

Is it ready to go? If you set yourself as the commit author, please add your SoB too.

I am not the author this time. But agree to the change and the MR is ready to go.
This MR makes me feel torn between different approaches, but in the end I think we should go forward.

@ogarcia
Copy link

ogarcia commented Sep 16, 2024

I am the author (hence the SoB is in my name, although it doesn't matter if you have to put it differently or not at all). It is ready to be merged. 😉

@1715173329
Copy link
Member

git commit --amend --author="Óscar García Amor <[email protected]>"

please update the commit then.

Split configuration in global and per-network sections.
This change breaks existing configurations.

The following per-network settings are available:

* allow_managed
* allow_global
* allow_default
* allow_dns

See  https://docs.zerotier.com/config/#network-specific-configuration

Signed-off-by: Óscar García Amor <[email protected]>
Reviewed-by: Moritz Warning <[email protected]>
@1715173329 1715173329 merged commit 5af8163 into openwrt:master Sep 17, 2024
13 checks passed
@1715173329
Copy link
Member

Merged, thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants