Skip to content

/packages

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

wireguard: add protocol dependency for endpoints #3680

Merged
merged 1 commit into from Dec 24, 2016

Conversation

Reviewers

@zx2c4 zx2c4

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@danrl @zx2c4 @hnyman @zorun @yousong
@danrl
Copy link
Contributor

commented Dec 20, 2016

Endpoint dependency implemented. The actual endpoint is used exclusively. Using
this approach we are dual-stack safe (not errors on missing protocol) and create
only the dependency that are really necessary.

Signed-off-by: Dan Luedtke mail@danrl.com

Maintainer: @zorun
Compile tested: x86_64
Run tested: x86_64

@zx2c4 @zorun: requesting comments and review. Like to improve the regex, e.g. for public key matching. Maybe no really necessary? What do you think?
@zx2c4

zx2c4 suggested changes Dec 20, 2016
View changes

Copy link
Contributor

left a comment

The idea of adding the dependencies at the end using IPs directly from wg is the correct approach. Nice. Just a few nits to cleanup.
net/wireguard/files/wireguard.sh Outdated
@@ -161,6 +140,13 @@ proto_wireguard_setup() {
exit 1
fi

# endpoint dependency
for wg_endpoint in $(wg show ${config} endpoints | grep -v '(none)'); do
ip=$(echo ${wg_endpoint} | sed 's/.*\t\[\?\([0-9a-f\.:]\+\)\]\?:[0-9]\{1,5\}/\1/')

This comment has been minimized.

Sign in to view
Copy link
@zx2c4

zx2c4 Dec 20, 2016

Contributor

There's a cleaner way to do this without the regex:

wg show "${config}" endpoints | while IFS=$'\t:' read -r key ip port; do
    [ -n "${port}" ] || continue
    proto_add_host_dependency "${config}" "${ip}"
done

(No need to declare key, ip, and port as local since that loop actually is in a subshell.)
@danrl

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 20, 2016

Integrated @zx2c4's changes. Tested, it works of course. Thanks!

(Squashed commits)
@zx2c4

zx2c4 reviewed Dec 20, 2016
View changes

net/wireguard/files/wireguard.sh Outdated
@@ -161,6 +140,13 @@ proto_wireguard_setup() {
exit 1
fi

# endpoint dependency
wg show ${config} endpoints | while IFS=$'\t:' read -r key ip port; do

This comment has been minimized.

Sign in to view
Copy link
@zx2c4

zx2c4 Dec 20, 2016

Contributor

Elsewhere in the script you quote ${config}, but not here. I have no idea if quoting of that variable is required, but I did notice the inconsistency. Probably doesn't matter, but FYI.
@danrl

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 20, 2016

Please wait with the merge. I'd like to fix the inconsistency, it bothers me. Good catch!
@zx2c4

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 20, 2016

Sorry, another nit:

This script no longer requires resolveip (I guess it never did, but was in the comment), which means you should remove +resolveip from DEPENDS:= in the Makefile. This change probably belongs in the same commit, since the removal is directly related.
@danrl

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 20, 2016

Right. Since @zorun asked me to co-maintain the package we might touch the Makefile anyway to reflect the organizational change.

Commit coming as soon as I am back at development machine. Thanks for the patience!
@danrl

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 20, 2016

Should be ready now, shouldn't it?
@zx2c4

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 20, 2016

Perfect! Ship it!

👍
@hnyman

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 22, 2016

@danrl

Right. Since @zorun asked me to co-maintain the package we might touch the Makefile anyway to reflect the organizational change.

You might add yourself as the second maintainer into the Makefile if you are going to be "co-maintainer". And @zorun might then ok that.
@danrl

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 22, 2016

@hnyman, I have to admit that I don't know what the right format is. I searched for examples and looked at other Makefiles. Adding the name and email and the end of the line? Or maybe 'MAINTAINER0:='? What dies the build system and other LEDE bots and software prefer?
@hnyman

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 22, 2016

The format is comma-separated list. (Like there would be two mail addresses in a recipient list)
Either on one line, or breaked with \

Examples:

perus@u1610:/Openwrt/lede/feeds/packages$ grep -ir "maintainer.*," .
./net/shadowsocks-client/Makefile:PKG_MAINTAINER:=Zhao, Gang <gang.zhao.42@gmail.com>
./net/aria2/Makefile:PKG_MAINTAINER:=Imre Kaloz <kaloz@openwrt.org>, Hsing-Wang Liao <kuoruan@gmail.com>
./net/sqm-scripts-extra/Makefile:  MAINTAINER:=Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>, \
./sound/shairport-sync/Makefile:PKG_MAINTAINER:=Ted Hess <thess@kitschensync.net>, \
./libs/alsa-lib/Makefile:PKG_MAINTAINER:=Ted Hess <thess@kitschensync.net>, \
./libs/libsoxr/Makefile:PKG_MAINTAINER:=Ted Hess <thess@kitschensync.net>, \

https://github.com/openwrt/packages/blob/master/net/aria2/Makefile#L18

PKG_MAINTAINER:=Imre Kaloz <kaloz@openwrt.org>, Hsing-Wang Liao <kuoruan@gmail.com>

https://github.com/openwrt/packages/blob/master/net/sqm-scripts-extra/Makefile#L23

MAINTAINER:=Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>, \
Sebastian Moeller <moeller0@gmx.de>
@danrl

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

commented Dec 22, 2016

Done!
@danrl
wireguard: add protocol dependency for endpoints 
Endpoint dependency implemented. The actual endpoint is used exclusively. Using
this approach we are dual-stack safe (not errors on missing protocol) and create
only the dependency that are really necessary.

Signed-off-by: Dan Luedtke <mail@danrl.com>
915cd47
@zx2c4

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Dec 23, 2016

Can we get this merged?

@zx2c4 zx2c4 referenced this pull request Dec 23, 2016

Closed

net/wireguard: version bump #3694

@yousong yousong merged commit 7d34e8e into openwrt:master Dec 24, 2016

@danrl danrl deleted the danrl:wireguard-proto-dep branch Dec 24, 2016

@zorun

This comment has been minimized.

Sign in to view
Copy link

commented Dec 25, 2016

A bit late, but ACK for both the change (thanks Dan!) and the shared maintainership with Dan (which would have been better in a separate commit/PR).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.